|
Description
David Gustafsson
2023-05-16 11:41:03 UTC
Created attachment 151273 [details] [review] Bug 33745: Lazily create attribute accessor methods in AUTOLOAD Lazily create accessor methods when getting or setting an Koha::Object attribute resulting in a significant speed up. To test: 1) Run the benchmark.pl script 2) Apply the patch 3) Run the script again, Koha::Object accessors be about 7 times faster 5) Ensure tests in t/db_dependent/Item.t still pass Created attachment 151274 [details]
Benchmark script
Created attachment 151275 [details] [review] Bug 33745: Lazily create attribute accessor methods in AUTOLOAD Lazily create accessor methods when getting or setting an Koha::Object attribute resulting in a significant speed up. To test: 1) Run the benchmark.pl script 2) Apply the patch 3) Run the script again, Koha::Object accessors should be about 7 times faster 5) Ensure tests in t/db_dependent/Item.t still pass Created attachment 151290 [details] [review] Bug 33745: Lazily create attribute accessor methods in AUTOLOAD Lazily create accessor methods when getting or setting an Koha::Object attribute resulting in a significant speed up. To test: 1) Run the benchmark.pl script 2) Apply the patch 3) Run the script again, Koha::Object accessors should be about 7 times faster 5) Ensure tests in t/db_dependent/Item.t still pass Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Nick for QA? Looking here $item->$col: 54.11 ms item->_result->get_column($col): 46.171 ms $item->$col: 437.823 ms item->_result->get_column($col): 35.706 ms I like the idea here and see the speed but the current approach might break things. Working on a follow-up now to not redefine AUTOLOAD but add a sub for the accessor.. Please wait :) (In reply to Marcel de Rooy from comment #8) > I like the idea here and see the speed but the current approach might break > things. > Working on a follow-up now to not redefine AUTOLOAD but add a sub for the > accessor.. Please wait :) No this is not correct Created attachment 153449 [details] [review] Bug 33745: Lazily create attribute accessor methods in AUTOLOAD Lazily create accessor methods when getting or setting an Koha::Object attribute resulting in a significant speed up. To test: 1) Run the benchmark.pl script 2) Apply the patch 3) Run the script again, Koha::Object accessors should be about 7 times faster 5) Ensure tests in t/db_dependent/Item.t still pass Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 153450 [details] [review] Bug 33745: (QA follow-up) Remove goto Test plan: Prove t/db_dependent/Koha Run the benchmark (on BZ) again Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 153451 [details] [review] Bug 33745: (QA follow-up) Tidy With second patch, no significant difference $item->$col: 54.991 ms item->_result->get_column($col): 36.4 ms $item->$col: 54.826 ms item->_result->get_column($col): 36.205 ms $item->$col: 51.613 ms item->_result->get_column($col): 34.288 ms All tests successful. Files=63, Tests=575, 297 wallclock secs ( 1.21 usr 0.29 sys + 234.90 cusr 41.90 csys = 278.30 CPU) Result: PASS FAIL Koha/Object.pm
FAIL critic
# TestingAndDebugging::ProhibitNoStrict: Got 1 violation(s).
OK forbidden patterns
OK git manipulation
OK pod
OK pod coverage
SKIP spelling
OK tidiness
OK valid
Please ignore this FAIL. We need to access the symbol table here.
But it is tidy LOL
Created attachment 153452 [details] [review] Bug 33745: (QA follow-up) Tidy Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> This looks very promising in terms of performance. Looks good to me. Deserves an early push to evaluate further? (In reply to Marcel de Rooy from comment #13) > With second patch, no significant difference Just to be complete, no significant difference with first patch only :) Great work! Just adding that this is great work, thanks so much for pursuing it! Pushed to master for 23.11. Nice work everyone, thanks! Hi, this broke a test, please take care: https://jenkins.koha-community.org/job/Koha_Master/lastCompletedBuild/testReport/(root)/t_00_testcritic_t/Test__Perl__Critic_for__Koha_Object_pm___86_/ (In reply to Tomás Cohen Arazi from comment #22) > Hi, this broke a test, please take care: > > https://jenkins.koha-community.org/job/Koha_Master/lastCompletedBuild/ > testReport/(root)/t_00_testcritic_t/ > Test__Perl__Critic_for__Koha_Object_pm___86_/ See comment15. We need the no strict refs to do *{ $AUTOLOAD }. We might try to do via a CPAN module for symbol table handling but it will not make life safer. We should imo add this exception to the perl critic module here. Any suggestions for doing so ? (In reply to Marcel de Rooy from comment #23) > (In reply to Tomás Cohen Arazi from comment #22) > > Hi, this broke a test, please take care: > > > > https://jenkins.koha-community.org/job/Koha_Master/lastCompletedBuild/ > > testReport/(root)/t_00_testcritic_t/ > > Test__Perl__Critic_for__Koha_Object_pm___86_/ > > See comment15. > We need the no strict refs to do *{ $AUTOLOAD }. > We might try to do via a CPAN module for symbol table handling but it will > not make life safer. > We should imo add this exception to the perl critic module here. > Any suggestions for doing so ? Yes, I want a follow-up aging the exception. Thanks! Created attachment 153510 [details] [review] Bug 33745: (follow-up) Fool perlcritic If you work with hashes, everything should be fine :) Test plan: Run qa tools or perlcritic Koha/Object.pm Prove t/db_dependent/Koha Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Tomás Cohen Arazi from comment #24) > (In reply to Marcel de Rooy from comment #23) > > (In reply to Tomás Cohen Arazi from comment #22) > > > Hi, this broke a test, please take care: > > > > > > https://jenkins.koha-community.org/job/Koha_Master/lastCompletedBuild/ > > > testReport/(root)/t_00_testcritic_t/ > > > Test__Perl__Critic_for__Koha_Object_pm___86_/ > > > > See comment15. > > We need the no strict refs to do *{ $AUTOLOAD }. > > We might try to do via a CPAN module for symbol table handling but it will > > not make life safer. > > We should imo add this exception to the perl critic module here. > > Any suggestions for doing so ? > > Yes, I want a follow-up aging the exception. Thanks! Tomas: Please check last follow-up (In reply to Marcel de Rooy from comment #26) > Tomas: Please check last follow-up No nice try but we loose our speed advantage again. Comment on attachment 153510 [details] [review] Bug 33745: (follow-up) Fool perlcritic $item->$col: 544.74 ms item->_result->get_column($col): 35.861 ms Bye bye speed So I recommend to move the no strict+redefine line to Koha::Prohibited or something and change 00-perlcritic to skip that module. Jenkins should be fine again. Note that reading perlmonks etc there is no easy better faster safer way than just doing what David did with symbol table. Will add a follow-up next week. Created attachment 153525 [details] [review] Bug 33745: (follow-up) Add exception to 00-testcritic.t Test plan: Run t/00-testcritic.t t/00-testcritic_2.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Marcel de Rooy from comment #29) > So I recommend to move the no strict+redefine line to Koha::Prohibited or > something and change 00-perlcritic to skip that module. Even adding a module made me loose speed here. So i tweaked the tests. Follow-up looks good to me, thanks Marcel :) (In reply to Marcel de Rooy from comment #30) > Created attachment 153525 [details] [review] [review] > Bug 33745: (follow-up) Add exception to 00-testcritic.t > > Test plan: > Run t/00-testcritic.t t/00-testcritic_2.t > > Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tests should process files that are part of the git repository, see bug 33743 and bug 32352. Created attachment 153584 [details] [review] Bug 33745: Tell perlcritic we know what we are doing I think this patch is the correct way to remove/hide the perlcritic error. Created attachment 153586 [details] [review] Bug 33745: Tell perlcritic we know what we are doing Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Jonathan Druart from comment #35) > I think this patch is the correct way to remove/hide the perlcritic error. Agreed. Much easier :) (In reply to Jonathan Druart from comment #33) > Tests should process files that are part of the git repository, see bug > 33743 and bug 32352. Should we actually open a new bug report for that? We are now just passing everything from current workdir. (In reply to Marcel de Rooy from comment #38) > (In reply to Jonathan Druart from comment #33) > > Tests should process files that are part of the git repository, see bug > > 33743 and bug 32352. > > Should we actually open a new bug report for that? We are now just passing > everything from current workdir. Bug 34303 Follow-up pushed to master. Enhancement not pushed to 23.05.x Sounds good! (In reply to Fridolin Somers from comment #41) > Enhancement not pushed to 23.05.x Not sure about that choice ? Frdolin: please see former comment (In reply to David Cook from comment #42) > Sounds good! Hi David Without context this is a confusing remark. What is good. The push or not backporting it ? I personally would not backport it (or later). Indeed, this is a very wide impact change, there is a risk of side-effects. 23.05.x is used in production now. We are close to the end of cycle so we may wait for 23.11 to come out. (In reply to Marcel de Rooy from comment #45) > (In reply to David Cook from comment #42) > > Sounds good! > > Hi David > Without context this is a confusing remark. What is good. The push or not > backporting it ? I have no idea now. Very confusing. Maybe I meant the overall change. I'll try to be more mindful of the context in future! |