Bug 26587 - Cache libraries in Branches TT plugin to improve performance
Summary: Cache libraries in Branches TT plugin to improve performance
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Björn Nylén
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 29826
  Show dependency treegraph
 
Reported: 2020-10-01 02:56 UTC by David Cook
Modified: 2022-06-14 21:01 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.02


Attachments
Cache Koha::Library object in Branches template plugin (4.86 KB, patch)
2021-04-29 09:45 UTC, Björn Nylén
Details | Diff | Splinter Review
Cache Koha::Library object in Branches template plugin (2.02 KB, patch)
2021-04-29 09:48 UTC, Björn Nylén
Details | Diff | Splinter Review
Cache authorised values in C4::Items::GetItemsInfo (4.86 KB, patch)
2021-04-29 09:49 UTC, Björn Nylén
Details | Diff | Splinter Review
Cache Koha::Library object in Branches template plugin (2.01 KB, patch)
2021-04-29 12:02 UTC, Björn Nylén
Details | Diff | Splinter Review
Cache authorised values in C4::Items::GetItemsInfo (corrected) (5.09 KB, patch)
2021-05-03 11:29 UTC, Björn Nylén
Details | Diff | Splinter Review
Cache Koha::Library object in Branches template plugin (modified per sug.) (2.30 KB, patch)
2021-05-03 11:30 UTC, Björn Nylén
Details | Diff | Splinter Review
Bug 26587: Cache libraries in Koha/Template/Plugins/Branches.pm to improve performance (2.49 KB, patch)
2021-11-06 16:01 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 26587: Use Koha::Cache::Memory::Lite (1.67 KB, patch)
2021-12-13 11:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26587: Cache libraries in Koha/Template/Plugins/Branches.pm to improve performance (2.55 KB, patch)
2021-12-13 14:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26587: Use Koha::Cache::Memory::Lite (1.72 KB, patch)
2021-12-13 14:31 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2020-10-01 02:56:22 UTC
Using Plack Koha, it took about 10 seconds to load detail.pl for a page with 350 items.

Looking at the code, we loop through order, subscriptions, and items.

I haven't done any benchmarking, but my guess would be the slowdown comes when looping through the items, as there are lots of database calls made in that loop.

I think optimizations come down to the following (which are not mutually exclusive):

1. Get the full count of items but only lookup/render a subset using an AJAX call 
2. Use fewer database calls in the loop by fetching all the data needed from the database and then processing it as necessary in the loop
3. Use fewer external cache lookups by doing things like C4::Context->preference('UseCourseReserves') outside of the loop rather than inside the loop
Comment 1 David Cook 2020-10-01 02:57:50 UTC
This is an issue that I (rather than a sponsor) noted, so I'm probably not going to work on it any time soon.

But logging it anyway and maybe someone else will use my ideas to improve the detail.pl performance.
Comment 2 Björn Nylén 2021-04-28 18:07:04 UTC
We've been profiling a bit and fond that the Branches template plugin is a culprit. GetName does a db-call every time. Tried caching the values in a package variable which cut down the time considerably. Not sure if "our" variables are kosher though?
Comment 3 David Cook 2021-04-29 05:16:02 UTC
(In reply to Björn Nylén from comment #2)
> We've been profiling a bit and fond that the Branches template plugin is a
> culprit. GetName does a db-call every time. Tried caching the values in a
> package variable which cut down the time considerably. Not sure if "our"
> variables are kosher though?

Nice one, Björn!

I haven't played around with the Template Toolkit plugins much, but according to http://template-toolkit.org/docs/modules/Template/Plugin.html, it looks like the Branches plugin is used as an object.

So in terms of scoping...

If you cache it with the object, I imagine it would be cached just for the lifetime of that request.

If you cache it at the package level, you'd cache it for the life of that Plack worker process, which could possibly be problematic. 

You could also use Koha::Cache, although I'm not sure that it lets you only cache it locally and not within Memcached. 

The easiest thing would probably be to create a hashmap in $self->{branches} where $branchcode is the key and the value is a Koha::Library object. 

That should limit the database calls while also keeping things quite fresh.
Comment 4 David Cook 2021-04-29 05:17:57 UTC
Alternatively, maybe we should add a "new" method that calls the database 1 time at [% USE Branches %] time which populates a hashmap and then GetName just consults that hashmap.

Anyway, just some thoughts.
Comment 5 Björn Nylén 2021-04-29 09:45:52 UTC
Created attachment 120298 [details] [review]
Cache Koha::Library object in Branches template plugin
Comment 6 Björn Nylén 2021-04-29 09:48:08 UTC
Created attachment 120299 [details] [review]
Cache Koha::Library object in Branches template plugin
Comment 7 Björn Nylén 2021-04-29 09:49:12 UTC
Created attachment 120300 [details] [review]
Cache authorised values in C4::Items::GetItemsInfo
Comment 8 Björn Nylén 2021-04-29 09:59:31 UTC
Realised I had thing a bit mixed up, was testing a number of things at once. Performance is also limited by some AV lookups in C4::Items::GetItemsInfo.
They should be cached in Memcached but I still get significan improvments when caching them in a hashmap while looping the items.

Some figures from my testing:
Initial opac-detail.pl: ~20s
Only Branches patch: ~15s
Only GetItemsInfo patch: ~12s
Both: ~9s

We're using a separate db-server so may be more sensitive to db-access. Tesing against that as we're interested in improving perf in our environment.
Comment 9 Björn Nylén 2021-04-29 10:59:46 UTC
Not sure about the second patch really, It's probably more of a bandaid, the issue is perhaps more in Koha::AuthorisedValues->get_description_by_koha_field. Setting to Needs signoff but is open to suggestions.
Comment 10 Björn Nylén 2021-04-29 12:02:57 UTC
Created attachment 120311 [details] [review]
Cache Koha::Library object in Branches template plugin

A doh-moment when doing that patch. New corrected Branches-patch.
Comment 11 David Cook 2021-04-30 00:22:04 UTC
Hey Björn, have you tested these patches? 

I haven't tested them, but at a glance it looks like they contain errors.

1)
For instance, in the first one, $av_cache doesn't look like it's declared anywhere. I think that will probably throw a fatal error (or at best a warning). You should be declaring that before the loop in GetItemsInfo. 

You're also using fairly deep hash structures which could break. For instance:
$av_cache->{$data->{frameworkcode}}->{'items.stack'}->{$data->{stack}}->{lib} = $data->{stack};

If $data->{frameworkcode} or more likely $data->{stack} don't exist , then you're going to probably get a fatal error. You might want to wrap some of this cache code in a function with more checks/conditions in it.

2)
I think that the second patch will generate either fatal errors or warnings as well since you're not testing for the existence of $self->{branches} before tyring $self->{branches}->{$branchcode}. 

You can either create $self->{branches} in a "sub new" function or just test for it first. Something like "$self->{branches} = {} unless $self->{branches}". Then You can test for $self->{branches}->{$branchcode} because you've initialized $self->{branches} as a hashref.
Comment 12 Björn Nylén 2021-05-03 11:29:10 UTC
Created attachment 120401 [details] [review]
Cache authorised values in C4::Items::GetItemsInfo (corrected)
Comment 13 Björn Nylén 2021-05-03 11:30:18 UTC
Created attachment 120402 [details] [review]
Cache Koha::Library object in Branches template plugin (modified per sug.)
Comment 14 Björn Nylén 2021-05-03 11:40:39 UTC
(In reply to David Cook from comment #11)
> Hey Björn, have you tested these patches? 
> 
> I haven't tested them, but at a glance it looks like they contain errors.
> 
> 1)
> For instance, in the first one, $av_cache doesn't look like it's declared
> anywhere. I think that will probably throw a fatal error (or at best a
> warning). You should be declaring that before the loop in GetItemsInfo. 
> 
> You're also using fairly deep hash structures which could break. For
> instance:
> $av_cache->{$data->{frameworkcode}}->{'items.stack'}->{$data->{stack}}-
> >{lib} = $data->{stack};
> 
> If $data->{frameworkcode} or more likely $data->{stack} don't exist , then
> you're going to probably get a fatal error. You might want to wrap some of
> this cache code in a function with more checks/conditions in it.
> 
> 2)
> I think that the second patch will generate either fatal errors or warnings
> as well since you're not testing for the existence of $self->{branches}
> before tyring $self->{branches}->{$branchcode}. 
> 
> You can either create $self->{branches} in a "sub new" function or just test
> for it first. Something like "$self->{branches} = {} unless
> $self->{branches}". Then You can test for $self->{branches}->{$branchcode}
> because you've initialized $self->{branches} as a hashref.

Yeah, I tested and developed in a separate repo and tried to just copy paste the code to the one with Koha-master code and missed the variable declaration. 

About GetItemsInfo:
Removed the deep hashes as they were unneccesary. (Framwork is the same for all items in biblio) Autovivification should do the trick with the undefined hash-keys but added them more explicitly. It's more obvious what's done that way.The only warnings I got were the item-values that are NULL in the database so I check for those.

Branches:
Added a "new" sub to init the cache in a more explicit manner. Autovivification did it before but is a bit more obscure.
Comment 15 David Cook 2021-05-04 03:36:42 UTC
Sounds great, Björn. I'll put this on my list to review.
Comment 16 Fridolin Somers 2021-07-02 21:35:52 UTC
Maybe we could prefer using Koha::Cache::Memory::Lite like in other places :
https://git.koha-community.org/Koha-community/Koha/src/commit/2e6b5483503e7feb0afc6ef526a675bde767dd8e/Koha/AuthorisedValues.pm#L104

In order to avoid having different cache features.
Comment 17 Fridolin Somers 2021-08-17 06:48:22 UTC
After a second look :
Second patch looks great (Cache Koha::Library object in Branches template plugin).
It should be on a new bug report.
Comment 18 Joonas Kylmälä 2021-11-06 16:01:33 UTC
Created attachment 127411 [details] [review]
Bug 26587: Cache libraries in Koha/Template/Plugins/Branches.pm to improve performance

This patch caches the Koha::Library object in a hashmap in the Branches object (GetName, GetURL)
to avoid multiple database lookups while looping many items.

To test:
1. Have a biblio with many items (1000's).
2. View in staff (detail.pl) and opac (opac-detail.pl). Note how long it takes to load.
3. Apply patch.
4. Repeat 2. Note that pages load faster.

Sponsored-by: Lund University Library
JK: replaced tab indendation with spaces
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 19 Joonas Kylmälä 2021-11-06 16:07:42 UTC
The library caching patch looks good, I tested with 600 items and the load time went from 22 seconds to 17 seconds on my machine. Björn, could you please move the other patch to a new bug report as also suggested by Fridolin? I mean the patch "Cache Koha::Library object in Branches template plugin (modified per sug.)". You can get the obsolete patch by clicking on Bugzilla on the "Show obsolete" in the attachment section. I obsoleted the patch so that we can now get the finished patch in that we have here, it is quite critical for bigger libraries.

I think the other patch still needs more work / discussion as it is using already the caching mechanism there but it is not enough so we need to figure out why and make a better global caching mechanism instead of fixing it only in C4/Items.pm (but that is something to do in the other bug report).

Signing off.
Comment 20 Jonathan Druart 2021-12-13 11:07:28 UTC
For information, I had a try at removing GetItemsInfo from detail.pl on bug 27272.
Comment 21 Jonathan Druart 2021-12-13 11:25:49 UTC
Created attachment 128446 [details] [review]
Bug 26587: Use Koha::Cache::Memory::Lite
Comment 22 Jonathan Druart 2021-12-13 11:28:19 UTC
(In reply to Fridolin Somers from comment #16)
> Maybe we could prefer using Koha::Cache::Memory::Lite like in other places :
> https://git.koha-community.org/Koha-community/Koha/src/commit/
> 2e6b5483503e7feb0afc6ef526a675bde767dd8e/Koha/AuthorisedValues.pm#L104
> 
> In order to avoid having different cache features.

I second that and I submitted a patch. I haven't noticed any significant changes in perf between the 2 versions (always 21-22s).

Tested using 1000 items with the following script to generate the biblio record:

use t::lib::TestBuilder;
my $builder = t::lib::TestBuilder->new;
my @branchcodes = Koha::Libraries->search->get_column('branchcode');
my $biblio = $builder->build_sample_biblio;
for my $i ( 1..1000 ) { 
    say $i; 
    my $branchcode = @branchcodes[int(rand(scalar @branchcodes))];
    $builder->build_sample_item({biblionumber => $biblio->biblionumber, homebranch => $branchcode, holdingbranch => $branchcode }); 
}
say "biblionumber=".$biblio->biblionumber;
Comment 23 Jonathan Druart 2021-12-13 11:30:57 UTC
(In reply to Jonathan Druart from comment #22)
> (In reply to Fridolin Somers from comment #16)
> > Maybe we could prefer using Koha::Cache::Memory::Lite like in other places :
> > https://git.koha-community.org/Koha-community/Koha/src/commit/
> > 2e6b5483503e7feb0afc6ef526a675bde767dd8e/Koha/AuthorisedValues.pm#L104
> > 
> > In order to avoid having different cache features.
> 
> I second that and I submitted a patch. I haven't noticed any significant
> changes in perf between the 2 versions (always 21-22s).

vs 25-26s using master.
Comment 24 Nick Clemens 2021-12-13 14:31:46 UTC
Created attachment 128470 [details] [review]
Bug 26587: Cache libraries in Koha/Template/Plugins/Branches.pm to improve performance

This patch caches the Koha::Library object in a hashmap in the Branches object (GetName, GetURL)
to avoid multiple database lookups while looping many items.

To test:
1. Have a biblio with many items (1000's).
2. View in staff (detail.pl) and opac (opac-detail.pl). Note how long it takes to load.
3. Apply patch.
4. Repeat 2. Note that pages load faster.

Sponsored-by: Lund University Library
JK: replaced tab indendation with spaces
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 25 Nick Clemens 2021-12-13 14:31:51 UTC
Created attachment 128471 [details] [review]
Bug 26587: Use Koha::Cache::Memory::Lite

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 26 Fridolin Somers 2021-12-15 21:37:44 UTC
(In reply to Jonathan Druart from comment #21)
> Created attachment 128446 [details] [review] [review]
> Bug 26587: Use Koha::Cache::Memory::Lite

Great.

But in my opinion cache key is too short :
  my $cache_key    = "Library_branchname:" . $branchcode;
We may have need for library datas cache in other places.

I propose :
  my $cache_key    = "Template_Plugin_Branches_branchname:" . $branchcode;
Comment 27 David Cook 2021-12-16 00:55:17 UTC
(In reply to Fridolin Somers from comment #26)
> (In reply to Jonathan Druart from comment #21)
> > Created attachment 128446 [details] [review] [review] [review]
> > Bug 26587: Use Koha::Cache::Memory::Lite
> 
> Great.
> 
> But in my opinion cache key is too short :
>   my $cache_key    = "Library_branchname:" . $branchcode;
> We may have need for library datas cache in other places.
> 
> I propose :
>   my $cache_key    = "Template_Plugin_Branches_branchname:" . $branchcode;

Good call on using Koha::Cache::Memory::Lite. 

I suppose the cache key is debateable. I don't think we have an index of cache keys, so Frido might be right about making a more targeted cache key, so that there isn't an accidental collision with another script that stores different data using the same key.
Comment 28 David Cook 2021-12-16 00:59:32 UTC
(In reply to David Cook from comment #27)
> I suppose the cache key is debateable. I don't think we have an index of
> cache keys, so Frido might be right about making a more targeted cache key,
> so that there isn't an accidental collision with another script that stores
> different data using the same key.

Sorry I shouldn't have said another script since it's a L1 cache. I mean a different module/function.

That said, the risk seems fairly low of that happening...

If we did have an index, re-using the same cache key across different modules seems appealing to me.

I'm on the fence.

On one hand, this is already Passed QA, so maybe we leave it as is, and just change it later if it's an issue.
Comment 29 Jonathan Druart 2021-12-16 09:27:43 UTC
To me the key is correct and actually perfect as it, we can imagine another method using it as well. It's storing a map of branchcode => branchname, no need to be more specific IMO.
Comment 30 Fridolin Somers 2021-12-17 20:43:03 UTC
OK super.

Good to have several feedbacks it help understanding how we develop.

So OK with this key, makes sens to reuse it.

I see we have a wiki page for that :
https://wiki.koha-community.org/wiki/Cache_handling_in_Koha#What_is_cached.3F

I'll had this case and try to improve this doc.

Best regards
Comment 31 Fridolin Somers 2021-12-22 06:55:11 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 32 Tomás Cohen Arazi 2022-01-06 12:14:55 UTC
This patch introduces a warning on tests:
$ prove t/db_dependent/Template/Plugin/Branches.t
Use of uninitialized value $branchcode in concatenation (.) or string at /kohadevbox/koha/Koha/Template/Plugin/Branches.pm line 35.

I'm not sure if it is ok to  call GetName with no branchcode param. Maybe it should explode in those cases as it is obviously a bug.
Comment 33 Kyle M Hall 2022-01-07 10:57:29 UTC
Pushed to 21.11.x for 21.11.02
Comment 34 Fridolin Somers 2022-01-08 01:34:28 UTC
(In reply to Tomás Cohen Arazi from comment #32)
> This patch introduces a warning on tests:
> $ prove t/db_dependent/Template/Plugin/Branches.t
> Use of uninitialized value $branchcode in concatenation (.) or string at
> /kohadevbox/koha/Koha/Template/Plugin/Branches.pm line 35.
> 
> I'm not sure if it is ok to  call GetName with no branchcode param. Maybe it
> should explode in those cases as it is obviously a bug.

I've created Bug 29826
Comment 35 Andrew Fuerste-Henry 2022-01-10 13:57:20 UTC
Enhancement, not backporting to 21.05. Please ask if needed.