Bug 28371

Summary: Improve performance of XSLTParse4Display
Product: Koha Reporter: Nick Clemens <nick>
Component: Architecture, internals, and plumbingAssignee: Nick Clemens <nick>
Status: CLOSED WONTFIX QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: andreas.jonsson, bjorn.nylen, david, dcook, emmi.takkinen, fridolin.somers, jonathan.druart, joonas.kylmala, m.de.rooy, martin.renvoize, severine.queune, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28373
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23035
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30848
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 15262    
Attachments: Bug 28371: WIP
Bug 28371: Unit tests
Bug 28371: Passpreviously fetched branches and itemtypes through and fetch all needed AV at once
Bug 28959: DBRev 21.06.00.039
Bug 28959: DBIC update
Bug 28371: Unit tests
Bug 28371: Passpreviously fetched branches and itemtypes through and fetch all needed AV at once
Bug 28371: Unit tests
Bug 28371: Passpreviously fetched branches and itemtypes through and fetch all needed AV at once
Bug 28371: Unit tests
Bug 28371: Passpreviously fetched branches and itemtypes through and fetch all needed AV at once
Bug 28371: Unit tests
Bug 28371: Passpreviously fetched branches and itemtypes through and fetch all needed AV at once
Bug 28371: (follow-up) Fetch values if not passed
Bug 28371: (folllow-up) Cover cn_source case for transforming av
Bug 28371: (follow-up) Fetch avs per framework

Description Nick Clemens 2021-05-18 13:19:50 UTC
This routine is quite slow, and heavy on DB lookups
Comment 1 Nick Clemens 2021-05-18 13:23:22 UTC
Created attachment 121111 [details] [review]
Bug 28371: WIP

A lot of our time is spent in transformMARC4XSLT

We do a db lookup per authval, every time - adding some caching here (probably in a better way) quickly increases performance

Additionally, passing through the framework code can save us a  lookup each time.
Comment 2 Björn Nylén 2021-05-20 11:51:01 UTC
We just did a very similar thing locally but added the caching in C4::Biblio::GetAuthorizedValueDesc instead. It's used in a few more places. Saved a lot of time in searchresults and add/edit item views when there are many items.
Comment 3 David Cook 2021-05-21 01:30:15 UTC
(In reply to Björn Nylén from comment #2)
> We just did a very similar thing locally but added the caching in
> C4::Biblio::GetAuthorizedValueDesc instead. It's used in a few more places.
> Saved a lot of time in searchresults and add/edit item views when there are
> many items.

You have a patch for that on Bugzilla too, right? I can't recall the number off the top of my head though.
Comment 4 David Cook 2021-05-21 01:40:53 UTC
Comment on attachment 121111 [details] [review]
Bug 28371: WIP

Review of attachment 121111 [details] [review]:
-----------------------------------------------------------------

::: C4/XSLT.pm
@@ +40,4 @@
>  
>  my $engine; #XSLT Handler object
>  my %authval_per_framework;
> +my %authval_descs;

Are you sure about this scope? 

This will be for the life of the Starman worker, which could lead to stale data and inconsistent results, if you're actively changing authorised value descriptions.

Having a cache scoped to the request would be safer I think. I suppose a "$cache" hashref passed to XSLTParse4Display and then on to transformMARCXML4XSLT would do the trick.

@@ +91,5 @@
> +                        } else {
> +                            if( $av->{$tag}->{$letter} ){
> +                                my $orig_val = $value;
> +                                $value = GetAuthorisedValueDesc( $tag, $letter, $value, '', $tagslib );
> +                                $authval_descs{$frameworkcode.$tag.$letter.$orig_val} = $value;

Sometimes, I wonder if we should just fetch all the authorised value descriptions at the start. The pro is that you have a max of 1 database lookup. The con is that you might be fetching a lot more data than you would if you just end up needing 1 description. However, I haven't done any benchmarking on the time of those pros and cons...
Comment 5 Tomás Cohen Arazi 2021-05-21 01:59:22 UTC
We already have a cache for the frameworks, we should fetch all the avs and cache then based on the fw. It would be interesting to profile it.

And speaking of scope, maybe it is time to think beyond C4. I haven't profiled it, but Nick's idea of Koha::Items->filter_by_visibility proved we can save lots of CPU cycles with better design. Previously, we called a method that queried item numbers, for then fetching each item in a loop (dbi era, GetItemsInfo).
Comment 6 David Cook 2021-05-21 02:08:40 UTC
(In reply to Tomás Cohen Arazi from comment #5)
> We already have a cache for the frameworks, we should fetch all the avs and
> cache then based on the fw. It would be interesting to profile it.

Sounds good to me. And if we invalidate the AV cache based on the AV interface in Administration, then that should be fresh.

> And speaking of scope, maybe it is time to think beyond C4. I haven't
> profiled it, but Nick's idea of Koha::Items->filter_by_visibility proved we
> can save lots of CPU cycles with better design. Previously, we called a
> method that queried item numbers, for then fetching each item in a loop (dbi
> era, GetItemsInfo).

+1

I've been doing a lot of non-Koha work on performance, and lookups on rarely changed data in loops are so often the problem waiting to be fixed.
Comment 7 Björn Nylén 2021-05-21 06:31:36 UTC
(In reply to David Cook from comment #3)
> (In reply to Björn Nylén from comment #2)
> > We just did a very similar thing locally but added the caching in
> > C4::Biblio::GetAuthorizedValueDesc instead. It's used in a few more places.
> > Saved a lot of time in searchresults and add/edit item views when there are
> > many items.
> 
> You have a patch for that on Bugzilla too, right? I can't recall the number
> off the top of my head though.

Bug 26587 . It's a bit diffrent, about caching in GetItemsInfo and Branches template plugin. To speed up detail.pl and opac-detail.pl. What I meant is that one could cache in GetAuthorizedValueDesc instead of XSLTParse4Display for the same effect.
There's the issue about caching for the life of the worker though.
Comment 8 Fridolin Somers 2021-07-02 07:52:06 UTC
*** Bug 28649 has been marked as a duplicate of this bug. ***
Comment 9 Nick Clemens 2021-08-25 15:35:23 UTC
Created attachment 124086 [details] [review]
Bug 28371: Unit tests
Comment 10 Nick Clemens 2021-08-25 15:35:27 UTC
Created attachment 124087 [details] [review]
Bug 28371: Passpreviously fetched branches and itemtypes through and fetch all needed AV at once

This patch updates the searchResuls code to pass through the pre-constructed branches and itemtype lookups
to XSLTParse4Display to avoid repeating this

It also updates getAuthorisedValues4MARCSubfields to fetch the values for mapped subfields and pass
then through to transforMarc4XSLT

Note that we currently blank invalid branches and itemtypes - I presrve this, we should open another bug
if we want to change this behaviour

Changes are covered by tests

To test:
1 - Perform searches in OPAC and staff client that return many records
2 - Use the 'Network' tab on the browser console (opened with F12 usually) to see the time taken
3 - Note the speed before the patch
4 - Apply patch
5 - restart all the things
6 - Note improvement in speed

**Note: The improvement is more drastic the more items per record, try adding large numbers of items to your search results to test**
Comment 11 Jonathan Druart 2021-10-13 07:10:56 UTC
*** Bug 23035 has been marked as a duplicate of this bug. ***
Comment 12 David Nind 2021-10-29 09:39:24 UTC
Created attachment 127082 [details] [review]
Bug 28959: DBRev 21.06.00.039

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2021-10-29 09:39:37 UTC
Created attachment 127083 [details] [review]
Bug 28959: DBIC update

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2021-10-29 09:41:40 UTC
Comment on attachment 127082 [details] [review]
Bug 28959: DBRev 21.06.00.039

Not sure what I did here - obsoleting,
Comment 15 David Nind 2021-10-29 09:42:29 UTC
Comment on attachment 127083 [details] [review]
Bug 28959: DBIC update

Not sure what I did here - obsoleting...
Comment 16 David Nind 2021-10-29 09:44:55 UTC
I think I didn't actually apply the patches!
Comment 17 David Nind 2021-10-29 09:55:56 UTC
I've confirmed that I didn't actually apply the patches - as the patch no longer applies .... (it must be Friday!):

Bug 28371 - Improve performance of XSLTParse4Display

124086 - Bug 28371: Unit tests
124087 - Bug 28371: Passpreviously fetched branches and itemtypes through and fetch all needed AV at once

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 28371: Unit tests
Using index info to reconstruct a base tree...
M	t/db_dependent/XSLT.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/XSLT.t
CONFLICT (content): Merge conflict in t/db_dependent/XSLT.t
error: Failed to merge in the changes.
Patch failed at 0001 Bug 28371: Unit tests
Comment 18 Nick Clemens 2021-10-29 10:44:11 UTC
Created attachment 127090 [details] [review]
Bug 28371: Unit tests
Comment 19 Nick Clemens 2021-10-29 10:44:15 UTC
Created attachment 127091 [details] [review]
Bug 28371: Passpreviously fetched branches and itemtypes through and fetch all needed AV at once

This patch updates the searchResuls code to pass through the pre-constructed branches and itemtype lookups
to XSLTParse4Display to avoid repeating this

It also updates getAuthorisedValues4MARCSubfields to fetch the values for mapped subfields and pass
then through to transforMarc4XSLT

Note that we currently blank invalid branches and itemtypes - I presrve this, we should open another bug
if we want to change this behaviour

Changes are covered by tests

To test:
1 - Perform searches in OPAC and staff client that return many records
2 - Use the 'Network' tab on the browser console (opened with F12 usually) to see the time taken
3 - Note the speed before the patch
4 - Apply patch
5 - restart all the things
6 - Note improvement in speed

**Note: The improvement is more drastic the more items per record, try adding large numbers of items to your search results to test**
Comment 20 David Nind 2021-10-29 17:35:31 UTC
Thanks Nick for making the patch apply again. I had a go at testing, but not sure I got the results expected. Will leave sign off to someone more knowledgable about performance testing...
Comment 21 Séverine Queune 2022-03-31 15:35:59 UTC
Too old one, doesn't apply anymore...

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 28371: Unit tests
Using index info to reconstruct a base tree...
M	t/db_dependent/XSLT.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/XSLT.t
CONFLICT (content): Merge conflict in t/db_dependent/XSLT.t
error: Failed to merge in the changes.
Patch failed at 0001 Bug 28371: Unit tests
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-28371-Unit-tests-N0CZB6.patch
Comment 22 Nick Clemens 2022-03-31 16:23:53 UTC
Created attachment 132801 [details] [review]
Bug 28371: Unit tests
Comment 23 Nick Clemens 2022-03-31 16:23:59 UTC
Created attachment 132802 [details] [review]
Bug 28371: Passpreviously fetched branches and itemtypes through and fetch all needed AV at once

This patch updates the searchResuls code to pass through the pre-constructed branches and itemtype lookups
to XSLTParse4Display to avoid repeating this

It also updates getAuthorisedValues4MARCSubfields to fetch the values for mapped subfields and pass
then through to transforMarc4XSLT

Note that we currently blank invalid branches and itemtypes - I presrve this, we should open another bug
if we want to change this behaviour

Changes are covered by tests

To test:
1 - Perform searches in OPAC and staff client that return many records
2 - Use the 'Network' tab on the browser console (opened with F12 usually) to see the time taken
3 - Note the speed before the patch
4 - Apply patch
5 - restart all the things
6 - Note improvement in speed

**Note: The improvement is more drastic the more items per record, try adding large numbers of items to your search results to test**
Comment 24 Owen Leonard 2022-04-04 15:50:07 UTC
Created attachment 132943 [details] [review]
Bug 28371: Unit tests

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 25 Owen Leonard 2022-04-04 15:50:12 UTC
Created attachment 132944 [details] [review]
Bug 28371: Passpreviously fetched branches and itemtypes through and fetch all needed AV at once

This patch updates the searchResuls code to pass through the pre-constructed branches and itemtype lookups
to XSLTParse4Display to avoid repeating this

It also updates getAuthorisedValues4MARCSubfields to fetch the values for mapped subfields and pass
then through to transforMarc4XSLT

Note that we currently blank invalid branches and itemtypes - I presrve this, we should open another bug
if we want to change this behaviour

Changes are covered by tests

To test:
1 - Perform searches in OPAC and staff client that return many records
2 - Use the 'Network' tab on the browser console (opened with F12 usually) to see the time taken
3 - Note the speed before the patch
4 - Apply patch
5 - restart all the things
6 - Note improvement in speed

**Note: The improvement is more drastic the more items per record, try adding large numbers of items to your search results to test**

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 26 Katrin Fischer 2022-04-24 13:17:00 UTC
Created attachment 133719 [details] [review]
Bug 28371: Unit tests

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Katrin Fischer 2022-04-24 13:17:05 UTC
Created attachment 133720 [details] [review]
Bug 28371: Passpreviously fetched branches and itemtypes through and fetch all needed AV at once

This patch updates the searchResuls code to pass through the pre-constructed branches and itemtype lookups
to XSLTParse4Display to avoid repeating this

It also updates getAuthorisedValues4MARCSubfields to fetch the values for mapped subfields and pass
then through to transforMarc4XSLT

Note that we currently blank invalid branches and itemtypes - I presrve this, we should open another bug
if we want to change this behaviour

Changes are covered by tests

To test:
1 - Perform searches in OPAC and staff client that return many records
2 - Use the 'Network' tab on the browser console (opened with F12 usually) to see the time taken
3 - Note the speed before the patch
4 - Apply patch
5 - restart all the things
6 - Note improvement in speed

**Note: The improvement is more drastic the more items per record, try adding large numbers of items to your search results to test**

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 28 Fridolin Somers 2022-04-25 20:01:17 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 29 Fridolin Somers 2022-04-25 20:02:17 UTC
I hope this performance fix can be backported to 21.11.x
Comment 30 Fridolin Somers 2022-04-26 06:27:44 UTC
This breaks t/db_dependent/Koha/Biblio.t :
    #   Failed test 'Authorised value is correctly parsed to show description rather than code'
    #   at t/db_dependent/Koha/Biblio.t line 708.
    #          got: 'CODE'
    #     expected: 'Description should show'

How should we fix all calls of transformMARCXML4XSLT ?

 > git grep 'transformMARCXML4XSLT('
C4/XSLT.pm:    my $record = transformMARCXML4XSLT($biblionumber, $orig_record, undef, $branches, $itemtypes, $authorised_values );
Koha/Biblio.pm:    $record = transformMARCXML4XSLT( $self->biblionumber, $record, $opac );
Koha/OAI/Server/Repository.pm:    $record = transformMARCXML4XSLT( $biblionumber, $record )
t/db_dependent/XSLT.t:    C4::XSLT::transformMARCXML4XSLT( 3, $record, 0, $branches, $itemtypes, $av );
t/db_dependent/XSLT.t:    C4::XSLT::transformMARCXML4XSLT( 3, $record, $branches, $itemtypes, $av );
Comment 31 Fridolin Somers 2022-04-26 06:54:25 UTC
Reverted from master
Comment 32 Fridolin Somers 2022-04-26 07:09:29 UTC
We may reconsider keeping code logic and add cache
Comment 33 Nick Clemens 2022-04-26 10:52:04 UTC
Created attachment 133872 [details] [review]
Bug 28371: (follow-up) Fetch values if not passed

There are two places where transformMARC4XSLT is called ouside of XSLTParse4Display

ILS-DI if expanding avs
get_marc_notes

The first was pushed after the initial patches here (bug 26195)
The second will only take effect if a notes field is mapped to an av

This patch adds a fallback in transformMARC4XSLT to cover these uses
and fixes a wrong call in tests
Comment 34 Katrin Fischer 2022-04-26 11:33:46 UTC
Thx to the tests! But at least it seems like it was hard to catch without them, which makes me feel a little less guilty for missing it!
Comment 35 Fridolin Somers 2022-04-27 09:13:02 UTC
(In reply to Katrin Fischer from comment #34)
> Thx to the tests! But at least it seems like it was hard to catch without
> them, which makes me feel a little less guilty for missing it!

Sure, that is the purpose of test suite.
Go catch them all ;)
Comment 36 Martin Renvoize 2022-05-26 11:07:21 UTC
Does this get cn_source right?  GetAuthorisedValueDesc appears to have some special handling for that 'category' but I'm not seeing the same here?
Comment 37 Martin Renvoize 2022-05-26 11:21:40 UTC
Also.. I can't remember.. but do we no longer do this stuff framework specific?

You now pass "" instead of the frameworkcode of the bib your processing...
Comment 38 Nick Clemens 2022-05-26 12:13:58 UTC
Created attachment 135373 [details] [review]
Bug 28371: (folllow-up) Cover cn_source case for transforming av
Comment 39 Nick Clemens 2022-05-26 12:56:57 UTC
Created attachment 135381 [details] [review]
Bug 28371: (follow-up) Fetch avs per framework
Comment 40 Martin Renvoize 2022-05-26 14:30:55 UTC
My brain gave up on me whilst trying to work on the tests..  in short.. I think we need to try and cover the frameworks case in the tests and also cover the case of passing/not passing $av, $libraries and $itemtypes.
Comment 41 Martin Renvoize 2022-05-26 16:18:13 UTC
We should still work on getting those tests so they fully cover this.. but.. I'm now really interested to see how bug 38048 compares.. in a quick and dirty benchmark it looked to me like it was in order of 2 times faster again?
Comment 42 David Cook 2022-05-27 01:47:26 UTC
(In reply to Martin Renvoize from comment #41)
> We should still work on getting those tests so they fully cover this.. but..
> I'm now really interested to see how bug 38048 compares.. in a quick and
> dirty benchmark it looked to me like it was in order of 2 times faster again?

I can't find that bug. Is that a typo?
Comment 43 Martin Renvoize 2022-05-27 05:44:02 UTC
Oop, bug 30848 I meant
Comment 44 Martin Renvoize 2022-06-07 13:55:16 UTC
OK.. I completely different approach to this that caches at the C4::Biblio::GetAuthorisedValueDesc level can be found on bug 30920.

When comparing directly to this I found the performance improvements rather favourable to the caching approach (and I also think the code is a little clearer not passing around the variables as much).
Comment 45 Nick Clemens 2022-06-10 12:13:32 UTC
Deprecating in favor of 30920 / 30848
Comment 46 Martin Renvoize 2022-06-13 13:43:35 UTC
30920 is where the real performance improvement comes from now.. 30848 is just some code cleaning to allow wider adoption of the value expansion code.