https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31216 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #2 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Looks good to me at first glance.. just about to head off on vacation though so will have to leave a proper review for me return. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31216 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- 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=31216 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #3 from Fridolin Somers <fridolin.somers@biblibre.com> --- Buuuuut, Koha::AuthorisedValues->get_description_by_koha_field already uses Koha::Cache::Memory::Lite. Do you have perf differences ? New code looks complex and is not covered by unit tests. It is worth it ? Maybe it can call get_descriptions_by_koha_field(). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31216 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion CC| |magnus@libriotech.no --- Comment #4 from Magnus Enger <magnus@libriotech.no> --- Looks like this needs some more discussion. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31216 --- Comment #5 from David Cook <dcook@prosentient.com.au> --- In bug 33989, I moved the AV description code into the "additemtoresults" function, because it was running the AV lookup for items that were *not* displayed. By making this change, I was vastly improving my inventory run times (e.g. taking 30 seconds instead of 60 seconds). I've just tried with and without selecting "Compare barcodes list to results", and I notice that even with it selected you don't end up displaying many results, so bug 33989 should take care of a lot. -- Of course, when writing bug 33989, I thought that I should add more caching, but as Fridolin mentions Koha::AuthorisedValues->get_description_by_koha_field appears to already use caching (since 2016). But... if the caching were working correctly... then I probably shouldn't have noticed such an improvement on bug 33989... I added some debugging code, and it turns out... Koha::AuthorisedValues->get_description_by_koha_field() doesn't cache null descriptions. Let me explain... Consider a cache key for a Default framework notforloan with a value of 0: AV_descriptions::items.notforloan:0 In k-t-d, the NOT_LOAN AV only has the following values: [-1,1,2,3] So for the cache key of AV_descriptions::items.notforloan:0, a database lookup will *always* be performed, and an empty hashref will *always* be returned. Since 0 is probably the most common value found, at least in k-t-d, you're looking at *a lot* of database calls for something that should probably be cached. Of course, changing Koha::AuthorisedValues->get_description_by_koha_field would have repercussions throughout a fair bit of Koha. But since it's a L1 cache... we really should be caching the empty hashref so that we're not doing unnecessary database lookups. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31216 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- I reckon we can probably mark this as a duplicate of bug 33989 and follow up the caching issue in bug 34051 ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31216 --- Comment #8 from David Cook <dcook@prosentient.com.au> --- P.S. Feel free to look at my patch at bug 31744 that helps handle long running inventory jobs... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31216 --- Comment #9 from Nick Clemens <nick@bywatersolutions.com> --- The work on caching greatly reduced the benefits here - it was the better wyay to go. Thanks for your work David! -- You are receiving this mail because: You are watching all bug changes.