[Bug 29672] New: Increase performance of Koha::Plugins->call
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Bug ID: 29672 Summary: Increase performance of Koha::Plugins->call Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: ASSIGNED Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: julian.maurice@biblibre.com Reporter: julian.maurice@biblibre.com QA Contact: testopia@bugs.koha-community.org Every call to Koha::Plugins->call do a lot of database queries: 1) One in GetPlugins searches for available plugin_class in plugin_methods table 2) One in Koha::Plugins::Base::is_enabled for each plugin returned by (1) 3) One or two in Koha::Plugins::Base::new for each plugin returned by (1) to check the install status and the installed version All of these requests only need to be made once per HTTP request at most. One could argue that it's needed only at Koha startup but that besides the point of this bug report. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #1 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 128376 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128376&action=edit Bug 29672: Increase performance of Koha::Plugins->call Make use of Koha::Cache::Memory::Lite to avoid hitting the database and creating plugins object for every call to Koha::Plugins->call Test plan: 1. Make sure plugins still work by executing `prove t/db_dependent/Koha/Plugins/Plugins.t` 2. Run the test script provided in the following patch to see how it affects performances -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #2 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 128377 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=128377&action=edit Bug 29672: [DO NOT PUSH] Test script Run test-plugins-call.pl without arguments with and without the patches, with some plugins enabled. Make sure to run misc/devel/install_plugins.pl before -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #3 from Julian Maurice <julian.maurice@biblibre.com> --- Times I get on master: 1 call(s): Elapsed 0.670997 seconds 10 call(s): Elapsed 0.049056 seconds 100 call(s): Elapsed 0.468099 seconds 1000 call(s): Elapsed 4.762553 seconds Times I get with the patch: 1 call(s): Elapsed 0.634319 seconds 10 call(s): Elapsed 0.000154 seconds 100 call(s): Elapsed 0.001282 seconds 1000 call(s): Elapsed 0.011682 seconds As you can see the first call takes the same time with or without the patch, but subsequent calls are much more faster with the patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=28172 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |7923 CC| |fridolin.somers@biblibre.co | |m Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7923 [Bug 7923] Performance omnibus -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #4 from Fridolin Somers <fridolin.somers@biblibre.com> --- I see it returns undef when plugins are disabled. It should return empty array non ? In order to allow : my @plugin_responses = Koha::Plugins->call( 'opac_results_xslt_variables', { lang => $lang, patron_id => $borrowernumber } ); for my $plugin_variables ( @plugin_responses ) { $variables = { %$variables, %$plugin_variables }; } -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #5 from Fridolin Somers <fridolin.somers@biblibre.com> --- (In reply to Fridolin Somers from comment #4)
I see it returns undef when plugins are disabled. It should return empty array non ?
In order to allow :
my @plugin_responses = Koha::Plugins->call( 'opac_results_xslt_variables', { lang => $lang, patron_id => $borrowernumber } ); for my $plugin_variables ( @plugin_responses ) { $variables = { %$variables, %$plugin_variables }; }
Oh i see it is covered by a unit test : t::lib::Mocks::mock_config('enable_plugins', 0); @responses = Koha::Plugins->call('check_password', { password => '1234' }); is_deeply(\@responses, [], 'call() should return an empty array if plugins are disabled'); Should be OK :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #6 from Fridolin Somers <fridolin.somers@biblibre.com> --- Instead of creating get_enabled_plugins(), Why not adding a param "only_enabled" in GetPlugins() ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #7 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Fridolin Somers from comment #6)
Instead of creating get_enabled_plugins(), Why not adding a param "only_enabled" in GetPlugins() ?
GetPlugins already returns only enabled plugins by default (without the 'all' parameter), but it also have other parameters that make it hard for its result to be put in cache. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=28173 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #8 from Fridolin Somers <fridolin.somers@biblibre.com> --- Ah OK thanks. get_enabled_plugins () is a private method ? maybe rename _get_enabled_plugins() Best regards, -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com --- Comment #9 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Julian, I am not sure, you are bypassing Koha::Plugin::Base->retrieve_data that is used in other places. As we are actually looking at caching the result, why not keep the usual way to retrieve the enabled plugins then cache them? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #10 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Fridolin Somers from comment #8)
Ah OK thanks.
get_enabled_plugins () is a private method ? maybe rename _get_enabled_plugins()
Best regards,
It's a class method, like all other methods from this module. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #11 from Fridolin Somers <fridolin.somers@biblibre.com> --- (In reply to Jonathan Druart from comment #10)
(In reply to Fridolin Somers from comment #8)
Ah OK thanks.
get_enabled_plugins () is a private method ? maybe rename _get_enabled_plugins()
Best regards,
It's a class method, like all other methods from this module.
Ah OK thanks. Now i see there is : my ($class) = @_; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #12 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Jonathan Druart from comment #9)
Julian, I am not sure, you are bypassing Koha::Plugin::Base->retrieve_data that is used in other places. As we are actually looking at caching the result, why not keep the usual way to retrieve the enabled plugins then cache them?
I guess you're talking about the is_enabled method (which calls retrieve_data) ? I think we should not ask the plugin itself if it's enabled because a plugin can override this method. It also produces less SQL queries that way (one query to retrieve all enabled plugins, instead of one query per module) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com, | |tomascohen@gmail.com --- Comment #13 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Kyle, Tomas, could you have a look at this improvement? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #14 from Kyle M Hall <kyle@bywatersolutions.com> ---
I guess you're talking about the is_enabled method (which calls retrieve_data) ? I think we should not ask the plugin itself if it's enabled because a plugin can override this method. It also produces less SQL queries that way (one query to retrieve all enabled plugins, instead of one query per module)
I think this is a reasonable stance to take. We control the internals so we don't need to make is_enabled a chokepoint. A single query selecting all plugins where plugins data __ENABLED__ is 1 seems like a good idea to me. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #15 from Tomás Cohen Arazi <tomascohen@gmail.com> --- I like the approach of using the Lite cache and avoid subsequent queries. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #16 from David Cook <dcook@prosentient.com.au> --- I like the concept. A few questions: 1) Why does get_enabled_plugins return the array rather than an array reference? (I suppose it might not matter much since the array will just be a list of references itself, but it just seems less efficient.) 2) Each Koha::Plugins->call() loops through the plugin list checking the methods available. If there is a large plugin list, this will scale linearly and be progressively slower. Since we're already looping through the plugins in get_enabled_plugins, it could be a good idea to do that method check at the same time and build a hash map of methods to plugins. (The downside is that you'd have to pre-check all methods rather than lazily check just the methods you're demanding at method call time, but that expense would be 1 time rather than N times.) I think that would maek sense since what we're really trying to do here is "get_enabled_plugins_with_method", right? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #17 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #16)
I think that would maek sense since what we're really trying to do here is "get_enabled_plugins_with_method", right?
Actually, why not create "get_enabled_plugins_with_method"? We store the methods in a table too, so you could fetch all enabled plugins for method X, and then cache those plugins. That way, you're just caching a list of plugins for methods that are used. That means every call of "$_->can($method)" *should* be true, so there should be no wasted loops. That said, you could wind up with multiple plugin objects in different cached hash maps. I suppose you could work around that by having 2 cached hash maps. But I suppose that's growing in complexity. Anyway just my two cents. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #18 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to David Cook from comment #16)
1) Why does get_enabled_plugins return the array rather than an array reference? (I suppose it might not matter much since the array will just be a list of references itself, but it just seems less efficient.) Is it really less efficient ? We have to de-reference the list at some point to iterate over it anyway. I will be happy to change that if you can show that it impacts performance.
2) Each Koha::Plugins->call() loops through the plugin list checking the methods available. If there is a large plugin list, this will scale linearly and be progressively slower. It only calls 'can' in the loop. How many calls to 'can' does it take to slow down Koha ? I tested this and on my laptop 5000 'can' calls on a DateTime object takes 1ms. Which sane person would install 5000 Koha plugins ? :) And if they do, that 1ms lost here is not what I would worry about.
Since we're already looping through the plugins in get_enabled_plugins, it could be a good idea to do that method check at the same time and build a hash map of methods to plugins. (The downside is that you'd have to pre-check all methods rather than lazily check just the methods you're demanding at method call time, but that expense would be 1 time rather than N times.) And most of the time 95% of the methods pre-checked won't be used until the next pre-check. That is wasted time IMO. I'm not sure if this would be an improvement.
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #19 from David Cook <dcook@prosentient.com.au> --- (In reply to Julian Maurice from comment #18)
Is it really less efficient ? We have to de-reference the list at some point to iterate over it anyway. I will be happy to change that if you can show that it impacts performance.
It would be less efficient as you're creating a new data structure in memory and copying the list contents over to it. But the difference is probably negligible, so I wouldn't worry about it. It just seemed like an odd choice to me.
It only calls 'can' in the loop. How many calls to 'can' does it take to slow down Koha ? I tested this and on my laptop 5000 'can' calls on a DateTime object takes 1ms. Which sane person would install 5000 Koha plugins ? :) And if they do, that 1ms lost here is not what I would worry about.
I thought that I wrote the same thing earlier but apparently I didn't. Yeah, "can" would be a very cheap call. No I/O to do. Just a wee bit of CPU time.
And most of the time 95% of the methods pre-checked won't be used until the next pre-check. That is wasted time IMO. I'm not sure if this would be an improvement.
In my follow-up comment, I was suggesting that you could actually just fetch the plugins that support the method you need and cache those plugins without fetching, instantiating, and caching all the plugins. But then you could argue there's not that many plugins, so you could just fetch them all. No point optimizing until you need to. -- In any case, +1 to caching -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #20 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- CONFLICT (content): Merge conflict in Koha/Plugins.pm Patch failed at 0001 Bug 29672: Increase performance of Koha::Plugins->call -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Patch doesn't apply -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128376|0 |1 is obsolete| | --- Comment #21 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 135660 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135660&action=edit Bug 29672: Increase performance of Koha::Plugins->call Make use of Koha::Cache::Memory::Lite to avoid hitting the database and creating plugins object for every call to Koha::Plugins->call Test plan: 1. Make sure plugins still work by executing `prove t/db_dependent/Koha/Plugins/Plugins.t` 2. Run the test script provided in the following patch to see how it affects performances -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #128377|0 |1 is obsolete| | --- Comment #22 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 135661 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135661&action=edit Bug 29672: [DO NOT PUSH] Test script Run test-plugins-call.pl without arguments with and without the patches, with some plugins enabled. Make sure to run misc/devel/install_plugins.pl before -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #23 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- I think we should invalidate/clean the cache when a new plugin is installed/enabled/disabled. You will certainly need the second patch from bug 29623. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |ASSIGNED --- Comment #24 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Jonathan Druart from comment #23)
I think we should invalidate/clean the cache when a new plugin is installed/enabled/disabled. I agree
You will certainly need the second patch from bug 29623. I don't think so. There is only one key to flush here, so we don't need the list of all keys.
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #25 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 135704 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135704&action=edit Bug 29672: Clear cache of enabled plugins when a plugin's state change -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |kyle@bywatersolutions.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #135661|0 |1 is obsolete| | --- Comment #26 from David Nind <david@davidnind.com> --- Created attachment 142844 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142844&action=edit Bug 29672: [DO NOT PUSH] Test script Run test-plugins-call.pl without arguments with and without the patches, with some plugins enabled. Make sure to run misc/devel/install_plugins.pl before Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #135660|0 |1 is obsolete| | --- Comment #27 from David Nind <david@davidnind.com> --- Created attachment 142845 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142845&action=edit Bug 29672: Increase performance of Koha::Plugins->call Make use of Koha::Cache::Memory::Lite to avoid hitting the database and creating plugins object for every call to Koha::Plugins->call Test plan: 1. Make sure plugins still work by executing `prove t/db_dependent/Koha/Plugins/Plugins.t` 2. Run the test script provided in the following patch to see how it affects performances Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #135704|0 |1 is obsolete| | --- Comment #28 from David Nind <david@davidnind.com> --- Created attachment 142846 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142846&action=edit Bug 29672: Clear cache of enabled plugins when a plugin's state change Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Status|Signed Off |Failed QA --- Comment #29 from David Nind <david@davidnind.com> --- Apologies, I signed off and then reealised that I hadn't run the tests after applying the patches! Tests now fail after the patches are applied. Weirdly, running the tests seemed to uninstall the plugins. I reinstalled the plugins again, flush_memcached + restart_all, and ran the tests again - they failed [4]. Testing notes (using koha-testing-docker): 1. Ran prove t/db_dependent/Koha/Plugins/Plugins.t - tests passed 2. Installed 2 plugins and enabled 3. Ran misc/devel/install_plugins.pl [1] 4. Applied test script patch and ran [2] 5. Applied other patches and flush_memcached + restart_all 6. Re-ran test script [3] 7. Ran prove t/db_dependent/Koha/Plugins/Plugins.t - tests failed [1] Output from running misc/devel/install_plugins.pl: misc/devel/install_plugins.pl Installed Example Kitchen-Sink Plugin version 2.3.0 Installed BiblbioCommons connector plugin for Koha version 0.5.3 All plugins successfully re-initialised [2] Output of the test script BEFORE the patches were applied: ./test-plugins-call.pl 1 call(s): Elapsed 0.686131 seconds 10 call(s): Elapsed 0.093689 seconds 100 call(s): Elapsed 0.572402 seconds 1000 call(s): Elapsed 5.629604 seconds [3] Output of the test script AFTER the patches were applied: ./test-plugins-call.pl 1 call(s): Elapsed 0.683855 seconds 10 call(s): Elapsed 0.000213 seconds 100 call(s): Elapsed 0.001843 seconds 1000 call(s): Elapsed 0.017745 seconds [4] Tests fail after patches are applied: prove t/db_dependent/Koha/Plugins/Plugins.t t/db_dependent/Koha/Plugins/Plugins.t .. 6/61 # Failed test at t/db_dependent/Koha/Plugins/Plugins.t line 112. # found warning: Failed to load Koha::Plugin::Com::ByWaterSolutions::KitchenSink: Could not find or check module 'Koha::Plugin::Com::ByWaterSolutions::KitchenSink' at /kohadevbox/koha/Koha/Plugins.pm line 116. # found warning: Failed to load Koha::Plugin::Com::Theke::BiblioCommons: Could not find or check module 'Koha::Plugin::Com::Theke::BiblioCommons' at /kohadevbox/koha/Koha/Plugins.pm line 116. # found warning: Plugin error (Test Plugin): Exception 'Koha::Exception' thrown 'item_barcode_transform called with parameter: 1' # found warning: Plugin error (Test Plugin for item_barcode_transform): Exception 'Koha::Exception' thrown 'item_barcode_transform called with parameter: 2' # expected to find warning: Plugin error (Test Plugin): Exception 'Koha::Exception' thrown 'item_barcode_transform called with parameter: 1' # expected to find warning: Plugin error (Test Plugin for item_barcode_transform): Exception 'Koha::Exception' thrown 'item_barcode_transform called with parameter: 2' # Looks like you failed 1 test of 6. # Failed test 'more call() tests' # at t/db_dependent/Koha/Plugins/Plugins.t line 129. t/db_dependent/Koha/Plugins/Plugins.t .. 48/61 # Looks like you failed 1 test of 61. t/db_dependent/Koha/Plugins/Plugins.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/61 subtests Test Summary Report ------------------- t/db_dependent/Koha/Plugins/Plugins.t (Wstat: 256 Tests: 61 Failed: 1) Failed test: 7 Non-zero exit status: 1 Files=1, Tests=61, 3 wallclock secs ( 0.03 usr 0.01 sys + 2.63 cusr 0.24 csys = 2.91 CPU) Result: FAIL -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #30 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 142916 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142916&action=edit Bug 29672: Fix test t/db_dependent/Koha/Plugins/Plugins.t Table plugin_data was not always cleared -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #142844|0 |1 is obsolete| | Attachment #142845|0 |1 is obsolete| | Attachment #142846|0 |1 is obsolete| | Attachment #142916|0 |1 is obsolete| | --- Comment #31 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 142919 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142919&action=edit Bug 29672: [DO NOT PUSH] Test script Run test-plugins-call.pl without arguments with and without the patches, with some plugins enabled. Make sure to run misc/devel/install_plugins.pl before Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #32 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 142920 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142920&action=edit Bug 29672: Increase performance of Koha::Plugins->call Make use of Koha::Cache::Memory::Lite to avoid hitting the database and creating plugins object for every call to Koha::Plugins->call Test plan: 1. Make sure plugins still work by executing `prove t/db_dependent/Koha/Plugins/Plugins.t` 2. Run the test script provided in the following patch to see how it affects performances Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #33 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 142921 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142921&action=edit Bug 29672: Clear cache of enabled plugins when a plugin's state change Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #34 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 142922 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142922&action=edit Bug 29672: Fix test t/db_dependent/Koha/Plugins/Plugins.t Table plugin_data was not always cleared Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #35 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 142923 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142923&action=edit Bug 29672: (QA followup) Add POD Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |22.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #36 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 22.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #37 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Julian Maurice from comment #7)
(In reply to Fridolin Somers from comment #6)
Instead of creating get_enabled_plugins(), Why not adding a param "only_enabled" in GetPlugins() ?
GetPlugins already returns only enabled plugins by default (without the 'all' parameter), but it also have other parameters that make it hard for its result to be put in cache.
The all parameter is not used often and could perhaps live on its own. The results of GetPlugins might be filtered by method or metadata, but that could be done too on the cached results, just limiting them further? We now have two similar routines. One has been optimized, the other one not. Note that GetPlugins is still called in the codebase, probably on the first places when we did not yet have ->call. Feels a bit like unfinished work? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Resolution|--- |FIXED Status|Pushed to master |RESOLVED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #38 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #17)
That way, you're just caching a list of plugins for methods that are used. That means every call of "$_->can($method)" *should* be true, so there should be no wasted loops.
By iterating through all the enabled plugins, loading them, and using $_->can($method), we've created the scenario which causes the problem described in bug 36736 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=36736 -- You are receiving this mail because: You are watching all bug changes.
By iterating through all the enabled plugins, loading them, and using $_->can($method), we've created the scenario which causes the problem described in bug 36736 Loading plugins in Koha::Plugins::call is not perfect, but we don't have a "bootstrap" stage that's common to the web app and CLI programs (which would be
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #39 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to David Cook from comment #38) the perfect time to load plugins IMO), so I believe it's the best we can do at the moment. Of course plugins can break Koha if they do things they're not supposed to :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29672 --- Comment #40 from David Cook <dcook@prosentient.com.au> --- (In reply to Julian Maurice from comment #39)
(In reply to David Cook from comment #38)
By iterating through all the enabled plugins, loading them, and using $_->can($method), we've created the scenario which causes the problem described in bug 36736
Loading plugins in Koha::Plugins::call is not perfect, but we don't have a "bootstrap" stage that's common to the web app and CLI programs (which would be the perfect time to load plugins IMO), so I believe it's the best we can do at the moment. Of course plugins can break Koha if they do things they're not supposed to :)
What I mean is that Koha::Plugins->call() could use the plugin_methods table to find only the plugins that it needs to load, rather than iterating through all of them. Although thinking about bug 36736 again... you're right. Having a bootstrap stage that loads all plugins is needed, because you never know what plugin is doing something funny deep down in the call stack. Fun! Haha -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org