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/libary/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), last one block eith times with that in-var-cache solution I am proposing: "On shelf holds allowed" != "If all unavailable": for 100 items: 5 seconds, for 200 items: 10 seconds, for 300 items: 11 seconds, "On shelf holds allowed" == "If all unavailable": for 100 items: 50 seconds, for 200 items: 3.1 minutes, for 300 items: 7.0 minutes, "On shelf holds allowed" == "If all unavailable" with cache: for 100 items: 7 seconds, for 200 items: 20 seconds, for 300 items: 38 seconds,
Created attachment 96079 [details] [review] 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.
Created attachment 96080 [details] [review] 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) 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) *** "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.
Thanks. Unfortunately, the patch doesn't apply, please rebase. Also the title "'If all unavailable' for 'on shelf holds' makes holds page slow" describes the problem instead what the commit does. So the commit title should be something like "Make holds page fast when/if/with setting ..." (https://wiki.koha-community.org/wiki/Commit_messages#Examples)
Created attachment 96151 [details] [review] 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.
Created attachment 96354 [details] [review] 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 slow. It happens because in subloop there is checking if all items available so it is O(n^2) and it re-checks each time the same info for each item with repeating DB/data requests. Fix: The inner loop 1:1 picked out from another loop and put before that items processing loop, 'items_any_available' variable precalculated, this variable passed to IsAvailableForItemLevelRequest to be used inside as the precalculated result. On tests it gave a drastic 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 freshly installed kohadevbox create/import one book, remember 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 a single circulation rule in admin "/cgi-bin/koha/admin/smart-rules.pl", that rule should match above book items/library/patron, check that rule to have a 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 a 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 on old code, densely increasing for every hundred items added. In the case of this solution, it's fast, and time increases a little only, linear. I tested on my computer in VirtualBox for page generation times, did 3-5 runs for same case to check if results are stable, and got such values: (old code): 100 items: 38 seconds 200 items: 2.5 minutes 300 items: 5.4 minutes (version with fix): 100 items: 3.7 seconds 200 items: 6.4 seconds 300 items: 8.7 seconds
Thanks! Couple issues: 1) you copy & paste the code inside the loop instead of abstracting it to a subroutine. Please fix this so the code is in a subroutine and performance problems like this can be prevented by seeing the problems immediately. This is important also for testability. 2) There should be one logical change per commit, leave "$can_item_be_reserved eq 'OK'" change in the && statement to another commit, and also leave "my $on_shelf_holds = Koha:: [...]" to another commit, plus any other changes I missed not related to fixing the O(n^2) issue here
Created attachment 96460 [details] [review] Bug 24185: Make holds page faster - improved "if" `$can_item_be_reserved eq 'OK'` moved in `&&` before `IsAvailableForItemLevelRequest` to cut away with static known values before calling to more resource consuming subroutine.
Created attachment 96461 [details] [review] Bug 24185: Make holds page faster - call to subroutine moved below "if" `$on_shelf_holds` was assigned before "return .. if" but not used in that code piece, so sometimes it was useless. Moved assignment after "return .. if".
Created attachment 96464 [details] [review] 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 slow. It happens because in subloop there is checking if all items available so it is O(n^2) and it re-checks each time the same info for each item with repeating DB/data requests. Fix: The inner loop 1:1 picked out from another loop and put before that items processing loop, 'items_any_available' variable precalculated, this variable passed to IsAvailableForItemLevelRequest to be used inside as the precalculated result. On tests it gave a drastic improvement in execution time. How to reproduce: 1) on freshly installed kohadevbox create/import one book, remember 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 a single circulation rule in admin "/cgi-bin/koha/admin/smart-rules.pl", that rule should match above book items/library/patron, check that rule to have a 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 a 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 on old code, densely increasing for every hundred items added. In the case of this solution, it's fast, and time increases a little only, linear. I tested on my computer in VirtualBox for page generation times, did 3-5 runs for same case to check if results are stable, and got such values: (old code): 100 items: 38 seconds 200 items: 2.5 minutes 300 items: 5.4 minutes (version with fix): 100 items: 3.7 seconds 200 items: 6.4 seconds 300 items: 8.7 seconds
Created attachment 96491 [details] [review] Bug 24185: Make holds page faster - improved "if" `$can_item_be_reserved eq 'OK'` moved in `&&` before `IsAvailableForItemLevelRequest` to cut away with static known values before calling to more resource consuming subroutine.
Created attachment 96492 [details] [review] Bug 24185: Make holds page faster - call to subroutine moved below "if" `$on_shelf_holds` was assigned before "return .. if" but not used in that code piece, so sometimes it was useless. Moved assignment after "return .. if".
Created attachment 96493 [details] [review] 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 slow. It happens because in subloop there is checking if all items available so it is O(n^2) and it re-checks each time the same info for each item with repeating DB/data requests. Fix: The inner loop 1:1 picked out from another loop and put before that items processing loop, 'items_any_available' variable precalculated, this variable passed to IsAvailableForItemLevelRequest to be used inside as the precalculated result. On tests it gave a drastic improvement in execution time. How to reproduce: 1) on freshly installed kohadevbox create/import one book, remember 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 a single circulation rule in admin "/cgi-bin/koha/admin/smart-rules.pl", that rule should match above book items/library/patron, check that rule to have a 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 a 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 on old code, densely increasing for every hundred items added. In the case of this solution, it's fast, and time increases a little only, linear. I tested on my computer in VirtualBox for page generation times, did 3-5 runs for same case to check if results are stable, and got such values: (old code): 100 items: 38 seconds 200 items: 2.5 minutes 300 items: 5.4 minutes (version with fix): 100 items: 3.7 seconds 200 items: 6.4 seconds 300 items: 8.7 seconds
Created attachment 96500 [details] [review] 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 slow. It happens because in subloop there is checking if all items available so it is O(n^2) and it re-checks each time the same info for each item with repeating DB/data requests. Fix: The inner loop 1:1 picked out from another loop and put before that items processing loop, 'items_any_available' variable precalculated, this variable passed to IsAvailableForItemLevelRequest to be used inside as the precalculated result. On tests it gave a drastic improvement in execution time. How to reproduce: 1) on freshly installed kohadevbox create/import one book, remember 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 a single circulation rule in admin "/cgi-bin/koha/admin/smart-rules.pl", that rule should match above book items/library/patron, check that rule to have a 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 a 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 on old code, densely increasing for every hundred items added. In the case of this solution, it's fast, and time increases a little only, linear. I tested on my computer in VirtualBox for page generation times, did 3-5 runs for same case to check if results are stable, and got such values: (old code): 100 items: 38 seconds 200 items: 2.5 minutes 300 items: 5.4 minutes (version with fix): 100 items: 3.7 seconds 200 items: 6.4 seconds 300 items: 8.7 seconds
Created attachment 97375 [details] [review] Bug 24185: Make holds page faster - improved "if" `$can_item_be_reserved eq 'OK'` moved in `&&` before `IsAvailableForItemLevelRequest` to cut away with static known values before calling to more resource consuming subroutine.
Created attachment 97376 [details] [review] Bug 24185: Make holds page faster - call to subroutine moved below "if" `$on_shelf_holds` was assigned before "return .. if" but not used in that code piece, so sometimes it was useless. Moved assignment after "return .. if".
Created attachment 97377 [details] [review] Bug 24185: Make holds page faster: preparatory refactoring This is just refactoring. extracting logically independent code to separate sub + tests update. No logic change yet. Searching for "any_available" item among all biblionumber items was done inside of "elsif on_shelf_holds == 2", and it is logically very independent piece of code (this "@items" loop), it needs just biblionumber and patron as parameters so it can be extracted into separate subroutine, and later also called/reused from somewhere else. This ability to call from another place also made for future patch to remove O(n^2) problem with nested loops.
Created attachment 97378 [details] [review] 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 slow. It happens because in subloop it is checking if all items available so it is O(n^2) and it re-checks each time the same info for each item with repeating DB/data requests. Fix: The inner loop 1:1 picked out into separate subroutine and called outside of the loop, saving data in 'items_any_available' variable once, this variable passed to IsAvailableForItemLevelRequest to be used inside as the precalculated result. This made algorithm O(n) instead of O(n^2) so there is noticeable speed increase. How to reproduce: 1) on freshly installed kohadevbox create/import one book, remember 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 a single circulation rule in admin "/cgi-bin/koha/admin/smart-rules.pl", that rule should match above book items/library/patron, check that rule to have a 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 a 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 on old code, densely increasing for every hundred items added. In the case of this solution, it's fast, and time increases a little only, linear. I tested on my computer in VirtualBox for page generation times, did 3-5 runs for same case to check if results are stable, and got such values: (old code): 100 items: 50 seconds 200 items: 3.2 minutes 300 items: 7.3 minutes (version with fix): 100 items: 4.4 seconds 200 items: 7.5 seconds 300 items: 10.4 seconds
Andrew, that makes sense to me, thanks for the excellent writing of these patches (split, comments and commit messages are perfect!).
Created attachment 101355 [details] [review] Bug 24185: Make holds page faster - improved "if" `$can_item_be_reserved eq 'OK'` moved in `&&` before `IsAvailableForItemLevelRequest` to cut away with static known values before calling to more resource consuming subroutine.
Created attachment 101356 [details] [review] Bug 24185: Make holds page faster - call to subroutine moved below "if" `$on_shelf_holds` was assigned before "return .. if" but not used in that code piece, so sometimes it was useless. Moved assignment after "return .. if".
Created attachment 101357 [details] [review] Bug 24185: Make holds page faster: preparatory refactoring This is just refactoring. extracting logically independent code to separate sub + tests update. No logic change yet. Searching for "any_available" item among all biblionumber items was done inside of "elsif on_shelf_holds == 2", and it is logically very independent piece of code (this "@items" loop), it needs just biblionumber and patron as parameters so it can be extracted into separate subroutine, and later also called/reused from somewhere else. This ability to call from another place also made for future patch to remove O(n^2) problem with nested loops.
Created attachment 101358 [details] [review] 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 slow. It happens because in subloop it is checking if all items available so it is O(n^2) and it re-checks each time the same info for each item with repeating DB/data requests. Fix: The inner loop 1:1 picked out into separate subroutine and called outside of the loop, saving data in 'items_any_available' variable once, this variable passed to IsAvailableForItemLevelRequest to be used inside as the precalculated result. This made algorithm O(n) instead of O(n^2) so there is noticeable speed increase. How to reproduce: 1) on freshly installed kohadevbox create/import one book, remember 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 a single circulation rule in admin "/cgi-bin/koha/admin/smart-rules.pl", that rule should match above book items/library/patron, check that rule to have a 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 a 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 on old code, densely increasing for every hundred items added. In the case of this solution, it's fast, and time increases a little only, linear. I tested on my computer in VirtualBox for page generation times, did 3-5 runs for same case to check if results are stable, and got such values: (old code): 100 items: 50 seconds 200 items: 3.2 minutes 300 items: 7.3 minutes (version with fix): 100 items: 4.4 seconds 200 items: 7.5 seconds 300 items: 10.4 seconds
Created attachment 101607 [details] [review] Bug 24185: Make holds page faster - improved "if" `$can_item_be_reserved eq 'OK'` moved in `&&` before `IsAvailableForItemLevelRequest` to cut away with static known values before calling to more resource consuming subroutine. Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Created attachment 101608 [details] [review] Bug 24185: Make holds page faster - call to subroutine moved below "if" `$on_shelf_holds` was assigned before "return .. if" but not used in that code piece, so sometimes it was useless. Moved assignment after "return .. if". Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Created attachment 101609 [details] [review] Bug 24185: Make holds page faster: preparatory refactoring This is just refactoring. extracting logically independent code to separate sub + tests update. No logic change yet. Searching for "any_available" item among all biblionumber items was done inside of "elsif on_shelf_holds == 2", and it is logically very independent piece of code (this "@items" loop), it needs just biblionumber and patron as parameters so it can be extracted into separate subroutine, and later also called/reused from somewhere else. This ability to call from another place also made for future patch to remove O(n^2) problem with nested loops. Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Rebased on current master with an obvious conflict because of Bug 24468. Performance is really better, tested on my laptop : 100 items => 54 seconds without patch, 8 seconds with patch
oh, thanks for REBASE! In a case, if another assistance needed – I am monitoring this actively.
Created attachment 101611 [details] [review] 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 slow. It happens because in subloop it is checking if all items available so it is O(n^2) and it re-checks each time the same info for each item with repeating DB/data requests. Fix: The inner loop 1:1 picked out into separate subroutine and called outside of the loop, saving data in 'items_any_available' variable once, this variable passed to IsAvailableForItemLevelRequest to be used inside as the precalculated result. This made algorithm O(n) instead of O(n^2) so there is noticeable speed increase. How to reproduce: 1) on freshly installed kohadevbox create/import one book, remember 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 a single circulation rule in admin "/cgi-bin/koha/admin/smart-rules.pl", that rule should match above book items/library/patron, check that rule to have a 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 a 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 on old code, densely increasing for every hundred items added. In the case of this solution, it's fast, and time increases a little only, linear. I tested on my computer in VirtualBox for page generation times, did 3-5 runs for same case to check if results are stable, and got such values: (old code): 100 items: 50 seconds 200 items: 3.2 minutes 300 items: 7.3 minutes (version with fix): 100 items: 4.4 seconds 200 items: 7.5 seconds 300 items: 10.4 seconds Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
(In reply to Andrew Nugged from comment #27) > oh, thanks for REBASE! In a case, if another assistance needed – I am > monitoring this actively. Your welcome, please check its OK
Created attachment 101613 [details] [review] Bug 24185: Make holds page faster - improved "if" `$can_item_be_reserved eq 'OK'` moved in `&&` before `IsAvailableForItemLevelRequest` to cut away with static known values before calling to more resource consuming subroutine. Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 101614 [details] [review] Bug 24185: Make holds page faster - call to subroutine moved below "if" `$on_shelf_holds` was assigned before "return .. if" but not used in that code piece, so sometimes it was useless. Moved assignment after "return .. if". Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 101615 [details] [review] Bug 24185: Make holds page faster: preparatory refactoring This is just refactoring. extracting logically independent code to separate sub + tests update. No logic change yet. Searching for "any_available" item among all biblionumber items was done inside of "elsif on_shelf_holds == 2", and it is logically very independent piece of code (this "@items" loop), it needs just biblionumber and patron as parameters so it can be extracted into separate subroutine, and later also called/reused from somewhere else. This ability to call from another place also made for future patch to remove O(n^2) problem with nested loops. Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Amended patch, I moved up the last line of the test, sounds more logical to me.
Created attachment 101616 [details] [review] 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 slow. It happens because in subloop it is checking if all items available so it is O(n^2) and it re-checks each time the same info for each item with repeating DB/data requests. Fix: The inner loop 1:1 picked out into separate subroutine and called outside of the loop, saving data in 'items_any_available' variable once, this variable passed to IsAvailableForItemLevelRequest to be used inside as the precalculated result. This made algorithm O(n) instead of O(n^2) so there is noticeable speed increase. How to reproduce: 1) on freshly installed kohadevbox create/import one book, remember 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 a single circulation rule in admin "/cgi-bin/koha/admin/smart-rules.pl", that rule should match above book items/library/patron, check that rule to have a 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 a 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 on old code, densely increasing for every hundred items added. In the case of this solution, it's fast, and time increases a little only, linear. I tested on my computer in VirtualBox for page generation times, did 3-5 runs for same case to check if results are stable, and got such values: (old code): 100 items: 50 seconds 200 items: 3.2 minutes 300 items: 7.3 minutes (version with fix): 100 items: 4.4 seconds 200 items: 7.5 seconds 300 items: 10.4 seconds Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Excellent work, thanks everyone!
Nice work everyone! Pushed to master for 20.05
does not apply to 19.11.x please rebase if needed.
I've backported this to v19.11.x because people requested: where I should push patches for 19.11.06? Should I create a separate ticket?
(In reply to Andrew Nugged from comment #38) > I've backported this to v19.11.x because people requested: where I should > push patches for 19.11.06? Should I create a separate ticket? Usually the same bug is used and you can state the version in your commit message for example by adding [19.11] at the beginning of the subject line so it shows in the list of patches here.