Simply put we have a lot of SET lines and such in our circ rules editor template. For large rule sets all the newlines add up and can balloon the rendered html size to many megabytes! We should add a newline chomper to these lines.
Created attachment 165325 [details] [review] Bug 36668: Excessive whitespace/newlines in circulation rules editor may cause slow loads with large rules sets Simply put we have a lot of SET lines and such in our circ rules editor template. For large rule sets all the newlines add up and can balloon the rendered html size to many megabytes! We should add a newline chomper to these lines. 1) Generate a large number of circ rules with the following perl script use Koha::CirculationRules; use Koha::Patron::Categories; use Koha::ItemTypes; use t::lib::TestBuilder; my $builder = t::lib::TestBuilder->new(); $builder->build( { source => 'Category' } ) for 0 .. 80; $builder->build( { source => 'Itemtype' } ) for 0 .. 80; C4::Context->dbh->do(" INSERT IGNORE INTO circulation_rules ( branchcode, categorycode, itemtype, rule_name, rule_value ) SELECT NULL, categories.categorycode, itemtypes.itemtype, 'issuelength', 5 FROM categories JOIN itemtypes;" ); 2) Open the network debugging on your web browser 3) Load smart-rules.pl 4) Notice the size of the file 5) Apply this patch 6) Restart all the things! 7) Reload smart-rules.pl 8) Note the file size has been drastically reduced!
Created attachment 166107 [details] [review] Bug 36668: Excessive whitespace/newlines in circulation rules editor may cause slow loads with large rules sets Simply put we have a lot of SET lines and such in our circ rules editor template. For large rule sets all the newlines add up and can balloon the rendered html size to many megabytes! We should add a newline chomper to these lines. 1) Generate a large number of circ rules with the following perl script use Koha::CirculationRules; use Koha::Patron::Categories; use Koha::ItemTypes; use t::lib::TestBuilder; my $builder = t::lib::TestBuilder->new(); $builder->build( { source => 'Category' } ) for 0 .. 80; $builder->build( { source => 'Itemtype' } ) for 0 .. 80; C4::Context->dbh->do(" INSERT IGNORE INTO circulation_rules ( branchcode, categorycode, itemtype, rule_name, rule_value ) SELECT NULL, categories.categorycode, itemtypes.itemtype, 'issuelength', 5 FROM categories JOIN itemtypes;" ); 2) Open the network debugging on your web browser 3) Load smart-rules.pl 4) Notice the size of the file 5) Apply this patch 6) Restart all the things! 7) Reload smart-rules.pl 8) Note the file size has been drastically reduced! Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Hey Kyle, some testing notes (thanks for the test script btw A+): Before patch: Time: 22-26 seconds / Size: 911kB After patch: Time: 20-25 seconds / Size: 530kB From my testing, this patch reduces the page size, but it does not fix the "slow load" issue. My suggestion to fix this for the future would be to implement infinite scroll on this page, i.e. additional circulation rules entries are loaded as the user navigates downwards (would say it's likely not trivial for this page and it may introduce issues I'm not considering atm). Bug 36641 is adding a circulation rules REST endpoint and although I don't think it would directly help for this specific use case it would be a starting point. Sorry for the tangent, just something to (maybe) discuss!
(In reply to Pedro Amorim from comment #3) > Hey Kyle, some testing notes (thanks for the test script btw A+): > > Before patch: > Time: 22-26 seconds / Size: 911kB > > After patch: > Time: 20-25 seconds / Size: 530kB > > From my testing, this patch reduces the page size, but it does not fix the > "slow load" issue. 36672 should help with the slow load - it turns out we are fetching individual rules for later sections of the page, rather than using the ones already fetched in the script > My suggestion to fix this for the future would be to implement infinite > scroll on this page, i.e. additional circulation rules entries are loaded as > the user navigates downwards (would say it's likely not trivial for this > page and it may introduce issues I'm not considering atm). > Bug 36641 is adding a circulation rules REST endpoint and although I don't > think it would directly help for this specific use case it would be a > starting point. > Sorry for the tangent, just something to (maybe) discuss! Interesting, but slightly frightening
Unfortanuetly, this one no longer applies cleanly to main.