Bugzilla – Attachment 96079 Details for
Bug 24185
'If all unavailable' state for 'on shelf holds' makes holds page very slow if there's a lot of items
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 24185: 'If all unavailable' for 'on shelf holds' makes holds page slow
Bug-24185-If-all-unavailable-for-on-shelf-holds-ma.patch (text/plain), 8.26 KB, created by
Andrii Nugged
on 2019-12-07 12:43:37 UTC
(
hide
)
Description:
Bug 24185: 'If all unavailable' for 'on shelf holds' makes holds page slow
Filename:
MIME Type:
Creator:
Andrii Nugged
Created:
2019-12-07 12:43:37 UTC
Size:
8.26 KB
patch
obsolete
>From 2f60f472a5e89af56324f50a22a628d8d8617528 Mon Sep 17 00:00:00 2001 >From: Andrew Nugged <nugged@gmail.com> >Date: Sat, 7 Dec 2019 14:37:58 +0200 >Subject: [PATCH] Bug 24185: 'If all unavailable' for 'on shelf holds' makes > holds page slow > >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) > >Proposed fix: I created patch to have 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. > >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) > >*** "On shelf holds allowed" != "If all unavailable": > 100 items: 5 seconds, > 200 items: 10 seconds, > 300 items: 11 seconds, > >*** "On shelf holds allowed" == "If all unavailable": > 100 items: 50 seconds, > 200 items: 3.1 minutes, > 300 items: 7.0 minutes, > >*** "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 c5014820f6..c6660d3cfd 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,7 +560,9 @@ foreach my $biblionumber (@biblionumbers) { > if ( > !$item->{cantreserve} > && !$exceeded_maxreserves >- && IsAvailableForItemLevelRequest($item_object, $patron) >+ # 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) > && $can_item_be_reserved->{status} eq 'OK' > ) > { >-- >2.17.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 24185
:
96079
|
96080
|
96151
|
96354
|
96460
|
96461
|
96464
|
96491
|
96492
|
96493
|
96500
|
97375
|
97376
|
97377
|
97378
|
101355
|
101356
|
101357
|
101358
|
101607
|
101608
|
101609
|
101611
|
101613
|
101614
|
101615
|
101616