From c1f3f133004196ac7393c180075f747554c68da4 Mon Sep 17 00:00:00 2001 From: Andrew Nugged Date: Sat, 7 Dec 2019 14:37:58 +0200 Subject: [PATCH] Bug 24185: Make holds page fast when 'on shelf holds' set to 'If all unavailable' When "reserve/request.pl -> C4/Reserves.pm::IsAvailableForItemLevelRequest" called many times with hundred of items and "on shelf holds" parameter set to "If all unavailable" for these items+patron, it goes very slow. This happens because in subloop there are re-checking if all other item unavailable so it is O(n^2) and it re-checks each time a lot of the same info for each item with repeating DB/data requests, these three: - GetBranchItemRule (per item type) - IsItemOnHoldAndFound (per item) - notforloan (per item) Fix: Use in-variable cache in upper-level subroutine before entering the first "items" loop and passing that hash reference down into subs to cache these three values by keys per-item and per-item-type. It is simplest way to make it fast without re-thinking pretty complicated whole algorithm and business logic, and, when tested by speed it gave big improvement in execution time. Also "$can_item_be_reserved->{status} eq 'OK'" moved in "&&" before IsAvailableForItemLevelRequest call to cut away if it false before calling to subroutine. How to reproduce: 1) on fresh installed kohadevbox create/import one book, record that biblionumber for later use it in down below, 2) add 100 items for that book for some library, 3) find some patron, that patron's card number we will use as a borrower down below to open holds page, 4) check for the rule or set up single circulation rule in admin "/cgi-bin/koha/admin/smart-rules.pl", so that rule will match above book items/library/patron, check that rule to have non-zero number of holds (total, daily, count) allowed, and, IMPORTANT: set up "On shelf holds allowed" to "If all unavailable", ("item level holds" doesn't matter). 5) open "Home > Catalog > THAT_BOOK > Place a hold on THAT_BOOK" page ("holds" tab), and enter patron code in the search field, or you can create direct link by yourself, for example in my case it was: /cgi-bin/koha/reserve/request.pl?biblionumber=4&findborrower=23529000686353 6) it should be pretty long page generation time. I tested on my computer in VirtualBox for this page generation times, (stats below is for many runs and smallest value taken from similar in a repetition) (NOT buggy clause, just example how it fast when it not enters that "subloop"): *** "On shelf holds allowed" != "If all unavailable": 100 items: 5 seconds, 200 items: 10 seconds, 300 items: 11 seconds, (buggy clause): *** "On shelf holds allowed" == "If all unavailable": 100 items: 50 seconds, 200 items: 3.1 minutes, 300 items: 7.0 minutes, (same as above clause but fixed with caching in hash): *** "On shelf holds allowed" == "If all unavailable" with cache: 100 items: 7 seconds, 200 items: 20 seconds, 300 items: 38 seconds, Upper last one block with times with this in-var-cache solution. --- C4/Reserves.pm | 70 ++++++++++++++++++++++++++++++++++++++-------- reserve/request.pl | 8 +++++- 2 files changed, 65 insertions(+), 13 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 7a3785724a..521b5041db 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -1188,7 +1188,10 @@ and canreservefromotherbranches. =cut sub IsAvailableForItemLevelRequest { - my ( $item, $patron, $pickup_branchcode ) = @_; + my $item = shift; + my $patron = shift; + my $pickup_branchcode = shift; + my $caches = shift; my $dbh = C4::Context->dbh; # must check the notforloan setting of the itemtype @@ -1223,19 +1226,62 @@ sub IsAvailableForItemLevelRequest { my $any_available = 0; foreach my $i (@items) { - my $reserves_control_branch = GetReservesControlBranch( $i->unblessed(), $patron->unblessed ); - my $branchitemrule = C4::Circulation::GetBranchItemRule( $reserves_control_branch, $i->itype ); + my $reserves_control_branch = + GetReservesControlBranch( $i->unblessed(), $patron->unblessed ); + + my $onholdandfound; + my $itemtype_notforloan; + my $branchitemrule; + + # caching algorithm implemented and keys/values pairs stored in provided from upper-level sub + # reference to hash, but if no ref provided, it works in old-style (for compatibility): + if($caches) { + + # %$caches has three subkeys: + # {branchitemrule} values cached by two-levels: $reserves_control_branch + $i->itype + # {onholdandfound} values cached by $i->id + # {notforloan} values cached by $i->effective_itemtype() + + my $i_itype = $i->itype; + $branchitemrule = + exists $caches->{branchitemrule}{$reserves_control_branch}{$i_itype} + ? $caches->{branchitemrule}{$reserves_control_branch}{$i_itype} + : ( + $caches->{branchitemrule}{$reserves_control_branch}{$i_itype} = + C4::Circulation::GetBranchItemRule( + $reserves_control_branch, $i_itype + ) ); + + my $i_id = $i->id; + $onholdandfound = + exists $caches->{onholdandfound}{$i_id} + ? $caches->{onholdandfound}{$i_id} + : ( $caches->{onholdandfound}{$i_id} = IsItemOnHoldAndFound($i_id) ); + + my $effective_itemtype = $i->effective_itemtype(); + $itemtype_notforloan = + $caches && exists $caches->{notforloan}{$effective_itemtype} + ? $caches->{notforloan}{$effective_itemtype} + : ( $caches->{notforloan}{$effective_itemtype} = + Koha::ItemTypes->find($effective_itemtype)->notforloan ); + } + else { + $branchitemrule = + C4::Circulation::GetBranchItemRule( $reserves_control_branch, $i->itype ); + $onholdandfound = IsItemOnHoldAndFound( $i->id ); + $itemtype_notforloan = Koha::ItemTypes->find( $i->effective_itemtype() )->notforloan; + } $any_available = 1 - unless $i->itemlost - || $i->notforloan > 0 - || $i->withdrawn - || $i->onloan - || IsItemOnHoldAndFound( $i->id ) - || ( $i->damaged - && !C4::Context->preference('AllowHoldsOnDamagedItems') ) - || Koha::ItemTypes->find( $i->effective_itemtype() )->notforloan - || $branchitemrule->{holdallowed} == 1 && $patron->branchcode ne $i->homebranch; + unless $i->itemlost + || $i->notforloan > 0 + || $i->withdrawn + || $i->onloan + || $onholdandfound + || ( $i->damaged + && ! C4::Context->preference('AllowHoldsOnDamagedItems') ) + || $itemtype_notforloan + || $branchitemrule->{holdallowed} == 1 && $patron->branchcode ne $i->homebranch; } return $any_available ? 0 : 1; diff --git a/reserve/request.pl b/reserve/request.pl index 4f80b8d5b9..0376c72516 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -444,6 +444,10 @@ foreach my $biblionumber (@biblionumbers) { $itemtypes->{ $biblioitem->{itemtype} }{imageurl} ); } + # this is hash cache for "if any unavailable" on-shelf holds check + # in IsAvailableForItemLevelRequest sub should persist outside from the loop + my %caches_avail_item_level; + foreach my $itemnumber ( @{ $itemnumbers_of_biblioitem{$biblioitemnumber} } ) { my $item = $iteminfos_of->{$itemnumber}; my $do_check; @@ -556,8 +560,10 @@ foreach my $biblionumber (@biblionumbers) { if ( !$item->{cantreserve} && !$exceeded_maxreserves - && IsAvailableForItemLevelRequest($item_object, $patron) && $can_item_be_reserved eq 'OK' + # caching %caches_avail_item_level done inside IsAvailableForItemLevelRequest, + # but hash should persist for all looped items: + && IsAvailableForItemLevelRequest($item_object, $patron, undef, \%caches_avail_item_level) ) { $item->{available} = 1; -- 2.17.1