Bug 16221

Summary: Use Storable::dclone() instead of Clone::clone() for L1 cache deep-copying mode
Product: Koha Reporter: Jacek Ablewicz <abl>
Component: Architecture, internals, and plumbingAssignee: Jacek Ablewicz <abl>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P2 CC: jonathan.druart, julian.maurice, kyle, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16044
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16166
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16140
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 16229    
Bug Blocks: 15342, 16166    
Attachments: Bug 16221: Use Storable::dclone() instead of Clone::clone() for L1 cache deep-copying mode
Bug 16221: follow-up for changes made by bug 16229
Bug 16221: Use Storable::dclone() instead of Clone::clone() for L1 cache deep-copying mode
Bug 16221: follow-up for changes made by bug 16229
Bug 16221: Use Storable::dclone() instead of Clone::clone() for L1 cache deep-copying mode
Bug 16221: follow-up for changes made by bug 16229
Bug 16221: Use Storable::dclone() instead of Clone::clone() for L1 cache deep-copying mode
Bug 16221: follow-up for changes made by bug 16229

Description Jacek Ablewicz 2016-04-07 08:26:22 UTC
Storable dclone() is over 3x faster then Clone::clone() when used for creating a deep copies of the big, complex data structures like MARC frameworks. See also Bug 16044 comments #18 & #20.

This is a trivial version of Bug 16166. Performance test results (see Bug 16140 comment #23 for test setup description):

# master
146.29 (14.64+14.53+14.51+14.57+14.72+14.56+14.53+15.06+14.56+14.61)

# master + patch from this bug report
96.5 (9.77+9.63+9.77+9.77+9.68+9.05+9.68+9.83+9.63+9.69)

# master + Bug 16166
90.7 (9.15+9.10+9.16+8.41+9.19+9.19+9.11+9.19+9.02+9.18)

Bug 16166 is a bit faster and more comprehensive implementation, but also more complex and harder to test/evaluate.
Comment 1 Jacek Ablewicz 2016-04-07 08:34:17 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-04-07 09:46:41 UTC
Jacek,
Bug 16166 looks fine by me. Are you suggesting to drop it?
Comment 3 Jacek Ablewicz 2016-04-08 10:46:50 UTC
(In reply to Jonathan Druart from comment #2)
> Jacek,
> Bug 16166 looks fine by me. Are you suggesting to drop it?

No, not necessarily anyway.

I just thought that it would be interesting to find out what are the exact causes of the speed improvements Bug 16166 provides: 1) different cloning method or 2) cache architectural changes. Turns out, both 1) and 2) are beneficial performance-wise, but 1) is much bigger factor.

I still think Bug 16166 may be a better solution, especially in the long term. When applied on top of Bug 16221, it would improve cache speed somehow further, and it provides one additional important feature (separating "safe" and "unsafe" cache fetches, so they don't interfere with each other). Note that after Bug 16044, cache fetches are NOT safe in the current master - even if there are no "unsafe => 1" parameters used anywhere in the code currently, because issue 1) mentioned in Bug 16044 comment #20 wasn't resolved before 16044 got to master (unless I'm very much mistaken ?).

Trouble with Bug 16166 is that it's one of those architectural changes which are pretty much untestable in any conventional way, and it's not easy to predict what kinds of regressions it may introduce (if any) just by looking at that code. Also, Bug 16166 is a bit of a mess right now, I guess I shoud at least squash patches #1 - #3 to make it more readable.

Bug 16221, OTOH, is a trivial follow-up of Bug 16044, it does improve caching performance substantially, and risks that it may cause some regressions are close to nil.
Comment 4 Jacek Ablewicz 2016-04-08 11:30:35 UTC
(In reply to Jacek Ablewicz from comment #3)

> after Bug 16044, cache fetches are NOT safe in the current master -
> even if there are no "unsafe => 1" parameters used anywhere in the code
> currently, because issue 1) mentioned in Bug 16044 comment #20 wasn't
> resolved before 16044 got to master (unless I'm very much mistaken ?).

And, after a closer look at the code from Bug 16044: this may be even a bigger issue, it doesn't affect just set_in_cache() calls. First get_from_cache(given_key) call in the script run is also implicitly unsafe in the current master, never mind if the 'unsafe => 1' parameter was given or not.
Comment 5 Jonathan Druart 2016-04-08 11:58:48 UTC
(In reply to Jacek Ablewicz from comment #4)
> (In reply to Jacek Ablewicz from comment #3)
> 
> > after Bug 16044, cache fetches are NOT safe in the current master -
> > even if there are no "unsafe => 1" parameters used anywhere in the code
> > currently, because issue 1) mentioned in Bug 16044 comment #20 wasn't
> > resolved before 16044 got to master (unless I'm very much mistaken ?).
> 
> And, after a closer look at the code from Bug 16044: this may be even a
> bigger issue, it doesn't affect just set_in_cache() calls. First
> get_from_cache(given_key) call in the script run is also implicitly unsafe
> in the current master, never mind if the 'unsafe => 1' parameter was given
> or not.

I am on it, I will submit a patch soon.
Comment 6 Jonathan Druart 2016-04-08 12:38:00 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2016-04-15 15:44:50 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2016-04-15 15:44:53 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2016-04-15 15:49:21 UTC
decrease from 134 to 64!
Comment 10 Tomás Cohen Arazi 2016-04-15 19:09:19 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2016-04-15 19:09:25 UTC Comment hidden (obsolete)
Comment 12 Jacek Ablewicz 2016-04-18 10:41:36 UTC
Created attachment 50321 [details] [review]
Bug 16221: Use Storable::dclone() instead of Clone::clone() for L1 cache deep-copying mode

Storable dclone() is over 3x faster then Clone::clone() when used for
creating a deep copies of the big, complex data structures like MARC
frameworks. See also Bug 16044 comments #18 & #20.

This is a trivial version of Bug 16166. Performance test
results (see Bug 16140 comment #23 for test setup description):

   master
   146.29 (14.64+14.53+14.51+14.57+14.72+14.56+14.53+15.06+14.56+14.61)

   master + Bug 16221
   96.5 (9.77+9.63+9.77+9.77+9.68+9.05+9.68+9.83+9.63+9.69)

   master + Bug 16166
   90.7 (9.15+9.10+9.16+8.41+9.19+9.19+9.11+9.19+9.02+9.18)

Bug 16166 is a bit faster and more comprehensive implementation, but
also more complex and harder to test/evaluate.

Test plan:

1) apply patch
2) 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.)

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Comment 13 Jacek Ablewicz 2016-04-18 10:41:43 UTC
Created attachment 50322 [details] [review]
Bug 16221: follow-up for changes made by bug 16229

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Comment 14 Jacek Ablewicz 2016-04-18 10:45:38 UTC
Re-added missing performance test cases descriptions, which went AWOL due to having '#' in the front of them.
Comment 15 Kyle M Hall 2016-04-29 10:41:33 UTC
Pushed to master! Will be in the May 2016 Release! Thanks Jacek!
Comment 16 Julian Maurice 2016-06-23 07:42:52 UTC
Patches pushed to 3.22.x, will be in 3.22.8