Created attachment 144626 [details] [review]
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

Created attachment 144627 [details] [review]
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

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.

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.

*so the entire cache is purged if ->flush is called somewhere else

Created attachment 144744 [details] [review]
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

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.

(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.)

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!

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.

Created attachment 144770 [details] [review]
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

Created attachment 144771 [details] [review]
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

Renamed _cached_accessor to _maybe_cached as the former implies a function is returned.

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;
}

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.

Created attachment 145036 [details] [review]
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

Created attachment 145037 [details] [review]
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

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

Created attachment 147145 [details] [review]
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

Rebased against master.

Created attachment 147148 [details] [review]
Bug 32092: fix tests

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

(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.

Created attachment 150778 [details] [review]
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

Created attachment 150779 [details] [review]
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

Interresting.

Maybe a silly question : 
why not caching entire Koha::Patron object in CanBookBeReserved/CanItemBeReserve ?

(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?

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

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 {

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();

(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

I see Bug 30860 has done a similar job.
We should use the same way in my opinion.

Created attachment 157241 [details] [review]
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

Rebased against master.

(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.

(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.

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.

Created attachment 157258 [details] [review]
Bug 32476: Add support for cachig methods with arguments

Created attachment 157259 [details] [review]
Bug 32476: Add support for caching methods with arguments

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'

Created attachment 162537 [details] [review]
Bug 32476: Add support for caching methods with arguments

Created attachment 162538 [details] [review]
Bug 32476: Fix Circulation tests and exclude cache param from cache key

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.

Created attachment 162539 [details] [review]
Bug 32476: Fix Circulation tests and exclude cache param from cache key

Patch does not apply