Bug 28702

Summary: Improve performance of C4/XSLT/buildKohaItemsNamespace
Product: Koha Reporter: Nick Clemens <nick>
Component: SearchingAssignee: Nick Clemens <nick>
Status: ASSIGNED --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: andreas.jonsson, dcook, jonathan.druart, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26587
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23035
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28299
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29606
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 28702: Make buildKohaItemsNamespace take an array
Helper script to add items to highlight performance benefit
Bug 28702: Make buildKohaItemsNamespace take an array
Bug 28702: Make buildKohaItemsNamespace take an array
Bug 28702: Make buildKohaItemsNamespace take an array
Bug 28702: Improve readability

Description Nick Clemens 2021-07-12 18:04:16 UTC
This subroutine prepares items for XSLT parsing in search results, or opac lists.

In bug 23414 it was refactored to use Koha::Items to reduce the number of lookups.

In reviewing this code, it can be improved further - it is only called by XSLTParse4Display which is only called in 2 places:
C4::Search->searchResults
opac/opac-shelves.pl
Comment 1 Nick Clemens 2021-07-12 18:11:04 UTC
Created attachment 122785 [details] [review]
Bug 28702: Make buildKohaItemsNamespace take an array

If we fetch some of the authorised values and before hand we can reduce the amount of work needed in this routine.
We stil require queries for pending holds and transfers, but these are lighter than fetching the items

To test:
1 - Perform a search on the OPAC
2 - Add the results to a list
3 - Load the list several times and use developer tools (F12) to view the time to load in the network tab
4 - Repeat a search several times and use developer tools (F12) to view the time to load in the network tab
5 - Record the times noted above
6 - Apply patch
7 - Repeat the search and list view and compare to before the patch
8 - prove -v t/db_dependent/XSLT.t
Comment 2 Nick Clemens 2021-07-12 18:13:22 UTC
Created attachment 122786 [details]
Helper script to add items to highlight performance benefit

The performance increase here is significantly greater the more items that exist per bib, this script will add 5 items to every bib in the catalog
perl randitems.pl
Comment 3 Michal Urban 2022-07-20 16:18:51 UTC
Created attachment 137939 [details] [review]
Bug 28702: Make buildKohaItemsNamespace take an array

If we fetch some of the authorised values and before hand we can reduce the amount of work needed in this routine.
We stil require queries for pending holds and transfers, but these are lighter than fetching the items

To test:
1 - Perform a search on the OPAC
2 - Add the results to a list
3 - Load the list several times and use developer tools (F12) to view the time to load in the network tab
4 - Repeat a search several times and use developer tools (F12) to view the time to load in the network tab
5 - Record the times noted above
6 - Apply patch
7 - Repeat the search and list view and compare to before the patch
8 - prove -v t/db_dependent/XSLT.t
Comment 4 Michal Urban 2022-07-20 16:20:04 UTC
Created attachment 137940 [details] [review]
Bug 28702: Make buildKohaItemsNamespace take an array

If we fetch some of the authorised values and before hand we can reduce the amount of work needed in this routine.
We stil require queries for pending holds and transfers, but these are lighter than fetching the items

To test:
1 - Perform a search on the OPAC
2 - Add the results to a list
3 - Load the list several times and use developer tools (F12) to view the time to load in the network tab
4 - Repeat a search several times and use developer tools (F12) to view the time to load in the network tab
5 - Record the times noted above
6 - Apply patch
7 - Repeat the search and list view and compare to before the patch
8 - prove -v t/db_dependent/XSLT.t

Signed-off-by: Michal Urban <michalurban177@gmail.com>
Comment 5 Jonathan Druart 2022-08-01 10:59:46 UTC
Hum, this didn't apply last month.

error: sha1 information is lacking or useless (C4/Search.pm).

and code is different in C4::XSLT
Comment 6 Nick Clemens 2022-08-01 12:18:03 UTC
Created attachment 138459 [details] [review]
Bug 28702: Make buildKohaItemsNamespace take an array

If we fetch some of the authorised values and before hand we can reduce the amount of work needed in this routine.
We stil require queries for pending holds and transfers, but these are lighter than fetching the items

To test:
1 - Perform a search on the OPAC
2 - Add the results to a list
3 - Load the list several times and use developer tools (F12) to view the time to load in the network tab
4 - Repeat a search several times and use developer tools (F12) to view the time to load in the network tab
5 - Record the times noted above
6 - Apply patch
7 - Repeat the search and list view and compare to before the patch
8 - prove -v t/db_dependent/XSLT.t
Comment 7 Jonathan Druart 2022-08-01 13:09:07 UTC
What about hidden_items?
Comment 8 Jonathan Druart 2022-08-01 13:17:55 UTC
Created attachment 138468 [details] [review]
Bug 28702: Improve readability

But removing the repetition

Also prevent useless GetReserveStatus and GetTransfers (in a
non-elegant way...)
Comment 9 Jonathan Druart 2022-08-01 13:23:21 UTC
> But removing the repetition

By*
Comment 10 Nick Clemens 2022-08-01 13:28:02 UTC
(In reply to Jonathan Druart from comment #7)
> What about hidden_items?

We already skip pushing them into the items array, so they do not need to be passed forward
Comment 11 Jonathan Druart 2022-08-01 14:06:53 UTC
I don't understand.
In C4::Search we are building @hiddenitems to pass to XSLTParse4Display.

Either you are right and we don't need that code, or you removed too much code.
Comment 12 Jonathan Druart 2022-08-01 14:08:23 UTC
Also removing items_rs is a step backward compared to bug 28299.
Comment 13 Nick Clemens 2022-08-01 15:40:05 UTC
(In reply to Jonathan Druart from comment #11)
> I don't understand.
> In C4::Search we are building @hiddenitems to pass to XSLTParse4Display.
> 
> Either you are right and we don't need that code, or you removed too much
> code.

We don't need it here, but you are corretc, we need it from opac-shelves

(In reply to Jonathan Druart from comment #12)
> Also removing items_rs is a step backward compared to bug 28299.

I think I was not aware of that when I initially wrote this, so those changes may not be reflected

It comes back to the fact that translating authorised values to descriptions is where we spend time, that's why i tried to move it all to one place - I could reduce this to the changes for getreservestatus