Description
Julian Maurice
2021-04-20 08:26:13 UTC
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... 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 Re: comment #14 In my (limited) experience, there is no such thing as a "typical" Koha request. Some requests might trigger several (10-20+ store calls), whereas others will only trigger a few. Even with the performance improvement, I'm not sure this could work in all environments. For example, we have installations with 7-8+ plugins. Would system-wide option to enable / disable the hooks make sense? If this is accepted as it stands, I would also prefer the hooks to be named "before_object_store" and "after_object_store", in order to be more consistent. (In reply to Olivier Hubert from comment #16) > In my (limited) experience, there is no such thing as a "typical" Koha > request. Some requests might trigger several (10-20+ store calls), whereas > others will only trigger a few. Even with the performance improvement, I'm > not sure this could work in all environments. For example, we have > installations with 7-8+ plugins. I have this same fear. > Would system-wide option to enable / > disable the hooks make sense? As I was reading your comment, I was thinking this same thing! I think this is a really interesting idea actually. > If this is accepted as it stands, I would also prefer the hooks to be named > "before_object_store" and "after_object_store", in order to be more > consistent. Makes sense to me too. (Although the wiki is down right now, so I'm not able to do a good review of the existent hooks. A bit telling that we need to rely on the wiki to know what hooks there are too. If we did have a system-wide configuration of hook availability, it would be a nice way of easily seeing what hooks a system implements...) |