[Koha-bugs] [Bug 32476] Add caching for relatively expensive patron methods

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32476

--- Comment #23 from David Gustafsson <glasklas at 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.