Bug 31216 - inventory.pl is slow when comparing many barcodes
Summary: inventory.pl is slow when comparing many barcodes
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-07-21 20:51 UTC by Nick Clemens (kidclamp)
Modified: 2023-12-22 13:18 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 31216: Make one call to fetch AVs before substituting (2.82 KB, patch)
2022-07-22 11:10 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2022-07-21 20:51:02 UTC
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
Comment 1 Nick Clemens (kidclamp) 2022-07-22 11:10:48 UTC
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
Comment 2 Martin Renvoize 2022-07-22 12:51:30 UTC
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.
Comment 3 Fridolin Somers 2022-12-22 07:05:58 UTC
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().
Comment 4 Magnus Enger 2023-03-28 13:32:14 UTC
Looks like this needs some more discussion.
Comment 5 David Cook 2023-06-19 00:54:13 UTC
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.
Comment 6 David Cook 2023-06-19 00:59:25 UTC
I reckon we can probably mark this as a duplicate of bug 33989 and follow up the caching issue in bug 34051 ?
Comment 7 David Cook 2023-06-19 01:11:40 UTC
(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.
Comment 8 David Cook 2023-06-19 01:32:03 UTC
P.S. Feel free to look at my patch at bug 31744 that helps handle long running inventory jobs...
Comment 9 Nick Clemens (kidclamp) 2023-12-22 13:18:47 UTC
The work on caching greatly reduced the benefits here - it was the better wyay to go.

Thanks for your work David!