Summary: | Slow checkouts, caused by many 'special_holidays' definitions | ||
---|---|---|---|
Product: | Koha | Reporter: | Mason James <mtj> |
Component: | Circulation | Assignee: | 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
Created attachment 39764 [details]
nytprof pic1
Created attachment 39765 [details]
nytprof pic1
attached some nytprof pix 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 i've confirmed this problem on master (3.21.00.003), with a similar execution time. Created attachment 39790 [details]
nytprof pic2
oops, attached wrong pic2 file
Comment on attachment 39764 [details]
nytprof pic1
incorrect file
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 (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? (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. 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. 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). *** This bug has been marked as a duplicate of bug 25723 *** 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> 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> Created attachment 106716 [details] [review] Bug 25723: Drop DateTime::Set from cpanfile Signed-off-by: Emma Perks <emma.perks@heartofengland.nhs.uk> 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> 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> 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> 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> 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> |