Bug 14315

Summary: Slow checkouts, caused by many 'special_holidays' definitions
Product: Koha Reporter: Mason James <mtj>
Component: CirculationAssignee: Mason James <mtj>
Status: RESOLVED DUPLICATE QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: gmcharlt, irma, jonathan.druart, kyle.m.hall, magnus, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 14522, 18519    
Bug Blocks:    
Attachments: nytprof pic1
nytprof pic1
nytprof pic2
Bug 25723: Drop DateTime::Set dependancy
Bug 25723: (follow-up) Fix Unit Test
Bug 25723: Drop DateTime::Set from cpanfile
Bug 25723: Use a hasref for lookup
Bug 25723: Use the same code for single and exception holidays
Bug 25723: Update cache flushing calls
Bug 25723: Remove tests for removed method
Bug 25723: (QA follow-up) Silent POD warning

Description Mason James 2015-06-02 10:43:23 UTC
i’ve been poking at a problematic Koha with slow checkout speed, that has around 400 special_holidays defined - only 50 special_holidays are ‘current’. 

i define ’current’ as holidays that fall after the issue_date, 

i’ve narrowed the problem down to Koha doing much slow date math on all the holiday rows in the special_holidays table, during a checkout

it seems for some situations (like checkouts) Koha only needs to process current holidays, and can safely skip older holidays for processing

to me, the most obvious solution seems to be to allow the Koha::Calendar::single_holidays() sub to be passed an ‘issue_date’ arg

this would allow single_holidays() to skip any holiday rows older than the $issue_date, for processing

Q: is there a better way for single_holidays() to know the issue_date, other than being passed it? 


i’ve made some hardcoded experiments with the single_holidays() sub skipping old holidays, and seen a 65% improvement to checkout times

tested in 3.20.0
Comment 1 Mason James 2015-06-02 10:46:15 UTC Comment hidden (obsolete)
Comment 2 Mason James 2015-06-02 10:47:48 UTC
Created attachment 39765 [details]
nytprof pic1
Comment 3 Mason James 2015-06-02 10:48:38 UTC
attached some nytprof pix
Comment 4 Mason James 2015-06-02 11:15:13 UTC
Hmm, one other detail I forgot to add is that the item I was testing with had a circ-rule with a large ‘loan period’ value.

The combination of a large number of defined special_holidays (400), and a large ‘loan period’ (280) seems to exacerbate this problem
Comment 5 Mason James 2015-06-02 11:26:10 UTC
i've confirmed this problem on master (3.21.00.003), with a similar execution time.
Comment 6 Mason James 2015-06-03 03:17:13 UTC
Created attachment 39790 [details]
nytprof pic2

oops, attached wrong pic2 file
Comment 7 Mason James 2015-06-03 04:02:27 UTC
Comment on attachment 39764 [details]
nytprof pic1

incorrect file
Comment 8 Mason James 2015-07-17 04:06:57 UTC
Update: A new bug has been created, that contains a patch for this problem...

  http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522
Comment 9 Jonathan Druart 2016-05-04 19:26:10 UTC
(In reply to Mason James from comment #8)
> Update: A new bug has been created, that contains a patch for this problem...
> 
>   http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522

So can we close this one?
Comment 10 Magnus Enger 2018-11-15 13:08:53 UTC
(In reply to Jonathan Druart from comment #9)
> (In reply to Mason James from comment #8)
> > Update: A new bug has been created, that contains a patch for this problem...
> > 
> >   http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522
> 
> So can we close this one?

14522 is CLOSED FIXED now.
Comment 11 Martin Renvoize (ashimema) 2020-06-12 10:06:12 UTC
I think this is still somewhat of an issue.. in my experience we've now worked through the IO bound performance issue with the caching bugs, but we still have some CPU bound issues relating to the slow instantiation speed of DateTime.
Comment 12 Martin Renvoize (ashimema) 2020-06-15 08:17:59 UTC
I've linked two bugs to this report; The first converts the 'exception holiday' handling to match that of 'special holiday' and removes DateTime::Set as a dependency (This gives a marked performance boost when exception holidays are numerous). The second updates the 'special holiday' handing to utilise a hashref lookup instead of an array iteration (This gives a marked performance boost when a series of special holidays are defined, especially if many of them are in the past, as we now do a single comparison as opposed to having to iterate from oldest defined holiday until we find the date in question, or no date at all).
Comment 13 Martin Renvoize (ashimema) 2020-06-17 06:59:18 UTC

*** This bug has been marked as a duplicate of bug 25723 ***
Comment 14 Martin Renvoize (ashimema) 2020-07-09 12:20:36 UTC
Created attachment 106714 [details] [review]
Bug 25723: Drop DateTime::Set dependancy

This patch removes our use of DateTime::Set and replaces it with a quick
hashref implimentation

Signed-off-by: Emma Perks <emma.perks@heartofengland.nhs.uk>
Comment 15 Martin Renvoize (ashimema) 2020-07-09 12:20:39 UTC
Created attachment 106715 [details] [review]
Bug 25723: (follow-up) Fix Unit Test

The unit test previously relied on the internals of exception_holidays.

Signed-off-by: Emma Perks <emma.perks@heartofengland.nhs.uk>
Comment 16 Martin Renvoize (ashimema) 2020-07-09 12:20:43 UTC
Created attachment 106716 [details] [review]
Bug 25723: Drop DateTime::Set from cpanfile

Signed-off-by: Emma Perks <emma.perks@heartofengland.nhs.uk>
Comment 17 Martin Renvoize (ashimema) 2020-07-09 12:20:47 UTC
Created attachment 106717 [details] [review]
Bug 25723: Use a hasref for lookup

Rather than using an array of dates and iterating them, use a hashref
and do a direct lookup

Signed-off-by: Emma Perks <emma.perks@heartofengland.nhs.uk>
Comment 18 Martin Renvoize (ashimema) 2020-07-09 12:20:50 UTC
Created attachment 106718 [details] [review]
Bug 25723: Use the same code for single and exception holidays

This patch refactors is_exception_holiday and is_single_holiday to
utilise a single _holidays method which combines the logic of the
previous single_holidays and exception_holidays methods.

As Koha::Calendar is instantiated at a branch level, we also move split
the cache into branches too.

Signed-off-by: Emma Perks <emma.perks@heartofengland.nhs.uk>
Comment 19 Martin Renvoize (ashimema) 2020-07-09 12:20:53 UTC
Created attachment 106719 [details] [review]
Bug 25723: Update cache flushing calls

This patch updates the previous single_holidays and exeption_holidays
cache flushing calls to match the new cache key structure of the updated
routines.

Signed-off-by: Emma Perks <emma.perks@heartofengland.nhs.uk>
Comment 20 Martin Renvoize (ashimema) 2020-07-09 12:20:57 UTC
Created attachment 106720 [details] [review]
Bug 25723: Remove tests for removed method

We removed the exception_holidays (and single_holidays) methods as part
of this patchset. As such, we should no longer test them ;)

Signed-off-by: Emma Perks <emma.perks@heartofengland.nhs.uk>
Comment 21 Martin Renvoize (ashimema) 2020-07-09 12:21:00 UTC
Created attachment 106721 [details] [review]
Bug 25723: (QA follow-up) Silent POD warning

I didn't introduce the method that is warned about, but adding POD for
it seemed reasonable and will silence the QA Script warning.

Signed-off-by: Emma Perks <emma.perks@heartofengland.nhs.uk>