Bug 17783 - Optimize Koha::IssuingRules->get_effective_issuing_rule
Summary: Optimize Koha::IssuingRules->get_effective_issuing_rule
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Lari Taskula
QA Contact: Testopia
URL:
Keywords:
Depends on: 17599
Blocks:
  Show dependency treegraph
 
Reported: 2016-12-15 15:04 UTC by Lari Taskula
Modified: 2017-12-07 22:16 UTC (History)
5 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 17783: Test to confirm correct effective issuing rule selection (7.73 KB, patch)
2016-12-15 17:43 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17783: Test to print performance of get_effective_issuing_rule (4.03 KB, patch)
2016-12-15 17:43 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17783: Add Koha::Objects->single (3.00 KB, patch)
2016-12-15 17:44 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 17783: Optimize Koha::IssuingRules->get_effective_issuing_rule (3.50 KB, patch)
2016-12-15 17:44 UTC, Lari Taskula
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17783: Test to confirm correct effective issuing rule selection (7.80 KB, patch)
2016-12-18 19:50 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17783: Test to print performance of get_effective_issuing_rule (4.10 KB, patch)
2016-12-18 19:50 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17783: Add Koha::Objects->single (3.06 KB, patch)
2016-12-18 19:50 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17783: Optimize Koha::IssuingRules->get_effective_issuing_rule (3.57 KB, patch)
2016-12-18 19:50 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17783: Prevent crash when providing an undefined value (4.09 KB, patch)
2016-12-20 15:07 UTC, Lari Taskula
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17783: Prevent crash when providing an undefined value (4.15 KB, patch)
2016-12-20 15:37 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17783: Test to confirm correct effective issuing rule selection (7.85 KB, patch)
2016-12-20 17:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17783: Test to print performance of get_effective_issuing_rule (4.14 KB, patch)
2016-12-20 17:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17783: Add Koha::Objects->single (3.11 KB, patch)
2016-12-20 17:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17783: Optimize Koha::IssuingRules->get_effective_issuing_rule (3.62 KB, patch)
2016-12-20 17:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17783: Prevent crash when providing an undefined value (4.20 KB, patch)
2016-12-20 17:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17783: Replace ok with is (1.42 KB, patch)
2016-12-20 17:08 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2016-12-15 15:04:49 UTC
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.
Comment 1 Lari Taskula 2016-12-15 17:43:40 UTC
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
Comment 2 Lari Taskula 2016-12-15 17:43:53 UTC
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
Comment 3 Lari Taskula 2016-12-15 17:44:06 UTC
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
Comment 4 Lari Taskula 2016-12-15 17:44:24 UTC
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.
Comment 5 Lari Taskula 2016-12-15 17:50:17 UTC
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.
Comment 6 Josef Moravec 2016-12-18 19:50:29 UTC
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>
Comment 7 Josef Moravec 2016-12-18 19:50:35 UTC
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>
Comment 8 Josef Moravec 2016-12-18 19:50:39 UTC
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>
Comment 9 Josef Moravec 2016-12-18 19:50:44 UTC
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>
Comment 10 Josef Moravec 2016-12-18 19:51:48 UTC
(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
Comment 11 Lari Taskula 2016-12-20 15:07:56 UTC
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
Comment 12 Lari Taskula 2016-12-20 15:09:18 UTC
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?
Comment 13 Josef Moravec 2016-12-20 15:37:32 UTC
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>
Comment 14 Josef Moravec 2016-12-20 15:38:26 UTC
(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.
Comment 15 Jonathan Druart 2016-12-20 17:08:04 UTC
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>
Comment 16 Jonathan Druart 2016-12-20 17:08:09 UTC
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>
Comment 17 Jonathan Druart 2016-12-20 17:08:13 UTC
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>
Comment 18 Jonathan Druart 2016-12-20 17:08:16 UTC
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>
Comment 19 Jonathan Druart 2016-12-20 17:08:20 UTC
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>
Comment 20 Jonathan Druart 2016-12-20 17:08:25 UTC
Created attachment 58314 [details] [review]
Bug 17783: Replace ok with is

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 21 Jonathan Druart 2016-12-20 17:10:47 UTC
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!
Comment 22 Lari Taskula 2016-12-20 17:24:47 UTC
(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 :)
Comment 23 Kyle M Hall 2016-12-23 12:04:23 UTC
Pushed to master for 17.05, thanks Lari!
Comment 24 Katrin Fischer 2016-12-27 22:21:39 UTC
This won't get ported back to 16.11.x as it is an enhancement.