Bug 36672 - Circulation rules are performing too many lookups
Summary: Circulation rules are performing too many lookups
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-04-22 18:08 UTC by Nick Clemens (kidclamp)
Modified: 2024-06-27 22:04 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00,24.05.02


Attachments
Bug 36672: Circulation rules are performing too many lookups (3.56 KB, patch)
2024-04-26 17:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36672: Circulation rules are performing too many lookups (6.38 KB, patch)
2024-04-26 18:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 36672: Circulation rules are performing too many lookups (6.44 KB, patch)
2024-05-03 15:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36672: Circulation rules are performing too many lookups (6.50 KB, patch)
2024-06-04 12:18 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2024-04-22 18:08:35 UTC
See bug 36668 - it tackles the size of the circulation page but doesn't significantly increase load time. Profiling it seems we spend about 50% of the loading time on Koha::Template::Plugin::CirculationRules::Search

Looking at the template, for every section after the main rules block we loop over categories or itemtypes, and lookup the value for each rule name.

In systems with large numbers of categories and item types this becomes very slow.

In the rules section, we have already built a hash of rules by category and itemtype - we should continue to use this throughout the page.

Additionally, we don't need to use all category/item types, we can use the 'used_categories' array from bug 36447 to only attempt categories that  we know have rules
Comment 1 Kyle M Hall 2024-04-26 17:51:15 UTC
Created attachment 165685 [details] [review]
Bug 36672: Circulation rules are performing too many lookups

Looking at the template, for every section after the main rules block we loop over categories or itemtypes, and lookup the value for each rule name.

In systems with large numbers of categories and item types this becomes very slow.

In the rules section, we have already built a hash of rules by category and itemtype - we should continue to use this throughout the page.

Test Plan:
1) Apply this patch
2) For each rule section, create, update and delete a rule
3) No change in behavior should be noted!
Comment 2 Kyle M Hall 2024-04-26 18:08:46 UTC
Created attachment 165687 [details] [review]
Bug 36672: Circulation rules are performing too many lookups

Looking at the template, for every section after the main rules block we loop over categories or itemtypes, and lookup the value for each rule name.

In systems with large numbers of categories and item types this becomes very slow.

In the rules section, we have already built a hash of rules by category and itemtype - we should continue to use this throughout the page.

Test Plan:
1) Apply this patch
2) For each rule section, create and delete a rule
3) No change in behavior should be noted!
Comment 3 Pedro Amorim 2024-05-03 15:45:07 UTC
Created attachment 166151 [details] [review]
Bug 36672: Circulation rules are performing too many lookups

Looking at the template, for every section after the main rules block we loop over categories or itemtypes, and lookup the value for each rule name.

In systems with large numbers of categories and item types this becomes very slow.

In the rules section, we have already built a hash of rules by category and itemtype - we should continue to use this throughout the page.

Test Plan:
1) Apply this patch
2) For each rule section, create and delete a rule
3) No change in behavior should be noted!

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 4 Pedro Amorim 2024-05-03 15:45:31 UTC
Testing notes:
Used Kyle's script bug 36668 (8,368 entries in * table )

-----
http://localhost:8081/cgi-bin/koha/admin/smart-rules.pl:
Before patch:
Time: 20.72s | 25.27s | 26.21s | 21.68s -> 23.47s avg

After patch:
Time: 17.24s | 22.31s | 19.96s | 16.80s -> 19.07s avg

Result: ~20% performance improvement
-----

-----
http://localhost:8081/cgi-bin/koha/admin/smart-rules.pl?branch=FFL (with a single rule entry)
Before patch:
Time 5.94s | 5.72s | 5.86s | 5.67s

After patch:
Time: 1.88s | 1.68 | 1.71s | 1.65s

Result: Lightning fast
-----

Adding and removing rules all appears to be working as expected.
Comment 5 Martin Renvoize 2024-06-04 12:18:08 UTC
Created attachment 167393 [details] [review]
Bug 36672: Circulation rules are performing too many lookups

Looking at the template, for every section after the main rules block we loop over categories or itemtypes, and lookup the value for each rule name.

In systems with large numbers of categories and item types this becomes very slow.

In the rules section, we have already built a hash of rules by category and itemtype - we should continue to use this throughout the page.

Test Plan:
1) Apply this patch
2) For each rule section, create and delete a rule
3) No change in behavior should be noted!

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2024-06-04 12:18:51 UTC
Nice performance improvement, thanks.

Passing QA
Comment 7 Martin Renvoize 2024-06-10 15:27:22 UTC
Thanks for all the hard work!

Pushed to main for the next 24.11.00 release as RM Assistant
Comment 8 Lucas Gass 2024-06-27 22:04:32 UTC
Backported to 24.05.x for upcoming 24.05.02