Bug 23414 - Improve performance of C4/XSLT/buildKohaItemsNamespace
Summary: Improve performance of C4/XSLT/buildKohaItemsNamespace
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks: 15262
  Show dependency treegraph
 
Reported: 2019-08-01 18:29 UTC by Nick Clemens
Modified: 2019-11-14 11:25 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00


Attachments
Bug 23414: Use Koha Item objects in buildKohaItemsNamespace (5.70 KB, patch)
2019-08-01 18:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23414: Prefetch transfers, use only default framework, use elsif (4.88 KB, patch)
2019-08-01 18:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23414: Prefetch reserves and use them for determining waiting (1.71 KB, patch)
2019-08-01 18:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23414: Use Koha::Holds->waiting (734 bytes, patch)
2019-10-22 12:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23414: Use Koha Item objects in buildKohaItemsNamespace (5.70 KB, patch)
2019-10-22 15:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23414: Prefetch transfers, use only default framework, use elsif (4.98 KB, patch)
2019-10-22 15:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23414: Unit tests (4.13 KB, patch)
2019-11-04 14:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23414: Use Koha Item objects in buildKohaItemsNamespace (5.70 KB, patch)
2019-11-04 14:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23414: Prefetch transfers, use only default framework, use elsif (4.98 KB, patch)
2019-11-04 14:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23414: Easy readability and simplify logic in buildKohaItemsNamespace (3.28 KB, patch)
2019-11-04 15:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23414: Use effective_itemtype (1.24 KB, patch)
2019-11-04 15:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23414: Use Koha Item objects in buildKohaItemsNamespace (5.75 KB, patch)
2019-11-05 18:30 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 23414: Prefetch transfers, use only default framework, use elsif (5.03 KB, patch)
2019-11-05 18:30 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 23414: Easy readability and simplify logic in buildKohaItemsNamespace (3.32 KB, patch)
2019-11-05 18:30 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 23414: Use effective_itemtype (1.28 KB, patch)
2019-11-05 18:30 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 23414: Unit tests (4.17 KB, patch)
2019-11-05 18:34 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 23414: Use Koha Item objects in buildKohaItemsNamespace (5.82 KB, patch)
2019-11-06 09:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23414: Prefetch transfers, use only default framework, use elsif (5.10 KB, patch)
2019-11-06 09:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23414: Easy readability and simplify logic in buildKohaItemsNamespace (3.39 KB, patch)
2019-11-06 09:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23414: Use effective_itemtype (1.35 KB, patch)
2019-11-06 09:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23414: Unit tests (4.24 KB, patch)
2019-11-06 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23414: Add tests for previous effective_itemtype change (3.06 KB, patch)
2019-11-06 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23414: Replace delimiter of regexs to ease readability (5.13 KB, patch)
2019-11-06 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 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 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 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 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 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 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 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 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 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