Bug 14057 - Inventory is painfully slow
Summary: Inventory is painfully slow
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P3 major (vote)
Assignee: M. Tompsett
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-24 04:39 UTC by M. Tompsett
Modified: 2016-06-21 21:40 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14057 - A Rough Start (2.04 KB, patch)
2015-04-24 04:43 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14057 - A Rough Start (2.04 KB, patch)
2015-04-24 13:40 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14057 - A Rough Start (2.10 KB, patch)
2015-04-24 13:42 UTC, M. Tompsett
Details | Diff | Splinter Review
[TESTPATCH] Bug 14057 - A Rough Start - Compare old and new code (2.14 KB, patch)
2015-04-24 20:28 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14057 - A Rough Start - Compare old and new code (2.55 KB, patch)
2015-04-25 09:49 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14057 - A Rough Start - Compare old and new code (2.62 KB, patch)
2015-04-25 09:53 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14057 - A Rough Start (7.10 KB, patch)
2015-04-27 05:30 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14057 - A Rough Start (15.25 KB, patch)
2015-04-29 02:18 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14057 - A Rough Start (15.37 KB, patch)
2015-04-29 03:12 UTC, M. Tompsett
Details | Diff | Splinter Review
[Signed-off] Bug 14057 - A Rough Start (15.67 KB, patch)
2015-04-29 06:50 UTC, Marc Véron
Details | Diff | Splinter Review
[PASSED QA] Bug 14057 - Inventory is painfully slow (15.46 KB, patch)
2015-05-01 12:29 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description M. Tompsett 2015-04-24 04:39:40 UTC
While attempting to run an inventory, I attempted to run an inventory on a single barcode with no filters. It gave me a gateway timeout error.

My goal is to speed it up. The major source of the problem is in C4::Item, where it attempts to replace the authorized value (e.g. 0) with a description (e.g. Lost -- these are hypothetical examples, so don't expect 0=Lost somewhere).
Comment 1 M. Tompsett 2015-04-24 04:43:04 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2015-04-24 06:39:17 UTC
It is really faster.
With 30 items I got for the loop
Without patch: 0.0321040153503418
With patch:    0.00271320343017578
Comment 3 M. Tompsett 2015-04-24 10:52:35 UTC
(In reply to Marc Véron from comment #2)
> It is really faster.
> With 30 items I got for the loop
> Without patch: 0.0321040153503418
> With patch:    0.00271320343017578

Thanks for the numeric confirmation. Did you notice any difference in output?
Comment 4 M. Tompsett 2015-04-24 13:40:02 UTC Comment hidden (obsolete)
Comment 5 M. Tompsett 2015-04-24 13:42:38 UTC Comment hidden (obsolete)
Comment 6 Marc Véron 2015-04-24 20:28:27 UTC Comment hidden (obsolete)
Comment 7 M. Tompsett 2015-04-24 20:43:37 UTC
(In reply to Marc Véron from comment #6)
> If found one difference: Old code could not handle Withdrawn status
> (displays 1 instead of the text)
952$0 means $sf=0, but is defined. See bug 14061.
Comment 8 Marc Véron 2015-04-25 09:49:21 UTC Comment hidden (obsolete)
Comment 9 Marc Véron 2015-04-25 09:51:24 UTC
Comment on attachment 38506 [details] [review]
Bug 14057 - A Rough Start - Compare old and new code

Sorry, attached wrong patch
Comment 10 Marc Véron 2015-04-25 09:53:16 UTC Comment hidden (obsolete)
Comment 11 Marc Véron 2015-04-25 10:01:12 UTC
Update: The reason was not in the select statment, but in line 1128;

$row->{OPAC}  should be: $row->{Intranet}

I replaced it in my test code, result: New version provides the same results as old version.
Comment 12 M. Tompsett 2015-04-27 03:20:18 UTC
(In reply to Marc Véron from comment #11)
> Update: The reason was not in the select statment, but in line 1128;
> 
> $row->{OPAC}  should be: $row->{Intranet}
> 
> I replaced it in my test code, result: New version provides the same results
> as old version.

The problem is the context is unknown for the second function, so opac is always used with staff client fall-back. If I recall correctly. Not looking at code right now.
Comment 13 M. Tompsett 2015-04-27 05:30:40 UTC Comment hidden (obsolete)
Comment 14 Marc Véron 2015-04-27 06:46:33 UTC
Comment on attachment 38507 [details] [review]
Bug 14057 - A Rough Start - Compare old and new code

Obsoleting my temporary patch with test code
Comment 15 M. Tompsett 2015-04-29 02:18:22 UTC Comment hidden (obsolete)
Comment 16 M. Tompsett 2015-04-29 03:12:39 UTC Comment hidden (obsolete)
Comment 17 M. Tompsett 2015-04-29 03:16:47 UTC
I'm up'ing the priority, since I have seen a user run inventory 4 times unfiltered, and it blew up their low-memory (1GB RAM) system with just 80MB of Koha DB.

If dcook, mveron, or perhaps anyone else could look at this one more time, I'll submit a patch for sign off if there is no feedback in 48 hours. :)
Comment 18 M. Tompsett 2015-04-29 04:11:01 UTC
I know there is no test plan, but if someone wishes to test it and sign it off with no test plan, why not? :)
Comment 19 Marc Véron 2015-04-29 06:50:07 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2015-05-01 12:29:19 UTC
Created attachment 38733 [details] [review]
[PASSED QA] Bug 14057 - Inventory is painfully slow

This patch is a rough start. I believe it runs exponentially
faster, but its equality to the previous version needs to be
tested before I clean it up to acceptable standards.

Nested hashes of hashes was being a debugging nightmare.

Moved the SQL select to C4::Koha.

Changed the GetItemsForInventory to have a hashref parameter.
Added interface, in case there is a need for 'opac' vs. 'staff'.

Added t/db_dependent/Items/GetItemsForInventory.t
Added t/db_dependent/Koha/GetKohaAuthorisedValuesMapping.t

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Tomás Cohen Arazi 2015-05-06 13:47:13 UTC
Delightfully pushed to master.

Thanks Mark!
Comment 22 David Cook 2015-10-19 05:43:01 UTC
Just noticed some typos in the POD

Checkout "stautshash" below. It should be "statushash" and it should have a comma after $statushash at the end. Probably worth fixing in the event of someone copying and pasting, but not really a bug...

=head2 GetItemsForInventory

($itemlist, $iTotalRecords) = GetItemsForInventory( {
  minlocation  => $minlocation,
  maxlocation  => $maxlocation,
  location     => $location,
  itemtype     => $itemtype,
  ignoreissued => $ignoreissued,
  datelastseen => $datelastseen,
  branchcode   => $branchcode,
  branch       => $branch,
  offset       => $offset,
  size         => $size,
  stautshash   => $statushash
  interface    => $interface,
} );