[Bug 32060] New: Improve performance of Koha::Item->columns_to_str
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 Bug ID: 32060 Summary: Improve performance of Koha::Item->columns_to_str 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: glasklas@gmail.com QA Contact: testopia@bugs.koha-community.org When creating a new item for a biblio with many items, for example serials which can have several hundreds, the table of existing items that is shown is currently very performance heavy to generate. The main culprit is Koha::Item->columns_to_str that can be sped up substantially by caching data that is looked up repeatedly for each call. With this patch columns_to_str will run about ~70x faster, reducing the load time of /cataloguing/additem.pl?biblionumber=... to about one fifth for a biblio with about 150 items (and even more with increasing number of items). -- 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=32060 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |glasklas@gmail.com |ity.org | -- 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=32060 --- Comment #1 from David Gustafsson <glasklas@gmail.com> --- Created attachment 142894 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142894&action=edit Bug 32060: Improve performance of columns_to_str To test: 1) Ensure tests in t/db_dependent/Koha/Item.t all pass 2) Go to a biblio (preferably as serial) with many items and click "New" -> "New item" and take not of the response time. 3) Apply the patch. 4) Ensure tests in t/db_dependent/Koha/Item.t still pass 5) Repeat step 1), the response time should be substantially improved. Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #142894|0 |1 is obsolete| | --- Comment #2 from David Gustafsson <glasklas@gmail.com> --- Created attachment 144348 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144348&action=edit Bug 32060: Improve performance of columns_to_str To test: 1) Ensure tests in t/db_dependent/Koha/Item.t all pass 2) Go to a biblio (preferably as serial) with many items and click "New" -> "New item" and take not of the response time. 3) Apply the patch. 4) Ensure tests in t/db_dependent/Koha/Item.t still pass 5) Repeat step 1), the response time should be substantially improved. Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=32462 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #3 from Fridolin Somers <fridolin.somers@biblibre.com> --- This is close to Bug 30920 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com Depends on| |30920 Status|Needs Signoff |Patch doesn't apply --- Comment #4 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Please rebase on top of bug 30920 (that is pushed to master already). Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 [Bug 30920] Add caching to C4::Biblio::GetAuthorisedValueDesc -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144348|0 |1 is obsolete| | --- Comment #5 from David Gustafsson <glasklas@gmail.com> --- Created attachment 150866 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150866&action=edit Bug 32060: Improve performance of columns_to_str To test: 1) Ensure tests in t/db_dependent/Koha/Item.t all pass 2) Go to a biblio (preferably as serial) with many items and click "New" -> "New item" and take not of the response time. 3) Apply the patch. 4) Ensure tests in t/db_dependent/Koha/Item.t still pass 5) Repeat step 1), the response time should be substantially improved. Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #6 from David Gustafsson <glasklas@gmail.com> --- Rebased against master. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 --- Comment #7 from David Gustafsson <glasklas@gmail.com> --- Since caching of GetAuthorisedValueDesc is now in master, the performance gain will not be as large, but still substantial. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 --- Comment #8 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 1. Then I would C4::Biblio::GetFrameworkCode and cache there. 2. - my $tagslib = C4::Biblio::GetMarcStructure(1, $frameworkcode); + my $tagslib = C4::Biblio::GetMarcStructure( 1, $frameworkcode, { unsafe => 1 } ); Yes, definitely. 3. - my $value = $self->$column; + my $value = $self->_result()->get_column($column); Really faster? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 --- Comment #9 from David Gustafsson <glasklas@gmail.com> --- (In reply to Jonathan Druart from comment #8)
1. Then I would C4::Biblio::GetFrameworkCode and cache there.
2. - my $tagslib = C4::Biblio::GetMarcStructure(1, $frameworkcode); + my $tagslib = C4::Biblio::GetMarcStructure( 1, $frameworkcode, { unsafe => 1 } );
Yes, definitely.
3. - my $value = $self->$column; + my $value = $self->_result()->get_column($column);
Really faster?
1. Could do that, but that change is a little bit scarier as we don't expire the cache and GetFrameworkCode is used extensively in other places. But could be done in ModBiblio, which is simple enough I guess. Would also speed up other calls to GetFrameworkCode, doubt would have much impact as it's already pretty fast and columns_to_str most likely the only place it makes much of a difference, but you never know. 3. I'm quite surprised myself, but using $self->$column is about 1.4x slower for a biblio with 1400 items. My guess the offending line of code is: if ( grep { $_ eq $method } @columns ) { in AUTOLOAD. Looping over @columns has O(n) time complexity but using a columns_info hash where columns can be looked in O(1) time. Could be wworth looking into in another bug to generally speed up accessing of properties in Koha. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 --- Comment #10 from David Gustafsson <glasklas@gmail.com> --- No, the loop is not it, I think AUTOLOAD in itself is slow, perhaps parsing of method name etc. I think there are techniques to speed this up by lazily defining actual setter/getter methods on AUTOLOAD, and could be worth looking into. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 --- Comment #11 from David Gustafsson <glasklas@gmail.com> --- This actually speeds speeds up so that calling the property method directly is as fast as$self->_result()->get_column($column) if ( grep { $_ eq $method } @columns ) { # if ( @_ ) { # $self->_result()->set_column( $method, @_); # return $self; #} else { # my $value = $self->_result()->get_column( $method ); # return $value; #} no strict 'refs'; *{$AUTOLOAD} = sub { my $self = shift; if ( @_ ) { $self->_result()->set_column( $method, @_); return $self; } else { my $value = $self->_result()->get_column( $method ); return $value; } }; This lazily defined new methods in the symbol table, and next time that method will be called instead of AUTOLOAD. Will have a look at creating another bug for this change. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 --- Comment #12 from David Gustafsson <glasklas@gmail.com> --- Accidentally omitted goto &{$AUTOLOAD}; which should be on the last line. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 --- Comment #13 from David Gustafsson <glasklas@gmail.com> --- And about 3. above, it was poorly formulated, I meant that using $self->$column results in the page taking 40% longer to generate, not that $self->$column is %40 more expensive than get_column. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150866|0 |1 is obsolete| | --- Comment #14 from David Gustafsson <glasklas@gmail.com> --- Created attachment 151131 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151131&action=edit Bug 32060: Improve performance of columns_to_str To test: 1) Ensure the following tests pass t/db_dependent/Koha/Item.t t/db_dependent/Koha/Bibio.t 2) Go to a biblio (preferably as serial) with many items and click "New" -> "New item" and note down the response time. 3) Apply the patch 4) Ensure tests in 1) still pass 5) Repeat step 2), the response time should be substantially improved Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #151131|0 |1 is obsolete| | --- Comment #15 from David Gustafsson <glasklas@gmail.com> --- Created attachment 151132 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151132&action=edit Bug 32060: Improve performance of columns_to_str To test: 1) Ensure the following tests pass t/db_dependent/Koha/Item.t t/db_dependent/Koha/Bibio.t 2) Go to a biblio (preferably as serial) with many items and click "New" -> "New item" and note down the response time. 3) Apply the patch 4) Ensure tests in 1) still pass 5) Repeat step 2), the response time should be substantially improved Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #151132|0 |1 is obsolete| | --- Comment #16 from David Gustafsson <glasklas@gmail.com> --- Created attachment 151133 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151133&action=edit Bug 32060: Improve performance of columns_to_str To test: 1) Ensure the following tests pass t/db_dependent/Koha/Item.t t/db_dependent/Koha/Bibio.t 2) Go to a biblio (preferably as serial) with many items and click "New" -> "New item" and note down the response time. 3) Apply the patch 4) Ensure tests in 1) still pass 5) Repeat step 2), the response time should be substantially improved Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 --- Comment #17 from David Gustafsson <glasklas@gmail.com> --- Added caching for GetFrameworkCode and now using that instead. Also saw that strings_map had the same issues as columns_to_str since they share some code, so applied the same changes there. Removed get_column and will create another bug for making columns methods as fast. Also removed: my $columns_info = $self->_result->result_source->columns_info; for my $column ( keys %$columns_info ) { and replaced by: for my $column ( @{$self->_columns}) { in columns_to_str and strings_map. Not much of a difference in terms of speed, but both confusing and unnecessary to fetch the whole info when you just need the column names. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=33745 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 Jonathan Druart <jonathan.druart+koha@gmail.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=32060 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #151133|0 |1 is obsolete| | --- Comment #18 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 151291 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151291&action=edit Bug 32060: Improve performance of columns_to_str To test: 1) Ensure the following tests pass t/db_dependent/Koha/Item.t t/db_dependent/Koha/Bibio.t 2) Go to a biblio (preferably as serial) with many items and click "New" -> "New item" and note down the response time. 3) Apply the patch 4) Ensure tests in 1) still pass 5) Repeat step 2), the response time should be substantially improved Sponsored-by: Gothenburg University Library Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 --- Comment #19 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 151292 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151292&action=edit Bug 32060: Use selectrow_array Trivial ergonomic patch, use selectrow_array, no need to prepare the query + perltidy the sub Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #20 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Nick for QA? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |33746 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33746 [Bug 33746] [omnibus] speed improvement -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtj@kohaaloha.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |33746 Depends on|33746 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33746 [Bug 33746] [omnibus] speed improvement -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #21 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Did we look for other occurrences of $biblio->frameworkcode that might benefit from this caching? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> 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=32060 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #151291|0 |1 is obsolete| | --- Comment #22 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 151464 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151464&action=edit Bug 32060: Improve performance of columns_to_str To test: 1) Ensure the following tests pass t/db_dependent/Koha/Item.t t/db_dependent/Koha/Bibio.t 2) Go to a biblio (preferably as serial) with many items and click "New" -> "New item" and note down the response time. 3) Apply the patch 4) Ensure tests in 1) still pass 5) Repeat step 2), the response time should be substantially improved Sponsored-by: Gothenburg University Library Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #151292|0 |1 is obsolete| | --- Comment #23 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 151465 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151465&action=edit Bug 32060: Use selectrow_array Trivial ergonomic patch, use selectrow_array, no need to prepare the query + perltidy the sub Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- 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=32060 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |23.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 --- Comment #24 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #25 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Great work here.. RMaints, I think this should be safely backported back to 22.11 which is when 30920 came in... for libraries getting hit by this performance issue this really constitutes a bugfix. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |minor -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |matt.blenkinsop@ptfs-europe | |.com Status|Pushed to master |Pushed to stable Version(s)|23.11.00 |23.11.00,23.05.01 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|23.11.00,23.05.01 |23.11.00,23.05.01,22.11.07 released in| | Status|Pushed to stable |Pushed to oldstable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32060 --- Comment #26 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- Nice work everyone! Pushed to oldstable for 22.11.x -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org