[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Attachment #162538|0 |1 is obsolete|| --- Comment #44 from David Gustafsson --- Created attachment 162539 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162539=edit Bug 32476: Fix Circulation tests and exclude cache param from cache key -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Status|Failed QA |Needs Signoff --- Comment #43 from David Gustafsson --- Circulation tests should now pass again, also fixed a bug where the cache parameter was included in the cache key causing issues clearing the method cache. -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #42 from David Gustafsson --- Created attachment 162538 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162538=edit Bug 32476: Fix Circulation tests and exclude cache param from cache key -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Attachment #157259|0 |1 is obsolete|| --- Comment #41 from David Gustafsson --- Created attachment 162537 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162537=edit Bug 32476: Add support for caching methods with arguments -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Emily Lamancusa changed: What|Removed |Added Status|BLOCKED |Failed QA --- Comment #40 from Emily Lamancusa --- Bug 35133 is now in Passed QA status, so this bug is no longer blocked. It does cause test failure in t/db_dependent/Circulation.t, though :( # Failed test '(Bug 8236), Cannot renew, this item is not overdue but patron has overdues' # at t/db_dependent/Circulation.t line 738. # got: '1' # expected: '0' # Failed test 'Correct error returned' # at t/db_dependent/Circulation.t line 739. # got: undef # expected: 'overdue' # Failed test '(Bug 8236), Cannot renew, this item is overdue so patron has overdues' # at t/db_dependent/Circulation.t line 741. # got: '1' # expected: '0' # Failed test 'Correct error returned' # at t/db_dependent/Circulation.t line 742. # got: undef # expected: 'overdue' -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Status|Needs Signoff |BLOCKED -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Depends on||35133 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35133 [Bug 35133] Accessors defined in AUTOLOAD does not work if called with SUPER -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Attachment #157258|0 |1 is obsolete|| --- Comment #39 from David Gustafsson --- Created attachment 157259 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157259=edit Bug 32476: Add support for caching methods with arguments -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #38 from David Gustafsson --- Created attachment 157258 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157258=edit Bug 32476: Add support for cachig methods with arguments -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #37 from David Gustafsson --- I just realized that Bug 29145 adds another passed option to has_overdues, so caching has to take possible other parameters into account. I refactored the patch to also include hashed arguments in the cache key. -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #36 from David Gustafsson --- (In reply to Fridolin Somers from comment #32) > I see Bug 30860 has done a similar job. > We should use the same way in my opinion. I had a look at the patch and while far from perfect, I think the current solution is preferable. It's also not obvious to me how to refactor this patched based on the caching methodology used in Bug 30860. -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #35 from David Gustafsson --- (In reply to Kyle M Hall from comment #28) > I really like this conceptually. The suggestion I would make is to reverse > the use of the cache param. Only require "cache => 0" to not use the cache. > You already have cache invalidation to the only time it's needed is in > _accessor_cache This could be done, and it would probably work, but I opted for conservatism as there is in practical terms no benefit in terms of performance using cache for the methods in places where they are called just a few times, and a non zero chance of introducing some subtle cache related bug. While not 100% I'm pretty sure I caught all the performance critical cases, and if we find any more it's simple to just cache them as well. I think the current solution is perhaps a little bit too over engineered for just caching these two methods in a few places, but perhaps also does not hurt to have a more standardized way of implementing per instance caching if more performance critical methods are added ore discovered in the future. -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Status|Patch doesn't apply |Needs Signoff --- Comment #34 from David Gustafsson --- Rebased against master. -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Attachment #150779|0 |1 is obsolete|| --- Comment #33 from David Gustafsson --- Created attachment 157241 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157241=edit Bug 32476: Add caching for relatively expensive patron methods To test: 1) Ensure tests in t/db_dependant/Koha/Patrons.t all pass Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #32 from Fridolin Somers --- I see Bug 30860 has done a similar job. We should use the same way in my opinion. -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Fridolin Somers changed: What|Removed |Added See Also||https://bugs.koha-community ||.org/bugzilla3/show_bug.cgi ||?id=30860 -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #31 from Fridolin Somers --- (In reply to Jonathan Druart from comment #27) > (In reply to Fridolin Somers from comment #26) > > Interresting. > > > > Maybe a silly question : > > why not caching entire Koha::Patron object in > > CanBookBeReserved/CanItemBeReserve ? > > "entire"? What do you mean? The unblessed/hash version? Never mind, I bet the most important methods have been tracked down -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Nick Clemens changed: What|Removed |Added CC||n...@bywatersolutions.com --- Comment #30 from Nick Clemens --- I like this too, but it does seem to make things a little more confusing to my mind when reading patron code. I wonder if it might be simpler to add $patron->can_renew / $patron->can_auto_renew functions, cached in the L1 cache, which check both the system preferences and patron values that are needed to make this determination. We could either call these in CanBookBeRenewed/_CanBookBeAutoRenewed - or avoid the calls all together: my ( $can_renew, $can_renew_error, $info ) = CanBookBeRenewed( $checkout_obj->patron, $checkout_obj ) if $patron->can_renew(); -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Emily Lamancusa changed: What|Removed |Added CC||emily.lamancusa@montgomeryc ||ountymd.gov -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476
--- Comment #29 from Kyle M Hall ---
I don't want to derail this bug, but an interesting follow-up would to be use
replace manually implementing the caching for each method with decorators like
https://metacpan.org/pod/Class::Decorator or
https://metacpan.org/pod/Python::Decorator
Then, enabling caching for a given method would be as simple as changing
sub is_expired {
to
@cached
sub is_expired {
--
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Kyle M Hall changed: What|Removed |Added CC||k...@bywatersolutions.com Status|Needs Signoff |Patch doesn't apply --- Comment #28 from Kyle M Hall --- I really like this conceptually. The suggestion I would make is to reverse the use of the cache param. Only require "cache => 0" to not use the cache. You already have cache invalidation to the only time it's needed is in _accessor_cache -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #27 from Jonathan Druart --- (In reply to Fridolin Somers from comment #26) > Interresting. > > Maybe a silly question : > why not caching entire Koha::Patron object in > CanBookBeReserved/CanItemBeReserve ? "entire"? What do you mean? The unblessed/hash version? -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Jonathan Druart changed: What|Removed |Added See Also||https://bugs.koha-community ||.org/bugzilla3/show_bug.cgi ||?id=34178 -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Fridolin Somers changed: What|Removed |Added CC||fridolin.som...@biblibre.co ||m --- Comment #26 from Fridolin Somers --- Interresting. Maybe a silly question : why not caching entire Koha::Patron object in CanBookBeReserved/CanItemBeReserve ? -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Jonathan Druart changed: What|Removed |Added Blocks||33746 Depends on|33746 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33746 [Bug 33746] [omnibus] speed improvement -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Mason James changed: What|Removed |Added CC||m...@kohaaloha.com -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Jonathan Druart changed: What|Removed |Added Depends on||33746 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33746 [Bug 33746] [omnibus] speed improvement -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Jonathan Druart changed: What|Removed |Added See Also||https://bugs.koha-community ||.org/bugzilla3/show_bug.cgi ||?id=32894 -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Attachment #150778|0 |1 is obsolete|| --- Comment #25 from David Gustafsson --- Created attachment 150779 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150779=edit Bug 32476: Add caching for relatively expensive patron methods To test: 1) Ensure tests in t/db_dependant/Koha/Patrons.t all pass Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Attachment #147145|0 |1 is obsolete|| --- Comment #24 from David Gustafsson --- Created attachment 150778 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150778=edit Bug 32476: Add caching for relatively expensive patron methods To test: 1) Ensure tests in t/db_dependant/Koha/Patrons.t all pass Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476
--- Comment #23 from David Gustafsson ---
(In reply to Jonathan Druart from comment #22)
> Did you identify where they (is_expired and has_overdues) would be called
> extensively?
>
> Did you benchmark your patches?
>
> I can easily imagine a scenario where it would bring hard to detect
> side-effects:
> * check if a patron can check an item out
> => yes, cache "yes"
> * check the item out
> * check if the patron can check an other item out
> => Get "yes" from cache
Yes, I did, the main culprits are CanBookBeRenewed and _CanBookBeAutoRenewed
since there are cases where these are called for each issue of a patron,
accessing is_expired and has_overdues. For patrons with hundreds of issues the
cost adds up to about 3-4% of the total execution time. It might not sound like
much, but when applying some other perforamce fixes like bug 31735 and bug
32496 I think it's more like 6-8% (though I have not run that particular
benchmark).
Looking at the patch again I see that there are cached called being made in
some other subs where the benefit is neglectable to non existent (like
AddRenewal and AddReturn, CanBookBeIssued), that are mostly called once or a
few times, so I removed the cached calls in those cases.
There are to my understanding no cases in the current code base where this
patch could cause issues, and if you where to write a script to produce a
similar case you are describing one would explicitly have to call has_overdues
with the {cache => 1} argument after for example calling AddReturn for a
delayed item. Now that AddReturn no longer used the has_overdues the cache is
cleared on returns, so even if one where to do that you would not get a stale
value. But you where correct in pointing out that more care should be taken
where caching is used, so disabling it for subroutines which are in no need of
optimization makes total sense, and instead expiring the cache in those cases
minimize the risk of future bugs are introduced as a result of stale cache
values.
--
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Jonathan Druart changed: What|Removed |Added CC||jonathan.druart+koha@gmail. ||com --- Comment #22 from Jonathan Druart --- Did you identify where they (is_expired and has_overdues) would be called extensively? Did you benchmark your patches? I can easily imagine a scenario where it would bring hard to detect side-effects: * check if a patron can check an item out => yes, cache "yes" * check the item out * check if the patron can check an other item out => Get "yes" from cache -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Attachment #147148|0 |1 is obsolete|| -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #21 from David Gustafsson --- Created attachment 147148 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=147148=edit Bug 32092: fix tests -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Status|Patch doesn't apply |Needs Signoff --- Comment #20 from David Gustafsson --- Rebased against master. -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Attachment #145037|0 |1 is obsolete|| --- Comment #19 from David Gustafsson --- Created attachment 147145 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=147145=edit Bug 32476: Add caching for relatively expensive patron methods To test: 1) Ensure tests in t/db_dependant/Koha/Patrons.t all pass Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Olivier Hubert changed: What|Removed |Added CC||olivier.hub...@inlibro.com Status|Needs Signoff |Patch doesn't apply --- Comment #18 from Olivier Hubert --- Applying: Bug 32476: Add caching for relatively expensive patron methods Using index info to reconstruct a base tree... M C4/Circulation.pm M Koha/Patron.pm Falling back to patching base and 3-way merge... Auto-merging Koha/Patron.pm Auto-merging C4/Circulation.pm CONFLICT (content): Merge conflict in C4/Circulation.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 32476: Add caching for relatively expensive patron methods -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Attachment #145036|0 |1 is obsolete|| --- Comment #17 from David Gustafsson --- Created attachment 145037 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145037=edit Bug 32476: Add caching for relatively expensive patron methods To test: 1) Ensure tests in t/db_dependant/Koha/Patrons.t all pass Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Attachment #144771|0 |1 is obsolete|| --- Comment #16 from David Gustafsson --- Created attachment 145036 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145036=edit Bug 32476: Add caching for relatively expensive patron methods To test: 1) Ensure tests in t/db_dependant/Koha/Patrons.t all pass Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #15 from David Gustafsson --- Sure, personally I don't see major issues using closures if resulting in less boiler plate and thus less error prone code. But using your suggestion allows for flexibility with regards to flushing cache on uncached calls, so I guess that could be a good thing. A downside is that it would lead to more convoluted code if wishing to cache methods with arguments. But perhaps in those cases caching should be handled manually anyways. I slightly modified your suggesting calling different methods instead of relying on arguments to dispatch to different behaviors (clearing/retrieving cache), but should otherwise be equivalent. -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #13 from David Gustafsson --- Renamed _cached_accessor to _maybe_cached as the former implies a function is returned. -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Attachment #144770|0 |1 is obsolete|| --- Comment #12 from David Gustafsson --- Created attachment 144771 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144771=edit Bug 32476: Add caching for relatively expensive patron methods To test: 1) Ensure tests in t/db_dependant/Koha/Patrons.t all pass Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Attachment #144744|0 |1 is obsolete|| --- Comment #11 from David Gustafsson --- Created attachment 144770 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144770=edit Bug 32476: Add caching for relatively expensive patron methods To test: 1) Ensure tests in t/db_dependant/Koha/Patrons.t all pass Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #10 from David Gustafsson --- Ok! My bad, now think I understand. There is an issue with your suggestion though, if I'm not still misinterpreting parts of it. The current behavior is to clear the cache if the accessor method is called without the cache parameter set to true. This is if course slightly ugly imho, as not at all obvious without looking at the code. The idea is that calling the accessor uncached should be done either when caching doesn't matter, or when the cached value could be stale. If the cache was not cleared subsequent cached calls could return a stale value, resulting in subtle bugs that could be difficult to track down. An alternative could be to always enter values into the cache regardless if caching is enabled or not, but this is also not what one would expect and it feels more intuitive that caching in that case is skipped altogether. I just can't see how to this behavior using your suggestion, but if there is something I'm missing perhaps you could provide a full example. I created a new version which avoids the code duplication of the caching behavior based on the previous version but instead using a class property for storing cached values as you suggested. It's a little bit more opaque than the original version, but the upside is that caching behavior is generalized and the code duplication can be avoided. -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #9 from David Cook --- I still think the caching is a good idea, and I'm not saying my way is the only way. Using Koha::Cache::Memory::Lite was just a suggestion/thought to explore. I didn't mean that you had to change to using it. I think it has its pros and cons. As you say, it's only cleared in a few places. This might not be the right place to use it. I haven't thought it 100% through. But I don't think I'd be alone in thinking there is too much copy/pasted code in these patches. I don't like this terminology but it "smells wrong". Very possible that other people will disagree with me there though! -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476
--- Comment #8 from David Cook ---
(In reply to David Gustafsson from comment #7)
> I missed the code example you posted, I don't think that method would work
> as I think you missed the case where we get a cache miss and need to
> retrieve the value.
I've implied that the "cache" method looks up the value by using the key. Take
the following example:
return $self->cache({ key => 'is_expired' }) if $param->{cache};
In the "cache" method, you'd check the cache and if there is a cache miss,
you'd do something like the following:
my $accessor_method = $args->{key};
my $value = $self->$accessor_method();
You don't need to use a closure like in your example. It's very minimal code.
(Note it would also live in Koha::Object and be inherited into Koha::Patron and
other friends.)
--
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #7 from David Gustafsson --- I missed the code example you posted, I don't think that method would work as I think you missed the case where we get a cache miss and need to retrieve the value. To abstract the cache logic I think we have to isolate the retrieval of the uncached value, in a closure for example, as the code above. -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Attachment #144627|0 |1 is obsolete|| --- Comment #6 from David Gustafsson --- Created attachment 144744 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144744=edit Bug 32476: Add caching for relatively expensive patron methods To test: 1) Ensure tests in t/db_dependant/Patrons.t all pass Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #5 from David Gustafsson --- *so the entire cache is purged if ->flush is called somewhere else -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476
--- Comment #4 from David Gustafsson ---
I don't think it is possibly to reduce code duplication without sacrificing
readability and/or increasing code complexity. The only way I can think of at
the top of my head would be to have a general method "handle_cache_lookup" (or
some better name) that takes a cache_key and and callback for getting the
uncached value, something like this:
sub is_expired {
my ($self, $params) = @_;
my $get_is_expired = sub {
my $is_expired =
$self->dateexpiry &&
$self->dateexpiry !~ '^' &&
dt_from_string( $self->dateexpiry ) < dt_from_string->truncate( to =>
'day' );
return $is_expired ? 1 : 0;
}
return $self->handle_cache_lookup($params->{cache}, 'Patron_is_expired' .
$self->borrowernumber, $get_is_expired);
}
sub handle_cache_lookup {
my ($self, $use_cache, $cache_key, $get_uncached) = @_;
my $cache = Koha::Cache::Memory::Lite->get_instance;
if ($use_cache) {
my $value = $cache->get_from_cache($cache_key);
return $value if defined $value;
}
else {
$cache->clear_from_cache($cache_key)
}
my $value = $get_uncached->();
if ($use_cache) {
$cache->set_in_cache($cache_key, $value);
}
return $value;
}
I personally sceptical it's worth the tradeoff.
I created a new version using Memory::Lite instead of class attributes for
caching. It does make the code a litte bit more verbose though even though I do
acknowledge the current way is inconsistent with how caching is performed in
the rest of the code base and a bit of a hack. I would prefer if Memory::Lite
had namespaces, right now there is only one bucket the entire cache if ->flush
is called somewhere else. Right now I think it's unlikely as the cache is
flushed in just a few places, but this is the primary reason why I opted
storing the cached values in the object itself.
--
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476
David Cook changed:
What|Removed |Added
CC||dc...@prosentient.com.au
--- Comment #3 from David Cook ---
I like the idea of this one, although I wonder if the cache could be more
generalizable and have less repeated code.
--
Koha::Object could create $self->{_cache} = {} in the new() constructor.
Then Koha::Patron could use a cache related function at the start and end of
"is_expired" and "has_overdues":
return $self->cache({ key => 'is_expired' }) if $param->{cache};
$param->{cache} ? $self->cache({ key => 'is_expired', value => $is_expired }) :
$self->cache({ key => 'is_expired', value => undef });
Not saying it has to... but just a thought to make things easier to read, test,
and maintain.
--
I wonder if Koha::Cache::Memory::Lite could be used instead as well.
--
Overall, I'm a big fan of caching and not re-fetching data that we've already
fetched.
--
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added See Also||https://bugs.koha-community ||.org/bugzilla3/show_bug.cgi ||?id=31735 -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Assignee|koha-b...@lists.koha-commun |glask...@gmail.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson changed: What|Removed |Added Attachment #144626|0 |1 is obsolete|| --- Comment #2 from David Gustafsson --- Created attachment 144627 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144627=edit Bug 32476: Add caching for relatively expensive patron methods To test: 1) Ensure tests in t/db_dependant/Patrons.t all pass Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #1 from David Gustafsson --- Created attachment 144626 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144626=edit Bug 32476: Add caching for relatively expensive patron methods To test: 1) Ensure tests in t/db_dependant/Patrons.t all pass Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes. ___ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
