[Bug 28371] New: Improve performance of XSLTParse4Display
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Bug ID: 28371 Summary: Improve performance of XSLTParse4Display Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: nick@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org This routine is quite slow, and heavy on DB lookups -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #1 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 121111 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121111&action=edit 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=28373 Blocks| |15262 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15262 [Bug 15262] Run Koha Run. Koha search should be fast again. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Björn Nylén <bjorn.nylen@ub.lu.se> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bjorn.nylen@ub.lu.se --- Comment #2 from Björn Nylén <bjorn.nylen@ub.lu.se> --- 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 are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #3 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #4 from David Cook <dcook@prosentient.com.au> --- Comment on attachment 121111 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121111 Bug 28371: WIP Review of attachment 121111: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=28371&attachment=121111) ----------------------------------------------------------------- ::: 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... -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #5 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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). -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #7 from Björn Nylén <bjorn.nylen@ub.lu.se> --- (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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |emmi.takkinen@koha-suomi.fi -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #8 from Fridolin Somers <fridolin.somers@biblibre.com> --- *** Bug 28649 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #121111|0 |1 is obsolete| | --- Comment #9 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 124086 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=124086&action=edit Bug 28371: Unit tests -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #10 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 124087 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=124087&action=edit 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** -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=23035 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com Keywords| |rel_21_11_candidate -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joonas.kylmala@helsinki.fi --- Comment #11 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- *** Bug 23035 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #12 from David Nind <david@davidnind.com> --- Created attachment 127082 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127082&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #13 from David Nind <david@davidnind.com> --- Created attachment 127083 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127083&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Attachment #127082|0 |1 is obsolete| | --- Comment #14 from David Nind <david@davidnind.com> --- Comment on attachment 127082 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127082 Bug 28959: DBRev 21.06.00.039 Not sure what I did here - obsoleting, -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127083|0 |1 is obsolete| | --- Comment #15 from David Nind <david@davidnind.com> --- Comment on attachment 127083 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127083 Bug 28959: DBIC update Not sure what I did here - obsoleting... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #16 from David Nind <david@davidnind.com> --- I think I didn't actually apply the patches! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply --- Comment #17 from David Nind <david@davidnind.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #124086|0 |1 is obsolete| | Attachment #124087|0 |1 is obsolete| | --- Comment #18 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 127090 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127090&action=edit Bug 28371: Unit tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #19 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 127091 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127091&action=edit 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** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #20 from David Nind <david@davidnind.com> --- 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... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_21_11_candidate |rel_22_05_candidate -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Andreas Jonsson <andreas.jonsson@kreablo.se> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andreas.jonsson@kreablo.se -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Séverine Queune <severine.queune@bulac.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |severine.queune@bulac.fr --- Comment #21 from Séverine Queune <severine.queune@bulac.fr> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127090|0 |1 is obsolete| | Attachment #127091|0 |1 is obsolete| | --- Comment #22 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 132801 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=132801&action=edit Bug 28371: Unit tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #23 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 132802 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=132802&action=edit 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** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #132801|0 |1 is obsolete| | Attachment #132802|0 |1 is obsolete| | --- Comment #24 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 132943 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=132943&action=edit Bug 28371: Unit tests Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #25 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 132944 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=132944&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #132943|0 |1 is obsolete| | --- Comment #26 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 133719 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=133719&action=edit Bug 28371: Unit tests Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #132944|0 |1 is obsolete| | --- Comment #27 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 133720 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=133720&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |22.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #28 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to master for 22.05, thanks to everybody involved 🦄 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #29 from Fridolin Somers <fridolin.somers@biblibre.com> --- I hope this performance fix can be backported to 21.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed --- Comment #30 from Fridolin Somers <fridolin.somers@biblibre.com> --- 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 );
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |ASSIGNED Version(s)|22.05.00 | released in| | --- Comment #31 from Fridolin Somers <fridolin.somers@biblibre.com> --- Reverted from master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #32 from Fridolin Somers <fridolin.somers@biblibre.com> --- We may reconsider keeping code logic and add cache -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #33 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 133872 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=133872&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #34 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #35 from Fridolin Somers <fridolin.somers@biblibre.com> --- (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 ;) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_22_05_candidate | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #36 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Does this get cn_source right? GetAuthorisedValueDesc appears to have some special handling for that 'category' but I'm not seeing the same here? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #37 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #38 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 135373 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135373&action=edit Bug 28371: (folllow-up) Cover cn_source case for transforming av -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #39 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 135381 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135381&action=edit Bug 28371: (follow-up) Fetch avs per framework -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #40 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #41 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #42 from David Cook <dcook@prosentient.com.au> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #43 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Oop, bug 30848 I meant -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=30848 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=30920 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #44 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |RESOLVED Resolution|--- |WONTFIX --- Comment #45 from Nick Clemens <nick@bywatersolutions.com> --- Deprecating in favor of 30920 / 30848 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 --- Comment #46 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28371 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org