https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@ptfs-europe |ity.org |.com -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com --- Comment #2 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 --- Comment #3 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Baby steps back at you ;P. I agree it would be nice to shift this stuff to Koha::.. but that's for another day. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 --- Comment #5 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 --- Comment #7 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 --- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 --- Comment #12 from Fridolin Somers <fridolin.somers@biblibre.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=29454 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #135771|0 |1 is obsolete| | --- Comment #13 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 136292 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=136292&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 --- Comment #15 from 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 --- Comment #17 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joonas.kylmala@iki.fi -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 --- Comment #19 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #21 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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 :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #136292|0 |1 is obsolete| | --- Comment #22 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 144817 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144817&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 --- Comment #24 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 144819 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144819&action=edit Bug 30920: Update cache keys This changes the cache keys to use ':' to separate levels and orders from generic to specific -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 --- Comment #26 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144818|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 --- Comment #27 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 148064 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=148064&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 --- Comment #29 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 148066 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=148066&action=edit Bug 30920: Update cache keys This changes the cache keys to use ':' to separate levels and orders from generic to specific -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #31 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Failing because ->delete is not invalidating cache. Can be verified by running: prove t/db_dependent/Koha/Item.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #148065|0 |1 is obsolete| | --- Comment #33 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 148088 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=148088&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #148067|0 |1 is obsolete| | --- Comment #35 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 148090 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=148090&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 --- Comment #37 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 148092 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=148092&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 --- Comment #39 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 148094 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=148094&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 --- Comment #40 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Thanks for the follow-ups, Martin. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 --- Comment #41 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 22.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable Version(s)|23.05.00 |23.05.00,22.11.04 released in| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |RESOLVED Resolution|--- |FIXED CC| |lucas@bywatersolutions.com --- Comment #43 from Lucas Gass <lucas@bywatersolutions.com> --- Merge conflicts with 22.05.x, no backport. If this is needed please rebase. -- You are receiving this mail because: You are watching all bug changes.