Created attachment 119872 [details] [review] Bug 28173: Add plugin hooks object_store_pre and object_store_post These two hooks are called in Koha::Object::store, which means any object stored in the database will trigger these hooks. object_store_pre is called just before `update_or_insert`, allowing to modify the object before sending the INSERT/UPDATE query object_store_post is called just after `update_or_insert`, so it can be used for any actions that require to have the final object, like indexing, logging, ... Test plan: 1. prove t/db_dependent/Koha/Plugins/Object_hooks.t
I like that. Just for hook name. In https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks we see existing hooks are calles after_xxx : after_biblio_action after_circ_action So i think whe should call this hooks : before_object_store after_object_store
Oh crap. We found a big performance issue with this hook. Each store() call is checking in DB the plugin version and if it is enabled. Particulary when DB is distant this is bad for performance.
This hook is used by Biblibre for action on record before saving. I suggest we change to implement hooks 'before_authority_action' and 'before_biblio_action'. With record as param.
(In reply to Fridolin Somers from comment #3) > Oh crap. > > We found a big performance issue with this hook. > > Each store() call is checking in DB the plugin version and if it is enabled. > > Particulary when DB is distant this is bad for performance. The problem is not this particular hook. It's Koha::Plugins->call that is too slow. I tried making it faster with bug 29672. Please test and let me know what you think
(In reply to Julian Maurice from comment #5) > > The problem is not this particular hook. It's Koha::Plugins->call that is > too slow. I tried making it faster with bug 29672. > Please test and let me know what you think I agree with you. Thanks a lot for the perf patch, looks great
When I saw the hook names, I was worried about performance, and I can see that worry was well founded. I'm curious if bug 29672 will be enough. These hooks are pretty broad. I wonder how much of a slowdown they would bring overall even with a well designed plugin...
Bug 29672 pushed master for 22.11.
The patch no longer applies (not unexpected!). Not sure about the comments after the patch 8-) git bz apply 28173 Bug 28173 - Add plugin hooks object_store_pre and object_store_post 119872 - Bug 28173: Add plugin hooks object_store_pre and object_store_post Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 28173: Add plugin hooks object_store_pre and object_store_post Using index info to reconstruct a base tree... M Koha/Object.pm A t/lib/Koha/Plugin/Test.pm Falling back to patching base and 3-way merge... Auto-merging t/lib/plugins/Koha/Plugin/Test.pm CONFLICT (content): Merge conflict in t/lib/plugins/Koha/Plugin/Test.pm Auto-merging Koha/Object.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 28173: Add plugin hooks object_store_pre and object_store_post ...
While I've had my reservations about this, having some kind of "pre" and "post" store hooks would be amazing...
Created attachment 163509 [details] [review] Bug 28173: Add plugin hooks object_store_pre and object_store_post These two hooks are called in Koha::Object::store, which means any object stored in the database will trigger these hooks. object_store_pre is called just before `update_or_insert`, allowing to modify the object before sending the INSERT/UPDATE query object_store_post is called just after `update_or_insert`, so it can be used for any actions that require to have the final object, like indexing, logging, ... Test plan: 1. prove t/db_dependent/Koha/Plugins/Object_hooks.t
Patch rebased on master
Created attachment 163516 [details] [review] Bug 28173: [DO NOT PUSH] Benchmark script
I ran the attached benchmark script in 3 different scenario: 1) master (c6fa96eeca) 2) with the patch, with 2 plugins, 1 enabled but without the object_store_* hooks, 1 with both hooks but disabled. 3) same as 2, but with both plugins enabled The code inside object_store_* hooks was very simple (1 call to $object->_type and 1 call to $object->make_column_dirty) 1) The time per `store` call is between 91µs and 104µs 2) The time per `store` call is between 98µs and 110µs 3) The time per `store` call is between 104µs and 115µs There is a noticeable slowdown, which is expected. I think 10µs per call is negligible if we only have a few store calls per http request, but I don't know how many store calls we have on a typical request. Does anyone know ?
I linked to Bug 34943 recently pushed master