Bug 35899 - Performance improvements for build_holds_queue
Summary: Performance improvements for build_holds_queue
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Andreas Jonsson
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-01-24 19:06 UTC by Andreas Jonsson
Modified: 2024-09-19 00:34 UTC (History)
2 users (show)

See Also:
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 (6.25 KB, patch)
2024-01-24 19:38 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 35899: don't perform expensive checks on non-local items when examining local items (2.89 KB, patch)
2024-01-24 19:38 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 35899: memoize C4::Circulation::GetBranchItemRule when building holds queue (6.25 KB, patch)
2024-01-24 19:38 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 35899: don't perform expensive checks on non-local items when examining local items (2.66 KB, patch)
2024-04-16 21:40 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 35899: memoize C4::Circulation::GetBranchItemRule when building holds queue (6.25 KB, patch)
2024-04-16 21:40 UTC, Andreas Jonsson
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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