[Bug 36350] New: Add subclass of Koha::Objects that provides caching for find
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 Bug ID: 36350 Summary: Add subclass of Koha::Objects that provides caching for find Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: glasklas@gmail.com QA Contact: testopia@bugs.koha-community.org At our library we have traced some performance issues to repeated find calls for a number of object types. To address this patch introduces a subclass for Koha::Object that if extended instead of the former provides a cached version of find. This patch enables caching for ItemTypes and Libraries which I believe are called repeatedly for the same objects much than the others. Other possible candidates are Patrons and Items but will have to do some more digging to find out if it makes much a difference. It's easy to enable/disable by just switching out the base class for the Koha::Object and Koha::Objects derived classes. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #1 from David Gustafsson <glasklas@gmail.com> --- Created attachment 163357 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163357&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |glasklas@gmail.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #2 from David Gustafsson <glasklas@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #163357|0 |1 is obsolete| | --- Comment #3 from David Gustafsson <glasklas@gmail.com> --- Created attachment 163361 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163361&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #163361|0 |1 is obsolete| | --- Comment #4 from David Gustafsson <glasklas@gmail.com> --- Created attachment 163857 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163857&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #163857|0 |1 is obsolete| | --- Comment #5 from David Gustafsson <glasklas@gmail.com> --- Created attachment 163858 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163858&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #163858|0 |1 is obsolete| | --- Comment #6 from David Gustafsson <glasklas@gmail.com> --- Created attachment 163872 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163872&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #7 from David Gustafsson <glasklas@gmail.com> --- Created attachment 163873 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163873&action=edit Before -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #8 from David Gustafsson <glasklas@gmail.com> --- Created attachment 163874 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163874&action=edit After -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #9 from David Gustafsson <glasklas@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Add subclass of |Add subclass of |Koha::Objects that provides |Koha::Objects that provides |caching for find |caching for find and search -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #163872|0 |1 is obsolete| | --- Comment #10 from David Gustafsson <glasklas@gmail.com> --- Created attachment 163945 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163945&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #11 from David Gustafsson <glasklas@gmail.com> --- Rebased on current master. The local caching of patrons in Bug 35921 is no longer needed and can be reverted. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Status|Needs Signoff |Failed QA --- Comment #12 from David Nind <david@davidnind.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #163945|0 |1 is obsolete| | --- Comment #13 from David Gustafsson <glasklas@gmail.com> --- Created attachment 177311 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177311&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #14 from David Gustafsson <glasklas@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #177311|0 |1 is obsolete| | --- Comment #15 from David Gustafsson <glasklas@gmail.com> --- Created attachment 177370 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177370&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #16 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Nind <david@davidnind.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=36350 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #177370|0 |1 is obsolete| | --- Comment #17 from David Nind <david@davidnind.com> --- Created attachment 177371 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177371&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #18 from David Nind <david@davidnind.com> --- The tests pass both before and after the patch. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #19 from David Gustafsson <glasklas@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #20 from David Cook <dcook@prosentient.com.au> --- 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... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #21 from David Cook <dcook@prosentient.com.au> --- Comment on attachment 177370 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177370 Bug 36350: Add caching for Objects::find and Objects::search Review of attachment 177370: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=36350&attachment=177370) ----------------------------------------------------------------- ::: 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... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #22 from David Cook <dcook@prosentient.com.au> --- Comment on attachment 177371 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177371 Bug 36350: Add caching for Objects::find and Objects::search Review of attachment 177371: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=36350&attachment=177371) ----------------------------------------------------------------- ::: 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... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #23 from David Cook <dcook@prosentient.com.au> --- Comment on attachment 177371 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177371 Bug 36350: Add caching for Objects::find and Objects::search Review of attachment 177371: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=36350&attachment=177371) ----------------------------------------------------------------- ::: 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". -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |In Discussion --- Comment #24 from David Cook <dcook@prosentient.com.au> --- Going to move this to "In Discussion" just for the moment and try to get more eyes on it... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #25 from David Cook <dcook@prosentient.com.au> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=32476 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #26 from Fridolin Somers <fridolin.somers@biblibre.com> --- 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 ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 Michał <schodkowy.omegi-0r@icloud.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |schodkowy.omegi-0r@icloud.c | |om --- Comment #27 from Michał <schodkowy.omegi-0r@icloud.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #177371|0 |1 is obsolete| | --- Comment #28 from David Gustafsson <glasklas@gmail.com> --- Created attachment 177379 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177379&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #29 from David Gustafsson <glasklas@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #30 from David Gustafsson <glasklas@gmail.com> --- (In reply to David Cook from comment #21)
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...
Thanks for catching this, it has now been fixed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #31 from David Gustafsson <glasklas@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #32 from David Gustafsson <glasklas@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #33 from David Gustafsson <glasklas@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #34 from David Gustafsson <glasklas@gmail.com> --- (In reply to David Cook from comment #23)
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".
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. -- You are receiving this mail because: You are watching all bug changes.
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
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com --- Comment #35 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to David Gustafsson from comment #34) 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 ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #36 from Michał <schodkowy.omegi-0r@icloud.com> --- (In reply to Julian Maurice from comment #35)
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
(In reply to David Gustafsson from comment #34) 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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |31856 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31856 [Bug 31856] Improve performance of serials subscriptions search -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #37 from David Gustafsson <glasklas@gmail.com> --- (In reply to Julian Maurice from comment #35)
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
(In reply to David Gustafsson from comment #34) 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.
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-locati... https://github.com/ub-digit/Koha-build/tree/gub-dev-acqui-home-speedup -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #38 from David Gustafsson <glasklas@gmail.com> --- (In reply to Michał from comment #36)
(In reply to Julian Maurice from comment #35)
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
(In reply to David Gustafsson from comment #34) 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #39 from David Gustafsson <glasklas@gmail.com> --- (In reply to David Gustafsson from comment #38)
(In reply to Michał from comment #36)
(In reply to Julian Maurice from comment #35)
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
(In reply to David Gustafsson from comment #34) 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #177379|0 |1 is obsolete| | --- Comment #40 from David Gustafsson <glasklas@gmail.com> --- Created attachment 178729 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=178729&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #41 from David Gustafsson <glasklas@gmail.com> --- Rebased against main. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #42 from David Gustafsson <glasklas@gmail.com> --- Created attachment 178835 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=178835&action=edit Bug 36350: Fix qa-script issues -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #43 from David Gustafsson <glasklas@gmail.com> --- Fixed some qa-script issues. The complaint about pod coverage with regard to Koha::Objects::Cached::Base is caused by a bug in the koha-qa.pl script that seems to detect new files as removed, and incorrectly sets the "previous" rating to 1. As Objects::Cached::Base only have private methods the rating is set to 0 and the script will complain. There are also some "Inconsistent hierarchy during C3 merge of class ..." I don't really understand and haven't looked into that much. Bug 35959 seems to be related to that issue, but have not yet figured out how to fix the problem (if it needs to be fixed). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #44 from David Gustafsson <glasklas@gmail.com> --- Created attachment 178837 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=178837&action=edit Bug 36350: Remove unused import -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #178729|0 |1 is obsolete| | --- Comment #45 from David Gustafsson <glasklas@gmail.com> --- Created attachment 179331 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=179331&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #178835|0 |1 is obsolete| | --- Comment #46 from David Gustafsson <glasklas@gmail.com> --- Created attachment 179332 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=179332&action=edit Bug 36350: Fix qa-script issues -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #178837|0 |1 is obsolete| | --- Comment #47 from David Gustafsson <glasklas@gmail.com> --- Created attachment 179333 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=179333&action=edit Bug 36350: Remove unused import -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #48 from David Gustafsson <glasklas@gmail.com> --- Rebased against main. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #179331|0 |1 is obsolete| | --- Comment #49 from David Gustafsson <glasklas@gmail.com> --- Created attachment 179342 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=179342&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #179332|0 |1 is obsolete| | --- Comment #50 from David Gustafsson <glasklas@gmail.com> --- Created attachment 179343 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=179343&action=edit Bug 36350: Fix qa-script issues -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #179333|0 |1 is obsolete| | --- Comment #51 from David Gustafsson <glasklas@gmail.com> --- Created attachment 179344 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=179344&action=edit Bug 36350: Remove unused import -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@openfifth.c | |o.uk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #179342|0 |1 is obsolete| | --- Comment #52 from David Gustafsson <glasklas@gmail.com> --- Created attachment 189934 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189934&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #179343|0 |1 is obsolete| | --- Comment #53 from David Gustafsson <glasklas@gmail.com> --- Created attachment 189935 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189935&action=edit Bug 36350: Fix qa-script issues -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #179344|0 |1 is obsolete| | --- Comment #54 from David Gustafsson <glasklas@gmail.com> --- Created attachment 189936 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189936&action=edit Bug 36350: Remove unused import -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff --- Comment #55 from David Gustafsson <glasklas@gmail.com> --- Rebased against main. Would be nice if anyone took some interest in this, I know this patch is a little bit scary, but we have been running this in production now for almost a year without discovering any issues. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #56 from David Nind <david@davidnind.com> --- One test fails 8-(... All the others pass. prove t/db_dependent/Koha/Libraries.t t/db_dependent/Koha/Libraries.t .. 13/14 # Looks like you planned 14 tests but ran 15. t/db_dependent/Koha/Libraries.t .. Dubious, test returned 255 (wstat 65280, 0xff00) All 14 subtests passed Test Summary Report ------------------- t/db_dependent/Koha/Libraries.t (Wstat: 65280 (exited 255) Tests: 15 Failed: 1) Failed test: 15 Non-zero exit status: 255 Parse errors: Bad plan. You planned 14 tests but ran 15. Files=1, Tests=15, 2 wallclock secs ( 0.02 usr 0.00 sys + 1.50 cusr 0.43 csys = 1.95 CPU) Result: FAIL Testing notes (using KTD): - prove t/db_dependent/Koha/Libraries.t - prove t/db_dependent/Koha/ItemTypes.t - prove t/db_dependent/Koha/Patrons.t - prove t/db_dependent/Koha/Acquisition/Currencies.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #189934|0 |1 is obsolete| | --- Comment #57 from David Gustafsson <glasklas@gmail.com> --- Created attachment 189956 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189956&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #189935|0 |1 is obsolete| | --- Comment #58 from David Gustafsson <glasklas@gmail.com> --- Created attachment 189957 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189957&action=edit Bug 36350: Fix qa-script issues -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #189936|0 |1 is obsolete| | --- Comment #59 from David Gustafsson <glasklas@gmail.com> --- Created attachment 189958 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189958&action=edit Bug 36350: Remove unused import -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #60 from David Gustafsson <glasklas@gmail.com> --- (In reply to David Nind from comment #56)
One test fails 8-(... All the others pass.
prove t/db_dependent/Koha/Libraries.t t/db_dependent/Koha/Libraries.t .. 13/14 # Looks like you planned 14 tests but ran 15. t/db_dependent/Koha/Libraries.t .. Dubious, test returned 255 (wstat 65280, 0xff00) All 14 subtests passed
Test Summary Report ------------------- t/db_dependent/Koha/Libraries.t (Wstat: 65280 (exited 255) Tests: 15 Failed: 1) Failed test: 15 Non-zero exit status: 255 Parse errors: Bad plan. You planned 14 tests but ran 15. Files=1, Tests=15, 2 wallclock secs ( 0.02 usr 0.00 sys + 1.50 cusr 0.43 csys = 1.95 CPU) Result: FAIL
Testing notes (using KTD): - prove t/db_dependent/Koha/Libraries.t - prove t/db_dependent/Koha/ItemTypes.t - prove t/db_dependent/Koha/Patrons.t - prove t/db_dependent/Koha/Acquisition/Currencies.t
Ok! The test count was wrong for some reason, fixed that now. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 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=36350 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #189956|0 |1 is obsolete| | --- Comment #61 from David Nind <david@davidnind.com> --- Created attachment 189983 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189983&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #189957|0 |1 is obsolete| | --- Comment #62 from David Nind <david@davidnind.com> --- Created attachment 189984 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189984&action=edit Bug 36350: Fix qa-script issues 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=36350 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #189958|0 |1 is obsolete| | --- Comment #63 from David Nind <david@davidnind.com> --- Created attachment 189985 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=189985&action=edit Bug 36350: Remove unused import 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=36350 --- Comment #64 from David Gustafsson <glasklas@gmail.com> --- Created attachment 192972 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=192972&action=edit Bug 36350: Don't cache limit searches to avoid custom expiration logic -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #65 from Jonathan Druart <jonathan.druart@gmail.com> --- 1. diff --git a/Koha/ILL/Requests.pm b/Koha/ILL/Requests.pm -use base qw(Koha::Objects::Mixin::ExtendedAttributes Koha::Objects); +use base qw(Koha::Objects); This does not seem correct. 2. diff --git a/Koha/Object.pm b/Koha/Object.pm @@ -1017,6 +1017,9 @@ sub AUTOLOAD { my $accessor = sub { my $self = shift; if (@_) { + if ($self->isa("Koha::Object::CachedExpiration")) { + $self->_objects_cache_expire(); diff --git a/Koha/Object/CachedExpiration.pm b/Koha/Object/CachedExpiration.pm +sub _objects_cache_expire { + my ($self) = @_; + if ($self->_result->in_storage) { + my @primary_keys = $self->_result->id; + my $ids_cache_key = $self->_objects_cache_cache_key('find', @primary_keys); + $self->_objects_cache_clear($ids_cache_key); + } +} Isn't it going to add one fetch for each insert/update statement? 3. _objects_cache_expire is unnecessary called when we store a new object (not blocker) 4. very quick benchmark to give an idea: ``` use Benchmark qw(:all); my $count = 10000; my $t = timeit($count, sub { my $l = Koha::Library->new({branchcode => 'foo', branchname => 'foo'})->store; $l->branchname('bar')->store; $l->delete; }); say timestr($t); ``` with the patches: 139 wallclock secs (59.03 usr + 5.42 sys = 64.45 CPU) @ 155.16/s (n=10000) without the patches: 133 wallclock secs (54.21 usr + 5.13 sys = 59.34 CPU) @ 168.52/s (n=10000) ``` use Benchmark qw(:all); my $count = 10000; my $i; my $t = timeit($count, sub { my $id = "foo" . $i++; my $l = Koha::Library->new({branchcode => $id, branchname => $id})->store; $l->branchname('bar')->store; }); ``` with the patches: 100 wallclock secs (45.76 usr + 4.15 sys = 49.91 CPU) @ 200.36/s (n=10000) without the patches: 98 wallclock secs (44.62 usr + 3.95 sys = 48.57 CPU) @ 205.89/s (n=10000) say timestr($t); => it's going to be slightly slower for inserts in bulk. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 --- Comment #66 from David Gustafsson <glasklas@gmail.com> --- (In reply to Jonathan Druart from comment #65)
1. diff --git a/Koha/ILL/Requests.pm b/Koha/ILL/Requests.pm
-use base qw(Koha::Objects::Mixin::ExtendedAttributes Koha::Objects); +use base qw(Koha::Objects);
This does not seem correct.
2.
diff --git a/Koha/Object.pm b/Koha/Object.pm
@@ -1017,6 +1017,9 @@ sub AUTOLOAD { my $accessor = sub { my $self = shift; if (@_) { + if ($self->isa("Koha::Object::CachedExpiration")) { + $self->_objects_cache_expire();
diff --git a/Koha/Object/CachedExpiration.pm b/Koha/Object/CachedExpiration.pm +sub _objects_cache_expire { + my ($self) = @_; + if ($self->_result->in_storage) { + my @primary_keys = $self->_result->id; + my $ids_cache_key = $self->_objects_cache_cache_key('find', @primary_keys); + $self->_objects_cache_clear($ids_cache_key); + } +}
Isn't it going to add one fetch for each insert/update statement?
3. _objects_cache_expire is unnecessary called when we store a new object (not blocker)
4. very quick benchmark to give an idea: ``` use Benchmark qw(:all); my $count = 10000; my $t = timeit($count, sub { my $l = Koha::Library->new({branchcode => 'foo', branchname => 'foo'})->store; $l->branchname('bar')->store; $l->delete; }); say timestr($t); ``` with the patches: 139 wallclock secs (59.03 usr + 5.42 sys = 64.45 CPU) @ 155.16/s (n=10000) without the patches: 133 wallclock secs (54.21 usr + 5.13 sys = 59.34 CPU) @ 168.52/s (n=10000)
``` use Benchmark qw(:all); my $count = 10000; my $i;
my $t = timeit($count, sub {
my $id = "foo" . $i++;
my $l = Koha::Library->new({branchcode => $id, branchname => $id})->store; $l->branchname('bar')->store;
}); ``` with the patches: 100 wallclock secs (45.76 usr + 4.15 sys = 49.91 CPU) @ 200.36/s (n=10000) without the patches: 98 wallclock secs (44.62 usr + 3.95 sys = 48.57 CPU) @ 205.89/s (n=10000)
say timestr($t);
=> it's going to be slightly slower for inserts in bulk.
I not sure I understand what you mean.
. _objects_cache_expire is unnecessary called when we store a new object.
Cache needs to be expired when an item is modified, or else we risk getting the modified item back if performing a find with the same id. So not sure what you mean by "unnecessary".
Isn't it going to add one fetch for each insert/update statement?
Perhaps you are confusing this line: my $ids_cache_key = $self->_objects_cache_cache_key('find',
@primary_keys);
With at database fetch? Is just the function for getting the cache key. There is going to be some overhead for setting columns, but considering its just a few percent, and the expiration logic can't be avoided, I don't think it's a problem. It should be a pretty rare operation to create of thousands of objects, in in those cases the extra overhead of cache expiration should be neglectable compared to other operations. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36350 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org