Bug 14315 - Slow checkouts, caused by many 'special_holidays' definitions
Summary: Slow checkouts, caused by many 'special_holidays' definitions
Status: RESOLVED DUPLICATE of bug 25723
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Mason James
QA Contact: Testopia
URL:
Keywords:
Depends on: 14522 18519
Blocks:
  Show dependency treegraph
 
Reported: 2015-06-02 10:43 UTC by Mason James
Modified: 2020-07-09 12:21 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
nytprof pic1 (108.67 KB, image/png)
2015-06-02 10:46 UTC, Mason James
Details
nytprof pic1 (238.44 KB, image/png)
2015-06-02 10:47 UTC, Mason James
Details
nytprof pic2 (289.00 KB, image/png)
2015-06-03 03:17 UTC, Mason James
Details
Bug 25723: Drop DateTime::Set dependancy (4.55 KB, patch)
2020-07-09 12:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25723: (follow-up) Fix Unit Test (2.50 KB, patch)
2020-07-09 12:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25723: Drop DateTime::Set from cpanfile (824 bytes, patch)
2020-07-09 12:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25723: Use a hasref for lookup (3.87 KB, patch)
2020-07-09 12:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25723: Use the same code for single and exception holidays (5.94 KB, patch)
2020-07-09 12:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25723: Update cache flushing calls (6.66 KB, patch)
2020-07-09 12:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25723: Remove tests for removed method (3.05 KB, patch)
2020-07-09 12:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25723: (QA follow-up) Silent POD warning (1.32 KB, patch)
2020-07-09 12:21 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 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 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 2020-06-17 06:59:18 UTC

*** This bug has been marked as a duplicate of bug 25723 ***
Comment 14 Martin Renvoize 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 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 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 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 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 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 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 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>