According to NYTProf, most of our time is spent in fetching authorised values and storing the items We should improve the use of cache here and reduce store where we can Just to note: GetItemsForInventory translates the AVs in a single call which seems to be quite fast
Created attachment 138008 [details] [review] Bug 31216: Make one call to fetch AVs before substituting This patch copies logic from GetItemsForInventory as it proves much faster at updating values Probably this should be a subroutine? BUt we have cachign patches in the works too (bug 30920) Additionally - it looks like current inventory code looks for framework from items, but I am not sure we fetch it anywhere
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.
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().
Looks like this needs some more discussion.
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.
I reckon we can probably mark this as a duplicate of bug 33989 and follow up the caching issue in bug 34051 ?
(In reply to David Cook from comment #6) > I reckon we can probably mark this as a duplicate of bug 33989 and follow up > the caching issue in bug 34051 ? Doing a patch for bug 34051 and I got my inventory time down from 42.5 seconds to 27.7 seconds for a list of 961 barcodes, and that's all from using the cached empty hashref and avoiding the database calls.
P.S. Feel free to look at my patch at bug 31744 that helps handle long running inventory jobs...
The work on caching greatly reduced the benefits here - it was the better wyay to go. Thanks for your work David!