Bug 35899

Summary: Performance improvements for build_holds_queue
Product: Koha Reporter: Andreas Jonsson <andreas.jonsson>
Component: Hold requestsAssignee: Andreas Jonsson <andreas.jonsson>
Status: Needs Signoff --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, phil
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 35899: memoize C4::Circulation::GetBranchItemRule when building holds queue
Bug 35899: don't perform expensive checks on non-local items when examining local items
Bug 35899: memoize C4::Circulation::GetBranchItemRule when building holds queue
Bug 35899: don't perform expensive checks on non-local items when examining local items
Bug 35899: memoize C4::Circulation::GetBranchItemRule when building holds queue

Description Andreas Jonsson 2024-01-24 19:06:35 UTC
On a large and busy system the holds queue can take several minutes to build adding a significant load to the database and koha server.

I have profiled the script and have noticed two major inefficiencies:

1. Calls to C4::Circulation::GetBranchItemRule constitute roughly 35% of the runtime.  But this method can easily be memoized.

2. With LocalHoldsPriority enabled, expensive checks are made for all available items, even if it is only relevant to do these checks for local items.  Depending on how the system is configured, either the unnecessary loading of patron objects, or calls to _checkHoldPolicy can take considerable time.

I have prepared two patches which together cuts the runtime of the script with abouth 50% if LocalHoldsPriority is enabled.

Profiling result before:

https://bv-test-opac.biblioteket.nu/resources/before/index.html

Profiling result after:

https://bv-test-opac.biblioteket.nu/resources/after/index.html
Comment 1 Andreas Jonsson 2024-01-24 19:38:05 UTC
Created attachment 161358 [details] [review]
Bug 35899: memoize C4::Circulation::GetBranchItemRule when building holds queue

Test plan:

* Examine patches and make sure they do not alter the functionality
  (provided there are no unexpected side effects in called
  subroutines)
* Run prove t/db_dependent/HoldsQueue.t
Comment 2 Andreas Jonsson 2024-01-24 19:38:41 UTC
Created attachment 161359 [details] [review]
Bug 35899: don't perform expensive checks on non-local items when examining local items
Comment 3 Andreas Jonsson 2024-01-24 19:38:44 UTC
Created attachment 161360 [details] [review]
Bug 35899: memoize C4::Circulation::GetBranchItemRule when building holds queue

Test plan:

* Examine patches and make sure they do not alter the functionality
  (provided there are no unexpected side effects in called
  subroutines)
* Run prove t/db_dependent/HoldsQueue.t
Comment 4 Andreas Jonsson 2024-04-16 21:40:42 UTC
Created attachment 164969 [details] [review]
Bug 35899: don't perform expensive checks on non-local items when examining local items
Comment 5 Andreas Jonsson 2024-04-16 21:40:45 UTC
Created attachment 164970 [details] [review]
Bug 35899: memoize C4::Circulation::GetBranchItemRule when building holds queue

Test plan:

* Examine patches and make sure they do not alter the functionality
  (provided there are no unexpected side effects in called
  subroutines)
* Run prove t/db_dependent/HoldsQueue.t