Bug 11112 - Koha::Calendar->new loads all holidays
Summary: Koha::Calendar->new loads all holidays
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 11061
Blocks:
  Show dependency treegraph
 
Reported: 2013-10-22 12:19 UTC by Jonathan Druart
Modified: 2015-07-14 15:57 UTC (History)
2 users (show)

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


Attachments
Bug 11112: Koha::Calendar needs some caching (4.25 KB, patch)
2013-10-22 12:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Benchmark scripts (1.29 KB, application/x-perl)
2013-10-22 12:25 UTC, Jonathan Druart
Details
Bug 11112: Koha::Calendar needs some caching (4.31 KB, patch)
2013-10-25 16:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED-QA] Bug 11112: Koha::Calendar needs some caching (4.37 KB, patch)
2013-11-19 05:53 UTC, Brendan Gallagher
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-10-22 12:19:37 UTC
Each time a new Koha::Calendar is created, all holidays are retrieved from the DB.
On a 3.8 installation, with 11000 holidays and without caching system, a checkin is extremely slow.
Comment 1 Jonathan Druart 2013-10-22 12:22:20 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2013-10-22 12:25:45 UTC
Created attachment 22268 [details]
Benchmark scripts

You can test the proposed patch with this script:
perl benchmarks.pl
git bz apply 11112
perl benchmarks.pl 1 # The parameter flag tells the patch is applied.

And compare the durations.
Comment 3 Kyle M Hall 2013-10-25 16:54:27 UTC Comment hidden (obsolete)
Comment 4 Brendan Gallagher 2013-11-19 05:53:29 UTC
Created attachment 23017 [details] [review]
[PASSED-QA] Bug 11112: Koha::Calendar needs some caching

holidays is built each time a Koha::Calendar object is created.

The Koha::Calendar's builder retrieves all holidays from the database
and create a DateTime::Set object with all holidays.

In one of our customer's DB, there are 11085 special_holidays and 598
repeatable_holidays.
On a return, there are 3 calls to Koha::Calendar->new.

Nytprof benchmarks (on a 3.8.x branch):
In DateTime::Set->from_datetimes:
3 times (5.49ms+4.90s) by Koha::Calendar::_init at line 80 of Koha/Calendar.pm, avg 1.63s/call
on a total of 7.67s (of 10.2s), executing 6353333 statements and 3031273 subroutine calls in 147 source files and 36 string evals.
for the circulation/return.pl page.

Comparing the access_log:

Without the patch:
checkout: time=2759838
checkin: time=1832751

Without the patch and with overdues:
checkout: time=1086727 + time=1144706
checkin: time=3928854 (x2)

With the patch and overdues:
checkout: time=1077839 + time=1060886
checkin: time=2420898

Test plan:
- checkout an item with a return date < today
- checkin the item and verify the suspension period is well calculated
  (depending on the holidays).
- prove t/db_dependent/Holidays.t
- t/Calendar.t

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 5 Galen Charlton 2013-12-10 18:30:06 UTC
Pushed to master.  Thanks, Jonathan!

Note that I've pushed two follow-ups.  One just repairs breakage of the add_holidays() method; the other adds a FIXME.

The FIXME is important: using of package-level variables for caching is not compatible with persistence engines.  However, I'm allowing this regression on the basis that I'll be resurrecting the patches for bug 8089.
Comment 6 Fridolin Somers 2013-12-24 11:08:46 UTC
Patches pushed to 3.14.x, will be in 3.14.2.