Bug 23414

Summary: Improve performance of C4/XSLT/buildKohaItemsNamespace
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: Architecture, internals, and plumbingAssignee: Nick Clemens (kidclamp) <nick>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, kyle, lucas, martin.renvoize, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23407
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19884
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11213
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00
Bug Depends on:    
Bug Blocks: 15262    
Attachments: Bug 23414: Use Koha Item objects in buildKohaItemsNamespace
Bug 23414: Prefetch transfers, use only default framework, use elsif
Bug 23414: Prefetch reserves and use them for determining waiting
Bug 23414: Use Koha::Holds->waiting
Bug 23414: Use Koha Item objects in buildKohaItemsNamespace
Bug 23414: Prefetch transfers, use only default framework, use elsif
Bug 23414: Unit tests
Bug 23414: Use Koha Item objects in buildKohaItemsNamespace
Bug 23414: Prefetch transfers, use only default framework, use elsif
Bug 23414: Easy readability and simplify logic in buildKohaItemsNamespace
Bug 23414: Use effective_itemtype
Bug 23414: Use Koha Item objects in buildKohaItemsNamespace
Bug 23414: Prefetch transfers, use only default framework, use elsif
Bug 23414: Easy readability and simplify logic in buildKohaItemsNamespace
Bug 23414: Use effective_itemtype
Bug 23414: Unit tests
Bug 23414: Use Koha Item objects in buildKohaItemsNamespace
Bug 23414: Prefetch transfers, use only default framework, use elsif
Bug 23414: Easy readability and simplify logic in buildKohaItemsNamespace
Bug 23414: Use effective_itemtype
Bug 23414: Unit tests
Bug 23414: Add tests for previous effective_itemtype change
Bug 23414: Replace delimiter of regexs to ease readability

Description Nick Clemens (kidclamp) 2019-08-01 18:29:13 UTC
There are some improvements we can make to this routine to improve the speed at which we return results
Comment 1 Nick Clemens (kidclamp) 2019-08-01 18:37:45 UTC
Created attachment 91913 [details] [review]
Bug 23414: Use Koha Item objects in buildKohaItemsNamespace

Removing the use of C4::GetItemsInfo cuts down on the number of DB requests and speeds things up

To test I added 100 items to each of the first 20 records returned by a search for 'a'
I saw a reduction from ~30 seconds to ~26 seconds

This also makes the code a little cleaner and moves us toward the Koha namespace

To test:
1 - Perform a search in the OPAC and STaff Client with the Browser Console opened (F12)
2 - View the netwrok tab and see how long the pages take to load
3 - Apply the patches, restart all the things
4 - Repeat and note if the results are returned faster
Comment 2 Nick Clemens (kidclamp) 2019-08-01 18:37:48 UTC
Created attachment 91914 [details] [review]
Bug 23414: Prefetch transfers, use only default framework, use elsif

This further cleans up the code, before we tested each condition though we return only one status line.

Koha mappings can no longer differ by framework so we don't need to fetch the biblio framework

Prefetching should offer a boost

To test:
Same as before - there is not as big a boost form this, but there shouldn't be a hit
Comment 3 Nick Clemens (kidclamp) 2019-08-01 18:37:50 UTC
Created attachment 91915 [details] [review]
Bug 23414: Prefetch reserves and use them for determining waiting

This dependss on bug 23413

Same tests as before, you shouldn't see a hit with this change
Comment 4 Tomás Cohen Arazi 2019-10-15 19:47:04 UTC
I like this enhancement. But would like to mention that it looks like there's an unnecessary processing of items from the MARC results, in the caller (searchResults) that is also making several DB calls. So it seems we need to make a decision, whether to use the data from Zebra, or use the one on the DB, and reuse the retrieved objects as much as we can (so maybe doing the search at a higher level, searchResults). Just a thought, and can be done later of course.
Comment 5 Jonathan Druart 2019-10-22 12:20:52 UTC
Created attachment 94537 [details] [review]
Bug 23414: Use Koha::Holds->waiting
Comment 6 Jonathan Druart 2019-10-22 12:26:33 UTC
The if...if sequence vs elsif will introduce a behavior's change. For instance an item that is lost and damaged will now be marked as lost whereas it was damaged before this patch. Is it expected? If not, you should reverse the order.
Comment 7 Nick Clemens (kidclamp) 2019-10-22 15:22:51 UTC
Created attachment 94580 [details] [review]
Bug 23414: Use Koha Item objects in buildKohaItemsNamespace

Removing the use of C4::GetItemsInfo cuts down on the number of DB requests and speeds things up

To test I added 100 items to each of the first 20 records returned by a search for 'a'
I saw a reduction from ~30 seconds to ~26 seconds

This also makes the code a little cleaner and moves us toward the Koha namespace

To test:
1 - Perform a search in the OPAC and STaff Client with the Browser Console opened (F12)
2 - View the netwrok tab and see how long the pages take to load
3 - Apply the patches, restart all the things
4 - Repeat and note if the results are returned faster
Comment 8 Nick Clemens (kidclamp) 2019-10-22 15:22:55 UTC
Created attachment 94581 [details] [review]
Bug 23414: Prefetch transfers, use only default framework, use elsif

This further cleans up the code, before we tested each condition though we return only one status line.

Koha mappings can no longer differ by framework so we don't need to fetch the biblio framework

Prefetching should offer a boost

To test:
Same as before - there is not as big a boost form this, but there shouldn't be a hit
Comment 9 Jonathan Druart 2019-10-30 12:06:14 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2019-10-30 12:07:06 UTC
And so we need a test.
Comment 11 Jonathan Druart 2019-11-04 11:09:54 UTC Comment hidden (obsolete)
Comment 12 Nick Clemens (kidclamp) 2019-11-04 14:29:19 UTC
Created attachment 95010 [details] [review]
Bug 23414: Unit tests

To test:
1 - Apply this patch alone
2 - prove -v t/db_dependent/XSLT.t
3 - All pass
4 - Apply other patches
5 - prove -v t/db_dependent/XSLT.t
6 - All pass
Comment 13 Nick Clemens (kidclamp) 2019-11-04 14:29:22 UTC
Created attachment 95011 [details] [review]
Bug 23414: Use Koha Item objects in buildKohaItemsNamespace

Removing the use of C4::GetItemsInfo cuts down on the number of DB requests and speeds things up

To test I added 100 items to each of the first 20 records returned by a search for 'a'
I saw a reduction from ~30 seconds to ~26 seconds

This also makes the code a little cleaner and moves us toward the Koha namespace

To test:
1 - Perform a search in the OPAC and STaff Client with the Browser Console opened (F12)
2 - View the netwrok tab and see how long the pages take to load
3 - Apply the patches, restart all the things
4 - Repeat and note if the results are returned faster
Comment 14 Nick Clemens (kidclamp) 2019-11-04 14:29:25 UTC
Created attachment 95012 [details] [review]
Bug 23414: Prefetch transfers, use only default framework, use elsif

This further cleans up the code, before we tested each condition though we return only one status line.

Koha mappings can no longer differ by framework so we don't need to fetch the biblio framework

Prefetching should offer a boost

To test:
Same as before - there is not as big a boost form this, but there shouldn't be a hit
Comment 15 Jonathan Druart 2019-11-04 14:44:56 UTC
Not sure about $item->itype, it seems that it should be $item->effective_itemtype
Comment 16 Jonathan Druart 2019-11-04 15:36:39 UTC
Created attachment 95014 [details] [review]
Bug 23414: Easy readability and simplify logic in buildKohaItemsNamespace

This patch improves true/false logic and avoid autovivication.
Also note that xml_escape already deals with empty string.

An unecessary call to GetReserveStatus is removed.
Comment 17 Jonathan Druart 2019-11-04 15:36:42 UTC
Created attachment 95015 [details] [review]
Bug 23414: Use effective_itemtype

We should use effective_itemtype instead of itype to take
the item-level_itypes pref into account.
Note that we should not need to test for the existence of the key as we
now assume that item types are correctly set to a valid item type.
Comment 18 Liz Rea 2019-11-05 18:30:45 UTC
Created attachment 95069 [details] [review]
Bug 23414: Use Koha Item objects in buildKohaItemsNamespace

Removing the use of C4::GetItemsInfo cuts down on the number of DB requests and speeds things up

To test I added 100 items to each of the first 20 records returned by a search for 'a'
I saw a reduction from ~30 seconds to ~26 seconds

This also makes the code a little cleaner and moves us toward the Koha namespace

To test:
1 - Perform a search in the OPAC and STaff Client with the Browser Console opened (F12)
2 - View the netwrok tab and see how long the pages take to load
3 - Apply the patches, restart all the things
4 - Repeat and note if the results are returned faster

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 19 Liz Rea 2019-11-05 18:30:53 UTC
Created attachment 95070 [details] [review]
Bug 23414: Prefetch transfers, use only default framework, use elsif

This further cleans up the code, before we tested each condition though we return only one status line.

Koha mappings can no longer differ by framework so we don't need to fetch the biblio framework

Prefetching should offer a boost

To test:
Same as before - there is not as big a boost form this, but there shouldn't be a hit

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 20 Liz Rea 2019-11-05 18:30:56 UTC
Created attachment 95071 [details] [review]
Bug 23414: Easy readability and simplify logic in buildKohaItemsNamespace

This patch improves true/false logic and avoid autovivication.
Also note that xml_escape already deals with empty string.

An unecessary call to GetReserveStatus is removed.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 21 Liz Rea 2019-11-05 18:30:58 UTC
Created attachment 95072 [details] [review]
Bug 23414: Use effective_itemtype

We should use effective_itemtype instead of itype to take
the item-level_itypes pref into account.
Note that we should not need to test for the existence of the key as we
now assume that item types are correctly set to a valid item type.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 22 Liz Rea 2019-11-05 18:34:02 UTC
Created attachment 95073 [details] [review]
Bug 23414: Unit tests

To test:
1 - Apply this patch alone
2 - prove -v t/db_dependent/XSLT.t
3 - All pass
4 - Apply other patches
5 - prove -v t/db_dependent/XSLT.t
6 - All pass

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 23 Jonathan Druart 2019-11-06 09:11:38 UTC
Created attachment 95083 [details] [review]
Bug 23414: Use Koha Item objects in buildKohaItemsNamespace

Removing the use of C4::GetItemsInfo cuts down on the number of DB requests and speeds things up

To test I added 100 items to each of the first 20 records returned by a search for 'a'
I saw a reduction from ~30 seconds to ~26 seconds

This also makes the code a little cleaner and moves us toward the Koha namespace

To test:
1 - Perform a search in the OPAC and STaff Client with the Browser Console opened (F12)
2 - View the netwrok tab and see how long the pages take to load
3 - Apply the patches, restart all the things
4 - Repeat and note if the results are returned faster

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2019-11-06 09:11:44 UTC
Created attachment 95084 [details] [review]
Bug 23414: Prefetch transfers, use only default framework, use elsif

This further cleans up the code, before we tested each condition though we return only one status line.

Koha mappings can no longer differ by framework so we don't need to fetch the biblio framework

Prefetching should offer a boost

To test:
Same as before - there is not as big a boost form this, but there shouldn't be a hit

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Jonathan Druart 2019-11-06 09:11:50 UTC
Created attachment 95085 [details] [review]
Bug 23414: Easy readability and simplify logic in buildKohaItemsNamespace

This patch improves true/false logic and avoid autovivication.
Also note that xml_escape already deals with empty string.

An unecessary call to GetReserveStatus is removed.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 26 Jonathan Druart 2019-11-06 09:11:55 UTC
Created attachment 95086 [details] [review]
Bug 23414: Use effective_itemtype

We should use effective_itemtype instead of itype to take
the item-level_itypes pref into account.
Note that we should not need to test for the existence of the key as we
now assume that item types are correctly set to a valid item type.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Jonathan Druart 2019-11-06 09:12:00 UTC
Created attachment 95087 [details] [review]
Bug 23414: Unit tests

To test:
1 - Apply this patch alone
2 - prove -v t/db_dependent/XSLT.t
3 - All pass
4 - Apply other patches
5 - prove -v t/db_dependent/XSLT.t
6 - All pass

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Jonathan Druart 2019-11-06 09:12:06 UTC
Created attachment 95088 [details] [review]
Bug 23414: Add tests for previous effective_itemtype change

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Jonathan Druart 2019-11-06 09:12:13 UTC
Created attachment 95089 [details] [review]
Bug 23414: Replace delimiter of regexs to ease readability

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Jonathan Druart 2019-11-06 09:15:44 UTC
Sorry, scope creep...
Comment 31 Martin Renvoize 2019-11-06 17:16:01 UTC
Well done getting this one over the finish line just in time for the freeze guys. I was about to update it to rel_20_05_target when I spotted the diligent final QAing.. Thanks!

Looks good, clarifies things and improves performance, Pushing :)
Comment 32 Martin Renvoize 2019-11-06 17:16:52 UTC
Nice work!

Pushed to master for 19.11.00
Comment 33 Lucas Gass 2019-12-10 22:50:20 UTC
enhancement won't be backported to 19.05.x