http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |14315 Assignee|gmcharlt@gmail.com |mtj@kohaaloha.com Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14315 [Bug 14315] Slow checkouts, caused by many 'special_holidays' definitions -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 --- Comment #1 from Mason James <mtj@kohaaloha.com> --- Created attachment 41050 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41050&action=edit Bug 14522 - Use Koha::Cache for accessing single_holidays() this patch adds Koha::Cache functionality to the 'single_holidays' table it is a performance patch for the problem described in BZ14315, only http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14315 it replaces slooow DateTime holiday objects with simple Ymd strings (19991230), then stores the strings in an @array using Koha::Cache it does not attempt to add caching to all holiday tables - just the single_holidays table (at this stage on my test (master-cd9a827); nytprof showed a time reduction of the single_holidays() sub - from 61.7s to 587ms here are some before/after nytprof runs, (really on master-cd9a827, not 3.20) http://x1.kohaaloha.com/i/nyt-bz14522-before/home-mason-g-k-3-20-x-Koha-Cale... http://x1.kohaaloha.com/i/nyt-bz14522-after/home-mason-g-k-3-20-x-Koha-Calen... to test... 1/ add a bunch of single_holidays to your test koha, (my table has 400 holiday rows) 2/ add a loong circ rule for an itemtype (my rule has 140 days) 3/ checkout an item to a user (took me 67 secs) apply patch... 4/ return item 5/ repeats steps 1..3, (took me 6 secs) 6/ add/change/delete some various single_holidays, via Home->Tools->Calendar ensure that your various changes have indeed saved correctly for extra points... 7/ run tests t/Calendar.t and t/db_dependent/Holidays.t, with all tests pass OK sudo koha-shell -c ' export PERL5LIB=/home/mason/g/k/master ; \ cd /home/mason/g/k/master ; perl t/Calendar.t ; perl t/db_dependent/Holidays.t ' testkoha 8/ run QA tool, with all tests pass OK sudo koha-shell -c ' \ export KOHA_CONF=/etc/koha/sites/mayo2/koha-conf.xml \ export PERL5LIB=/home/mason/g/k/master:/home/mason/qa-test-tools/ ; \ cd /home/mason/g/k/master ; perl /home/mason/qa-test-tools/koha-qa.pl -c 1 ' testkoha -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |8089 Referenced Bugs: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8089 [Bug 8089] Use Koha::Cache everywhere -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 Bob Birchall <bob@calyx.net.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bob@calyx.net.au -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 Zeno Tajoli <z.tajoli@cineca.it> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |z.tajoli@cineca.it Patch complexity|--- |Medium patch -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 --- Comment #3 from Mason James <mtj@kohaaloha.com> --- (In reply to Joonas Kylmälä from comment #2)

(In reply to Mason James from comment #1)

...

1/ add a bunch of single_holidays to your test koha, (my table has 400 holiday >rows)

Could you give some example table? It would be easier to test.

hmm, yes i could, but... you can quickly create a range of special/unique holidays in a few clicks ... from '2014-01-01' to '2016-01-01', would give you 730 holidays for testing i'll attach a pic

...

2/ add a loong circ rule for an itemtype (my rule has 140 days)

Do you mean loan period?

oops yes, thats what i mean :) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 --- Comment #5 from Mason James <mtj@kohaaloha.com> --- (In reply to Mason James from comment #3)

(In reply to Joonas Kylmälä from comment #2)

...

(In reply to Mason James from comment #1)

...

1/ add a bunch of single_holidays to your test koha, (my table has 400 holiday >rows)

Could you give some example table? It would be easier to test.

hmm, yes i could, but...

you can quickly create a range of special/unique holidays in a few clicks ... from '2014-01-01' to '2016-01-01', would give you 730 holidays for testing

i'll attach a pic

mysql> select count(*) from special_holidays where title = 'aaa' ; +----------+ | count(*) | +----------+ | 614 | +----------+ 1 row in set (0.00 sec) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 --- Comment #7 from Mason James <mtj@kohaaloha.com> --- (In reply to James from comment #6)

Problem in testing, when trying to create holidays using patch the date format is being read incorrectly resulting in the following message for a holiday range ending on 26th November:

Software error:

The 'month' parameter ("26") to DateTime::new did not pass the 'an integer between 1 and 12' callback at /usr/lib/perl5/DateTime.pm line 199 DateTime::new(undef, 'year', 2015, 'month', 26, 'day', 11) called at /home/koha/src/tools/newHolidays.pl line 67

Hmm, it looks like I might have only tested this patch with a 'Y-m-d' DateFormat syspref value. I'll take another look at this -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 --- Comment #9 from Mason James <mtj@kohaaloha.com> ---

..as this sequence of events is quite rare, and would only occur during deliberate testing - i think this bug is a effectively a WONTFIX

Sorry, when I say WONTFIX - I mean to say, the error that James experienced can be ignored for this bug i don't mean that this bug itself is a WONTFIX :) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 Nick Clemens <nick@quecheelibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41050|0 |1 is obsolete| | --- Comment #10 from Nick Clemens <nick@quecheelibrary.org> --- Created attachment 42851 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42851&action=edit Bug 14522 - Use Koha::Cache for accessing single_holidays() this patch adds Koha::Cache functionality to the 'single_holidays' table it is a performance patch for the problem described in BZ14315, only http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14315 it replaces slooow DateTime holiday objects with simple Ymd strings (19991230), then stores the strings in an @array using Koha::Cache it does not attempt to add caching to all holiday tables - just the single_holidays table (at this stage on my test (master-cd9a827); nytprof showed a time reduction of the single_holidays() sub - from 61.7s to 587ms here are some before/after nytprof runs, (really on master-cd9a827, not 3.20) http://x1.kohaaloha.com/i/nyt-bz14522-before/home-mason-g-k-3-20-x-Koha-Cale... http://x1.kohaaloha.com/i/nyt-bz14522-after/home-mason-g-k-3-20-x-Koha-Calen... to test... 1/ add a bunch of single_holidays to your test koha, (my table has 400 holiday rows) 2/ add a loong circ rule for an itemtype (my rule has 140 days) 3/ checkout an item to a user (took me 67 secs) apply patch... 4/ return item 5/ repeats steps 1..3, (took me 6 secs) 6/ add/change/delete some various single_holidays, via Home->Tools->Calendar ensure that your various changes have indeed saved correctly for extra points... 7/ run tests t/Calendar.t and t/db_dependent/Holidays.t, with all tests pass OK sudo koha-shell -c ' export PERL5LIB=/home/mason/g/k/master ; \ cd /home/mason/g/k/master ; perl t/Calendar.t ; perl t/db_dependent/Holidays.t ' testkoha 8/ run QA tool, with all tests pass OK sudo koha-shell -c ' \ export KOHA_CONF=/etc/koha/sites/mayo2/koha-conf.xml \ export PERL5LIB=/home/mason/g/k/master:/home/mason/qa-test-tools/ ; \ cd /home/mason/g/k/master ; perl /home/mason/qa-test-tools/koha-qa.pl -c 1 ' testkoha Signed-off-by: Nick Clemens <nick@quecheelibrary.org> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 Seth <smayo@stpeters.nsw.edu.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |smayo@stpeters.nsw.edu.au -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #42855|0 |1 is obsolete| | --- Comment #13 from Mason James <mtj@kohaaloha.com> --- Comment on attachment 42855 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42855 Bug 14522 - Use Koha::Cache for accessing single_holidays() oops -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 --- Comment #15 from Mason James <mtj@kohaaloha.com> --- (In reply to Nick Clemens from comment #11)

Test plan and all tests pass. QA test tools fail for pod in Koha::Calendar.pm

Line 97 looks like you said =c when you meant =cut

thanks, i attached a new patch to fix that.. mason@xen1:~/g/k/master$ /home/mason/git/koha-qa-tools/koha-qa.pl -c 2 testing 2 commit(s) (applied to f38f8b8 'Fixing release notes') Processing files before patches |========================>| 5 / 5 (100.00%) Processing files after patches |========================>| 5 / 5 (100.00%) OK Koha/Calendar.pm OK C4/Calendar.pm OK t/Calendar.t OK t/db_dependent/Holidays.t OK tools/newHolidays.pl -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 --- Comment #17 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 42888 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42888&action=edit Bug 14522 [QA Followup] - Koha complains about missing C4::Dates on master -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 --- Comment #18 from Kyle M Hall <kyle.m.hall@gmail.com> --- My testing has not shown any regressions, but I've been unable to prove any speed improvement. Maston, can you provide a unit test or even just a perl script to demonstrate the speedup? I set up about 350 holidays and tried checkout periods of 180 and 9999 days. Both were about the same speed with and without the patches respectively. I do have memcached installed and configured for Koha. memcdump --servers localhost koha_kohaqaismemcached koha_kohaqasingle_holidays koha_kohaqaAuthorisedValues-BOR_NOTES--0-CPL koha_kohaqaColumnsSettingsYaml koha_kohaqakohaconf Looks like they are getting cached! -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 --- Comment #20 from Kyle M Hall <kyle.m.hall@gmail.com> ---

...

I set up about 350 holidays and tried checkout periods of 180 and 9999 days. Both were about the same speed with and without the patches respectively.

your circ speed will only be slow if you test against a patron who has a branch with many holidays *and* an item with a matching long loan period rule

Confirmed! I think the issue was my rule was selecting a library other than the one my holidays were set for! -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #42851|0 |1 is obsolete| | Attachment #42887|0 |1 is obsolete| | Attachment #42888|0 |1 is obsolete| | --- Comment #21 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 42919 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42919&action=edit [PASSED QA] Bug 14522 - Use Koha::Cache for accessing single_holidays() this patch adds Koha::Cache functionality to the 'single_holidays' table it is a performance patch for the problem described in BZ14315, only http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14315 it replaces slooow DateTime holiday objects with simple Ymd strings (19991230), then stores the strings in an @array using Koha::Cache it does not attempt to add caching to all holiday tables - just the single_holidays table (at this stage on my test (master-cd9a827); nytprof showed a time reduction of the single_holidays() sub - from 61.7s to 587ms here are some before/after nytprof runs, (really on master-cd9a827, not 3.20) http://x1.kohaaloha.com/i/nyt-bz14522-before/home-mason-g-k-3-20-x-Koha-Cale... http://x1.kohaaloha.com/i/nyt-bz14522-after/home-mason-g-k-3-20-x-Koha-Calen... to test... 1/ add a bunch of single_holidays to your test koha, (my table has 400 holiday rows) 2/ add a loong circ rule for an itemtype (my rule has 140 days) 3/ checkout an item to a user (took me 67 secs) apply patch... 4/ return item 5/ repeats steps 1..3, (took me 6 secs) 6/ add/change/delete some various single_holidays, via Home->Tools->Calendar ensure that your various changes have indeed saved correctly for extra points... 7/ run tests t/Calendar.t and t/db_dependent/Holidays.t, with all tests pass OK sudo koha-shell -c ' export PERL5LIB=/home/mason/g/k/master ; \ cd /home/mason/g/k/master ; perl t/Calendar.t ; perl t/db_dependent/Holidays.t ' testkoha 8/ run QA tool, with all tests pass OK sudo koha-shell -c ' \ export KOHA_CONF=/etc/koha/sites/mayo2/koha-conf.xml \ export PERL5LIB=/home/mason/g/k/master:/home/mason/qa-test-tools/ ; \ cd /home/mason/g/k/master ; perl /home/mason/qa-test-tools/koha-qa.pl -c 1 ' testkoha Signed-off-by: Nick Clemens <nick@quecheelibrary.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 --- Comment #23 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 42921 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42921&action=edit [PASSED QA] Bug 14522 [QA Followup] - Koha complains about missing C4::Dates on master Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Change sponsored?|--- |Sponsored -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 --- Comment #28 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 43070 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=43070&action=edit Bug 14522: (QA followup) tidy tests Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 --- Comment #29 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 43071 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=43071&action=edit Bug 14522 [QA Followup] * Change POD to a brief comment * Fix UT so it doesn't fail on Sundays -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522 --- Comment #31 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #25)

Comment on attachment 42919 [details] [review] [PASSED QA] Bug 14522 - Use Koha::Cache for accessing single_holidays()

Review of attachment 42919 [details] [review]: -----------------------------------------------------------------

It seems we were here at the same time :)

I have also tried to reproduce the time reduction, without success. A script I have used is coming...

It would have been good to have a script to highlight the time reduction of this caching. -- You are receiving this mail because: You are watching all bug changes.