Bug 35921 - Improve performance of acquisitions start page when there are many budgets
Summary: Improve performance of acquisitions start page when there are many budgets
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Julian Maurice
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 7923
  Show dependency treegraph
 
Reported: 2024-01-26 15:18 UTC by Julian Maurice
Modified: 2024-03-21 10:06 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.04


Attachments
Bug 35921: Improve perfs of acqui-home.pl when there are many budgets (2.27 KB, patch)
2024-01-26 15:19 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 35921: Improve perfs of acqui-home.pl when there are many budgets (2.33 KB, patch)
2024-02-23 10:34 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35921: (QA follow-up): Less code, less nested code (1.13 KB, patch)
2024-02-23 10:34 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35921: Improve perfs of acqui-home.pl when there are many budgets (2.38 KB, patch)
2024-03-16 22:34 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35921: Improve perfs of acqui-home.pl when there are many budgets (2.45 KB, patch)
2024-03-18 14:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35921: Improve perfs of acqui-home.pl when there are many budgets (2.46 KB, patch)
2024-03-18 14:23 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2024-01-26 15:18:51 UTC
When there are a lot of budgets with the same owner, most of the time of acqui-home.pl is spent loading the same patron over and over.
Comment 1 Julian Maurice 2024-01-26 15:19:43 UTC
Created attachment 161546 [details] [review]
Bug 35921: Improve perfs of acqui-home.pl when there are many budgets

When there are a lot of budgets with the same owner, most of the time of
acqui-home.pl is spent loading the same patron over and over.

This patch makes sure each borrower is loaded only once.

Test plan:
0. Do not apply the patch yet
1. Create a thousand budgets with the following command (make sure the
   budget_owner_id is an existing borrowernumber):
       perl -MKoha::Database -e '
           my $schema = Koha::Database->schema;
           my $period = $schema->resultset("Aqbudgetperiod")->create({
               budget_period_startdate => "2000-01-01",
               budget_period_enddate => "2999-12-31"
           });
           $schema->resultset("Aqbudget")->create({
               budget_owner_id => 1,
               budget_period_id => $period->id
           }) for (1..1000)
       '
2. Measure the time it takes to load acqui/acqui-home.pl (do it several
   times and keep the average time)
3. Apply the patch
4. Repeat step 2
Comment 2 Pedro Amorim 2024-02-23 10:34:36 UTC
Created attachment 162372 [details] [review]
Bug 35921: Improve perfs of acqui-home.pl when there are many budgets

When there are a lot of budgets with the same owner, most of the time of
acqui-home.pl is spent loading the same patron over and over.

This patch makes sure each borrower is loaded only once.

Test plan:
0. Do not apply the patch yet
1. Create a thousand budgets with the following command (make sure the
   budget_owner_id is an existing borrowernumber):
       perl -MKoha::Database -e '
           my $schema = Koha::Database->schema;
           my $period = $schema->resultset("Aqbudgetperiod")->create({
               budget_period_startdate => "2000-01-01",
               budget_period_enddate => "2999-12-31"
           });
           $schema->resultset("Aqbudget")->create({
               budget_owner_id => 1,
               budget_period_id => $period->id
           }) for (1..1000)
       '
2. Measure the time it takes to load acqui/acqui-home.pl (do it several
   times and keep the average time)
3. Apply the patch
4. Repeat step 2

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 3 Pedro Amorim 2024-02-23 10:34:38 UTC
Created attachment 162373 [details] [review]
Bug 35921: (QA follow-up): Less code, less nested code

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 4 Pedro Amorim 2024-02-23 10:35:37 UTC
Hi Julian, I've SOed and provided a follow-up.
Please take a look and see if you agree, or make it obsolete if not.
Comment 5 Julian Maurice 2024-02-23 10:49:45 UTC
(In reply to Pedro Amorim from comment #4)
> Hi Julian, I've SOed and provided a follow-up.
> Please take a look and see if you agree, or make it obsolete if not.
There is a small difference between your code and mine: in yours, if budget_owner_id does not correspond to an existing borrower, it will repeatedly try to fetch it. As there is no foreign key constraint on budget_owner_id, it can happen.
Quite an edge case, but certainly possible.
I'll let QA decide
Comment 6 Pedro Amorim 2024-02-23 12:04:53 UTC
(In reply to Julian Maurice from comment #5)
> (In reply to Pedro Amorim from comment #4)
> > Hi Julian, I've SOed and provided a follow-up.
> > Please take a look and see if you agree, or make it obsolete if not.
> There is a small difference between your code and mine: in yours, if
> budget_owner_id does not correspond to an existing borrower, it will
> repeatedly try to fetch it. As there is no foreign key constraint on
> budget_owner_id, it can happen.
> Quite an edge case, but certainly possible.
> I'll let QA decide

No, you're right. The point of the patch was to optimize code without changing behavior, but it does. I've made it obsolete!
Comment 7 Jonathan Druart 2024-02-29 10:49:23 UTC
You are mixing unblessed and blessed in budget_owner.
The logged in patron will be a hashref and others will be Koha::Patron objects.
Comment 8 Julian Maurice 2024-02-29 10:54:37 UTC
(In reply to Jonathan Druart from comment #7)
> You are mixing unblessed and blessed in budget_owner.
> The logged in patron will be a hashref and others will be Koha::Patron
> objects.
No ? The %patrons hash only contains Koha::Patron object. $loggedinpatron is a hashref but it's not in the %patrons hash and is not used for budget_owner, only as a CanUserUseBudget parameter.
Comment 9 Victor Grousset/tuxayo 2024-03-16 22:34:30 UTC
(In reply to Jonathan Druart from comment #7)
> You are mixing unblessed and blessed in budget_owner.

(In reply to Julian Maurice from comment #8)
> The %patrons hash only contains Koha::Patron object.

I seems that's indeed the case, dumping %patrons:
> $VAR2 = undef;
> $VAR3 = '1';
> $VAR4 = bless( {
> [...]
>               }, 'Koha::Patron' );
> $VAR5 = '51';
> $VAR6 = bless( {
> [...]
>               }, 'Koha::Patron' );
Comment 10 Victor Grousset/tuxayo 2024-03-16 22:34:55 UTC
Created attachment 163288 [details] [review]
Bug 35921: Improve perfs of acqui-home.pl when there are many budgets

When there are a lot of budgets with the same owner, most of the time of
acqui-home.pl is spent loading the same patron over and over.

This patch makes sure each borrower is loaded only once.

Test plan:
0. Do not apply the patch yet
1. Create a thousand budgets with the following command (make sure the
   budget_owner_id is an existing borrowernumber):
       perl -MKoha::Database -e '
           my $schema = Koha::Database->schema;
           my $period = $schema->resultset("Aqbudgetperiod")->create({
               budget_period_startdate => "2000-01-01",
               budget_period_enddate => "2999-12-31"
           });
           $schema->resultset("Aqbudget")->create({
               budget_owner_id => 1,
               budget_period_id => $period->id
           }) for (1..1000)
       '
2. Measure the time it takes to load acqui/acqui-home.pl (do it several
   times and keep the average time)
3. Apply the patch
4. Repeat step 2

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 11 Victor Grousset/tuxayo 2024-03-16 23:48:29 UTC
It works! :)
Nice improvement, 11 sec vs 21.5 sec

Adding a second signoff.
Comment 12 Martin Renvoize 2024-03-18 14:22:20 UTC
Created attachment 163336 [details] [review]
Bug 35921: Improve perfs of acqui-home.pl when there are many budgets

When there are a lot of budgets with the same owner, most of the time of
acqui-home.pl is spent loading the same patron over and over.

This patch makes sure each borrower is loaded only once.

Test plan:
0. Do not apply the patch yet
1. Create a thousand budgets with the following command (make sure the
   budget_owner_id is an existing borrowernumber):
       perl -MKoha::Database -e '
           my $schema = Koha::Database->schema;
           my $period = $schema->resultset("Aqbudgetperiod")->create({
               budget_period_startdate => "2000-01-01",
               budget_period_enddate => "2999-12-31"
           });
           $schema->resultset("Aqbudget")->create({
               budget_owner_id => 1,
               budget_period_id => $period->id
           }) for (1..1000)
       '
2. Measure the time it takes to load acqui/acqui-home.pl (do it several
   times and keep the average time)
3. Apply the patch
4. Repeat step 2

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2024-03-18 14:23:50 UTC
Created attachment 163337 [details] [review]
Bug 35921: Improve perfs of acqui-home.pl when there are many budgets

When there are a lot of budgets with the same owner, most of the time of
acqui-home.pl is spent loading the same patron over and over.

This patch makes sure each borrower is loaded only once.

Test plan:
0. Do not apply the patch yet
1. Create a thousand budgets with the following command (make sure the
   budget_owner_id is an existing borrowernumber):
       perl -MKoha::Database -e '
           my $schema = Koha::Database->schema;
           my $period = $schema->resultset("Aqbudgetperiod")->create({
               budget_period_startdate => "2000-01-01",
               budget_period_enddate => "2999-12-31"
           });
           $schema->resultset("Aqbudget")->create({
               budget_owner_id => 1,
               budget_period_id => $period->id
           }) for (1..1000)
       '
2. Measure the time it takes to load acqui/acqui-home.pl (do it several
   times and keep the average time)
3. Apply the patch
4. Repeat step 2

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Jonathan Druart 2024-03-18 15:18:20 UTC
(In reply to Julian Maurice from comment #8)
> (In reply to Jonathan Druart from comment #7)
> > You are mixing unblessed and blessed in budget_owner.
> > The logged in patron will be a hashref and others will be Koha::Patron
> > objects.
> No ? The %patrons hash only contains Koha::Patron object. $loggedinpatron is
> a hashref but it's not in the %patrons hash and is not used for
> budget_owner, only as a CanUserUseBudget parameter.

Yes, sorry, it seems that I misread something!
Comment 15 Katrin Fischer 2024-03-18 17:05:02 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 16 Fridolin Somers 2024-03-21 10:06:23 UTC
Pushed to 23.11.x for 23.11.04