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.

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?

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

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.

Created attachment 120298 [details] [review]
Cache Koha::Library object in Branches template plugin

Created attachment 120299 [details] [review]
Cache Koha::Library object in Branches template plugin

Created attachment 120300 [details] [review]
Cache authorised values in C4::Items::GetItemsInfo

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.

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.

Created attachment 120311 [details] [review]
Cache Koha::Library object in Branches template plugin

A doh-moment when doing that patch. New corrected Branches-patch.

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.

Created attachment 120401 [details] [review]
Cache authorised values in C4::Items::GetItemsInfo (corrected)

Created attachment 120402 [details] [review]
Cache Koha::Library object in Branches template plugin (modified per sug.)

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

Sounds great, Björn. I'll put this on my list to review.

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.

After a second look :
Second patch looks great (Cache Koha::Library object in Branches template plugin).
It should be on a new bug report.

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>

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.

For information, I had a try at removing GetItemsInfo from detail.pl on bug 27272.

Created attachment 128446 [details] [review]
Bug 26587: Use Koha::Cache::Memory::Lite

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

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

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>

Created attachment 128471 [details] [review]
Bug 26587: Use Koha::Cache::Memory::Lite

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

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

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

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

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.

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

Pushed to master for 22.05, thanks to everybody involved ﷐[U+1F984]﷑

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.

Pushed to 21.11.x for 21.11.02

(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

Enhancement, not backporting to 21.05. Please ask if needed.