Bug 21015 - Members.pm slow because it loads twice Koha::Schema
Summary: Members.pm slow because it loads twice Koha::Schema
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: j.kylmala
QA Contact: Marcel de Rooy
URL:
Keywords:
: 21160 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-06-29 08:11 UTC by j.kylmala
Modified: 2018-11-28 14:41 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21015: fix performance issue with C4::Members (1.40 KB, patch)
2018-06-29 08:32 UTC, j.kylmala
Details | Diff | Splinter Review
Bug 21015: fix performance issue with C4::Members (1.47 KB, patch)
2018-10-17 18:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21015: Remove unecessary 'use Koha::Schema' statements in t/ (1.78 KB, patch)
2018-10-17 18:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21015: fix performance issue with C4::Members (1.56 KB, patch)
2018-11-02 07:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21015: Remove unecessary 'use Koha::Schema' statements in t/ (1.85 KB, patch)
2018-11-02 07:11 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 j.kylmala 2018-06-29 08:11:02 UTC
Koha/C4/Members.pm currently loads by itself Koha::Schema using "use Koha::Schema" even though this is not required as we could use Koha::Database that has already done that (another further issue is why does Koha::Database load all the classes in Koha/Schema/Result/ when it could just load the ones that are needed).

Loading the Koha::Schema takes for me around 2 seconds on my machine so it would be quite a big performance boost to remove it. I got a 30% faster load time on cgi-bin/koha/members/moremember.pl with a patch I made. I will attach it here but be warned: I have not run any tests and I don't know if it has even syntax errors because I'm doing the patch on a machine without a proper development environment.
Comment 1 j.kylmala 2018-06-29 08:32:44 UTC
Created attachment 76556 [details] [review]
Bug 21015: fix performance issue with C4::Members

This is the proposed patch but please run the tests as I have not done so (yet).
Comment 2 Katrin Fischer 2018-07-01 17:56:10 UTC
Hi, this is already looking good. Could you still add a little test plan to see where the routine is used for the testers?
Comment 3 Jonathan Druart 2018-10-17 18:40:10 UTC
*** Bug 21160 has been marked as a duplicate of this bug. ***
Comment 4 Jonathan Druart 2018-10-17 18:49:41 UTC
Created attachment 80751 [details] [review]
Bug 21015: fix performance issue with C4::Members

loading Koha::Schema ("use Koha::Schema;") takes significantly time as
it loads almost a couple hundred classes. Koha::Database has done that
already once and we can use it to get the ResultSet "Borrower" as
well, so let's use that. This also make the code more unified because
Koha::Database is used throughout the code instead of Koha::Schema.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 5 Jonathan Druart 2018-10-17 18:49:45 UTC
Created attachment 80752 [details] [review]
Bug 21015: Remove unecessary 'use Koha::Schema' statements in t/

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 6 Jonathan Druart 2018-10-17 18:55:41 UTC
ho, excellent!

% more t.pl
use C4::Members;
print get_cardnumber_length();

Without the patch: ~1.6s
With the patch: 0.8s
Comment 7 David Cook 2018-10-17 23:03:54 UTC
Awesome! I should've looked more thoroughly when I reported #21160. 

This should have some nice knock on effects, especially for CGI users.
Comment 8 Marcel de Rooy 2018-11-02 07:11:06 UTC
Created attachment 81865 [details] [review]
Bug 21015: fix performance issue with C4::Members

loading Koha::Schema ("use Koha::Schema;") takes significantly time as
it loads almost a couple hundred classes. Koha::Database has done that
already once and we can use it to get the ResultSet "Borrower" as
well, so let's use that. This also make the code more unified because
Koha::Database is used throughout the code instead of Koha::Schema.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2018-11-02 07:11:11 UTC
Created attachment 81866 [details] [review]
Bug 21015: Remove unecessary 'use Koha::Schema' statements in t/

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Nick Clemens 2018-11-08 02:26:41 UTC
Awesome work all!

Pushed to master for 18.11
Comment 11 Martin Renvoize 2018-11-16 13:33:08 UTC
Pushed to 18.05.x for 18.05.06
Comment 12 Fridolin SOMERS 2018-11-28 14:41:22 UTC
Pushed to 17.11.x for 17.11.13