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
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
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
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
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>
Hum, this didn't apply last month. error: sha1 information is lacking or useless (C4/Search.pm). and code is different in C4::XSLT
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
What about hidden_items?
Created attachment 138468 [details] [review] Bug 28702: Improve readability But removing the repetition Also prevent useless GetReserveStatus and GetTransfers (in a non-elegant way...)
> But removing the repetition By*
(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
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.
Also removing items_rs is a step backward compared to bug 28299.
(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
Created attachment 186070 [details] [review] Bug 28702: Reduce DB lookups in buildKohaItemsNamespace This patch moves the prefetch to current_branchtransfers and uses the prefetched reserves and transfers to reduce the number of lookups from the DB 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 - Find or create a record with many items, biblionumber 3 in my example 7 - on the command line: export DBIC_TRACE=1 perl -e 'use C4::XSLT; my $i = C4::XSLT::buildKohaItemsNamespace( 3 );' note the lookups in reserves and transfers 8 - Apply patch 9 - Repeat 7 and note less lookups 10 - Repeat the search and list view and compare times to before the patch 11 - prove -v t/db_dependent/XSLT.t
Created attachment 186071 [details] [review] Bug 28702: Don't fetch the transfer, knowing we have current transfers is enough We don't use the specific transfer, so the count should be sufficient
- } elsif ( $item->holds->waiting->count ) { + } elsif ( $item->_result->reserves->count && $item->holds->waiting->count ) { $status = 'other'; Any good reasons to not use $item->holds->count instead of $item->_result->reserves->count?
(In reply to Jonathan Druart from comment #16) > - } elsif ( $item->holds->waiting->count ) { > + } elsif ( $item->_result->reserves->count && > $item->holds->waiting->count ) { > $status = 'other'; > > Any good reasons to not use $item->holds->count instead of > $item->_result->reserves->count? In my testing the original code generated a new database query, while the new code does not - we are trying to make sure we take advantage of the prefetch data, rather than hitting the DB again
Created attachment 186192 [details] [review] Bug 28702: Reduce DB lookups in buildKohaItemsNamespace This patch moves the prefetch to current_branchtransfers and uses the prefetched reserves and transfers to reduce the number of lookups from the DB We also don't fetch the specific current transfer, knowing we have current transfers is enough 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 - Find or create a record with many items, biblionumber 3 in my example 7 - on the command line: export DBIC_TRACE=1 perl -e 'use C4::XSLT; my $i = C4::XSLT::buildKohaItemsNamespace( 3 );' note the lookups in reserves and transfers 8 - Apply patch 9 - Repeat 7 and note less lookups 10 - Repeat the search and list view and compare times to before the patch 11 - prove -v t/db_dependent/XSLT.t
(In reply to Jonathan Druart from comment #16) > - } elsif ( $item->holds->waiting->count ) { > + } elsif ( $item->_result->reserves->count && > $item->holds->waiting->count ) { > $status = 'other'; > > Any good reasons to not use $item->holds->count instead of > $item->_result->reserves->count? I understand now, patch amended
The second one can then certainly be replaced with ->get_transfers->count then.
s/ then.$/./
(In reply to Jonathan Druart from comment #20) > The second one can then certainly be replaced with ->get_transfers->count This generates a new query in my testing, not sure why, maybe the ordering?
Created attachment 186193 [details] [review] Bug 28702: Reduce DB lookups in buildKohaItemsNamespace This patch moves the prefetch to current_branchtransfers and uses the prefetched reserves and transfers to reduce the number of lookups from the DB We also don't fetch the specific current transfer, knowing we have current transfers is enough 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 - Find or create a record with many items, biblionumber 3 in my example 7 - on the command line: export DBIC_TRACE=1 perl -e 'use C4::XSLT; my $i = C4::XSLT::buildKohaItemsNamespace( 3 );' note the lookups in reserves and transfers 8 - Apply patch 9 - Repeat 7 and note less lookups 10 - Repeat the search and list view and compare times to before the patch 11 - prove -v t/db_dependent/XSLT.t Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Nick Clemens (kidclamp) from comment #22) > (In reply to Jonathan Druart from comment #20) > > The second one can then certainly be replaced with ->get_transfers->count > > This generates a new query in my testing, not sure why, maybe the ordering? Yes, indeed!