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.
Created attachment 50011 [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.)
Jacek, Bug 16166 looks fine by me. Are you suggesting to drop it?
(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.
(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.
(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.
Created attachment 50077 [details] [review] Bug 16221: follow-up for changes made by bug 16229
Created attachment 50280 [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): 146.29 (14.64+14.53+14.51+14.57+14.72+14.56+14.53+15.06+14.56+14.61) 96.5 (9.77+9.63+9.77+9.77+9.68+9.05+9.68+9.83+9.63+9.69) 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>
Created attachment 50281 [details] [review] Bug 16221: follow-up for changes made by bug 16229 Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
decrease from 134 to 64!
Created attachment 50288 [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): 146.29 (14.64+14.53+14.51+14.57+14.72+14.56+14.53+15.06+14.56+14.61) 96.5 (9.77+9.63+9.77+9.77+9.68+9.05+9.68+9.83+9.63+9.69) 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>
Created attachment 50289 [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>
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>
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>
Re-added missing performance test cases descriptions, which went AWOL due to having '#' in the front of them.
Pushed to master! Will be in the May 2016 Release! Thanks Jacek!
Patches pushed to 3.22.x, will be in 3.22.8