|
Description
David Gustafsson
2022-11-01 16:31:37 UTC
Created attachment 142894 [details] [review] 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 Created attachment 144348 [details] [review] 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 Please rebase on top of bug 30920 (that is pushed to master already). Created attachment 150866 [details] [review] 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 Rebased against master. Since caching of GetAuthorisedValueDesc is now in master, the performance gain will not be as large, but still substantial. 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?
(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. 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. 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.
Accidentally omitted
goto &{$AUTOLOAD};
which should be on the last line.
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. Created attachment 151131 [details] [review] 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 Created attachment 151132 [details] [review] 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 Created attachment 151133 [details] [review] 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 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.
Created attachment 151291 [details] [review] 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> Created attachment 151292 [details] [review] 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> Nick for QA? Did we look for other occurrences of $biblio->frameworkcode that might benefit from this caching? Created attachment 151464 [details] [review] 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> Created attachment 151465 [details] [review] 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> Pushed to master for 23.11. Nice work everyone, thanks! 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. Nice work everyone! Pushed to oldstable for 22.11.x |