Bug 19792 - Reduce number of SQL calls in GetBudgetHierarchy
Summary: Reduce number of SQL calls in GetBudgetHierarchy
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Charles Farmer
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 19586 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-12-11 19:28 UTC by David Bourgault
Modified: 2018-05-11 14:12 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19792 - Perf. boost for GetBudgetHierarchy (5.52 KB, patch)
2017-12-11 19:37 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 19792 - Perf. boost for GetBudgetHierarchy (5.57 KB, patch)
2017-12-20 17:28 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 19792 - Perf. boost for GetBudgetHierarchy (5.67 KB, patch)
2018-01-05 15:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19792: (QA follow-up) Move anonymous sub out of GetHierarchy (2.05 KB, patch)
2018-01-05 15:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19792: (QA follow-up) Move anonymous sub out of GetHierarchy (3.15 KB, patch)
2018-01-05 15:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19792: Few minor fixes (2.44 KB, patch)
2018-01-05 16:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19792 - Perf. boost for GetBudgetHierarchy (5.17 KB, patch)
2018-02-20 05:29 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 19792: (QA follow-up) Move anonymous sub out of GetHierarchy (3.11 KB, patch)
2018-02-20 05:29 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 19792: Few minor fixes (2.43 KB, patch)
2018-02-20 05:29 UTC, Charles Farmer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 19792 - Perf. boost for GetBudgetHierarchy (5.24 KB, patch)
2018-05-02 15:24 UTC, Jon Knight
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 19792: (QA follow-up) Move anonymous sub out of GetHierarchy (3.17 KB, patch)
2018-05-02 15:25 UTC, Jon Knight
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 19792: Few minor fixes (2.50 KB, patch)
2018-05-02 15:25 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 19792: Performance boost for GetBudgetHierarchy (5.27 KB, patch)
2018-05-11 07:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19792: (QA follow-up) Move anonymous sub out of GetHierarchy (3.20 KB, patch)
2018-05-11 07:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19792: Few minor fixes (2.58 KB, patch)
2018-05-11 07:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Bourgault 2017-12-11 19:28:15 UTC
The way GetBudgetHierarchy works right now can result in a (very) high number of SQL calls for databases with many budgets.
This causes slow downs and performance issues for some libraries.
Comment 1 David Bourgault 2017-12-11 19:37:35 UTC
Created attachment 69702 [details] [review]
Bug 19792 - Perf. boost for GetBudgetHierarchy

This patches reduces the number of SQL calls by combining multiple calls to the recursive functions GetBudgetSpent(), GetBudgetOrdered(), etc. into 4 big queries.
It also removes duplicate function calls from acqui-home.pl

Test plan:
0) Visit Acquisition home
0) Apply patch
1) Refresh page. It shoud look identical.
2) prove t/db_dependent/budgets.t
Comment 2 David Bourgault 2017-12-11 19:54:04 UTC
*** Bug 19586 has been marked as a duplicate of this bug. ***
Comment 3 David Bourgault 2017-12-11 19:55:00 UTC
From Bug 19586 (codebase is essentially the same):

> I used the followup patch with CCSR to cut the DB calls from 30000 to 5.  Display idle time went from 30+ seconds to 1.
Comment 4 Jon Knight 2017-12-20 17:28:23 UTC
Created attachment 69960 [details] [review]
Bug 19792 - Perf. boost for GetBudgetHierarchy

This patches reduces the number of SQL calls by combining multiple calls to the recursive functions GetBudgetSpent(), GetBudgetOrdered(), etc. into 4 big queries.
It also removes duplicate function calls from acqui-home.pl

Test plan:
0) Visit Acquisition home
0) Apply patch
1) Refresh page. It shoud look identical.
2) prove t/db_dependent/budgets.t

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 5 Marcel de Rooy 2017-12-22 12:06:00 UTC
I like the idea of such a performance boost.
Could you move the anonymous sub out of GetBudgetHierarchy? Can it be tested separately? Or do you think that the current code in Budgets.t sufficiently covers your changes (including processing a budget tree) ?

Changing status to reflect need for some adjustments/feedback.
Comment 6 David Bourgault 2017-12-22 14:25:07 UTC
I don't think I can move the recursive sub out of GetBudgetHierarchy, since it calls variables local to that function. If it were moved out it would need to either rebuild them everytime, which voids the perf boost, or receive them as parameters, which would make the code less readable, and requires the dev to know the exact SQL query result to pass.

In this case I strongly believe an anonymous inner function is the best way to go. As for tests, the current test to budgets.t should be sufficient as this does not (should not!) change behavior. It is supposed to replace the inner workings of the function with no impact on input or output.
Comment 7 Marcel de Rooy 2018-01-05 15:41:40 UTC
(In reply to David Bourgault from comment #6)
> I don't think I can move the recursive sub out of GetBudgetHierarchy, since
> it calls variables local to that function. If it were moved out it would
> need to either rebuild them everytime, which voids the perf boost, or
> receive them as parameters, which would make the code less readable, and
> requires the dev to know the exact SQL query result to pass.
> 
> In this case I strongly believe an anonymous inner function is the best way
> to go. As for tests, the current test to budgets.t should be sufficient as
> this does not (should not!) change behavior. It is supposed to replace the
> inner workings of the function with no impact on input or output.

Will add a (trivial) follow-up that times around the same, and improves imo readability. Since we just pass references to hash structures, we are not rebuilding structures.
Comment 8 Marcel de Rooy 2018-01-05 15:43:32 UTC
Created attachment 70296 [details] [review]
Bug 19792 - Perf. boost for GetBudgetHierarchy

This patches reduces the number of SQL calls by combining multiple calls to the recursive functions GetBudgetSpent(), GetBudgetOrdered(), etc. into 4 big queries.
It also removes duplicate function calls from acqui-home.pl

Test plan:
0) Visit Acquisition home
0) Apply patch
1) Refresh page. It shoud look identical.
2) prove t/db_dependent/budgets.t

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2018-01-05 15:43:37 UTC
Created attachment 70297 [details] [review]
Bug 19792: (QA follow-up) Move anonymous sub out of GetHierarchy

Test plan:
Run t/db_dependent/Budgets.t.
Without both patches, running the test is slower than with only the first
or both patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2018-01-05 15:53:46 UTC
Created attachment 70298 [details] [review]
Bug 19792: (QA follow-up) Move anonymous sub out of GetHierarchy

Test plan:
Run t/db_dependent/Budgets.t.
Without both patches, running the test is slower than with only the first
or both patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Jonathan Druart 2018-01-05 16:40:50 UTC
I am not convinced.

1. I prefer the version without Marcel's patch (especially because of the size of the parameter list)

2. GetBudgetHierarchy is called from a lot of other places, with a budget_id
For all these calls we are going to get worst performance(?)

3. GetBudgetHierarchy is not tested when called without parameter

4. There is a FIXME you removed, but you did not fix the problem, you hid it

5.
> This causes slow downs and performance issues for some libraries.

How many budgets/funds? What are the performances before/after this patch?
Comment 12 Jonathan Druart 2018-01-05 16:42:17 UTC
Created attachment 70306 [details] [review]
Bug 19792: Few minor fixes

- No string extrapolation needed on SQL queries
- '== undef' must be written 'if not defined'
Comment 13 Marcel de Rooy 2018-01-05 16:47:46 UTC
(In reply to Jonathan Druart from comment #11)
> 1. I prefer the version without Marcel's patch (especially because of the
> size of the parameter list)

I have seen worse :)
Comment 14 Nicolas Legrand 2018-01-26 11:58:26 UTC
Well, what about a prompt correction such as :

- by default on acquisition home, GetBudgetHierarchy loads the last budget period
- add a form on acquisition home to make GetBudgetHierarchy choose a different budget period or all of them

It remains bloated, but it'll be more expeditious for those who appreciate having an unreasonable amount of budgets.
Comment 15 Blou 2018-01-26 13:17:49 UTC
> How many budgets/funds? What are the performances before/after this patch?

Over 500 aqbudgets.
Performance went from 30+ seconds to less than 1 second.
Comment 16 Blou 2018-01-26 14:03:49 UTC
(In reply to Nicolas Legrand from comment #14)
> Well, what about a prompt correction such as :
> 
> - by default on acquisition home, GetBudgetHierarchy loads the last budget
> period
> - add a form on acquisition home to make GetBudgetHierarchy choose a
> different budget period or all of them
> 

Prompt correction?  As in simple & fast?  There's nothing quick and dirty about changing default behavior in a screen.  That would be a totally different ticket.

Although, feel welcome to pile on this one.  We fixed it for the sponsor, we pointed it to the community, but if it wants to go in a different direction it's as well, we're pretty detached regarding it.
Comment 17 Nicolas Legrand 2018-01-26 15:08:08 UTC
I did a benchmark on a 16.11.00 with NYTProf, with and without the patch, with GetBudgetHierarchy being called with arguments.

We have 517 aqbudgets lines.

acqui-home without 19792 ran in 19.5s, 4.83s is spent in GetBudgetHierarchy.
acqui-home with 19792 ran in 12.3s, 38.6ms is spent in GetBudgetHierarchy.

So it does improve our performances even when GetBudgetHierarchy is called without arguments. I'll test monday on orderreceive which load GetBudgetHierarchy with a budgetid.
Comment 18 Nicolas Legrand 2018-01-29 10:56:42 UTC
(In reply to Blou from comment #16)
> (In reply to Nicolas Legrand from comment #14)
[...]
> 
> Prompt correction?  As in simple & fast?  There's nothing quick and dirty
> about changing default behavior in a screen.  That would be a totally
> different ticket.

Ha sure! Nevertheless, I didn't mean “quick and dirty”. For the moment, I hardcoded the budgetid for GetBudgetHierarchy in acquisition home. That's quick and dirty ;).
Comment 19 Nicolas Legrand 2018-01-29 11:13:27 UTC
I just tried NYTProf on orderreceive.pl which loaded twice  GetBudgetHierarchy with a budget id.

orderreceive without 19792 ran in 6.05s, 38.9ms is spent in GetBudgetHierarchy.
orderreceive with 19792 ran in 4.81s, 359ms is spent in GetBudgetHierarchy.

Therefore, the function is still faster with this patch, even when launched with a budget id.

We will use this patch in production as it saves some seconds in a module that became a sluggishness nightmare since 16.11. We would like to warmly thank InLibro and Collecto for this improvement!
Comment 20 Katrin Fischer 2018-01-29 20:58:04 UTC
(In reply to Nicolas Legrand from comment #14)
> Well, what about a prompt correction such as :
> 
> - by default on acquisition home, GetBudgetHierarchy loads the last budget
> period

We have libraries with multiple active budgets in the same or similar timeframes (timeframes could overlap, external funds for 2 years, regular budget for 1 year, ...). If we do this, I'd prefer to default to active budgets and only load inactive on demand.
Comment 21 Charles Farmer 2018-02-20 05:29:40 UTC
Created attachment 71989 [details] [review]
Bug 19792 - Perf. boost for GetBudgetHierarchy

This patches reduces the number of SQL calls by combining multiple calls to the recursive functions GetBudgetSpent(), GetBudgetOrdered(), etc. into 4 big queries.
It also removes duplicate function calls from acqui-home.pl

Test plan:
0) Visit Acquisition home
0) Apply patch
1) Refresh page. It shoud look identical.
2) prove t/db_dependent/budgets.t

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Charles Farmer 2018-02-20 05:29:44 UTC
Created attachment 71990 [details] [review]
Bug 19792: (QA follow-up) Move anonymous sub out of GetHierarchy

Test plan:
Run t/db_dependent/Budgets.t.
Without both patches, running the test is slower than with only the first
or both patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Charles Farmer 2018-02-20 05:29:48 UTC
Created attachment 71991 [details] [review]
Bug 19792: Few minor fixes

- No string extrapolation needed on SQL queries
- '== undef' must be written 'if not defined'
Comment 24 Charles Farmer 2018-02-20 05:33:34 UTC
Rebasing over the new version of acqui/acqui-home.pl
Comment 25 Jon Knight 2018-05-02 15:24:58 UTC
Created attachment 74983 [details] [review]
[SIGNED-OFF] Bug 19792 - Perf. boost for GetBudgetHierarchy

This patches reduces the number of SQL calls by combining multiple calls to the recursive functions GetBudgetSpent(), GetBudgetOrdered(), etc. into 4 big queries.
It also removes duplicate function calls from acqui-home.pl

Test plan:
0) Visit Acquisition home
0) Apply patch
1) Refresh page. It shoud look identical.
2) prove t/db_dependent/Budgets.t

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 26 Jon Knight 2018-05-02 15:25:02 UTC
Created attachment 74984 [details] [review]
[SIGNED-OFF] Bug 19792: (QA follow-up) Move anonymous sub out of GetHierarchy

Test plan:
Run t/db_dependent/Budgets.t.
Without both patches, running the test is slower than with only the first
or both patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 27 Jon Knight 2018-05-02 15:25:06 UTC
Created attachment 74985 [details] [review]
[SIGNED-OFF] Bug 19792: Few minor fixes

- No string extrapolation needed on SQL queries
- '== undef' must be written 'if not defined'

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 28 Jon Knight 2018-05-02 15:27:18 UTC
Oh, and whilst signing off again, I noted the speed increase running the tests. Not huge but there is one even on a system with limited budgets.

Without these patches:

real	0m2.269s
user	0m1.712s
sys	0m0.344s

With these patches:

real	0m1.982s
user	0m1.548s
sys	0m0.308s
Comment 29 Marcel de Rooy 2018-05-11 07:58:00 UTC
Created attachment 75258 [details] [review]
Bug 19792: Performance boost for GetBudgetHierarchy

This patches reduces the number of SQL calls by combining multiple calls to the recursive functions GetBudgetSpent(), GetBudgetOrdered(), etc. into 4 big queries.
It also removes duplicate function calls from acqui-home.pl

Test plan:
0) Visit Acquisition home
0) Apply patch
1) Refresh page. It shoud look identical.
2) prove t/db_dependent/budgets.t

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 30 Marcel de Rooy 2018-05-11 07:58:07 UTC
Created attachment 75259 [details] [review]
Bug 19792: (QA follow-up) Move anonymous sub out of GetHierarchy

Test plan:
Run t/db_dependent/Budgets.t.
Without both patches, running the test is slower than with only the first
or both patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 31 Marcel de Rooy 2018-05-11 07:58:12 UTC
Created attachment 75260 [details] [review]
Bug 19792: Few minor fixes

- No string extrapolation needed on SQL queries
- '== undef' must be written 'if not defined'

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2018-05-11 07:59:58 UTC
Back into PQA, no strong attachment to the second patch, just preferential of course ;)
Comment 33 Jonathan Druart 2018-05-11 14:12:56 UTC
Pushed to master for 18.05, thanks to everybody involved!