Bug 32060 - Improve performance of Koha::Item->columns_to_str
Summary: Improve performance of Koha::Item->columns_to_str
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: David Gustafsson
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 30920
Blocks: 33746
  Show dependency treegraph
 
Reported: 2022-11-01 16:31 UTC by David Gustafsson
Modified: 2023-12-28 20:47 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.01,22.11.07


Attachments
Bug 32060: Improve performance of columns_to_str (6.30 KB, patch)
2022-11-01 16:46 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 32060: Improve performance of columns_to_str (6.27 KB, patch)
2022-11-30 15:21 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 32060: Improve performance of columns_to_str (2.06 KB, patch)
2023-05-09 13:40 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 32060: Improve performance of columns_to_str (6.17 KB, patch)
2023-05-12 13:27 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 32060: Improve performance of columns_to_str (5.72 KB, patch)
2023-05-12 13:28 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 32060: Improve performance of columns_to_str (5.72 KB, patch)
2023-05-12 13:29 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 32060: Improve performance of columns_to_str (5.79 KB, patch)
2023-05-16 14:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32060: Use selectrow_array (1.65 KB, patch)
2023-05-16 14:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32060: Improve performance of columns_to_str (5.88 KB, patch)
2023-05-19 09:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32060: Use selectrow_array (1.74 KB, patch)
2023-05-19 09:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Gustafsson 2022-11-01 16:31:37 UTC
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).
Comment 1 David Gustafsson 2022-11-01 16:46:55 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
Comment 2 David Gustafsson 2022-11-30 15:21:35 UTC
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
Comment 3 Fridolin Somers 2022-12-13 19:55:24 UTC
This is close to Bug 30920
Comment 4 Jonathan Druart 2023-05-04 12:56:42 UTC
Please rebase on top of bug 30920 (that is pushed to master already).
Comment 5 David Gustafsson 2023-05-09 13:40:02 UTC
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
Comment 6 David Gustafsson 2023-05-09 13:40:45 UTC
Rebased against master.
Comment 7 David Gustafsson 2023-05-09 13:47:23 UTC
Since caching of GetAuthorisedValueDesc is now in master, the performance gain will not be as large, but still substantial.
Comment 8 Jonathan Druart 2023-05-10 04:47:36 UTC
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?
Comment 9 David Gustafsson 2023-05-11 15:08:23 UTC
(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.
Comment 10 David Gustafsson 2023-05-11 15:40:30 UTC
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.
Comment 11 David Gustafsson 2023-05-11 16:28:17 UTC
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.
Comment 12 David Gustafsson 2023-05-11 16:30:46 UTC
Accidentally omitted
goto &{$AUTOLOAD};
which should be on the last line.
Comment 13 David Gustafsson 2023-05-11 16:45:15 UTC
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.
Comment 14 David Gustafsson 2023-05-12 13:27:14 UTC
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
Comment 15 David Gustafsson 2023-05-12 13:28:40 UTC
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
Comment 16 David Gustafsson 2023-05-12 13:29:34 UTC
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
Comment 17 David Gustafsson 2023-05-12 13:38:30 UTC
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.
Comment 18 Jonathan Druart 2023-05-16 14:39:40 UTC
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>
Comment 19 Jonathan Druart 2023-05-16 14:39:43 UTC
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>
Comment 20 Jonathan Druart 2023-05-16 14:44:22 UTC
Nick for QA?
Comment 21 Marcel de Rooy 2023-05-19 09:34:23 UTC
Did we look for other occurrences of $biblio->frameworkcode that might benefit from this caching?
Comment 22 Marcel de Rooy 2023-05-19 09:36:36 UTC
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>
Comment 23 Marcel de Rooy 2023-05-19 09:36:39 UTC
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>
Comment 24 Tomás Cohen Arazi 2023-06-06 19:20:36 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 25 Martin Renvoize 2023-06-08 06:45:06 UTC
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.
Comment 26 Matt Blenkinsop 2023-06-12 11:42:56 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x