Bug 33745 - Speed up Koha::Object attribute accessors
Summary: Speed up Koha::Object attribute accessors
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Gustafsson
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 33746 34357
  Show dependency treegraph
 
Reported: 2023-05-16 11:41 UTC by David Gustafsson
Modified: 2023-10-25 11:58 UTC (History)
8 users (show)

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


Attachments
Bug 33745: Lazily create attribute accessor methods in AUTOLOAD (2.21 KB, patch)
2023-05-16 11:55 UTC, David Gustafsson
Details | Diff | Splinter Review
Benchmark script (738 bytes, application/x-perl)
2023-05-16 11:57 UTC, David Gustafsson
Details
Bug 33745: Lazily create attribute accessor methods in AUTOLOAD (2.21 KB, patch)
2023-05-16 12:04 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 33745: Lazily create attribute accessor methods in AUTOLOAD (2.29 KB, patch)
2023-05-16 14:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33745: Lazily create attribute accessor methods in AUTOLOAD (2.38 KB, patch)
2023-07-14 09:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33745: (QA follow-up) Remove goto (1.98 KB, patch)
2023-07-14 09:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33745: (QA follow-up) Tidy (1.35 KB, patch)
2023-07-14 09:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33745: (QA follow-up) Tidy (1.41 KB, patch)
2023-07-14 09:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33745: (follow-up) Fool perlcritic (1.02 KB, patch)
2023-07-15 17:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33745: (follow-up) Add exception to 00-testcritic.t (2.05 KB, patch)
2023-07-17 07:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33745: Tell perlcritic we know what we are doing (739 bytes, patch)
2023-07-18 08:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33745: Tell perlcritic we know what we are doing (834 bytes, patch)
2023-07-18 10:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Gustafsson 2023-05-16 11:41:03 UTC
In Bug 32060 object accessor methods surprisingly turned out to have a not huge, but significant negative effect on performance. By dynamically/lazily creating accessor subroutines in AUTOLOAD, which on subsequent calls will be called directly instead of AUTOLOAD accessing/setting an object attribute will become about 7 times faster.
Comment 1 David Gustafsson 2023-05-16 11:55:47 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
Comment 2 David Gustafsson 2023-05-16 11:57:40 UTC
Created attachment 151274 [details]
Benchmark script
Comment 3 David Gustafsson 2023-05-16 12:04:32 UTC
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
Comment 4 Jonathan Druart 2023-05-16 14:22:57 UTC
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>
Comment 5 Jonathan Druart 2023-05-16 14:44:42 UTC
Nick for QA?
Comment 6 Marcel de Rooy 2023-07-14 07:49:45 UTC
Looking here
Comment 7 Marcel de Rooy 2023-07-14 07:56:35 UTC
$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
Comment 8 Marcel de Rooy 2023-07-14 08:44:55 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2023-07-14 09:20:59 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2023-07-14 09:55:38 UTC
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>
Comment 11 Marcel de Rooy 2023-07-14 09:55:41 UTC
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>
Comment 12 Marcel de Rooy 2023-07-14 09:55:43 UTC
Created attachment 153451 [details] [review]
Bug 33745: (QA follow-up) Tidy
Comment 13 Marcel de Rooy 2023-07-14 09:56:45 UTC
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
Comment 14 Marcel de Rooy 2023-07-14 09:56:51 UTC
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
Comment 15 Marcel de Rooy 2023-07-14 09:57:50 UTC
 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
Comment 16 Marcel de Rooy 2023-07-14 09:58:27 UTC
Created attachment 153452 [details] [review]
Bug 33745: (QA follow-up) Tidy

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2023-07-14 10:02:23 UTC
This looks very promising in terms of performance. Looks good to me. Deserves an early push to evaluate further?
Comment 18 Marcel de Rooy 2023-07-14 10:11:13 UTC
(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 :)
Comment 19 Paul Derscheid 2023-07-14 10:15:05 UTC
Great work!
Comment 20 Martin Renvoize 2023-07-14 10:17:55 UTC
Just adding that this is great work, thanks so much for pursuing it!
Comment 21 Tomás Cohen Arazi 2023-07-14 18:23:59 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 23 Marcel de Rooy 2023-07-15 06:42:35 UTC
(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 ?
Comment 24 Tomás Cohen Arazi 2023-07-15 11:27:48 UTC
(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!
Comment 25 Marcel de Rooy 2023-07-15 17:14:21 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2023-07-15 17:14:47 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2023-07-15 17:17:53 UTC Comment hidden (obsolete)
Comment 28 Marcel de Rooy 2023-07-15 17:18:23 UTC Comment hidden (obsolete)
Comment 29 Marcel de Rooy 2023-07-15 17:21:27 UTC Comment hidden (obsolete)
Comment 30 Marcel de Rooy 2023-07-17 07:24:56 UTC
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>
Comment 31 Marcel de Rooy 2023-07-17 08:16:49 UTC
(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.
Comment 32 Martin Renvoize 2023-07-17 08:39:22 UTC
Follow-up looks good to me, thanks Marcel :)
Comment 33 Jonathan Druart 2023-07-18 08:15:47 UTC
(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.
Comment 34 Jonathan Druart 2023-07-18 08:23:18 UTC
Created attachment 153584 [details] [review]
Bug 33745: Tell perlcritic we know what we are doing
Comment 35 Jonathan Druart 2023-07-18 08:23:55 UTC
I think this patch is the correct way to remove/hide the perlcritic error.
Comment 36 Marcel de Rooy 2023-07-18 10:43:18 UTC
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>
Comment 37 Marcel de Rooy 2023-07-18 10:43:41 UTC
(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 :)
Comment 38 Marcel de Rooy 2023-07-18 10:44:14 UTC
(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.
Comment 39 Marcel de Rooy 2023-07-18 10:45:52 UTC
(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
Comment 40 Tomás Cohen Arazi 2023-07-18 15:30:13 UTC
Follow-up pushed to master.
Comment 41 Fridolin Somers 2023-08-04 01:13:27 UTC
Enhancement not pushed to 23.05.x
Comment 42 David Cook 2023-08-04 01:53:15 UTC
Sounds good!
Comment 43 Marcel de Rooy 2023-08-04 06:00:43 UTC
(In reply to Fridolin Somers from comment #41)
> Enhancement not pushed to 23.05.x

Not sure about that choice ?
Comment 44 Marcel de Rooy 2023-10-05 12:11:36 UTC
Frdolin: please see former comment
Comment 45 Marcel de Rooy 2023-10-05 12:12:28 UTC
(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 ?
Comment 46 Jonathan Druart 2023-10-06 07:48:36 UTC
I personally would not backport it (or later).
Comment 47 Fridolin Somers 2023-10-06 08:58:00 UTC
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.
Comment 48 David Cook 2023-10-08 22:42:00 UTC
(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!