Bug 30920 - Add caching to C4::Biblio::GetAuthorisedValueDesc
Summary: Add caching to C4::Biblio::GetAuthorisedValueDesc
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement with 8 votes (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-06-07 13:47 UTC by Martin Renvoize
Modified: 2022-08-12 14:19 UTC (History)
5 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:


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

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 :)