Bug 16044 introduced two-level caching mechanism to Koha::Cache. By default, get_from_cache() returns a deep copy of the data structures stored in L1 cache (aka "safe mode"). For extremely big and/or complex data structures (like MARC framework hash-of-hashes-of-hashes returned by GetMarcStructure() ), deep-cloning is currently very sub-optimal (i.e., 2-3x slower after Bug 16044); this patch addresses that particular issue. See also: Bug 16044 comments #18 - #23
Created attachment 49670 [details] [review] Bug 16166: Improve L1 cache performance Bug 16044 introduced two-level caching mechanism to Koha::Cache. By default, get_from_cache() returns a deep copy of the data structures stored in L1 cache (aka "safe mode"). For extremely big and/or complex data structures (like MARC framework hash-of-hashes-of-hashes returned by GetMarcStructure() ), deep-cloning is currently very sub-optimal (i.e., 2-3x slower after Bug 16044); this patch addresses that particular issue. See also: Bug 16044 comments #18 - #23 Test plan: 1) apply patch 2) flush L2 cache if necessary (restart memcached daemon) 3) profile GetMarcStructure() calls before / after patch, e.g. by running some script which calls it often (like catalogue search w/ XSLT processing turned on, etc.) 4) after Bug 16044 + Bug 16166, GetMarcStructure() should be faster than before, in all possible circumstances
Did you have a look at the patches on bug 16140?
Jacek, this patch makes a couple of t/Cache.t tests fail.
(In reply to Jonathan Druart from comment #3) > Jacek, this patch makes a couple of t/Cache.t tests fail. Tests #34 & #37? It's not a bug, it's a feature ;). In this implementation, direct data references returned by get_from_cache(..., { unsafe => 1 }) are shared only between other (..., { unsafe => 1 }) calls. If you call get_from_cache() in safe mode, never mind how many unsafe calls were done before, and what happened to that poor data structure in the meantime, you will always get a fresh clone of the original thing.
Created attachment 49729 [details] [review] Bug 16166: Bugfix for t/Cache.t tests #34 & #37 Test plan addendum: 5) prove t/Cache.t
Created attachment 49730 [details] [review] Bug 16166: Try to add readability
(In reply to Jonathan Druart from comment #6) > Created attachment 49730 [details] [review] [review] > Bug 16166: Try to add readability I tried to add readability to the Koha::Cache method, but not sure I did it. I have also moved a thaw, but it does not look correct: now it will be done when fetching the L2 instead of on demand when fetching the L1 (only the first time). Just share for discussion, but feel free to obsolete.
Created attachment 49739 [details] [review] Bug 16166: Try to improve code readability Test plan addendum #2: 6) Don't forget to flush L2 cache (restart mecached deameon) after testing as well - if you just return to master branch without this step, all sorts of weird things would happen
(In reply to Jonathan Druart from comment #7) > (In reply to Jonathan Druart from comment #6) > > Created attachment 49730 [details] [review] [review] [review] > > Bug 16166: Try to add readability > > I tried to add readability to the Koha::Cache method, but not sure I did it. Some parts of it certainly look good to me, thanks. I skipped some parts because: - wrapping scalars in { value => ... } makes it a bit slower for caching small scalars (like system preferences) - not much slower, but measurably slower, and this patch set is supposed to be as speed-neutral as possible even in the "worst case scenarios" - I think that we need to keep a separatelly thawed structure in L1 for a sole purpose of unsafe => 1 calls; if not, the "feature" described in comment #4 would be broken; also, there is no need to do 2nd thaw immediately / in advance, it may be never used e.g. if all get_from_cache() calls for a given key are of the "safe" kind (and a lot of scripts are doing just one single call) - it's more optimal to delay 2nd thaw to the moment when the 1st "unsafe" call is being made; if the 1st call is "unsafe", there would be only one thaw - I did s/{value}/{thawed}/ - there are too many $value variables in that code, it's hard to distiguish what is what
Created attachment 50192 [details] [review] Bug 16166: Improve L1 cache performance and safety Bug 16044 introduced two-level caching mechanism to Koha::Cache. By default, get_from_cache() returns a deep copy of the data structures stored in L1 cache (aka "safe mode"). For extremely big and/or complex data structures (like MARC framework hash-of-hashes-of-hashes returned by GetMarcStructure() ), deep-cloning is currently very sub-optimal (i.e., 2-3x slower after Bug 16044); this patch addresses that particular issue. It also fixes some borderline "safety" isuess remaining after Bug 16044 (i.e., 1st get_from_cache() call being implicitly unsafe etc.) and provides the ability to intermix "safe" and "unsafe" cache feches, eliminating the risk involved with "unsafe" calls possibly compromising further "safe" calls. Test plan: 1) apply patch 2) flush L2 cache if necessary (restart memcached daemon) 3) profile GetMarcStructure() calls before / after patch, e.g. by running some script which calls it often (like catalogue search w/ XSLT processing turned on, etc.) 4) after Bug 16044 + Bug 16166, GetMarcStructure() should be faster than before, in all possible circumstances 5) prove t/Cache.t 6) after testing, before returning to the master branch, flush L2 cache again (restart memcached daemon) - otherwise all system preferences returned from L2 cache would be suffixed with '-CF1'
Here's something to think about: trying to make the L1 safe while keeping its performance seems to be rather tricky. Could we consider making cloning (or locking) something that can be enabled for testing, and leave it turned off in production? The data structures returned by L1-memoized functions should really not be modified by the code that uses them in a way that interferes with future uses anyway. See https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=49488 for code that does modify the value stored in cache, but in a way that doesn't cause problems.
Created attachment 50708 [details] [review] Bug 16166: Improve L1 cache performance and safety Bug 16044 introduced two-level caching mechanism to Koha::Cache. By default, get_from_cache() returns a deep copy of the data structures stored in L1 cache (aka "safe mode"). For extremely big and/or complex data structures (like MARC framework hash-of-hashes-of-hashes returned by GetMarcStructure() ), deep-cloning is currently sub-optimal; this patch addresses that particular issue. It also provides an ability to intermix "safe" and "unsafe" cache feches, in such way that they don't interfere with each other (i.e., eliminating the risk involved with "unsafe" calls possibly compromising further "safe" calls). Test plan: 1) apply patch 2) flush L2 cache if necessary (restart memcached daemon) 3) profile GetMarcStructure() calls before / after patch, e.g. by running some script which calls it often (like catalogue search w/ XSLT processing turned on, etc.) - GetMarcStructure() should be faster than before, in all possible circumstances (eg. 18 msec per call -> 10 msec p/call) 5) prove -v t/Cache.t - all 39 tests should pass; if you see something like 'ok 39 # skip Cache not enabled', it means that cache is not active and you need to manually revert Bug 16104 first 6) after testing, before returning to the master branch, flush L2 cache again (restart memcached daemon) - otherwise all system preferences returned from L2 cache would be suffixed with '-CF0'
Created attachment 50709 [details] [review] Bug 16166: Fix t/Cache.t tests Thou shalt not forget to include changes made in the t/Cache.t in the patch set.
(In reply to Jesse Weaver from comment #11) > See > https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=49488 for code > that does modify the value stored in cache, but in a way that doesn't cause > problems. I tried to review GetMarcStructure() calls to ensure that "unsafe" variant does not cause any problems, but this is a mind boggling task, there are way to many of those calls (direct or undirect) in the code. So I created Bug 16365 instead - not such elegant solution, but less dangerous and more manageable one IMO. Not sure about GetAuthorisedValue() - I vaguely recollect that somewhere in the past it definitely wasn't "safe", but I can't remember why exactly; will try to find the relevant bug report.
(In reply to Jacek Ablewicz from comment #14) > Not sure about GetAuthorisedValue() - I vaguely recollect that somewhere in > the past it definitely wasn't "safe", but I can't remember why exactly; will > try to find the relevant bug report. Bug 9967 comment #10 - but this particular issue probably got resolved long time ago by Bug 11944.
Created attachment 51763 [details] [review] Bug 16166: Improve L1 cache performance and safety Bug 16044 introduced two-level caching mechanism to Koha::Cache. By default, get_from_cache() returns a deep copy of the data structures stored in L1 cache (aka "safe mode"). For extremely big and/or complex data structures (like MARC framework hash-of-hashes-of-hashes returned by GetMarcStructure() ), deep-cloning is currently sub-optimal; this patch addresses that particular issue. It also provides an ability to intermix "safe" and "unsafe" cache feches, in such way that they don't interfere with each other (i.e., eliminating the risk involved with "unsafe" calls possibly compromising further "safe" calls). Test plan: 1) apply patch 2) flush L2 cache if necessary (restart memcached daemon) 3) profile GetMarcStructure() calls before / after patch, e.g. by running some script which calls it often (like catalogue search w/ XSLT processing turned on, etc.) - GetMarcStructure() should be faster than before, in all possible circumstances (eg. 18 msec per call -> 10 msec p/call) 5) prove -v t/Cache.t - all 39 tests should pass; if you see something like 'ok 39 # skip Cache not enabled', it means that cache is not active and you need to manually revert Bug 16104 first 6) after testing, before returning to the master branch, flush L2 cache again (restart memcached daemon) - otherwise all system preferences returned from L2 cache would be suffixed with '-CF0'
Created attachment 51764 [details] [review] Bug 16166: Fix t/Cache.t tests Thou shalt not forget to include changes made in the t/Cache.t in the patch set.
Created attachment 51765 [details] [review] Bug 16166: Update number of tests to run in t/Cache.t Update number of tests to run + number of tests to skip in case when cache is not active.
Created attachment 52246 [details] [review] Bug 16166: Proposed followup to use Sereal for serialization This manages to eke out a bit more performance on my machine.
Comment on attachment 52246 [details] [review] Bug 16166: Proposed followup to use Sereal for serialization Moved to bug 16715
Created attachment 52265 [details] [review] Bug 16166: Improve L1 cache performance and safety Bug 16044 introduced two-level caching mechanism to Koha::Cache. By default, get_from_cache() returns a deep copy of the data structures stored in L1 cache (aka "safe mode"). For extremely big and/or complex data structures (like MARC framework hash-of-hashes-of-hashes returned by GetMarcStructure() ), deep-cloning is currently sub-optimal; this patch addresses that particular issue. It also provides an ability to intermix "safe" and "unsafe" cache feches, in such way that they don't interfere with each other (i.e., eliminating the risk involved with "unsafe" calls possibly compromising further "safe" calls). Test plan: 1) apply patch 2) flush L2 cache if necessary (restart memcached daemon) 3) profile GetMarcStructure() calls before / after patch, e.g. by running some script which calls it often (like catalogue search w/ XSLT processing turned on, etc.) - GetMarcStructure() should be faster than before, in all possible circumstances (eg. 18 msec per call -> 10 msec p/call) 4) after testing, before returning to the master branch, flush L2 cache again (restart memcached daemon) - otherwise all system preferences returned from L2 cache would be suffixed with '-CF0'
Created attachment 52266 [details] [review] Bug 16166: Fix t/Cache.t tests Thou shalt not forget to include changes made in the t/Cache.t in the patch set.
See bug 16715 for more information about perfs gain.
Created attachment 52509 [details] [review] Bug 16166: Improve L1 cache performance and safety Bug 16044 introduced two-level caching mechanism to Koha::Cache. By default, get_from_cache() returns a deep copy of the data structures stored in L1 cache (aka "safe mode"). For extremely big and/or complex data structures (like MARC framework hash-of-hashes-of-hashes returned by GetMarcStructure() ), deep-cloning is currently sub-optimal; this patch addresses that particular issue. It also provides an ability to intermix "safe" and "unsafe" cache feches, in such way that they don't interfere with each other (i.e., eliminating the risk involved with "unsafe" calls possibly compromising further "safe" calls). Test plan: 1) apply patch 2) flush L2 cache if necessary (restart memcached daemon) 3) profile GetMarcStructure() calls before / after patch, e.g. by running some script which calls it often (like catalogue search w/ XSLT processing turned on, etc.) - GetMarcStructure() should be faster than before, in all possible circumstances (eg. 18 msec per call -> 10 msec p/call) 4) after testing, before returning to the master branch, flush L2 cache again (restart memcached daemon) - otherwise all system preferences returned from L2 cache would be suffixed with '-CF0' Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 52510 [details] [review] Bug 16166: Fix t/Cache.t tests Thou shalt not forget to include changes made in the t/Cache.t in the patch set. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Forgot to add my signed-off-by lines!
Created attachment 52863 [details] [review] Bug 16166: Improve L1 cache performance and safety Bug 16044 introduced two-level caching mechanism to Koha::Cache. By default, get_from_cache() returns a deep copy of the data structures stored in L1 cache (aka "safe mode"). For extremely big and/or complex data structures (like MARC framework hash-of-hashes-of-hashes returned by GetMarcStructure() ), deep-cloning is currently sub-optimal; this patch addresses that particular issue. It also provides an ability to intermix "safe" and "unsafe" cache feches, in such way that they don't interfere with each other (i.e., eliminating the risk involved with "unsafe" calls possibly compromising further "safe" calls). Test plan: 1) apply patch 2) flush L2 cache if necessary (restart memcached daemon) 3) profile GetMarcStructure() calls before / after patch, e.g. by running some script which calls it often (like catalogue search w/ XSLT processing turned on, etc.) - GetMarcStructure() should be faster than before, in all possible circumstances (eg. 18 msec per call -> 10 msec p/call) 4) after testing, before returning to the master branch, flush L2 cache again (restart memcached daemon) - otherwise all system preferences returned from L2 cache would be suffixed with '-CF0' Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 52864 [details] [review] Bug 16166: Fix t/Cache.t tests Thou shalt not forget to include changes made in the t/Cache.t in the patch set. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master for 16.11, thanks Jacek!