Bug 11112

Summary: Koha::Calendar->new loads all holidays
Product: Koha Reporter: Jonathan Druart <jonathan.druart+koha>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart+koha>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, gmcharlt
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12801
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14522
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 11061    
Bug Blocks:    
Attachments: Bug 11112: Koha::Calendar needs some caching
Benchmark scripts
Bug 11112: Koha::Calendar needs some caching
[PASSED-QA] Bug 11112: Koha::Calendar needs some caching

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.