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.
Changing status to normal as it really is a performance bug.
Created attachment 177735 [details] [review] Bug 39092: Unit tests
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
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.
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).
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.
This overlaps -to some degree- with bug 36197. Marking as RESOLVED until I figure what is the best path for success here.
'In Discussion' suits better I think.
Created attachment 181127 [details] [review] Bug 39092: Unit tests
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
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.
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).
Created attachment 181200 [details] [review] Bug 39092: Unit tests Signed-off-by: David Nind <david@davidnind.com>
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>
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>
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>
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>
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>
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>
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>
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
(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.
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
Pushed for 25.05! Well done everyone, thank you!
If the POD is wrong again, we can fix separately.