Bug 30920 - Add caching to C4::Biblio::GetAuthorisedValueDesc
Summary: Add caching to C4::Biblio::GetAuthorisedValueDesc
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 32060 33214
  Show dependency treegraph
 
Reported: 2022-06-07 13:47 UTC by Martin Renvoize
Modified: 2023-12-28 20:42 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.04


Attachments
Bug 30920: Add caching to GetAuthorizedValueDesc (10.07 KB, patch)
2022-06-07 13:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30920: Add caching to GetAuthorizedValueDesc (10.12 KB, patch)
2022-06-17 15:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30920: (follow-up) Add missing use (710 bytes, patch)
2022-06-17 15:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30920: Add caching to GetAuthorizedValueDesc (10.11 KB, patch)
2022-12-23 12:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30920: (follow-up) Add missing use (689 bytes, patch)
2022-12-23 12:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30920: Update cache keys (1.71 KB, patch)
2022-12-23 12:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30920: Add caching to GetAuthorizedValueDesc (10.12 KB, patch)
2023-03-10 12:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30920: (follow-up) Add missing use (691 bytes, patch)
2023-03-10 12:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30920: Update cache keys (1.71 KB, patch)
2023-03-10 12:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30920: (QA follow-up) Use get_dirty_columns to avoid DB query (1.20 KB, patch)
2023-03-10 12:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30920: Add caching to GetAuthorizedValueDesc (10.17 KB, patch)
2023-03-10 16:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30920: (follow-up) Add missing use (754 bytes, patch)
2023-03-10 16:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30920: Update cache keys (1.77 KB, patch)
2023-03-10 16:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30920: (QA follow-up) Use get_dirty_columns to avoid DB query (1.26 KB, patch)
2023-03-10 16:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30920: (follow-up) Cache key fixes (2.57 KB, patch)
2023-03-10 16:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30920: (follow-up) Flush on delete (5.16 KB, patch)
2023-03-10 16:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30920: (follow-up) Cleanup warning from C4::Biblio::GetAuthorisedValueDesc (898 bytes, patch)
2023-03-10 16:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30920: (follow-up) Manually flush caches in tests (2.89 KB, patch)
2023-03-10 16:38 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2022-06-07 13:47:27 UTC
Whilst working on bug 30848 inspired by bug 28371 it became apparent we're using this function a huge amount inside of various loops.

We should cache the data to allow for performance improvements.

The patches submitted here have been pulled out of the submission for bug 30848 as  they have a much wider effect and need proper independent testing.  In benchmarking I found that enabling the caching in the way here gave me very significant performance improvements as compared to bug 28371.
Comment 1 Martin Renvoize 2022-06-07 13:50:24 UTC
Created attachment 135771 [details] [review]
Bug 30920: Add caching to GetAuthorizedValueDesc

This patch adds caching to C4::Biblio::GetAuthorisedValueDesc for performance.

We cache each hash involved in the method use for description lookup and
we're careful to clear said caches appropraitely on changes.
Comment 2 Jonathan Druart 2022-06-07 14:25:34 UTC
I thought this sub was going to be removed.
We could improve it that way, but then it should be used from the other places (git grep GetClassSources).
Comment 3 Martin Renvoize 2022-06-07 14:30:57 UTC
Baby steps back at you ;P.

I agree it would be nice to shift this stuff to Koha::.. but that's for another day.
Comment 4 Jonathan Druart 2022-06-07 14:42:49 UTC
Yes sure, my main point was about the removal. I thought it was kind of obsolete.
Comment 5 Jonathan Druart 2022-06-07 15:19:08 UTC
(In reply to Jonathan Druart from comment #2)
> We could improve it that way, but then it should be used from the other
> places (git grep GetClassSources).

This is actually not relevant. Other occurrences are building the dropdown list.
Comment 6 Jonathan Druart 2022-06-08 07:18:03 UTC
Quick remark about the naming. I would suggest to use ':' to separate concept in the cache_key, and from more generic to more specific.

$lang . 'ItemTypeDescriptions' => "itemtypes:description:$lang"

"AVDescriptions-" . $category => AV_descriptions:category (we already have AV_descriptions somewhere else)

LibraryNames => libraries:name
Comment 7 Jonathan Druart 2022-06-08 12:42:06 UTC
(In reply to Martin Renvoize from comment #0)
> In benchmarking I found that enabling the caching in the way here gave me
> very significant performance improvements as compared to bug 28371.

Do you have the numbers?

Did you identify where we are using this subroutine expansively?
Comment 8 Martin Renvoize 2022-06-09 08:37:23 UTC
The biggest impact is in the search results loops.. it was all inspired really by Nicks work on bug 28371.

I do have a benchmark script I was using but it needs a little adapting to get it working with this latest version of the tree. It compared not passing hashed along as params, passing hashes along as params, using my ExpandCodedFields stuff and then the caching on top.
Comment 9 Martin Renvoize 2022-06-09 08:39:39 UTC
                     Rate biblio10_no biblio1_no biblio100_no biblio0_no biblio1_pass biblio100_pass biblio0_pass biblio10_pass biblio100_process biblio10_process biblio1_process biblio0_process
biblio10_no         108/s          --       -25%         -25%       -27%         -98%           -98%         -98%          -98%              -99%             -99%            -99%            -99%
biblio1_no          143/s         32%         --          -1%        -3%         -97%           -97%         -97%          -97%              -99%             -99%            -99%            -99%
biblio100_no        145/s         34%         1%           --        -2%         -97%           -97%         -97%          -97%              -99%             -99%            -99%            -99%
biblio0_no          147/s         36%         3%           2%         --         -97%           -97%         -97%          -97%              -99%             -99%            -99%            -99%
biblio1_pass       5101/s       4618%      3461%        3424%      3368%           --            -0%          -5%           -8%              -57%             -63%            -64%            -65%
biblio100_pass     5112/s       4628%      3469%        3431%      3375%           0%             --          -5%           -8%              -57%             -63%            -64%            -65%
biblio0_pass       5360/s       4857%      3642%        3603%      3544%           5%             5%           --           -3%              -55%             -61%            -62%            -64%
biblio10_pass      5533/s       5017%      3762%        3722%      3661%           8%             8%           3%            --              -53%             -60%            -61%            -63%
biblio100_caching 11804/s      10817%      8140%        8054%      7924%         131%           131%         120%          113%                --             -14%            -16%            -20%
biblio10_caching  13731/s      12600%      9486%        9386%      9234%         169%           169%         156%          148%               16%               --             -3%             -7%
biblio1_caching   14121/s      12961%      9758%        9655%      9500%         177%           176%         163%          155%               20%               3%              --             -4%
biblio0_caching   14778/s      13569%     10217%       10109%      9946%         190%           189%         176%          167%               25%               8%              5%              --

Those are the results prior to me splitting this caching piece out of the ExpandCodedFields.. I will run it again once I've checked out the right set of stuff again and got the benchmark script working again.
Comment 10 Martin Renvoize 2022-06-09 08:42:54 UTC
I'm happy for this stuff to be a long target for this cycle though and work on migrating the method to Koha::.. or rather split it into parts or whatever we decide.
Comment 11 Fridolin Somers 2022-06-14 20:37:58 UTC
(In reply to Martin Renvoize from comment #8)
> The biggest impact is in the search results loops.. 
I'm looking at items search where performance is quite bad.
Hope this can have a good impact.
Comment 12 Fridolin Somers 2022-06-14 20:54:54 UTC
Just to be sure.

The is already a L1 cache for authorized values in get_description(s)_by_koha_field()
See Bug 17642 
It says L2 cache is unsafe :
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17642#c35

If L2 cache is fine and flush managed in store().
Could we cache directly in find() ?
That whould increase performance everywhere using ORM.
Comment 13 Nick Clemens 2022-06-17 15:56:05 UTC
Created attachment 136292 [details] [review]
Bug 30920: Add caching to GetAuthorizedValueDesc

This patch adds caching to C4::Biblio::GetAuthorisedValueDesc for performance.

We cache each hash involved in the method use for description lookup and
we're careful to clear said caches appropraitely on changes.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Nick Clemens 2022-06-17 15:56:09 UTC
Created attachment 136293 [details] [review]
Bug 30920: (follow-up) Add missing use

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Nick Clemens 2022-06-17 15:59:33 UTC
(In reply to Jonathan Druart from comment #7)
> (In reply to Martin Renvoize from comment #0)
> > In benchmarking I found that enabling the caching in the way here gave me
> > very significant performance improvements as compared to bug 28371.
> 
> Do you have the numbers?
> 
> Did you identify where we are using this subroutine expansively?

searchResults calls this a fair amount of times, the time spent in this sub is reduced by 50% and seems to be more dramatic under ES than Zebra - I saw a 2 second improvement in NYTProf after the patches

I think this does also set a good policy, and the caches here can be used elsewhere too
Comment 16 Martin Renvoize 2022-06-17 16:13:00 UTC Comment hidden (obsolete)
Comment 17 Martin Renvoize 2022-06-17 16:13:41 UTC
(In reply to Fridolin Somers from comment #12)
> Just to be sure.
> 
> The is already a L1 cache for authorized values in
> get_description(s)_by_koha_field()
> See Bug 17642 
> It says L2 cache is unsafe :
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17642#c35
> 
> If L2 cache is fine and flush managed in store().
> Could we cache directly in find() ?
> That would increase performance everywhere using ORM.

Caching a find doesn't make much sense.. simple key lookup is super fast in SQL. What's worth caching is complex searches or complex structures built from multiple searches.. especially when we're running them inside loops.

This change makes sense because we call the function inside the search result loop a lot and it's a fairly complex structure so having it immediately there to lookup from is good.

As for 'not safe'.. I have no idea where that comment comes from. So long as we catch flushing correctly I don't see how any L2 is unsafe as such... Sometimes it's not worthwhile, but not unsafe if it's done correctly.
Comment 18 Fridolin Somers 2022-06-20 06:32:38 UTC
Great thanks for this details Martin.
Comment 19 Jonathan Druart 2022-07-04 12:47:37 UTC
I am sorry but I don't manage to convince myself that we are doing it correctly.

I think this change is coming whereas we don't use the same code to display the descriptions, and it could lead to confusion (theoretically at least): fetch from cache and fetch from DB.

There is at least one thing that could go wrong, itemtype's descriptions. First we only deal with 'en', then it is actually wrong with 'en': it could be overwritten by the "Translate into other languages" feature (yes that's a weird use case but the UI allows it).

Also comment 6 hasn't been addressed.

Letting it for another QAer.
Comment 20 Martin Renvoize 2022-07-12 07:35:27 UTC
OK, I missed comment 6 entirely.. I'll take a look at your suggestions now.
Comment 21 Katrin Fischer 2022-08-12 14:19:27 UTC
(In reply to Martin Renvoize from comment #20)
> OK, I missed comment 6 entirely.. I'll take a look at your suggestions now.

Setting to Failed QA to get some attention :)
Comment 22 Nick Clemens 2022-12-23 12:58:07 UTC
Created attachment 144817 [details] [review]
Bug 30920: Add caching to GetAuthorizedValueDesc

This patch adds caching to C4::Biblio::GetAuthorisedValueDesc for performance.

We cache each hash involved in the method use for description lookup and
we're careful to clear said caches appropraitely on changes.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 23 Nick Clemens 2022-12-23 12:58:12 UTC
Created attachment 144818 [details] [review]
Bug 30920: (follow-up) Add missing use

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 24 Nick Clemens 2022-12-23 12:58:16 UTC
Created attachment 144819 [details] [review]
Bug 30920: Update cache keys

This changes the cache keys to use ':' to separate levels
and orders from generic to specific
Comment 25 Nick Clemens 2022-12-23 13:00:03 UTC
(In reply to Jonathan Druart from comment #19)
> I think this change is coming whereas we don't use the same code to display
> the descriptions, and it could lead to confusion (theoretically at least):
> fetch from cache and fetch from DB.

Can you explain this a bit more? Where are we not using this that could be a conflict?

> There is at least one thing that could go wrong, itemtype's descriptions.
> First we only deal with 'en', then it is actually wrong with 'en': it could
> be overwritten by the "Translate into other languages" feature (yes that's a
> weird use case but the UI allows it).

In testing I can provide a French translation and the correct one shows when changing languages in search results on both staff and OPAC

> Also comment 6 hasn't been addressed.

Addressed
Comment 26 Jonathan Druart 2023-01-03 14:12:31 UTC
(In reply to Nick Clemens from comment #25)
> (In reply to Jonathan Druart from comment #19)
> > I think this change is coming whereas we don't use the same code to display
> > the descriptions, and it could lead to confusion (theoretically at least):
> > fetch from cache and fetch from DB.
> 
> Can you explain this a bit more? Where are we not using this that could be a
> conflict?

Everywhere we fetch branchname, itemtype's description, av's lib (mainly from C4)

> > There is at least one thing that could go wrong, itemtype's descriptions.
> > First we only deal with 'en', then it is actually wrong with 'en': it could
> > be overwritten by the "Translate into other languages" feature (yes that's a
> > weird use case but the UI allows it).

> In testing I can provide a French translation and the correct one shows when
> changing languages in search results on both staff and OPAC

Sorry, I think it didn't make sense. I guess I was referring to Koha::Itemtype->store

+    if ($flush) {
+        my $cache = Koha::Caches->get_instance();
+        my $key = "enItemTypeDescriptions";
+        $cache->clear_from_cache($key);
+    }

But it actually sounds good after having a second look at it.
Comment 27 Tomás Cohen Arazi 2023-03-10 12:10:08 UTC
Created attachment 148064 [details] [review]
Bug 30920: Add caching to GetAuthorizedValueDesc

This patch adds caching to C4::Biblio::GetAuthorisedValueDesc for performance.

We cache each hash involved in the method use for description lookup and
we're careful to clear said caches appropraitely on changes.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 28 Tomás Cohen Arazi 2023-03-10 12:10:11 UTC
Created attachment 148065 [details] [review]
Bug 30920: (follow-up) Add missing use

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 29 Tomás Cohen Arazi 2023-03-10 12:10:14 UTC
Created attachment 148066 [details] [review]
Bug 30920: Update cache keys

This changes the cache keys to use ':' to separate levels
and orders from generic to specific
Comment 30 Tomás Cohen Arazi 2023-03-10 12:10:16 UTC
Created attachment 148067 [details] [review]
Bug 30920: (QA follow-up) Use get_dirty_columns to avoid DB query

We can rely on DBIC's get_dirty_columns to check if the descriptions
have changed, and set things for flushing without an extra DB query.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 31 Tomás Cohen Arazi 2023-03-10 14:46:21 UTC
Failing because ->delete is not invalidating cache. Can be verified by running:

prove t/db_dependent/Koha/Item.t
Comment 32 Martin Renvoize 2023-03-10 16:37:58 UTC
Created attachment 148087 [details] [review]
Bug 30920: Add caching to GetAuthorizedValueDesc

This patch adds caching to C4::Biblio::GetAuthorisedValueDesc for performance.

We cache each hash involved in the method use for description lookup and
we're careful to clear said caches appropraitely on changes.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Martin Renvoize 2023-03-10 16:38:01 UTC
Created attachment 148088 [details] [review]
Bug 30920: (follow-up) Add missing use

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Martin Renvoize 2023-03-10 16:38:04 UTC
Created attachment 148089 [details] [review]
Bug 30920: Update cache keys

This changes the cache keys to use ':' to separate levels
and orders from generic to specific

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 35 Martin Renvoize 2023-03-10 16:38:07 UTC
Created attachment 148090 [details] [review]
Bug 30920: (QA follow-up) Use get_dirty_columns to avoid DB query

We can rely on DBIC's get_dirty_columns to check if the descriptions
have changed, and set things for flushing without an extra DB query.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Martin Renvoize 2023-03-10 16:38:14 UTC
Created attachment 148091 [details] [review]
Bug 30920: (follow-up) Cache key fixes

We missed a few places, namely the flushes, when renaming the cache
keys.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Martin Renvoize 2023-03-10 16:38:17 UTC
Created attachment 148092 [details] [review]
Bug 30920: (follow-up) Flush on delete

We missed the flush on delete triggers for the various caches introduced
here.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Martin Renvoize 2023-03-10 16:38:20 UTC
Created attachment 148093 [details] [review]
Bug 30920: (follow-up) Cleanup warning from C4::Biblio::GetAuthorisedValueDesc

We simply return early and empty if $value is not passed

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Martin Renvoize 2023-03-10 16:38:23 UTC
Created attachment 148094 [details] [review]
Bug 30920: (follow-up) Manually flush caches in tests

We can't rely on our cache flushing in either 'store' or 'delete'
triggers as t/lib/TestBuilder::build_object skips the Koha::Object store
and uses direct DB inserts in preference.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 40 Tomás Cohen Arazi 2023-03-13 17:46:34 UTC
Thanks for the follow-ups, Martin.
Comment 41 Tomás Cohen Arazi 2023-03-13 18:13:31 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 42 Matt Blenkinsop 2023-03-14 10:13:47 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 43 Lucas Gass 2023-04-14 16:55:02 UTC
Merge conflicts with 22.05.x, no backport. If this is needed please rebase.