|
Description
Martin Renvoize (ashimema)
2022-06-07 13:47:27 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. 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). Baby steps back at you ;P. I agree it would be nice to shift this stuff to Koha::.. but that's for another day. Yes sure, my main point was about the removal. I thought it was kind of obsolete. (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. 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 (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? 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. 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. 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. (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. 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. 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> Created attachment 136293 [details] [review] Bug 30920: (follow-up) Add missing use Signed-off-by: Nick Clemens <nick@bywatersolutions.com> (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 (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. (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. Great thanks for this details Martin. 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. OK, I missed comment 6 entirely.. I'll take a look at your suggestions now. (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 :) 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> Created attachment 144818 [details] [review] Bug 30920: (follow-up) Add missing use Signed-off-by: Nick Clemens <nick@bywatersolutions.com> 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 (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 (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. 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> Created attachment 148065 [details] [review] Bug 30920: (follow-up) Add missing use Signed-off-by: Nick Clemens <nick@bywatersolutions.com> 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 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> Failing because ->delete is not invalidating cache. Can be verified by running: prove t/db_dependent/Koha/Item.t 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> 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> 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> 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> 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> 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> 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> 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> Thanks for the follow-ups, Martin. Pushed to master for 22.11. Nice work everyone, thanks! Nice work everyone! Pushed to stable for 22.11.x Merge conflicts with 22.05.x, no backport. If this is needed please rebase. |