Bug 33795 - Holds Queue builder should do cheap checks before expensive checks
Summary: Holds Queue builder should do cheap checks before expensive checks
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 33794
Blocks:
  Show dependency treegraph
 
Reported: 2023-05-22 15:20 UTC by Kyle M Hall
Modified: 2023-06-12 21:45 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 33795: Holds Queue builder should do cheap checks before expensive checks (2.18 KB, patch)
2023-05-22 16:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33795: Holds Queue builder should do cheap checks before expensive checks (2.22 KB, patch)
2023-05-24 20:17 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 33795: Holds Queue builder should do cheap checks before expensive checks (2.14 KB, patch)
2023-06-12 11:00 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2023-05-22 15:20:21 UTC
The holds queue builder runs many checks in a somewhat arbitrary order. We should order those checks such that the most expensive checks are at the end and can be avoided if a faster check fails.
Comment 1 Kyle M Hall 2023-05-22 16:24:23 UTC
Created attachment 151535 [details] [review]
Bug 33795: Holds Queue builder should do cheap checks before expensive checks

The holds queue builder runs many checks in a somewhat arbitrary order. We should order those checks such that the most expensive checks are at the end and can be avoided if a faster check fails.

Test Plan:
1) prove t/db_dependent/HoldsQueue.t
2) Apply this patch
3) prove t/db_dependent/HoldsQueue.t
4) Tests still pass!
Comment 2 Sam Lau 2023-05-24 20:17:29 UTC
Created attachment 151654 [details] [review]
Bug 33795: Holds Queue builder should do cheap checks before expensive checks

The holds queue builder runs many checks in a somewhat arbitrary order. We should order those checks such that the most expensive checks are at the end and can be avoided if a faster check fails.

Test Plan:
1) prove t/db_dependent/HoldsQueue.t
2) Apply this patch
3) prove t/db_dependent/HoldsQueue.t
4) Tests still pass!

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 3 Kyle M Hall 2023-06-12 11:00:30 UTC
Created attachment 152276 [details] [review]
Bug 33795: Holds Queue builder should do cheap checks before expensive checks

The holds queue builder runs many checks in a somewhat arbitrary order. We should order those checks such that the most expensive checks are at the end and can be avoided if a faster check fails.

Test Plan:
1) prove t/db_dependent/HoldsQueue.t
2) Apply this patch
3) prove t/db_dependent/HoldsQueue.t
4) Tests still pass!

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 4 Victor Grousset/tuxayo 2023-06-12 21:45:11 UTC
Sorry, I didn't have the time to finish this message yesterday to avoid you coming back twice to this ticket:

In addition to the conflict: the tests don't cover the changes, one can straight up comment the lines that the patch would have changed and the tests still passe. 😱

So, test plan invalid.

---

About the aim of the patch:
«We should order those checks such that the most expensive checks are at the end and can be avoided if a faster check fails.»

It's not necessary faster in practice to have the checks ordered by cost. It can be better to have the most likely to succeed first. Because fast checks that very rarely are reached in the original code would actually be an additional time if we just order by speed.
But!: If they are so cheap compared to the more expensive checks, then it doesn't make a difference. And the few times they succeed before the more expensive checks make up for that and will be a net gain. Assuming they don't often succeed, which will be eventually the next question.
Now the question is: how cheap are they compared to the other checks?
To me it looks like they just access already available object attributes that I guess are loaded eagerly by the ORM. And the other checks need to fetch stuff in the DB so the difference is massive. What do you think?