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.
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
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>
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>
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.
(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
(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!
You are mixing unblessed and blessed in budget_owner. The logged in patron will be a hashref and others will be Koha::Patron objects.
(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.
(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' );
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>
It works! :) Nice improvement, 11 sec vs 21.5 sec Adding a second signoff.
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>
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>
(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!
Pushed for 24.05! Well done everyone, thank you!
Pushed to 23.11.x for 23.11.04
Backported to 23.05.x for upcoming 23.05.12