Bug 39092 - When loading an ILL backend plugin it should be cached
Summary: When loading an ILL backend plugin it should be cached
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Pedro Amorim
URL:
Keywords:
Depends on: 36197
Blocks: 38340
  Show dependency treegraph
 
Reported: 2025-02-11 13:32 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-05-16 15:28 UTC (History)
3 users (show)

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


Attachments
Bug 39092: Unit tests (4.96 KB, patch)
2025-02-11 15:17 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 39092: Cache the plugin object when loading plugin backend (1.69 KB, patch)
2025-02-11 15:17 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 39092: (follow-up) Do not look for plugins if Standard backend used (1.06 KB, patch)
2025-02-11 15:17 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 39092: Use pre-loaded plugin (1.04 KB, patch)
2025-02-11 15:17 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 39092: Unit tests (5.48 KB, patch)
2025-04-17 18:57 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 39092: Cache the plugin object when loading plugin backend (1.69 KB, patch)
2025-04-17 18:57 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 39092: (follow-up) Do not look for plugins if Standard backend used (1.06 KB, patch)
2025-04-17 18:57 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 39092: Use pre-loaded plugin (1.04 KB, patch)
2025-04-17 18:57 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 39092: Unit tests (5.53 KB, patch)
2025-04-19 08:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 39092: Cache the plugin object when loading plugin backend (1.74 KB, patch)
2025-04-19 08:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 39092: (follow-up) Do not look for plugins if Standard backend used (1.10 KB, patch)
2025-04-19 08:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 39092: Use pre-loaded plugin (1.09 KB, patch)
2025-04-19 08:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 39092: Unit tests (5.59 KB, patch)
2025-04-22 10:05 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 39092: Cache the plugin object when loading plugin backend (1.80 KB, patch)
2025-04-22 10:05 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 39092: (follow-up) Do not look for plugins if Standard backend used (1.16 KB, patch)
2025-04-22 10:05 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 39092: Use pre-loaded plugin (1.15 KB, patch)
2025-04-22 10:05 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi (tcohen) 2025-02-11 13:32:46 UTC
When `load_backend()` is called, one of the first things it does is instantiating the backend plugin if required, in order to call its `new_ill_backend()` method.

We should be storing the `$plugin` object in case we need it. A paradigmatic case is the `expand_template()` method, which calls `_backend()` (which internally calls `load_backend()`, and then queries for the plugin again in order to call a plugin method.
Comment 1 Tomás Cohen Arazi (tcohen) 2025-02-11 13:56:16 UTC
Changing status to normal as it really is a performance bug.
Comment 2 Tomás Cohen Arazi (tcohen) 2025-02-11 15:17:34 UTC
Created attachment 177735 [details] [review]
Bug 39092: Unit tests
Comment 3 Tomás Cohen Arazi (tcohen) 2025-02-11 15:17:36 UTC
Created attachment 177736 [details] [review]
Bug 39092: Cache the plugin object when loading plugin backend

This patch makes the `load_backend()` method cache the plugin instance
object when calculated. This will reduce the calls in other places where
we query for the plugin.

It is also the excuse to add some tests for this methods which were
lacking.

To test:
1. Apply the unit tests
2. Run:
   $ ktd --shell
 k$ prove t/db_dependent/Koha/Plugins/ILL/Backends.t
=> FAIL: Tests fail, plugin instance caching is not happening
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D
Comment 4 Tomás Cohen Arazi (tcohen) 2025-02-11 15:17:38 UTC
Created attachment 177737 [details] [review]
Bug 39092: (follow-up) Do not look for plugins if Standard backend used

If we've been asked the `Standard` backend we shouldn't be trying to
load plugins.

Trivial refactoring of the order things are done.
Comment 5 Tomás Cohen Arazi (tcohen) 2025-02-11 15:17:40 UTC
Created attachment 177738 [details] [review]
Bug 39092: Use pre-loaded plugin

This patch removes plugin loading from the method, and makes it leverage
on the plugin instance already loaded by `load_backend` (if any).
Comment 6 Pedro Amorim 2025-02-11 15:30:16 UTC
You're adding Backends.t but I've added that in bug 35604 and bug 36197:

https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=35604&attachment=176688

https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=36197&attachment=174508

Same for use Koha::Plugin::Test::ILLBackend;

Those bugs have been in bugzilla for a year.
Comment 7 Tomás Cohen Arazi (tcohen) 2025-02-11 17:20:45 UTC
This overlaps -to some degree- with bug 36197.

Marking as RESOLVED until I figure what is the best path for success here.
Comment 8 Tomás Cohen Arazi (tcohen) 2025-02-11 17:45:49 UTC
'In Discussion' suits better I think.
Comment 9 Tomás Cohen Arazi (tcohen) 2025-04-17 18:57:09 UTC
Created attachment 181127 [details] [review]
Bug 39092: Unit tests
Comment 10 Tomás Cohen Arazi (tcohen) 2025-04-17 18:57:12 UTC
Created attachment 181128 [details] [review]
Bug 39092: Cache the plugin object when loading plugin backend

This patch makes the `load_backend()` method cache the plugin instance
object when calculated. This will reduce the calls in other places where
we query for the plugin.

It is also the excuse to add some tests for this methods which were
lacking.

To test:
1. Apply the unit tests
2. Run:
   $ ktd --shell
 k$ prove t/db_dependent/Koha/Plugins/ILL/Backends.t
=> FAIL: Tests fail, plugin instance caching is not happening
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D
Comment 11 Tomás Cohen Arazi (tcohen) 2025-04-17 18:57:14 UTC
Created attachment 181129 [details] [review]
Bug 39092: (follow-up) Do not look for plugins if Standard backend used

If we've been asked the `Standard` backend we shouldn't be trying to
load plugins.

Trivial refactoring of the order things are done.
Comment 12 Tomás Cohen Arazi (tcohen) 2025-04-17 18:57:16 UTC
Created attachment 181130 [details] [review]
Bug 39092: Use pre-loaded plugin

This patch removes plugin loading from the method, and makes it leverage
on the plugin instance already loaded by `load_backend` (if any).
Comment 13 David Nind 2025-04-19 08:51:36 UTC
Created attachment 181200 [details] [review]
Bug 39092: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2025-04-19 08:51:38 UTC
Created attachment 181201 [details] [review]
Bug 39092: Cache the plugin object when loading plugin backend

This patch makes the `load_backend()` method cache the plugin instance
object when calculated. This will reduce the calls in other places where
we query for the plugin.

It is also the excuse to add some tests for this methods which were
lacking.

To test:
1. Apply the unit tests
2. Run:
   $ ktd --shell
 k$ prove t/db_dependent/Koha/Plugins/ILL/Backends.t
=> FAIL: Tests fail, plugin instance caching is not happening
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 15 David Nind 2025-04-19 08:51:41 UTC
Created attachment 181202 [details] [review]
Bug 39092: (follow-up) Do not look for plugins if Standard backend used

If we've been asked the `Standard` backend we shouldn't be trying to
load plugins.

Trivial refactoring of the order things are done.

Signed-off-by: David Nind <david@davidnind.com>
Comment 16 David Nind 2025-04-19 08:51:44 UTC
Created attachment 181203 [details] [review]
Bug 39092: Use pre-loaded plugin

This patch removes plugin loading from the method, and makes it leverage
on the plugin instance already loaded by `load_backend` (if any).

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 Pedro Amorim 2025-04-22 10:05:32 UTC
Created attachment 181240 [details] [review]
Bug 39092: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Comment 18 Pedro Amorim 2025-04-22 10:05:34 UTC
Created attachment 181241 [details] [review]
Bug 39092: Cache the plugin object when loading plugin backend

This patch makes the `load_backend()` method cache the plugin instance
object when calculated. This will reduce the calls in other places where
we query for the plugin.

It is also the excuse to add some tests for this methods which were
lacking.

To test:
1. Apply the unit tests
2. Run:
   $ ktd --shell
 k$ prove t/db_dependent/Koha/Plugins/ILL/Backends.t
=> FAIL: Tests fail, plugin instance caching is not happening
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Comment 19 Pedro Amorim 2025-04-22 10:05:37 UTC
Created attachment 181242 [details] [review]
Bug 39092: (follow-up) Do not look for plugins if Standard backend used

If we've been asked the `Standard` backend we shouldn't be trying to
load plugins.

Trivial refactoring of the order things are done.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Comment 20 Pedro Amorim 2025-04-22 10:05:40 UTC
Created attachment 181243 [details] [review]
Bug 39092: Use pre-loaded plugin

This patch removes plugin loading from the method, and makes it leverage
on the plugin instance already loaded by `load_backend` (if any).

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Comment 21 Pedro Amorim 2025-04-22 10:05:55 UTC
Did some testing to ensure overall functionality works. With additional backends. All tests pass.

Applied a profiling patch:
https://github.com/openfifth/koha/commit/5afe33e44ecb983d92c2ec5a5f1c5e7fb0156814

Before 39092 patches, creating a new Standard request (tailing /var/log/koha/kohadev/*), I get the following 11 calls to load_backend:
[2025/04/22 09:48:45] [WARN] $VAR1 = 'load_backend took 0.0182640552520752 seconds to execute
[2025/04/22 09:48:45] [WARN] $VAR1 = 'load_backend took 0.0149691104888916 seconds to execute
[2025/04/22 09:48:45] [WARN] $VAR1 = 'load_backend took 0.0139071941375732 seconds to execute
[2025/04/22 09:48:45] [WARN] $VAR1 = 'load_backend took 0.0144112110137939 seconds to execute
[2025/04/22 09:48:45] [WARN] $VAR1 = 'load_backend took 0.01344895362854 seconds to execute
[2025/04/22 09:48:45] [WARN] $VAR1 = 'load_backend took 0.0147809982299805 seconds to execute
[2025/04/22 09:48:45] [WARN] $VAR1 = 'load_backend took 0.0178771018981934 seconds to execute
[2025/04/22 09:48:45] [WARN] $VAR1 = 'load_backend took 0.0235528945922852 seconds to execute
[2025/04/22 09:48:46] [WARN] $VAR1 = 'load_backend took 0.0250940322875977 seconds to execute
[2025/04/22 09:48:46] [WARN] $VAR1 = 'load_backend took 0.0210509300231934 seconds to execute
[2025/04/22 09:48:46] [WARN] $VAR1 = 'load_backend took 0.018543004989624 seconds to execute
Average of 0.0169 secs

[2025/04/22 09:51:30] [WARN] $VAR1 = 'load_backend took 0.0199542045593262 seconds to execute
[2025/04/22 09:51:30] [WARN] $VAR1 = 'load_backend took 0.0174210071563721 seconds to execute
[2025/04/22 09:51:30] [WARN] $VAR1 = 'load_backend took 0.0181491374969482 seconds to execute
[2025/04/22 09:51:30] [WARN] $VAR1 = 'load_backend took 0.0170440673828125 seconds to execute
[2025/04/22 09:51:30] [WARN] $VAR1 = 'load_backend took 0.00216317176818848 seconds to execute
[2025/04/22 09:51:30] [WARN] $VAR1 = 'load_backend took 0.00219011306762695 seconds to execute
[2025/04/22 09:51:31] [WARN] $VAR1 = 'load_backend took 0.0211529731750488 seconds to execute
[2025/04/22 09:51:31] [WARN] $VAR1 = 'load_backend took 0.0275499820709229 seconds to execute
[2025/04/22 09:51:31] [WARN] $VAR1 = 'load_backend took 0.026799201965332 seconds to execute
[2025/04/22 09:51:31] [WARN] $VAR1 = 'load_backend took 0.0242869853973389 seconds to execute
[2025/04/22 09:51:31] [WARN] $VAR1 = 'load_backend took 0.00526189804077148 seconds to execute
Average of 0.0134 secs

20% increase in performance.

QA script complains about pod coverage:
t/lib/plugins/Koha/Plugin/BackendClass.pm
t/lib/plugins/Koha/Plugin/ILL/TestClass.pm
Comment 22 Tomás Cohen Arazi (tcohen) 2025-04-22 13:43:36 UTC
(In reply to Pedro Amorim from comment #21)
> Did some testing to ensure overall functionality works. With additional
> backends. All tests pass.

Thank you, Pedro. Much appreciated.
Comment 23 Katrin Fischer 2025-04-25 15:03:34 UTC
We probably need to fix these, because of bug 39367:

 FAIL	t/lib/plugins/Koha/Plugin/BackendClass.pm
   FAIL	  pod coverage
		POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666

 FAIL	t/lib/plugins/Koha/Plugin/ILL/TestClass.pm
   FAIL	  pod coverage
		POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
Comment 24 Katrin Fischer 2025-04-25 16:36:27 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 25 Katrin Fischer 2025-05-16 15:28:09 UTC
If the POD is wrong again, we can fix separately.