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).
Created attachment 38472 [details] [review] Bug 14057 - A Rough Start 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.
It is really faster. With 30 items I got for the loop Without patch: 0.0321040153503418 With patch: 0.00271320343017578
(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?
Created attachment 38482 [details] [review] Bug 14057 - A Rough Start 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 hard to debug.
Created attachment 38483 [details] [review] Bug 14057 - A Rough Start 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.
Created attachment 38500 [details] [review] [TESTPATCH] Bug 14057 - A Rough Start - Compare old and new code This patch gets values using both methods to compare the results. For testing purposes only. To test: Apply patch Compare results by commenting /uncommenting appropriate lines after line 1154 in C4/Items.pm If found one difference: Old code could not handle Withdrawn status (displays 1 instead of the text)
(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.
Created attachment 38506 [details] [review] Bug 14057 - A Rough Start - Compare old and new code This patch gets values using both methods to compare the results. For testing purposes only. To test: Apply patch Compare results by commenting /uncommenting appropriate lines after line 1154 in C4/Items.pm If found one difference: Old code could not handle Withdrawn status (displays 1 instead of the text) ------ Amended: I changed the test code to reflect the old behaviour fixed with patch from Bug 14061 It writes a warning if the old and the new value are not the same. I still got differences with the Withdrawn status and found out, that it took the auth value for OPAC. This is due to a part of the select statement in the first patch (Line 1122) that overrules the value for the staff client: IF(TB.lib_opac>'',TB.lib_opac,TB.lib)
Comment on attachment 38506 [details] [review] Bug 14057 - A Rough Start - Compare old and new code Sorry, attached wrong patch
Created attachment 38507 [details] [review] Bug 14057 - A Rough Start - Compare old and new code This patch gets values using both methods to compare the results. For testing purposes only. To test: Apply patch Compare results by commenting /uncommenting appropriate lines after line 1154 in C4/Items.pm If found one difference: Old code could not handle Withdrawn status (displays 1 instead of the text) ------ Amended: I changed the test code to reflect the old behaviour fixed with patch from Bug 14061 It writes a warning if the old and the new value are not the same. I still got differences with the Withdrawn status and found out, that it took the auth value for OPAC. This is due to a part of the select statement in the first patch (Line 1122) that overrules the value for the staff client: IF(TB.lib_opac>'',TB.lib_opac,TB.lib)
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.
(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.
Created attachment 38525 [details] [review] Bug 14057 - A Rough Start 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'. Untested.
Comment on attachment 38507 [details] [review] Bug 14057 - A Rough Start - Compare old and new code Obsoleting my temporary patch with test code
Created attachment 38621 [details] [review] Bug 14057 - A Rough Start 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
Created attachment 38622 [details] [review] Bug 14057 - A Rough Start 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 I see what you mean about opac vs. staff now.
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. :)
I know there is no test plan, but if someone wishes to test it and sign it off with no test plan, why not? :)
Created attachment 38636 [details] [review] [Signed-off] Bug 14057 - A Rough Start 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 --- How I tested: - Apply patch - Do some full and some filtered Inventories, behaviour is the same (but faster as without patch) - Run both tests Signed-off-by: Marc Véron <veron@veron.ch>
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>
Delightfully pushed to master. Thanks Mark!
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, } );