|
Description
David Bourgault
2017-12-11 19:28:15 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 *** Bug 19586 has been marked as a duplicate of this bug. *** 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. 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> 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. 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. (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. 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> 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> 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> 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?
Created attachment 70306 [details] [review] Bug 19792: Few minor fixes - No string extrapolation needed on SQL queries - '== undef' must be written 'if not defined' (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 :) 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.
> 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.
(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. 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. (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 ;). 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! (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. 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> 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> Created attachment 71991 [details] [review] Bug 19792: Few minor fixes - No string extrapolation needed on SQL queries - '== undef' must be written 'if not defined' Rebasing over the new version of acqui/acqui-home.pl 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> 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> 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> 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 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> 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> 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> Back into PQA, no strong attachment to the second patch, just preferential of course ;) Pushed to master for 18.05, thanks to everybody involved! |