Bug 30359

Summary: GetBudgetHierarchy is slow on order receive page
Product: Koha Reporter: Johanna Räisä <johanna.raisa>
Component: AcquisitionsAssignee: Johanna Räisä <johanna.raisa>
Status: CLOSED FIXED QA Contact: Joonas Kylmälä <joonas.kylmala>
Severity: normal    
Priority: P5 - low CC: arthur.suzuki, dcook, emmi.takkinen, joonas.kylmala, lucas, nick, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32377
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.07, 21.11.14
Bug Depends on:    
Bug Blocks: 31459    
Attachments: Bug 30359: GetBudgetHierarchy is slow on order receive page
Bug 30359: GetBudgetHierarchy is slow on order receive page
Bug 30359: Unit tests
Bug 30359: GetBudgetHierarchy is slow on order receive page
Bug 30359: Unit tests

Description Johanna Räisä 2022-03-25 09:49:38 UTC
GetBudgetHierarchy is called when opening orderreceive.pl and if there are lot of budgets the page takes lot of time to open. On our biggest groups it takes almost 2 minute, which is a lifetime on internet.

Seems that the problem is calculating the totals, which are done with separate queries to the database. On the order receive page it is not needed since it is using only the budget_id, budget_amount and budget_name parameters.
Comment 1 Johanna Räisä 2022-03-28 05:58:03 UTC
Created attachment 132283 [details] [review]
Bug 30359: GetBudgetHierarchy is slow on order receive page

This patch adds skiptotals parameter to GetBudgetHierarchy so calculating
totals can be skipped from some pages.

Test plan:
1) Open browser's Inspect -> Network
2) Go to receive orders
3) Check the timings for page load
4) Apply the patch
5) Refresh the page
6) Check the timings again
7) prove t/db_dependent/Budgets.t

Sponsored-by: Koha-Suomi Oy
Comment 2 Nick Clemens 2022-03-29 10:42:12 UTC
One this page we are calling GetBudgetHierarchy in a loop on al the budget periods - this seems to be extra work.

Compare this to the way things are handled in neworderempty.pl:
333 my $budget =  GetBudget($budget_id);
334 # build budget list
335 my $budget_loop = []; 
336 my $budgets = GetBudgetHierarchy;
337 foreach my $r (@{$budgets}) {
338     next unless (CanUserUseBudget($patron, $r, $userflags));
339     push @{$budget_loop}, {
340         b_id  => $r->{budget_id},
341         b_txt => $r->{budget_name},
342         b_sort1_authcat => $r->{'sort1_authcat'},
343         b_sort2_authcat => $r->{'sort2_authcat'},
344         b_active => $r->{budget_period_active},
345         b_sel => ( $r->{budget_id} == $budget_id ) ? 1 : 0,
346         b_level => $r->{budget_level},
347     };
348 }

I think we have other occurrences as well - we should consolidate this code and make the dropdowns work the same

I am not against removing the totals, but I think the loop in the script is the bigger culprit here
Comment 3 Johanna Räisä 2022-03-30 05:08:19 UTC
(In reply to Nick Clemens from comment #2)
> One this page we are calling GetBudgetHierarchy in a loop on al the budget
> periods - this seems to be extra work.
> 
> Compare this to the way things are handled in neworderempty.pl:
> 333 my $budget =  GetBudget($budget_id);
> 334 # build budget list
> 335 my $budget_loop = []; 
> 336 my $budgets = GetBudgetHierarchy;
> 337 foreach my $r (@{$budgets}) {
> 338     next unless (CanUserUseBudget($patron, $r, $userflags));
> 339     push @{$budget_loop}, {
> 340         b_id  => $r->{budget_id},
> 341         b_txt => $r->{budget_name},
> 342         b_sort1_authcat => $r->{'sort1_authcat'},
> 343         b_sort2_authcat => $r->{'sort2_authcat'},
> 344         b_active => $r->{budget_period_active},
> 345         b_sel => ( $r->{budget_id} == $budget_id ) ? 1 : 0,
> 346         b_level => $r->{budget_level},
> 347     };
> 348 }
> 
> I think we have other occurrences as well - we should consolidate this code
> and make the dropdowns work the same
> 
> I am not against removing the totals, but I think the loop in the script is
> the bigger culprit here

Yes, there might be other occurrences which need optimization too. When I investigated the culprit the loop didn't stand out. The real affect came from skipping the calculations. The improvement was huge, the waiting time was first 1.89 minutes and after this patch it went to 460ms.

If some places we are using only part of what GetBudgetHierarchy offers, would it be a good idea make separate functions?
Comment 4 Nick Clemens 2022-08-25 11:20:13 UTC
Created attachment 139765 [details] [review]
Bug 30359: GetBudgetHierarchy is slow on order receive page

This patch adds skiptotals parameter to GetBudgetHierarchy so calculating
totals can be skipped from some pages.

Test plan:
1) Open browser's Inspect -> Network
2) Go to receive orders
3) Check the timings for page load
4) Apply the patch
5) Refresh the page
6) Check the timings again
7) prove t/db_dependent/Budgets.t

Sponsored-by: Koha-Suomi Oy

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Nick Clemens 2022-08-25 11:20:17 UTC
Created attachment 139766 [details] [review]
Bug 30359: Unit tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 6 Joonas Kylmälä 2022-08-31 18:25:27 UTC
Created attachment 140017 [details] [review]
Bug 30359: GetBudgetHierarchy is slow on order receive page

This patch adds skiptotals parameter to GetBudgetHierarchy so calculating
totals can be skipped from some pages.

Test plan:
1) Open browser's Inspect -> Network
2) Go to receive orders
3) Check the timings for page load
4) Apply the patch
5) Refresh the page
6) Check the timings again
7) prove t/db_dependent/Budgets.t

Sponsored-by: Koha-Suomi Oy

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 7 Joonas Kylmälä 2022-08-31 18:25:31 UTC
Created attachment 140018 [details] [review]
Bug 30359: Unit tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 8 Tomás Cohen Arazi 2022-09-21 22:05:00 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 9 Lucas Gass 2022-10-31 22:29:49 UTC
Backported to 22.05.x for upcoming 22.05.07 release
Comment 10 Arthur Suzuki 2022-11-14 16:28:59 UTC
applied to 21.11 for 21.11.14
Comment 11 Victor Grousset/tuxayo 2022-11-24 20:55:52 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document it seems, marking resolved.