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.
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!
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>
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>
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. [U+1F631] 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?