Koha::IssuingRules->get_effective_issuing_rule() seems to be making 8 different queries in worst case scenario. The behavior is inherited from the old C4::Circulation::GetIssuingRule(). Instead, a faster solution is to get the effective issuing rule with one single query by sorting and limiting the result appropriately. This will make a significant improvement in performance in situations where issuing rules need to be requested multiple times.
Created attachment 58228 [details] [review] Bug 17783: Test to confirm correct effective issuing rule selection This patch adds a test to cover the validity of effective issuing rule selection in correct order. To test: 1. Run t/db_dependent/Koha/IssuingRules.t
Created attachment 58229 [details] [review] Bug 17783: Test to print performance of get_effective_issuing_rule This test prints the amount of issuing rule matches per second for 1. worst case, when non-existent branchcode, categorycode and itemtype is being searched (currently 8 queries) 2. mid case (rule found on 4th query) 3. 2nd best case (rule found on 2nd query) 4. best case, when an issuing rule is defined for exactly those branchcode, categorycode and itemtype (currently 1 query) To test: 1. Run t/db_dependent/Koha/IssuingRules.t 2. Write down the per-second amount to compare with next patch
Created attachment 58230 [details] [review] Bug 17783: Add Koha::Objects->single Returns one and only one object that is part of this set. Returns undef if there are no objects found. ->single is faster than ->search->next This is optimal as it will grab the first returned result without instantiating a cursor. It is useful for this Bug as we only want to select the top row of found issuing rules. To test: 1. Run t/db_dependent/Koha/Objects.t
Created attachment 58231 [details] [review] Bug 17783: Optimize Koha::IssuingRules->get_effective_issuing_rule This patch modifies method get_effective_issuing_rule in Koha::IssuingRules aiming to optimize the search for matching issuing rule. Before this patch, in worst case scenario, we have had to make a SELECT query eight times. This will have a negative impact on performance where-ever we need to find matching issuing rule multiple times, if the search is not directly matching an issuing rule on the first query. This patch makes get_effective_issuing_rule have a stable performance on both best and worst case, whereas the old method was really fast on the best case and really slow on the worst case. However, this patch slightly lowers the performance for best case, where matching issuing rule is found instantly before (branchcode, categorycode and itemtype all are specifically defined in issuing rules). For all other cases this patch offers a performance improvement. To test: 1. Run t/db_dependent/Koha/IssuingRules.t and compare the results with previous tests.
Definitions: We want to find an issuing rule with parameters branchcode 'B', categorycode 'C' and itemtype 'I'. For current implementation, - Best case = there is an issuing rule defined with 'B', 'C' and 'I'. - Second best case = there is an issuing rule defined with 'B', 'C' and '*'. - Mid case = there is an issuing rule defined with 'B', '*', '*'. - Worst case = matching issuing rule will be '*', '*', '*'. (These are following the order defined in circulation & fines rules page) My results: OLD IMPLEMENTATION: ok 1 - In worst case, get_effective_issuing_rule finds matching rule 74.29 times per second. ok 2 - In mid case, get_effective_issuing_rule finds matching rule 144.09 times per second. ok 3 - In second best case, get_effective_issuing_rule finds matching rule 289.02 times per second. ok 4 - In best case, get_effective_issuing_rule finds matching rule 561.80 times per second. NEW IMPLEMENTATION (= after applying last patch): ok 1 - In worst case, get_effective_issuing_rule finds matching rule 434.78 times per second. ok 2 - In mid case, get_effective_issuing_rule finds matching rule 427.35 times per second. ok 3 - In second best case, get_effective_issuing_rule finds matching rule 431.03 times per second. ok 4 - In best case, get_effective_issuing_rule finds matching rule 423.73 times per second.
Created attachment 58268 [details] [review] [SIGNED-OFF] Bug 17783: Test to confirm correct effective issuing rule selection This patch adds a test to cover the validity of effective issuing rule selection in correct order. To test: 1. Run t/db_dependent/Koha/IssuingRules.t Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 58269 [details] [review] [SIGNED-OFF] Bug 17783: Test to print performance of get_effective_issuing_rule This test prints the amount of issuing rule matches per second for 1. worst case, when non-existent branchcode, categorycode and itemtype is being searched (currently 8 queries) 2. mid case (rule found on 4th query) 3. 2nd best case (rule found on 2nd query) 4. best case, when an issuing rule is defined for exactly those branchcode, categorycode and itemtype (currently 1 query) To test: 1. Run t/db_dependent/Koha/IssuingRules.t 2. Write down the per-second amount to compare with next patch Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 58270 [details] [review] [SIGNED-OFF] Bug 17783: Add Koha::Objects->single Returns one and only one object that is part of this set. Returns undef if there are no objects found. ->single is faster than ->search->next This is optimal as it will grab the first returned result without instantiating a cursor. It is useful for this Bug as we only want to select the top row of found issuing rules. To test: 1. Run t/db_dependent/Koha/Objects.t Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 58271 [details] [review] [SIGNED-OFF] Bug 17783: Optimize Koha::IssuingRules->get_effective_issuing_rule This patch modifies method get_effective_issuing_rule in Koha::IssuingRules aiming to optimize the search for matching issuing rule. Before this patch, in worst case scenario, we have had to make a SELECT query eight times. This will have a negative impact on performance where-ever we need to find matching issuing rule multiple times, if the search is not directly matching an issuing rule on the first query. This patch makes get_effective_issuing_rule have a stable performance on both best and worst case, whereas the old method was really fast on the best case and really slow on the worst case. However, this patch slightly lowers the performance for best case, where matching issuing rule is found instantly before (branchcode, categorycode and itemtype all are specifically defined in issuing rules). For all other cases this patch offers a performance improvement. To test: 1. Run t/db_dependent/Koha/IssuingRules.t and compare the results with previous tests. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
(In reply to Lari Taskula from comment #5) > Definitions: > We want to find an issuing rule with parameters branchcode 'B', categorycode > 'C' and itemtype 'I'. > > For current implementation, > - Best case = there is an issuing rule defined with 'B', 'C' and 'I'. > - Second best case = there is an issuing rule defined with 'B', 'C' and '*'. > - Mid case = there is an issuing rule defined with 'B', '*', '*'. > - Worst case = matching issuing rule will be '*', '*', '*'. > (These are following the order defined in circulation & fines rules page) > > My results: > OLD IMPLEMENTATION: > ok 1 - In worst case, get_effective_issuing_rule finds matching rule 74.29 > times per second. > ok 2 - In mid case, get_effective_issuing_rule finds matching rule 144.09 > times per second. > ok 3 - In second best case, get_effective_issuing_rule finds matching rule > 289.02 times per second. > ok 4 - In best case, get_effective_issuing_rule finds matching rule 561.80 > times per second. I have : 80.13 160.26 318.47 617.28 > > NEW IMPLEMENTATION (= after applying last patch): > ok 1 - In worst case, get_effective_issuing_rule finds matching rule 434.78 > times per second. > ok 2 - In mid case, get_effective_issuing_rule finds matching rule 427.35 > times per second. > ok 3 - In second best case, get_effective_issuing_rule finds matching rule > 431.03 times per second. > ok 4 - In best case, get_effective_issuing_rule finds matching rule 423.73 > times per second. And: 581.40 568.18 581.40 581.40
Created attachment 58300 [details] [review] Bug 17783: Prevent crash when providing an undefined value When calling the proposed version of get_effective_issuing_rule with undefined parameter values, a following crash occurs: SQL::Abstract::puke(): [SQL::Abstract::__ANON__] Fatal: SQL::Abstract before v1.75 used to generate incorrect SQL when the -IN operator was given an undef-containing list: !!!AUDIT YOUR CODE AND DATA!!! (the upcoming Data::Query-based version of SQL::Abstract will emit the logically correct SQL instead of raising this exception) at /home/ubuntu/kohaclone/Koha/Objects.pm line 182 This patch adds a test to cover this problem and fixes the issue. To test: 1. Run t/db_dependent/Koha/IsssuingRules.t
I found a bug described above as I started to use this modification in my other project. Josef, could you confirm and sign-off that patch as well?
Created attachment 58304 [details] [review] [SIGNED-OFF] Bug 17783: Prevent crash when providing an undefined value When calling the proposed version of get_effective_issuing_rule with undefined parameter values, a following crash occurs: SQL::Abstract::puke(): [SQL::Abstract::__ANON__] Fatal: SQL::Abstract before v1.75 used to generate incorrect SQL when the -IN operator was given an undef-containing list: !!!AUDIT YOUR CODE AND DATA!!! (the upcoming Data::Query-based version of SQL::Abstract will emit the logically correct SQL instead of raising this exception) at /home/ubuntu/kohaclone/Koha/Objects.pm line 182 This patch adds a test to cover this problem and fixes the issue. To test: 1. Run t/db_dependent/Koha/IsssuingRules.t Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
(In reply to Lari Taskula from comment #12) > I found a bug described above as I started to use this modification in my > other project. Josef, could you confirm and sign-off that patch as well? Yes, I was able to get this error too.
Created attachment 58309 [details] [review] Bug 17783: Test to confirm correct effective issuing rule selection This patch adds a test to cover the validity of effective issuing rule selection in correct order. To test: 1. Run t/db_dependent/Koha/IssuingRules.t Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 58310 [details] [review] Bug 17783: Test to print performance of get_effective_issuing_rule This test prints the amount of issuing rule matches per second for 1. worst case, when non-existent branchcode, categorycode and itemtype is being searched (currently 8 queries) 2. mid case (rule found on 4th query) 3. 2nd best case (rule found on 2nd query) 4. best case, when an issuing rule is defined for exactly those branchcode, categorycode and itemtype (currently 1 query) To test: 1. Run t/db_dependent/Koha/IssuingRules.t 2. Write down the per-second amount to compare with next patch Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 58311 [details] [review] Bug 17783: Add Koha::Objects->single Returns one and only one object that is part of this set. Returns undef if there are no objects found. ->single is faster than ->search->next This is optimal as it will grab the first returned result without instantiating a cursor. It is useful for this Bug as we only want to select the top row of found issuing rules. To test: 1. Run t/db_dependent/Koha/Objects.t Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 58312 [details] [review] Bug 17783: Optimize Koha::IssuingRules->get_effective_issuing_rule This patch modifies method get_effective_issuing_rule in Koha::IssuingRules aiming to optimize the search for matching issuing rule. Before this patch, in worst case scenario, we have had to make a SELECT query eight times. This will have a negative impact on performance where-ever we need to find matching issuing rule multiple times, if the search is not directly matching an issuing rule on the first query. This patch makes get_effective_issuing_rule have a stable performance on both best and worst case, whereas the old method was really fast on the best case and really slow on the worst case. However, this patch slightly lowers the performance for best case, where matching issuing rule is found instantly before (branchcode, categorycode and itemtype all are specifically defined in issuing rules). For all other cases this patch offers a performance improvement. To test: 1. Run t/db_dependent/Koha/IssuingRules.t and compare the results with previous tests. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 58313 [details] [review] Bug 17783: Prevent crash when providing an undefined value When calling the proposed version of get_effective_issuing_rule with undefined parameter values, a following crash occurs: SQL::Abstract::puke(): [SQL::Abstract::__ANON__] Fatal: SQL::Abstract before v1.75 used to generate incorrect SQL when the -IN operator was given an undef-containing list: !!!AUDIT YOUR CODE AND DATA!!! (the upcoming Data::Query-based version of SQL::Abstract will emit the logically correct SQL instead of raising this exception) at /home/ubuntu/kohaclone/Koha/Objects.pm line 182 This patch adds a test to cover this problem and fixes the issue. To test: 1. Run t/db_dependent/Koha/IsssuingRules.t Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 58314 [details] [review] Bug 17783: Replace ok with is Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
I am not sure the 'Performance' tests are useful (as they will always return true) but it will not hurt to have them. Well done, Lari!
(In reply to Jonathan Druart from comment #21) > I am not sure the 'Performance' tests are useful (as they will always return > true) but it will not hurt to have them. > > Well done, Lari! Yeah probably not so useful afterwards, but surely useful for showing the change before and after. Maybe someone will eventually come up with a better improvement and can use that test to confirm it :)
Pushed to master for 17.05, thanks Lari!
This won't get ported back to 16.11.x as it is an enhancement.