Bug 36672 - Circulation rules are performing too many lookups
Summary: Circulation rules are performing too many lookups
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low minor
Assignee: Kyle M Hall (khall)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks: 37255 37263
  Show dependency treegraph
 
Reported: 2024-04-22 18:08 UTC by Nick Clemens (kidclamp)
Modified: 2024-12-05 15:57 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This improves the performance of the circulation and rules page by reducing the number of lookups. This should improve the page loading times (including when editing and saving) when a library has many categories and item types.
Version(s) released in:
24.11.00,24.05.02
Circulation function:


Attachments
Bug 36672: Circulation rules are performing too many lookups (3.56 KB, patch)
2024-04-26 17:51 UTC, Kyle M Hall (khall)
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 (khall)
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 (ashimema)
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 (khall) 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 (khall) 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 (ashimema) 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 (ashimema) 2024-06-04 12:18:51 UTC
Nice performance improvement, thanks.

Passing QA
Comment 7 Martin Renvoize (ashimema) 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 (lukeg) 2024-06-27 22:04:32 UTC
Backported to 24.05.x for upcoming 24.05.02
Comment 9 Andrii Nugged 2024-07-05 04:21:58 UTC
IMO We have a bug when elements with "All" (*) don't shown for the user and even die with a 500 error:


Because smart-rules.tt have this removed:

-    [% SET c = '*' UNLESS c.defined AND c != '' %]

      ...

-        [% SET i = '*' UNLESS i.defined AND i != '' %]


this makes wrong logic in 

    [% IF i == '*' %]
        <em>All</em>
and

    [% IF c == '*' %]
        <em>All</em>



If to return lines after "SET waiting_hold_cancellation..":

     [% SET waiting_hold_cancellation = all_rules.$c.$i.waiting_hold_cancellation %]

+    [% SET c = '*' UNLESS c.defined AND c != '' %]
+    [% SET i = '*' UNLESS i.defined AND i != '' %]

     [% IF ( waiting_hold_cancellation.defined && waiting_hold_cancellation != '' ) %]


It works normally, ... but I just did the preliminary test, so I suspect it.
I will test more.


But maybe my insight will give the authors a hint, and this can be confirmed faster (I am now in the middle of the travel).
Comment 10 Fridolin Somers 2024-07-17 13:44:08 UTC
I prefer not to backport on oldstable 23.11.x