When a site has many holds the automatic renewals cron can take a long time to run This is increased if the system preference 'AllowRenewalIfOtherItemsAvailable' is set to allow
Created attachment 127610 [details] [review] Bug 29474: Return too_soon before checking renewals This patch allows us to avoid checking reserves when the issue is too_soon and we are running the cron Code is covered by existing tests To test: 1 - prove -v t/db_dependent/Circulation.t
If you remove the unless then you can delete the line > return ( 0, "on_reserve" ) if $resfound && $cron; # '' when no hold was found Makes the code much simpler. Please also move the cron return right after the call to > $auto_renew = _CanBookBeAutoRenewed({ so that now that the return value decision is not in an if-else condition at the end the requirement for returning the values in a different order is clear and the order won't be changed by some other developer accidentally. I think adding a comment like "cron wants 'too_soon' over 'on_reserve'" Please include stats too, but maybe you are at it, I noticed that this bug is not even yet for sign-off and I'm here reviewing ;D
(In reply to Joonas Kylmälä from comment #2) > I think adding a comment like "cron wants 'too_soon' over 'on_reserve'" This sentence was unfinished.. I meant adding this comment next to the code when returning the 'too_soon' reason near the _CanBookBeAutoRenewed function call.
Created attachment 127611 [details] [review] Bug 29474: Return too_soon before checking renewals This patch allows us to avoid checking reserves when the issue is too_soon and we are running the cron Code is covered by existing tests To test: 1 - prove -v t/db_dependent/Circulation.t
(In reply to Joonas Kylmälä from comment #2) > If you remove the unless ... makes the code much simpler. Done > Please also move the cron return right after the call to > > $auto_renew = _CanBookBeAutoRenewed({ I don't believe I can. That is inside "unless ($override_limit)" so it would make 'too_soon' subject to override > Please include stats too, but maybe you are at it, I noticed that this bug > is not even yet for sign-off and I'm here reviewing ;D Yes, seeing what else I can improve first, will add when ready. Feel free to add to/alter my patches - I appreciate the help!
Created attachment 127612 [details] [review] Bug 29474: Return too_soon before checking renewals This patch allows us to avoid checking reserves when the issue is too_soon and we are running the cron Code is covered by existing tests To test: 1 - prove -v t/db_dependent/Circulation.t
(In reply to Nick Clemens from comment #5) > > Please also move the cron return right after the call to > > > $auto_renew = _CanBookBeAutoRenewed({ > > I don't believe I can. That is inside "unless ($override_limit)" so it would > make 'too_soon' subject to override > I was wrong, done
Created attachment 127613 [details] [review] Bug 29474: Check ItemsAnyAvailableAndNOtRestricted outside of loop IsAvailableForItemLevelRequest takes $items_any_available to avoid a double loop - this was done in request.pl, but it makes sense to do the same here
5 renewals allowed No renewal before unset (i.e. not before due) For a single biblio 100 items on record 10 items checked out and marked auto_renew 10 record level holds Without these patches: ~3.7 second With these patches ~1.5 second Then I set 'On shelf holds allowed' to 'If all unavailable' Without these patches: ~3 MINUTES! With these patches ~1.5 seconds To further test 'ItemsAnyAvailable' I set all the items as overdue, changed nothing else Without these patches: ~3 minutes With these patches ~1.25 minutes
The last patch is wrong, it checks the availability for the patron who is wanting to do the renewal and not the other patrons as it should (notice that the $patron variable is redefined here). You could however achieve the performance boost by adding the $items_any_available value to a hash table where the keys are borrowernumbers. However, I think we might be able to avoid that (it makes the code pretty ugly imho if we use the hash table) by splitting somehow the while loops and maybe ordering it so that we iterate patrons in the outer loop. Like I think "next if IsItemOnHoldAndFound( $item->itemnumber );" could be moved to its own loop.
oh, I think I discovered a bug in the CanBookBeRenewed function. We are calling > next unless IsAvailableForItemLevelRequest($item, $patron); and we are not passing the ignore_hold_counts=>1 here even though we should be. The patch "Bug 28013: (follow-up) Correct and update call to CanItemBeReserved" which is pushed already fixed it for the CanItemBeReserved() function call just the line below but this has the same bug. If you follow this code path you will notice the bug: IsAvailableForItemLevelRequest -> ItemsAnyAvailableAndNotRestricted -> CanItemBeReserved So this needs to kept also in mind while doing the perf work here.
In the first patch you added the comment: # override_limit will override anything else except on_reserve or too_soon This is actually not what the code does, but it should do this I think, it used to do this but after the patch "Bug 19014: Return auto_too_soon before on_reserve" (51f5f6) the behaviour was changed. It was unadvertised change so it seems like a regression introduced by bug 19014. So actually the too_soon return should be outside the $override_limit if block as you had it the first time but before that we need to fix the regression from bug 19014.
(In reply to Joonas Kylmälä from comment #12) > In the first patch you added the comment: > > # override_limit will override anything else except on_reserve or too_soon > > This is actually not what the code does, but it should do this I think, it > used to do this but after the patch "Bug 19014: Return auto_too_soon before > on_reserve" (51f5f6) the behaviour was changed. It was unadvertised change > so it seems like a regression introduced by bug 19014. So actually the > too_soon return should be outside the $override_limit if block as you had it > the first time but before that we need to fix the regression from bug 19014. Hmm, actually there is no regression, there was the $override_limit check also before bug 19014. So the comment you added should be removed. Sorry for the noise, the code is just so hard to understand...
Created attachment 127614 [details] [review] Bug 29474: Return too_soon before checking renewals This patch allows us to avoid checking reserves when the issue is too_soon and we are running the cron Code is covered by existing tests To test: 1 - prove -v t/db_dependent/Circulation.t
Created attachment 127615 [details] [review] Bug 29474: Fix incorrect comment The too_soon is actually being overriden if $override_limit is true.
Created attachment 127616 [details] [review] Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed The too_soon check does not relate at all to auto-renewed issues and should be moved outside the _CanBookBeAutoRenewed() function. To test: 1) prove t/db_dependent/Circulation.t
Created attachment 127617 [details] [review] Bug 29474: Remove one layer of indendation by adding if check in the begginning If we simply return "no" immediately from the function when the checkout is not an autorenewed checkout we can drop one layer of indendation and the code becomes much easier to read. To test: 1) prove t/db_dependent/Circulation.t
I fixed the incorrect comment about too_soon not being overriden and added couple improvements to the code as well. I obsoleted also the patch from comment #8 now as it was buggy (see comment #10). I could try to work on those perf improvements as what was tried in the patch attached in comment #8 but I'm not sure I will manage to do that in the nearby days so please go ahead and have a new attempt at it as if I will it might take time.
Created attachment 127632 [details] [review] Bug 29474: Return too_soon before checking renewals This patch allows us to avoid checking reserves when the issue is too_soon and we are running the cron Code is covered by existing tests To test: 1 - prove -v t/db_dependent/Circulation.t
Created attachment 127633 [details] [review] Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed The too_soon check does not relate at all to auto-renewed issues and should be moved outside the _CanBookBeAutoRenewed() function. To test: 1) prove t/db_dependent/Circulation.t
Created attachment 127634 [details] [review] Bug 29474: Remove one layer of indendation by adding if check in the begginning If we simply return "no" immediately from the function when the checkout is not an autorenewed checkout we can drop one layer of indendation and the code becomes much easier to read. To test: 1) prove t/db_dependent/Circulation.t
Created attachment 127635 [details] [review] Bug 29474: Pass rules/branchcode to subroutines, further simplify logic If we are going to decouple 'too_soon' from auto_renewals we should only call GetSoonestDate once Additionally, we can fetch all needed rules at once and pass to subroutines as needed Rather than check in the sub, let's not call _CanBookBeAutoRenewed if we aren't set for auto_renew
(In reply to Joonas Kylmälä from comment #18) > I fixed the incorrect comment about too_soon not being overriden and added > couple improvements to the code as well. Excellent! Thanks! I squashed the comment removal, I thought I had pulled that, I was confused when I added it :-) > I obsoleted also the patch from > comment #8 now as it was buggy (see comment #10). I could try to work on > those perf improvements as what was tried in the patch attached in comment > #8 but I'm not sure I will manage to do that in the nearby days so please go > ahead and have a new attempt at it as if I will it might take time. I am going to move that to it's own bug since it will take a little more reworking.
Created attachment 127658 [details] [review] Bug 29474: Small efficiency improvement
The renewal date calculation is buggy for auto-renewals, I made a fix for that a few days ago in bug 29476. I will mark this bug to now depend on that bug as "Bug 29474: Pass rules/branchcode to subroutines, further simplify logic" uses now GetSoonestRenewDate also for auto-renewals as well, thus requires the bug fix. Also, now that we depend on bug 29476 I can make at least one further simplification in the code, will attach a patch shortly.
(In reply to Joonas Kylmälä from comment #25) > The renewal date calculation is buggy for auto-renewals, I made a fix for > that a few days ago in bug 29476. I will mark this bug to now depend on that > bug as "Bug 29474: Pass rules/branchcode to subroutines, further simplify > logic" uses now GetSoonestRenewDate also for auto-renewals as well, thus > requires the bug fix. Also, now that we depend on bug 29476 I can make at > least one further simplification in the code, will attach a patch shortly. After further verification, there is no dependency so I created a completely new bug report for that: bug 29537. It should help *tiny* bit with performance as well so you might consider signing-off that.
Created attachment 127903 [details] [review] Bug 29474: Return too_soon before checking renewals This patch allows us to avoid checking reserves when the issue is too_soon and we are running the cron Code is covered by existing tests To test: 1 - prove -v t/db_dependent/Circulation.t Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 127904 [details] [review] Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed The too_soon check does not relate at all to auto-renewed issues and should be moved outside the _CanBookBeAutoRenewed() function. To test: 1) prove t/db_dependent/Circulation.t
Created attachment 127905 [details] [review] Bug 29474: Remove one layer of indendation by adding if check in the begginning If we simply return "no" immediately from the function when the checkout is not an autorenewed checkout we can drop one layer of indendation and the code becomes much easier to read. To test: 1) prove t/db_dependent/Circulation.t
Created attachment 127906 [details] [review] Bug 29474: Pass rules/branchcode to subroutines, further simplify logic If we are going to decouple 'too_soon' from auto_renewals we should only call GetSoonestDate once Additionally, we can fetch all needed rules at once and pass to subroutines as needed Rather than check in the sub, let's not call _CanBookBeAutoRenewed if we aren't set for auto_renew Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 127907 [details] [review] Bug 29474: Small efficiency improvement Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Added sign-off to all the patches that were not from me, Nick or Kyle, you could sign-off the patches from me to get this to signed-off state :)
Created attachment 127949 [details] [review] Bug 29474: Return too_soon before checking renewals This patch allows us to avoid checking reserves when the issue is too_soon and we are running the cron Code is covered by existing tests To test: 1 - prove -v t/db_dependent/Circulation.t Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 127950 [details] [review] Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed The too_soon check does not relate at all to auto-renewed issues and should be moved outside the _CanBookBeAutoRenewed() function. To test: 1) prove t/db_dependent/Circulation.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 127951 [details] [review] Bug 29474: Remove one layer of indendation by adding if check in the begginning If we simply return "no" immediately from the function when the checkout is not an autorenewed checkout we can drop one layer of indendation and the code becomes much easier to read. To test: 1) prove t/db_dependent/Circulation.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 127952 [details] [review] Bug 29474: Pass rules/branchcode to subroutines, further simplify logic If we are going to decouple 'too_soon' from auto_renewals we should only call GetSoonestDate once Additionally, we can fetch all needed rules at once and pass to subroutines as needed Rather than check in the sub, let's not call _CanBookBeAutoRenewed if we aren't set for auto_renew Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 127953 [details] [review] Bug 29474: Small efficiency improvement Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Nick, I was wondering this morning (bug 29474 comment 9) if _CanBookBeAutoRenewed shouldn't actually call GetSoonestRenewDate. There is also bug 29537 that will conflict with this. Could we have a clean tree we could test and QA together?
Forget the second part, it's in the dependency tree already!
(In reply to Jonathan Druart from comment #38) > Nick, I was wondering this morning (bug 29474 comment 9) if > _CanBookBeAutoRenewed shouldn't actually call GetSoonestRenewDate. Can you elaborate? We pass the soonest renew date now, rather than calling twice, why do we need to call directly?
(In reply to Nick Clemens from comment #40) > (In reply to Jonathan Druart from comment #38) > > Nick, I was wondering this morning (bug 29474 comment 9) if > > _CanBookBeAutoRenewed shouldn't actually call GetSoonestRenewDate. > > Can you elaborate? We pass the soonest renew date now, rather than calling > twice, why do we need to call directly? Sorry, I meant bug 29476 comment 9. And it's actually what Joonas did here in the second patch.
I agree with most of the changes, but I don't think it's a good idea to pass the issuing rules and the "soonest date". It's just hiding problem with the code, which shouldn't be resolved that way. For instance if we are facing perf issues with the fetch of the circ rules, we should then cache them. Other areas would benefit from that kind of change. Code will stay clean and we wouldn't need write acrobatic code in this C4::Circ module, it does not need that :)
And... thinking loud, sorry for the noise if it does not make sense. There is a _CanBookBeAutoRenewed sub but we are still doing "auto renewal" related checks in CanBookBeRenewed. It should be simpler than that actually, we should call _CanBookBeAutoRenewed that will call CanBookBeRenewed, not the reverse. Ideally Koha::AutoRenewalHold inherit from Koha::Hold Koha::AutoRenewalHold->can_be_renewed will do its specific check for auto renew then can SUPER::can_be_renewed Aren't we actually doing the reverse here?
(In reply to Jonathan Druart from comment #42) > I agree with most of the changes, but I don't think it's a good idea to pass > the issuing rules and the "soonest date". It's just hiding problem with the > code, which shouldn't be resolved that way. > > For instance if we are facing perf issues with the fetch of the circ rules, > we should then cache them. Other areas would benefit from that kind of > change. Code will stay clean and we wouldn't need write acrobatic code in > this C4::Circ module, it does not need that :) I see your point, however, in this instance _CanBookBeAutoRenewed is just a helper function here, moved out to make the logic easier to read. Passing variables we are fetching that are common to both routines seems logical to me. It's not so much that there is a perf problem fetching them, but that we are fetching the same data here multiple times (In reply to Jonathan Druart from comment #43) > And... thinking loud, sorry for the noise if it does not make sense. > > There is a _CanBookBeAutoRenewed sub but we are still doing "auto renewal" > related checks in CanBookBeRenewed. It should be simpler than that actually, > we should call _CanBookBeAutoRenewed that will call CanBookBeRenewed, not > the reverse. > > Ideally > Koha::AutoRenewalHold inherit from Koha::Hold > Koha::AutoRenewalHold->can_be_renewed will do its specific check for auto > renew then can SUPER::can_be_renewed > I assume you mean 'Issue' not 'Hold' > Aren't we actually doing the reverse here? Automatic renewals and renewals are not discrete - a regular renewal block will prevent auto_renewal and is cheaper to check the simple bits first - it makes sense to go through the simple renewal checks that will block, before checking the auto_renewal blocks.
(In reply to Jonathan Druart from comment #42) > For instance if we are facing perf issues with the fetch of the circ rules, > we should then cache them. Other areas would benefit from that kind of > change. Code will stay clean and we wouldn't need write acrobatic code in > this C4::Circ module, it does not need that :) I've attached a patch on bug 29623, for discussion.
(In reply to Nick Clemens from comment #44) > (In reply to Jonathan Druart from comment #42) > > Ideally > > Koha::AutoRenewalHold inherit from Koha::Hold > > Koha::AutoRenewalHold->can_be_renewed will do its specific check for auto > > renew then can SUPER::can_be_renewed > > > I assume you mean 'Issue' not 'Hold' Of course! I actually replaced "Issue" with "Hold" instead of "Checkout", erk!
(In reply to Jonathan Druart from comment #45) > (In reply to Jonathan Druart from comment #42) > > For instance if we are facing perf issues with the fetch of the circ rules, > > we should then cache them. Other areas would benefit from that kind of > > change. Code will stay clean and we wouldn't need write acrobatic code in > > this C4::Circ module, it does not need that :) > > I've attached a patch on bug 29623, for discussion. I tested that similar to here, I did not se any significant performance change. I then tested here without the last two patches to see how much passing rules and dates save us. It turns out it make the routine take longer. I am not sure how/why, but it seems fetching the values was faster than passing them. I obsoleted the last two patches, as those are the ones you didn't like hopefully we can move this forward ;-)
Created attachment 128554 [details] [review] Bug 29474: Return too_soon before checking renewals This patch allows us to avoid checking reserves when the issue is too_soon and we are running the cron Code is covered by existing tests To test: 1 - prove -v t/db_dependent/Circulation.t Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 128555 [details] [review] Bug 29474: Decouple renewal checks of auto-renewed issues from non-autorenewed The too_soon check does not relate at all to auto-renewed issues and should be moved outside the _CanBookBeAutoRenewed() function. To test: 1) prove t/db_dependent/Circulation.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 128556 [details] [review] Bug 29474: Remove one layer of indendation by adding if check in the begginning If we simply return "no" immediately from the function when the checkout is not an autorenewed checkout we can drop one layer of indendation and the code becomes much easier to read. To test: 1) prove t/db_dependent/Circulation.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Pushed to 21.11.x for 21.11.03
Missing dependency (bug 27032), not backported to 21.05. Please request if needed.
*** Bug 22333 has been marked as a duplicate of this bug. ***