[Bug 32476] New: Add caching for relatively expensive patron methods
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Bug ID: 32476 Summary: Add caching for relatively expensive patron methods Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: glasklas@gmail.com QA Contact: testopia@bugs.koha-community.org Add caching for is_expired and has_overdues in Koha::Patron, since these are accessed a lot and involves some database and datetime-operations that really only needs to be evaluated once per patron object it adds up to quite a lot in some use cases (where patrons have a significant amount of checkouts). -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #1 from David Gustafsson <glasklas@gmail.com> --- Created attachment 144626 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144626&action=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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144626|0 |1 is obsolete| | --- Comment #2 from David Gustafsson <glasklas@gmail.com> --- Created attachment 144627 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144627&action=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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |glasklas@gmail.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #3 from David Cook <dcook@prosentient.com.au> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #4 from David Gustafsson <glasklas@gmail.com> --- 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 !~ '^9999' && 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #5 from David Gustafsson <glasklas@gmail.com> --- *so the entire cache is purged if ->flush is called somewhere else -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144627|0 |1 is obsolete| | --- Comment #6 from David Gustafsson <glasklas@gmail.com> --- Created attachment 144744 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144744&action=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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #7 from David Gustafsson <glasklas@gmail.com> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #8 from David Cook <dcook@prosentient.com.au> --- (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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #9 from David Cook <dcook@prosentient.com.au> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #10 from David Gustafsson <glasklas@gmail.com> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144744|0 |1 is obsolete| | --- Comment #11 from David Gustafsson <glasklas@gmail.com> --- Created attachment 144770 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144770&action=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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144770|0 |1 is obsolete| | --- Comment #12 from David Gustafsson <glasklas@gmail.com> --- Created attachment 144771 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144771&action=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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #13 from David Gustafsson <glasklas@gmail.com> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #14 from David Cook <dcook@prosentient.com.au> --- I did have some little mistakes in my previous example. Hopefully this is more illustrative. You don't need to use closures and personally I'm not a fan of "maybe_*" functions. -- Koha::Patron sub is_expired { my ($self, $params) = @_; ( $param->{cache} ) ? return $self->cache({ method => 'is_expired' }) : $self->cache({ method => 'is_expired', reset => 1 }); return ( $self->dateexpiry && $self->dateexpiry !~ '^9999' && dt_from_string( $self->dateexpiry ) < dt_from_string->truncate( to => 'day' ) ) ? 1 : 0; } sub has_overdues { my ($self, $params) = @_; ( $param->{cache} ) ? return $self->cache({ method => 'has_overdues' }) : $self->cache({ method => 'has_overdues', reset => 1 }); my $dtf = Koha::Database->new->schema->storage->datetime_parser; return $self->_result->issues->search({ date_due => { '<' => $dtf->format_datetime( dt_from_string() ) } })->count; } Koha::Object sub cache { my ($self, $args) = @_; my $method = $args->{method}; my $reset = $args->{reset}; my $value; if ($method){ if ($reset){ delete $self->{_cache}->{$method}; } else { if ( defined $self->{_cache}->{$method } ){ $value = $self->{_cache}->{$method }; } else { $value = $self->$method(); #Set cache $self->{_cache}->{$method} = $value; } } } return $value; } -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #15 from David Gustafsson <glasklas@gmail.com> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144771|0 |1 is obsolete| | --- Comment #16 from David Gustafsson <glasklas@gmail.com> --- Created attachment 145036 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145036&action=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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145036|0 |1 is obsolete| | --- Comment #17 from David Gustafsson <glasklas@gmail.com> --- Created attachment 145037 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145037&action=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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Olivier Hubert <olivier.hubert@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |olivier.hubert@inlibro.com Status|Needs Signoff |Patch doesn't apply --- Comment #18 from Olivier Hubert <olivier.hubert@inlibro.com> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145037|0 |1 is obsolete| | --- Comment #19 from David Gustafsson <glasklas@gmail.com> --- Created attachment 147145 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=147145&action=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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #20 from David Gustafsson <glasklas@gmail.com> --- Rebased against master. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #21 from David Gustafsson <glasklas@gmail.com> --- Created attachment 147148 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=147148&action=edit Bug 32092: fix tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #147148|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com --- Comment #22 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #23 from David Gustafsson <glasklas@gmail.com> --- (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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #147145|0 |1 is obsolete| | --- Comment #24 from David Gustafsson <glasklas@gmail.com> --- Created attachment 150778 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150778&action=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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150778|0 |1 is obsolete| | --- Comment #25 from David Gustafsson <glasklas@gmail.com> --- Created attachment 150779 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150779&action=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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Jonathan Druart <jonathan.druart+koha@gmail.com> 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Jonathan Druart <jonathan.druart+koha@gmail.com> 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtj@kohaaloha.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Jonathan Druart <jonathan.druart+koha@gmail.com> 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #26 from Fridolin Somers <fridolin.somers@biblibre.com> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Jonathan Druart <jonathan.druart+koha@gmail.com> 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #27 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com Status|Needs Signoff |Patch doesn't apply --- Comment #28 from Kyle M Hall <kyle@bywatersolutions.com> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #29 from Kyle M Hall <kyle@bywatersolutions.com> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |emily.lamancusa@montgomeryc | |ountymd.gov -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #30 from Nick Clemens <nick@bywatersolutions.com> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #31 from Fridolin Somers <fridolin.somers@biblibre.com> --- (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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Fridolin Somers <fridolin.somers@biblibre.com> 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #32 from Fridolin Somers <fridolin.somers@biblibre.com> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150779|0 |1 is obsolete| | --- Comment #33 from David Gustafsson <glasklas@gmail.com> --- Created attachment 157241 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157241&action=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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #34 from David Gustafsson <glasklas@gmail.com> --- Rebased against master. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #35 from David Gustafsson <glasklas@gmail.com> --- (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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #36 from David Gustafsson <glasklas@gmail.com> --- (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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #37 from David Gustafsson <glasklas@gmail.com> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #38 from David Gustafsson <glasklas@gmail.com> --- Created attachment 157258 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157258&action=edit Bug 32476: Add support for cachig methods with arguments -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157258|0 |1 is obsolete| | --- Comment #39 from David Gustafsson <glasklas@gmail.com> --- Created attachment 157259 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157259&action=edit Bug 32476: Add support for caching methods with arguments -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |BLOCKED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Failed QA --- Comment #40 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157259|0 |1 is obsolete| | --- Comment #41 from David Gustafsson <glasklas@gmail.com> --- Created attachment 162537 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162537&action=edit Bug 32476: Add support for caching methods with arguments -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #42 from David Gustafsson <glasklas@gmail.com> --- Created attachment 162538 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162538&action=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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #43 from David Gustafsson <glasklas@gmail.com> --- 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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #162538|0 |1 is obsolete| | --- Comment #44 from David Gustafsson <glasklas@gmail.com> --- Created attachment 162539 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=162539&action=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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Bug 32476 depends on bug 35133, which changed state. Bug 35133 Summary: Accessors defined in AUTOLOAD does not work if called with SUPER https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35133 What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Shi Yao Wang <shi-yao.wang@inLibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |shi-yao.wang@inLibro.com --- Comment #45 from Shi Yao Wang <shi-yao.wang@inLibro.com> --- Patch does not apply -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=36350 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157241|0 |1 is obsolete| | --- Comment #46 from David Gustafsson <glasklas@gmail.com> --- Created attachment 178179 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=178179&action=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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #162537|0 |1 is obsolete| | --- Comment #47 from David Gustafsson <glasklas@gmail.com> --- Created attachment 178180 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=178180&action=edit Bug 32476: Add support for caching methods with arguments -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #162539|0 |1 is obsolete| | --- Comment #48 from David Gustafsson <glasklas@gmail.com> --- Created attachment 178181 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=178181&action=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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #49 from David Gustafsson <glasklas@gmail.com> --- Rebased against main. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=33745 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also|https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=33745 | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 --- Comment #50 from David Gustafsson <glasklas@gmail.com> --- I realize it's relevant to mention that the Koha codebase to my knowledge doesn't have any places where CanBooKBeRenewed is called a sufficient number of times for performance of the patron methods cached in this patch to be an issue. We have replaced the opac with our own application that calles CanBookBeRenewed for each patron issue (and some patron have a rather large number of issues). So I can see if there is little interest for this. A good think is that bug 3635 will cache the search query in has_overdues ans abolishing the need to cache that method, and from I can remember that was the major culprit, is_expired can probably be left uncached without a major difference in performance for our case. If more methods are in need of caching in the future his feature could be useful, but as previously mentioned I can see if this feature is deemed more trouble than it's worth. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Status|Needs Signoff |Patch doesn't apply -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org