Bug 16166 - Improve L1 cache performance and safety
Summary: Improve L1 cache performance and safety
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P2 enhancement (vote)
Assignee: Jacek Ablewicz
QA Contact: Testopia
URL:
Keywords:
Depends on: 16044 16221 16229
Blocks: 16365 16579 16715
  Show dependency treegraph
 
Reported: 2016-03-30 07:17 UTC by Jacek Ablewicz
Modified: 2017-12-07 22:16 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 16166: Improve L1 cache performance (3.70 KB, patch)
2016-03-30 07:43 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 16166: Bugfix for t/Cache.t tests #34 & #37 (1.74 KB, patch)
2016-03-31 10:40 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 16166: Try to add readability (2.59 KB, patch)
2016-03-31 10:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16166: Try to improve code readability (3.03 KB, patch)
2016-03-31 15:16 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 16166: Improve L1 cache performance and safety (5.71 KB, patch)
2016-04-13 12:06 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 16166: Improve L1 cache performance and safety (5.59 KB, patch)
2016-04-26 07:03 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 16166: Fix t/Cache.t tests (2.56 KB, patch)
2016-04-26 07:08 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 16166: Improve L1 cache performance and safety (5.96 KB, patch)
2016-05-25 09:44 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 16166: Fix t/Cache.t tests (2.58 KB, patch)
2016-05-25 09:44 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 16166: Update number of tests to run in t/Cache.t (953 bytes, patch)
2016-05-25 09:44 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 16166: Proposed followup to use Sereal for serialization (2.81 KB, patch)
2016-06-10 15:46 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 16166: Improve L1 cache performance and safety (5.77 KB, patch)
2016-06-12 08:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16166: Fix t/Cache.t tests (2.61 KB, patch)
2016-06-12 08:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16166: Improve L1 cache performance and safety (5.84 KB, patch)
2016-06-18 12:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16166: Fix t/Cache.t tests (2.68 KB, patch)
2016-06-18 12:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16166: Improve L1 cache performance and safety (5.89 KB, patch)
2016-06-24 18:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16166: Fix t/Cache.t tests (2.74 KB, patch)
2016-06-24 18:48 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jacek Ablewicz 2016-03-30 07:17:33 UTC
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
Comment 1 Jacek Ablewicz 2016-03-30 07:43:21 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-03-30 11:08:40 UTC
Did you have a look at the patches on bug 16140?
Comment 3 Jonathan Druart 2016-03-31 09:23:06 UTC
Jacek, this patch makes a couple of t/Cache.t tests fail.
Comment 4 Jacek Ablewicz 2016-03-31 10:27:59 UTC
(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.
Comment 5 Jacek Ablewicz 2016-03-31 10:40:36 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2016-03-31 10:57:10 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2016-03-31 10:59:57 UTC
(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.
Comment 8 Jacek Ablewicz 2016-03-31 15:16:17 UTC Comment hidden (obsolete)
Comment 9 Jacek Ablewicz 2016-03-31 16:00:04 UTC
(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
Comment 10 Jacek Ablewicz 2016-04-13 12:06:30 UTC Comment hidden (obsolete)
Comment 11 Jesse Weaver 2016-04-21 20:24:25 UTC
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.
Comment 12 Jacek Ablewicz 2016-04-26 07:03:10 UTC Comment hidden (obsolete)
Comment 13 Jacek Ablewicz 2016-04-26 07:08:28 UTC Comment hidden (obsolete)
Comment 14 Jacek Ablewicz 2016-04-27 12:31:45 UTC
(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.
Comment 15 Jacek Ablewicz 2016-04-27 12:39:55 UTC
(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.
Comment 16 Jacek Ablewicz 2016-05-25 09:44:35 UTC Comment hidden (obsolete)
Comment 17 Jacek Ablewicz 2016-05-25 09:44:42 UTC Comment hidden (obsolete)
Comment 18 Jacek Ablewicz 2016-05-25 09:44:47 UTC Comment hidden (obsolete)
Comment 19 Jesse Weaver 2016-06-10 15:46:13 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2016-06-12 07:50:09 UTC
Comment on attachment 52246 [details] [review]
Bug 16166: Proposed followup to use Sereal for serialization

Moved to bug 16715
Comment 21 Jonathan Druart 2016-06-12 08:05:58 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2016-06-12 08:06:05 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2016-06-12 08:08:34 UTC
See bug 16715 for more information about perfs gain.
Comment 24 Jonathan Druart 2016-06-18 12:50:14 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2016-06-18 12:50:18 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2016-06-18 12:51:05 UTC
Forgot to add my signed-off-by lines!
Comment 27 Kyle M Hall 2016-06-24 18:48:27 UTC
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>
Comment 28 Kyle M Hall 2016-06-24 18:48:34 UTC
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>
Comment 29 Kyle M Hall 2016-07-08 14:17:32 UTC
Pushed to master for 16.11, thanks Jacek!