Bug 15333

Summary: Use Koha::Cache for caching all holidays
Product: Koha Reporter: Mason James <mtj>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart+koha>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: abl, brendan, florent.mara, jonathan.druart+koha, julian.maurice, m.de.rooy, martin.renvoize
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 14522    
Bug Blocks: 7172    
Attachments: Bug 15333: Use Koha::Cache to cache exception_holidays instead of a package variable
Bug 15333: Use Koha::Cache to cache exception_holidays instead of a package variable
Bug 15333: [QA Follow-up] Add few tests

Description Mason James 2015-12-08 20:18:32 UTC
Now that bug 14522 is pushed to master, this follow-up bug is to add Koha::Cache style caching for all 'holiday' subs()
Comment 1 Martin Renvoize 2015-12-08 21:18:22 UTC
Fantastic that your looking into this mason. The patches on the prior bug go a long way to improving performance.. Nit only by reducing database hits by caching but also by reducing DateTime instantiations within loops, which are a well known performance bottleneck. So pleased to see work going on in this area.
Comment 2 Martin Renvoize 2015-12-09 08:53:44 UTC
Whilst we're on performance, I'd be interested to see if there was any discernible difference in using List::Utils any function over the loop found around line 140 of Koha::Calendar?

Anyone fancy testing that..

use List::Utils qw/any/; #(It's in core perl in more recent releases anyways)

return (any { /$date/ } @$holidays);

They do exactly the same thing, only I think List::Utils functions does so in c instead of perl so I 'think' is usually quicker ;)
Comment 3 Jonathan Druart 2016-05-04 19:39:51 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-05-04 19:40:57 UTC
Note that we won't add any performance gain with this patch (on the contrary), but at least we will get rid of possible weird behaviors.
Comment 5 Florent Mara 2016-05-17 00:20:20 UTC
Small patch. No apparent adverse effect. 
(No UT or test plan)
Comment 6 Marcel de Rooy 2016-05-20 08:33:19 UTC
Created attachment 51637 [details] [review]
Bug 15333: Use Koha::Cache to cache exception_holidays instead of a package variable

On the same way as bug 14522, we should use Koha::Cache to cache
exception_holidays.
It's not safe to use a package variable if running under Plack.

There is not test plan, just make sure the changes make sense.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2016-05-20 08:33:25 UTC
Created attachment 51638 [details] [review]
Bug 15333: [QA Follow-up] Add few tests

This report had no test plan and no unit tests.
Adding some lines to Holidays.t.
Added a trivial line move in Calendar.pm.

Test plan:
Run t/db_dependent/Holidays.t with and without cache.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested with memcached, Cache::Memory and no cache (edit Cache.pm).
Comment 8 Brendan Gallagher 2016-05-25 22:19:03 UTC
Pushed to master - Should be in the May 2016 Release.  Thanks!
Comment 9 Julian Maurice 2016-06-09 07:34:52 UTC
Patches pushed to 3.22.x, will be in 3.22.8