more info to come
Created attachment 41050 [details] [review] 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-Calendar-pm-1485-line.html#237 http://x1.kohaaloha.com/i/nyt-bz14522-after/home-mason-g-k-3-20-x-Koha-Calendar-pm-1485-line.html#280 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
(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. >2/ add a loong circ rule for an itemtype (my rule has 140 days) Do you mean loan period?
(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 :)
Created attachment 42255 [details] pic: create range of holidays
(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)
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
(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
(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, this bug seems to be caused by... 1/ loading the /cgi-bin/koha/tools/holidays.pl page 2/ changing the 'dateformat' syspref 3/ creating a new holiday, without first reloading the existing holidays.pl page ..as this sequence of events is quite rare, and would only occur during deliberate testing - i think this bug is a effectively a WONTFIX FYI: this bug is reproducible without this patch, and has likely existed since the introduction of the calendar/holidays feature in 2006.
> ..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 :)
Created attachment 42851 [details] [review] 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-Calendar-pm-1485-line.html#237 http://x1.kohaaloha.com/i/nyt-bz14522-after/home-mason-g-k-3-20-x-Koha-Calendar-pm-1485-line.html#280 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>
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
Created attachment 42855 [details] [review] 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-Calendar-pm-1485-line.html#237 http://x1.kohaaloha.com/i/nyt-bz14522-after/home-mason-g-k-3-20-x-Koha-Calendar-pm-1485-line.html#280 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: Mason James <mtj@kohaaloha.com>
Comment on attachment 42855 [details] [review] Bug 14522 - Use Koha::Cache for accessing single_holidays() oops
Created attachment 42856 [details] [review] fix =cut typo
(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
Created attachment 42887 [details] [review] Bug 14522 [Followup] - fix =cut typo
Created attachment 42888 [details] [review] Bug 14522 [QA Followup] - Koha complains about missing C4::Dates on master
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!
(In reply to Kyle M Hall from comment #18) > 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 have neither of those > 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
> > 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!
Created attachment 42919 [details] [review] [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-Calendar-pm-1485-line.html#237 http://x1.kohaaloha.com/i/nyt-bz14522-after/home-mason-g-k-3-20-x-Koha-Calendar-pm-1485-line.html#280 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>
Created attachment 42920 [details] [review] [PASSED QA] Bug 14522 [Followup] - fix =cut typo http://bugs.koha-community.org/show_bug.cgi?id=14522 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 42921 [details] [review] [PASSED QA] Bug 14522 [QA Followup] - Koha complains about missing C4::Dates on master Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 42922 [details] [review] [PASSED QA] Bug 14522 [QA Followup] - Remove $single_holidays global Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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... ::: Koha/Calendar.pm @@ +94,5 @@ > my $branchcode = $self->{branchcode}; > + my $cache = Koha::Cache->get_instance(); > + my $single_holidays = $cache->get_from_cache('single_holidays'); > + > +=c Should be a simple comment, no need to put it in the pod. @@ +395,5 @@ > + $single_holidays->{'MPL'} = [$ymd]; > + $cache->set_in_cache( 'single_holidays', $single_holidays, 76800 ); > + > + # ...but *dont* reset the cache, as this holiday was not really written to the db > + # its only used to mock a holiday insert for 1 test in t/db_dependent/Holidays.t Should not be a method here, but moved to the test file or a test module.
Created attachment 42926 [details] Script to try and reproduce the problem
I like the implementation (overall) but I don't think we should introduce things like Koha::Calendar::add_dummy_holiday. Worse, it relies on the 'MPL' branch existing on the database.
Created attachment 43070 [details] [review] Bug 14522: (QA followup) tidy tests Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Created attachment 43071 [details] [review] Bug 14522 [QA Followup] * Change POD to a brief comment * Fix UT so it doesn't fail on Sundays
Patches pushed to master. Thanks Mason and Kyle!
(In reply to Jonathan Druart from comment #25) > Comment on attachment 42919 [details] [review] [review] > [PASSED QA] Bug 14522 - Use Koha::Cache for accessing single_holidays() > > Review of attachment 42919 [details] [review] [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.