Description
David Gustafsson
2024-03-18 16:28:08 UTC
Created attachment 163357 [details] [review] Bug 36350: Add caching for Objects::find Add subclass of Koha::Objects that provides caching for find. To test: 1) Ensure tests in t/db_dependend/Koha/Libraries.t passes Sponsored-by: Gothenburg University Library Tests are only added for Libraries and should provide full testing coverage even if used for other classes, but could add a stripped down testing suite also for ItemTypes, but don't know if it would accomplish very much as the caching is also implicitly tested through the other tests for that object type. Created attachment 163361 [details] [review] Bug 36350: Add caching for Objects::find Add subclass of Koha::Objects that provides caching for find. To test: 1) Ensure tests in t/db_dependend/Koha/Libraries.t passes 2) Ensure tests in t/db_dependent/Koha/ItemTypes.t passes Sponsored-by: Gothenburg University Library Created attachment 163857 [details] [review] Bug 36350: Add caching for Objects::find and Objects::search Add subclass of Koha::Objects that provides cached find and search. To test: 1) Ensure the following tests pass: - t/db_dependend/Koha/Libraries.t - t/db_dependent/Koha/ItemTypes.t - t/db_dependent/Koha/Patrons.t - t/db_dependent/Koha/Acquisition/Currencies.t Sponsored-by: Gothenburg University Library Created attachment 163858 [details] [review] Bug 36350: Add caching for Objects::find and Objects::search Add subclass of Koha::Objects that provides cached find and search. To test: 1) Ensure the following tests pass: - t/db_dependend/Koha/Libraries.t - t/db_dependent/Koha/ItemTypes.t - t/db_dependent/Koha/Patrons.t - t/db_dependent/Koha/Acquisition/Currencies.t Sponsored-by: Gothenburg University Library Created attachment 163872 [details] [review] Bug 36350: Add caching for Objects::find and Objects::search Add subclass of Koha::Objects that provides cached find and search. To test: 1) Ensure the following tests pass: - t/db_dependend/Koha/Libraries.t - t/db_dependent/Koha/ItemTypes.t - t/db_dependent/Koha/Patrons.t - t/db_dependent/Koha/Acquisition/Currencies.t Sponsored-by: Gothenburg University Library Created attachment 163873 [details]
Before
Created attachment 163874 [details]
After
Added caching for Patrons and Currencies and attach a flamegraph for the acquisitions page where Patron::find and Currencies->search are called a large number of time with the same conditions. Probably only significant when having a large number of budgets, but it's an example of a page where caching can make a noticeable difference. Created attachment 163945 [details] [review] Bug 36350: Add caching for Objects::find and Objects::search Add subclass of Koha::Objects that provides cached find and search. To test: 1) Ensure the following tests pass: - t/db_dependend/Koha/Libraries.t - t/db_dependent/Koha/ItemTypes.t - t/db_dependent/Koha/Patrons.t - t/db_dependent/Koha/Acquisition/Currencies.t Sponsored-by: Gothenburg University Library Rebased on current master. The local caching of patrons in Bug 35921 is no longer needed and can be reverted. Tests for don't pass on main for prove t/db_dependent/Koha/Patrons.t: Test Summary Report ------------------- t/db_dependent/Koha/Patrons.t (Wstat: 2816 (exited 11) Tests: 31 Failed: 2) Failed tests: 24, 31 Non-zero exit status: 11 Parse errors: Bad plan. You planned 43 tests but ran 31. Files=1, Tests=31, 21 wallclock secs ( 0.05 usr 0.02 sys + 12.00 cusr 6.95 csys = 19.02 CPU) Result: FAIL In the test plan, prove t/db_dependend/Koha/Libraries.t should be prove t/db_dependent/Koha/Libraries.t Created attachment 177311 [details] [review] Bug 36350: Add caching for Objects::find and Objects::search Add subclass of Koha::Objects that provides cached find and search. To test: 1) Ensure the following tests pass: - t/db_dependent/Koha/Libraries.t - t/db_dependent/Koha/ItemTypes.t - t/db_dependent/Koha/Patrons.t - t/db_dependent/Koha/Acquisition/Currencies.t Sponsored-by: Gothenburg University Library Rebased on mastrr, the test fails becuase of a new mixin that was introduced that replaces the search method, I think I know how to fix and will amend the patch tomorrow. Created attachment 177370 [details] [review] Bug 36350: Add caching for Objects::find and Objects::search Add subclass of Koha::Objects that provides cached find and search. To test: 1) Ensure the following tests pass: - t/db_dependent/Koha/Libraries.t - t/db_dependent/Koha/ItemTypes.t - t/db_dependent/Koha/Patrons.t - t/db_dependent/Koha/Acquisition/Currencies.t Sponsored-by: Gothenburg University Library (In reply to David Gustafsson from comment #14) > Rebased on mastrr, the test fails becuase of a new mixin that was introduced > that replaces the search method, I think I know how to fix and will amend > the patch tomorrow. It looks like your patch drops the new mixin, which surely can't be right. Created attachment 177371 [details] [review] Bug 36350: Add caching for Objects::find and Objects::search Add subclass of Koha::Objects that provides cached find and search. To test: 1) Ensure the following tests pass: - t/db_dependent/Koha/Libraries.t - t/db_dependent/Koha/ItemTypes.t - t/db_dependent/Koha/Patrons.t - t/db_dependent/Koha/Acquisition/Currencies.t Sponsored-by: Gothenburg University Library Signed-off-by: David Nind <david@davidnind.com> The tests pass both before and after the patch. Now things should work again. Had to make somewhat dramatic changes though, but personally I think they are for the better. I moved the code Koha/Obects.Mixin/ExtendedAttributes.pm to the Koha::Objects class instead. The mixin replaced the search method, does it's thing, and then re-implements what Koha::Objects->search does, which is not very nice. I think I know why since it can't call $self->SUPER::search since Koha::Objects a sibling class of the mixing, not a parent, and thus can't be accessed. I'm personally not a fan of mixins and multiple inheritence in Perl since it creates these kind of confusing situations and ugly workarounds. I also removed use base qw(Koha::Objects::Mixin::ExtendedAttributes); from Koha/Objects/Mixin/AdditionalFields.pm. The mixin is not need anymore since included in Koha::Objects, and I don't see why it should inherit from Koha::Objects when used as a mixin, it doesn't uses SUPER, and if it did it would probably not work as expected. Overall, I'm quite intrigued, although I think it'll be easier for you to get this through if you just start with 1 thing first (ie just ItemTypes or just Libraries), so that it's easier to review and test. While I like the idea of caching data and reducing DB calls in theory, widespread unintended consequences are scary... Comment on attachment 177370 [details] [review] Bug 36350: Add caching for Objects::find and Objects::search Review of attachment 177370 [details] [review]: ----------------------------------------------------------------- ::: Koha/Patron.pm @@ +2063,4 @@ > @restricted_branchcodes = uniq(@restricted_branchcodes); > @restricted_branchcodes = sort(@restricted_branchcodes); > > + $self->{'_cache'}->{"_resetricted_branchcodes:$permission:$subpermission:$group_feature"} = \@restricted_branchcodes; You've got a typo here "resetricted" instead of "restricted" although I think you've made the typo consistently so it shouldn't really matter too much... Comment on attachment 177371 [details] [review] Bug 36350: Add caching for Objects::find and Objects::search Review of attachment 177371 [details] [review]: ----------------------------------------------------------------- ::: Koha/Objects/Cached.pm @@ +30,5 @@ > + my $cache_key = $class->_objects_cache_cache_key('find', @args); > + # Store in the args bucket if has column value condiditions > + # or for some reason the attributes argument is provided > + my $has_options = @args > 1 && ref $args[$#args] eq 'HASH'; > + my $cache = !($has_options && defined $args[$#args]->{cache} && !$args[$#args]->{cache}); I'm a bit wary of cache being used here, since "cache" is a DBIx::Class attribute for DBIx::Class::ResultSet methods: https://metacpan.org/pod/DBIx::Class::ResultSet#cache ::: Koha/Patron.pm @@ +730,5 @@ > next if $patron_id eq $anonymous_patron; > > + # Don't use cache as this could risk deleting a patron object > + # which has a reference outside of this class > + my $patron = Koha::Patrons->find( $patron_id, { cache => 0 } ); Bit wary of this cache attribute as per my other comment... Comment on attachment 177371 [details] [review] Bug 36350: Add caching for Objects::find and Objects::search Review of attachment 177371 [details] [review]: ----------------------------------------------------------------- ::: Koha/Objects/Cached.pm @@ +67,5 @@ > + else { > + my @args; > + push(@args , $cond) if defined $cond; > + # Don't include if only contains "cache" key > + push(@args, $attrs) unless keys %{$attrs} == 1 && defined $attrs->{cache}; As I note above, "cache" is actually an attribute that DBIx::Class actually uses for caching search results in a ResultSet, so I don't think we should meddle with it. I'm still not sold on this change overall, but you might want to think of a key like "koha_cache" to clearly differentiate it from DBIx::Class's "cache". Going to move this to "In Discussion" just for the moment and try to get more eyes on it... Could you talk more about "This patch enables caching for ItemTypes and Libraries which I believe are called repeatedly for the same objects much than the others"? Do you mean that $obj1 calls something like libraries() repeatedly and each call fetches the libraries again? Or that $obj1, $obj2, $obj3, etc all call libraries() and thus send the same DB query needlessly? Looks great, hight level feature ;) In my opinion, one should split between adding the feature of Object Cache and using it in item types and libraries. Also separate unit tests to be able to see them fail without main patch. I understand this uses Koha::Cache::Memory::Lite, is it theoretically possible to use Memcached ? I'm somewhat curious, could you benchmark before applying the patch but with MariaDB query cache enabled to see how it compares against the two other benchmark results?
> I understand this uses Koha::Cache::Memory::Lite, is it theoretically possible to use Memcached ?
I don't think it's a good idea, it could flood the memcached storage severely and potentially be not much faster if not slower than just retrieval with SQL, plus longer-term preservation of cache in this manner could lead to way more issues/risks, such as someone updating the database externally or with some maintenance script.
Created attachment 177379 [details] [review] Bug 36350: Add caching for Objects::find and Objects::search Add subclass of Koha::Objects that provides cached find and search. To test: 1) Ensure the following tests pass: - t/db_dependent/Koha/Libraries.t - t/db_dependent/Koha/ItemTypes.t - t/db_dependent/Koha/Patrons.t - t/db_dependent/Koha/Acquisition/Currencies.t Sponsored-by: Gothenburg University Library (In reply to David Cook from comment #20) > Overall, I'm quite intrigued, although I think it'll be easier for you to > get this through if you just start with 1 thing first (ie just ItemTypes or > just Libraries), so that it's easier to review and test. > > While I like the idea of caching data and reducing DB calls in theory, > widespread unintended consequences are scary... I agree this the change is a bit scary, and could possibly cause issues even though the cache expiration logic should in theory be exhaustive enough there could be cases where it is bypassed by using methods on _resultset directly. There is also the case where find is used internally in some class on objects where a reference exists outside of the class mutating the shared object causing unexpected issues. In theses cases care must be taken to use uncached calls to prevent this from happening. I have reviewed the code and believe I have caught all theses cases, but one can never be 100% sure. We will be using this patch in production for our next release, and since we have quite a large instance using most features in Koha I'm fairly confident any possibly remaining bugs not covered by the tests will be caught. So I recommend postponing merging before we have tested this in production for a while. (In reply to David Cook from comment #21) > Comment on attachment 177370 [details] [review] [review] > Bug 36350: Add caching for Objects::find and Objects::search > > Review of attachment 177370 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: Koha/Patron.pm > @@ +2063,4 @@ > > @restricted_branchcodes = uniq(@restricted_branchcodes); > > @restricted_branchcodes = sort(@restricted_branchcodes); > > > > + $self->{'_cache'}->{"_resetricted_branchcodes:$permission:$subpermission:$group_feature"} = \@restricted_branchcodes; > > You've got a typo here "resetricted" instead of "restricted" although I > think you've made the typo consistently so it shouldn't really matter too > much... Thanks for catching this, it has now been fixed. (In reply to David Cook from comment #25) > Could you talk more about "This patch enables caching for ItemTypes and > Libraries which I believe are called repeatedly for the same objects much > than the others"? > > Do you mean that $obj1 calls something like libraries() repeatedly and each > call fetches the libraries again? > > Or that $obj1, $obj2, $obj3, etc all call libraries() and thus send the same > DB query needlessly? I think cache is skipped in cases where search is called on an existing result-set since these cases are not possible to cache using arguments only as cache key. But there are lots of instances where search or find is called repeatedly with the same ids/query, some of which have custom local caching, some of which we previously added custom local caching which can be replaced by this. I believe I previously reviewed the entire code base and came to the conclusion that practically all cases where this is an issue is address by caching the objects for which caching has been added in this patch. So I believe there is currently little to gain by adding caching for any more Koha object types. (In reply to Fridolin Somers from comment #26) > Looks great, hight level feature ;) > > In my opinion, one should split between adding the feature of Object Cache > and using it in item types and libraries. > Also separate unit tests to be able to see them fail without main patch. > > I understand this uses Koha::Cache::Memory::Lite, is it theoretically > possible to use Memcached ? This could be done, but we will be testing this in production as I previously wrote and most likely weed out any possible remaining issues. Even though the surface area of caching related bugs is increased by adding this features for all candidates instead of one by one, it would also be nice to get it all done in one go and not having to repeat the work for each separate object class. If the consensus is that it feels safer to perform the change one by one this could of course be an option. It's possible to use Nemcached but very little to be gained in terms of performance while the risk of introducing bugs is substantially increased. Debugging would also be a nightmare since state could be introduced over a number of page reloads making reproduction of bugs extremely hard and complex. (In reply to Michał from comment #27) > I'm somewhat curious, could you benchmark before applying the patch but with > MariaDB query cache enabled to see how it compares against the two other > benchmark results? > > > I understand this uses Koha::Cache::Memory::Lite, is it theoretically possible to use Memcached ? > > I don't think it's a good idea, it could flood the memcached storage > severely and potentially be not much faster if not slower than just > retrieval with SQL, plus longer-term preservation of cache in this manner > could lead to way more issues/risks, such as someone updating the database > externally or with some maintenance script. The database query itself constitutes only a tiny fraction of the overhaed, like less than a few percent at most, query cache or not, it's the DBIx magic that is the major problem. (In reply to David Cook from comment #23) > Comment on attachment 177371 [details] [review] [review] > Bug 36350: Add caching for Objects::find and Objects::search > > Review of attachment 177371 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: Koha/Objects/Cached.pm > @@ +67,5 @@ > > + else { > > + my @args; > > + push(@args , $cond) if defined $cond; > > + # Don't include if only contains "cache" key > > + push(@args, $attrs) unless keys %{$attrs} == 1 && defined $attrs->{cache}; > > As I note above, "cache" is actually an attribute that DBIx::Class actually > uses for caching search results in a ResultSet, so I don't think we should > meddle with it. > > I'm still not sold on this change overall, but you might want to think of a > key like "koha_cache" to clearly differentiate it from DBIx::Class's "cache". I am aware of this, and though the cache option in DBix is very poorly documented I think it's actually necessary to enable it in DBix as well, at lease for search, as ->next otherwise will perform the query again. With the cache option set we are able to cache the resultset and being able to call next, reset etc without any further trips to the database. (In reply to David Gustafsson from comment #34) > I am aware of this, and though the cache option in DBix is very poorly > documented I think it's actually necessary [...] This part worries me. Using an obscure option because we /think/ it's necessary does not sound like a good idea. We need to be sure that it's necessary and we need know exactly what this option does. But even with that put aside, I'm skeptical about the implementation, in particular the fact that the cache is managed at the Koha::Object(s) level. If I read the flame graphs correctly, the bad performance was due to Koha::Number::Format::Price::_format_params repeatedly fetching the active currency. Why not put the cache there ? It would be a lot simpler and safer, and would allow for the same magnitude of performance boost. The patch also touches libraries, patrons, ILL requests and item types but there is no evidence given that it does improve performance. Can you provide some ? (In reply to Julian Maurice from comment #35) > (In reply to David Gustafsson from comment #34) > > I am aware of this, and though the cache option in DBix is very poorly > > documented I think it's actually necessary [...] > This part worries me. > Using an obscure option because we /think/ it's necessary does not sound > like a good idea. We need to be sure that it's necessary and we need know > exactly what this option does. > > But even with that put aside, I'm skeptical about the implementation, in > particular the fact that the cache is managed at the Koha::Object(s) level. > If I read the flame graphs correctly, the bad performance was due to > Koha::Number::Format::Price::_format_params repeatedly fetching the active > currency. > Why not put the cache there ? It would be a lot simpler and safer, and would > allow for the same magnitude of performance boost. > > The patch also touches libraries, patrons, ILL requests and item types but > there is no evidence given that it does improve performance. Can you provide > some ? Very valid point, it seems that `Koha::Acquisition::Currencies->get_active` does a filter search on parent with `$self->SUPER::search( { active => 1 } )->next`, there's no index on `active` which probably doesn't help either, but the real problem is that this thing is probably called very many times. No full debugger html report was provided, meaning we don't see how many times the function was called. I think it'd be nice if Koha had some feature to actually dump the list of SQL queries being executed in the given request and the time it took to get a response for each, would make it easy to identify bottlenecks like this. And current currency definitely sounds like something that could be just cached in Memcached (and invalidate it when active one is being changed). (In reply to Julian Maurice from comment #35) > (In reply to David Gustafsson from comment #34) > > I am aware of this, and though the cache option in DBix is very poorly > > documented I think it's actually necessary [...] > This part worries me. > Using an obscure option because we /think/ it's necessary does not sound > like a good idea. We need to be sure that it's necessary and we need know > exactly what this option does. > > But even with that put aside, I'm skeptical about the implementation, in > particular the fact that the cache is managed at the Koha::Object(s) level. > If I read the flame graphs correctly, the bad performance was due to > Koha::Number::Format::Price::_format_params repeatedly fetching the active > currency. > Why not put the cache there ? It would be a lot simpler and safer, and would > allow for the same magnitude of performance boost. > > The patch also touches libraries, patrons, ILL requests and item types but > there is no evidence given that it does improve performance. Can you provide > some ? Sorry, I should have been more clear with that yes, I sure it is needed, and it's being used correctly. From https://metacpan.org/pod/DBIx::Class::ResultSet: Set to 1 to cache search results. This prevents extra SQL queries if you revisit rows in your ResultSet: my $resultset = $schema->resultset('Artist')->search( undef, { cache => 1 } ); while( my $artist = $resultset->next ) { ... do stuff ... } $rs->first; # without cache, this would issue a query At hour local library we have done a lot of such optimizations already, using custom memory cache at various places for performance critical uses of ->search and ->find, The whole point of the patch is to get rid of this code duplication and extra work, and instead put the cache at a higher level where it belongs. Besides this bug there is bug 31856 in bugzilla which I have refactored after adding this one as a dependency. I will be posting a flamegraph as soon as get my local devbox with our production data up and running. As mentioned we have also have a number of performance related branches which we where planning to contribute, but instead opted to replace with central caching and provide this patch instead. https://github.com/ub-digit/Koha-build/tree/gub-dev-cache-itemtypes-find https://github.com/ub-digit/Koha-build/tree/gub-dev-cache-libraries-find https://github.com/ub-digit/Koha-build/tree/gub-dev-cache-item-pickup-locations https://github.com/ub-digit/Koha-build/tree/gub-dev-acqui-home-speedup (In reply to Michał from comment #36) > (In reply to Julian Maurice from comment #35) > > (In reply to David Gustafsson from comment #34) > > > I am aware of this, and though the cache option in DBix is very poorly > > > documented I think it's actually necessary [...] > > This part worries me. > > Using an obscure option because we /think/ it's necessary does not sound > > like a good idea. We need to be sure that it's necessary and we need know > > exactly what this option does. > > > > But even with that put aside, I'm skeptical about the implementation, in > > particular the fact that the cache is managed at the Koha::Object(s) level. > > If I read the flame graphs correctly, the bad performance was due to > > Koha::Number::Format::Price::_format_params repeatedly fetching the active > > currency. > > Why not put the cache there ? It would be a lot simpler and safer, and would > > allow for the same magnitude of performance boost. > > > > The patch also touches libraries, patrons, ILL requests and item types but > > there is no evidence given that it does improve performance. Can you provide > > some ? > > Very valid point, it seems that `Koha::Acquisition::Currencies->get_active` > does a filter search on parent with `$self->SUPER::search( { active => 1 } > )->next`, there's no index on `active` which probably doesn't help either, > but the real problem is that this thing is probably called very many times. > > No full debugger html report was provided, meaning we don't see how many > times the function was called. > > I think it'd be nice if Koha had some feature to actually dump the list of > SQL queries being executed in the given request and the time it took to get > a response for each, would make it easy to identify bottlenecks like this. > And current currency definitely sounds like something that could be just > cached in Memcached (and invalidate it when active one is being changed). Again, from experience the SQL-query itself, with index or not, is totally neglectable when it comes to performance. It the DBIx-abstractions that are responsible for the slowness of search and find. The table where currencies are stored also don't have that many rows, so an index would in that case not make any difference. (In reply to David Gustafsson from comment #38) > (In reply to Michał from comment #36) > > (In reply to Julian Maurice from comment #35) > > > (In reply to David Gustafsson from comment #34) > > > > I am aware of this, and though the cache option in DBix is very poorly > > > > documented I think it's actually necessary [...] > > > This part worries me. > > > Using an obscure option because we /think/ it's necessary does not sound > > > like a good idea. We need to be sure that it's necessary and we need know > > > exactly what this option does. > > > > > > But even with that put aside, I'm skeptical about the implementation, in > > > particular the fact that the cache is managed at the Koha::Object(s) level. > > > If I read the flame graphs correctly, the bad performance was due to > > > Koha::Number::Format::Price::_format_params repeatedly fetching the active > > > currency. > > > Why not put the cache there ? It would be a lot simpler and safer, and would > > > allow for the same magnitude of performance boost. > > > > > > The patch also touches libraries, patrons, ILL requests and item types but > > > there is no evidence given that it does improve performance. Can you provide > > > some ? > > > > Very valid point, it seems that `Koha::Acquisition::Currencies->get_active` > > does a filter search on parent with `$self->SUPER::search( { active => 1 } > > )->next`, there's no index on `active` which probably doesn't help either, > > but the real problem is that this thing is probably called very many times. > > > > No full debugger html report was provided, meaning we don't see how many > > times the function was called. > > > > I think it'd be nice if Koha had some feature to actually dump the list of > > SQL queries being executed in the given request and the time it took to get > > a response for each, would make it easy to identify bottlenecks like this. > > And current currency definitely sounds like something that could be just > > cached in Memcached (and invalidate it when active one is being changed). > > Again, from experience the SQL-query itself, with index or not, is totally > neglectable when it comes to performance. It the DBIx-abstractions that are > responsible for the slowness of search and find. The table where currencies > are stored also don't have that many rows, so an index would in that case > not make any difference. I will revise this statement slightly. I know that I before have seen drastic increases in performance by replaceing DBIx-statments with DBI-equivalents, but having a second look on the flemegraphs from serials search it seams that the majority of time is actually spent in DBI::st::execute. I don't know if there parhaps have been improvements in DBIx performance since last I checked. This had me thinking there might also be a missing index, and saw that the additionaL_field_values table was missing an index for the record_id column. After adding one the execution time was cut to about 1/3 of before, but the cached version is still about 5 times faster than that and 16 times faster than without the index. See bug 31856 where I attached flamegraphs after refactoring and adding this bug as a dependency. |