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
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!
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!
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>
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.
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>
Nice performance improvement, thanks. Passing QA
Thanks for all the hard work! Pushed to main for the next 24.11.00 release as RM Assistant
Backported to 24.05.x for upcoming 24.05.02
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).
I prefer not to backport on oldstable 23.11.x