[Bug 16221] New: Use Storable::dclone() instead of Clone::clone() for L1 cache deep-copying mode
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 Bug ID: 16221 Summary: Use Storable::dclone() instead of Clone::clone() for L1 cache deep-copying mode Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: abl@biblos.pk.edu.pl QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 Jacek Ablewicz <ablewicz@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Patch complexity|--- |Trivial patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 --- Comment #1 from Jacek Ablewicz <ablewicz@gmail.com> --- Created attachment 50011 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=50011&action=edit 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.) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- Priority|P5 - low |P2 Assignee|gmcharlt@gmail.com |abl@biblos.pk.edu.pl See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=16044 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=16166 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=16140 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |15342 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15342 [Bug 15342] Performance 3.22 - Omnibus -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 --- Comment #2 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Jacek, Bug 16166 looks fine by me. Are you suggesting to drop it? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 --- Comment #3 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 --- Comment #4 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |16229 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16229 [Bug 16229] Koha::Cache should be on the safer side -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 --- Comment #6 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 50077 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=50077&action=edit Bug 16221: follow-up for changes made by bug 16229 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 Jonathan Druart <jonathan.druart@bugs.koha-community.org> 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=16221 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #50011|0 |1 is obsolete| | Attachment #50077|0 |1 is obsolete| | --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 50280 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=50280&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 50281 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=50281&action=edit Bug 16221: follow-up for changes made by bug 16229 Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- decrease from 134 to 64! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #50280|0 |1 is obsolete| | --- Comment #10 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 50288 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=50288&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #50281|0 |1 is obsolete| | --- Comment #11 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 50289 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=50289&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 Jacek Ablewicz <ablewicz@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #50288|0 |1 is obsolete| | --- Comment #12 from Jacek Ablewicz <ablewicz@gmail.com> --- Created attachment 50321 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=50321&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 Jacek Ablewicz <ablewicz@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #50289|0 |1 is obsolete| | --- Comment #13 from Jacek Ablewicz <ablewicz@gmail.com> --- Created attachment 50322 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=50322&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 --- Comment #14 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- Re-added missing performance test cases descriptions, which went AWOL due to having '#' in the front of them. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 Bug 16221 depends on bug 16229, which changed state. Bug 16229 Summary: Koha::Cache should be on the safe side https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16229 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |16166 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16166 [Bug 16166] Improve L1 cache performance and safety -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |kyle@bywatersolutions.com --- Comment #15 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to master! Will be in the May 2016 Release! Thanks Jacek! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16221 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |julian.maurice@biblibre.com --- Comment #16 from Julian Maurice <julian.maurice@biblibre.com> --- Patches pushed to 3.22.x, will be in 3.22.8 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org