[PATCH] sv_grow: performance improvement for short strings #14047
Comments
From @rurbanThis is a bug report for perl from rurban@cpanel.net, sv_grow: performance improvement for short strings Rationale: All tests: Flags: Site configuration information for perl 5.21.4: Configured by rurban at Wed Aug 27 11:43:16 CDT 2014. Summary of my perl5 (revision 5 version 21 subversion 4) configuration: Locally applied patches: @INC for perl 5.21.4: Environment for perl 5.21.4: |
From @rurban0001-sv_grow-performance-improvement-for-short-strings.patchFrom 32818149be28edd2a58ea345eea388bf0d3dc329 Mon Sep 17 00:00:00 2001
From: Reini Urban <rurban@x-ray.at>
Date: Wed, 27 Aug 2014 12:48:35 -0500
Subject: [PATCH] sv_grow: performance improvement for short strings
Empty COW strings with CUR=0 ended up allocated as LEN=10.
Now they are rounded up to 4 or 8.
Timings:
+0 16.394324103 0.27%
+2 16.114379842 0.01%
+4 16.305622265 1.03%
+8 16.337438609 1.30%
+10 16.675009468 0.59%
with LD_LIBRARY_PATH=`pwd` perf stat -r2 ./perl t/harness t/op/*.t
+2 was consistently the best number, and +10 the worst.
---
sv.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git sv.c sv.c
index 7c334d0..3c27303 100644
--- sv.c
+++ sv.c
@@ -1553,7 +1553,7 @@ Perl_sv_grow(pTHX_ SV *const sv, STRLEN newlen)
#ifdef PERL_NEW_COPY_ON_WRITE
/* the new COW scheme uses SvPVX(sv)[SvLEN(sv)-1] (if spare)
- * to store the COW count. So in general, allocate one more byte than
+ * to store the CowREFCNT. So in general, allocate one more byte than
* asked for, to make it likely this byte is always spare: and thus
* make more strings COW-able.
* If the new size is a big power of two, don't bother: we assume the
@@ -1569,7 +1569,7 @@ Perl_sv_grow(pTHX_ SV *const sv, STRLEN newlen)
if (newlen > SvLEN(sv)) { /* need more room? */
STRLEN minlen = SvCUR(sv);
- minlen += (minlen >> PERL_STRLEN_EXPAND_SHIFT) + 10;
+ minlen += (minlen >> PERL_STRLEN_EXPAND_SHIFT) + 2;
if (newlen < minlen)
newlen = minlen;
#ifndef PERL_UNWARANTED_CHUMMINESS_WITH_MALLOC
--
2.1.0
|
From @cpansproutOn Wed Aug 27 11:03:24 2014, rurban@cpanel.net wrote:
Thank you. Applied as 880c169. -- Father Chrysostomos |
|
The RT System itself - Status changed from 'new' to 'open' |
|
@cpansprout - Status changed from 'open' to 'pending release' |
|
@khwilliamson - Status changed from 'pending release' to 'open' |
From @khwilliamsonI am reopening this ticket and raising its priority because this patch breaks blead under ASAN. Attached is an output of ASAN on t/op/attrs.t (uni/attrs.t is also broken) 880c169 is the first bad commit sv_grow: performance improvement for short strings :100644 100644 5a77d6bd438396bd06e58ed4685d57eca178541b 2940942fc6333dd91dcd2acb17bbd558171680ee M sv.c -- |
From @khwilliamson |
From @khwilliamsonOn Sat Aug 30 21:11:52 2014, khw wrote:
-- |
From @khwilliamson |
From [Unknown Contact. See original ticket]On Sat Aug 30 21:11:52 2014, khw wrote:
-- |
From @cpansproutOn Sat Aug 30 21:11:52 2014, khw wrote:
How do I translate this into file and line number? 0x60200004dbd5 is located 0 bytes to the right of 5-byte region [0x60200004dbd0,0x60200004dbd5) -- Father Chrysostomos |
From @khwilliamsonofflist, I have no idea, except to google it On 08/31/2014 07:29 AM, Father Chrysostomos via RT wrote:
|
From @demerphqOn 31 August 2014 15:29, Father Chrysostomos via RT <
I think this patch should be reverted. The timing differences were in the noise, and not backed up by robust Specifically, this logic is part of the overallocation logic that made a If this patch is to live the magic number involved should be a compile time Yves -- |
From @cpansproutOn Mon Sep 01 00:14:55 2014, demerphq wrote:
I have reverted it for now in 3c239be. -- Father Chrysostomos |
From @rurbanOn Mon, Sep 1, 2014 at 9:14 AM, demerphq <demerphq@gmail.com> wrote:
set ASAN_SYMBOLIZER I use: READ of size 6 at 0x60200004ec15 thread T0 This is caused by a wrong memcmp in attributes.xs:100
Sorry, I used more robust benchmarking than dumbbench, but on a noisy machine.
Not really. The wrong realloc overallocation logic which caused The current scheme is still very bad, as it does not round properly Discussion with my problem: Perf. regression measured:
No magic numbers. 2 is the space needed for the cow refcount of an -- -- |
From @rurban0001-fix-attributes-memcmp-without-len-6-asan-catch.patchFrom 57c11c30d34df080365281180c0228801f408d89 Mon Sep 17 00:00:00 2001
From: Reini Urban <rurban@x-ray.at>
Date: Mon, 1 Sep 2014 13:50:12 +0200
Subject: [PATCH] fix attributes memcmp without len<6 (asan catch)
also tune the sv_grow overallocation from 10 to 8, to avoid realloc stress
---
ext/attributes/attributes.xs | 2 +-
sv.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git ext/attributes/attributes.xs ext/attributes/attributes.xs
index dbb644d..56e7762 100644
--- ext/attributes/attributes.xs
+++ ext/attributes/attributes.xs
@@ -97,7 +97,7 @@ modify_SV_attributes(pTHX_ SV *sv, SV **retlist, SV **attrlist, int numattrs)
}
break;
default:
- if (memEQs(name, 6, "shared")) {
+ if (len == 6 && memEQs(name, 6, "shared")) {
if (negated)
Perl_croak(aTHX_ "A variable may not be unshared");
SvSHARE(sv);
diff --git sv.c sv.c
index 7d4c964..1c4e9ec 100644
--- sv.c
+++ sv.c
@@ -1534,7 +1534,7 @@ Perl_sv_grow(pTHX_ SV *const sv, STRLEN newlen)
sv_backoff(sv);
s = SvPVX_mutable(sv);
if (newlen > SvLEN(sv))
- newlen += 10 * (newlen - SvCUR(sv)); /* avoid copy each time */
+ newlen += 8 * (newlen - SvCUR(sv)); /* avoid copy each time */
}
else
{
--
2.0.4
|
From @demerphqOn 1 September 2014 09:14, demerphq <demerphq@gmail.com> wrote:
My concern is use cases like this: on systems with a poor implementation of realloc the following program -- |
From @rurbanOn 09/09/2014 10:40 AM, yves orton via RT wrote:
Sure, and I already debunked this claim of yours in my original patch In this patch I only reverted the initial +10 overallocation for short And the benchmarks (with command-line to repro) showed that's a win. Reverting the initial overallocation change was also a win for your Let's do it again with my original ackerman one-liner (with better op LD_LIBRARY_PATH=`pwd` perf stat -r5 ./perl -e'sub f{my($n)=@_;$n==8 and now with c4c61c6: with +2 linear overalloc (and +8 for OOK) without any overalloc at all (as with 5.12) on a sane libc pure-realloc: your sample (without IO): m -s libperl.so blead: with print and recurse 12: (print buffering skews the result too much) testing big string realloc: blead: bigger strings: blead: Now compare that to the overall results testing all t/op/*.t I'd say the original 5.12 version was the best on non-broken realloc -- Working towards a true Modern Perl. |
From @rurban+2overalloc.patchdiff --git sv.c sv.c
index 748d3e6..4b4d890 100644
--- sv.c
+++ sv.c
@@ -1534,7 +1534,7 @@ Perl_sv_grow(pTHX_ SV *const sv, STRLEN newlen)
sv_backoff(sv);
s = SvPVX_mutable(sv);
if (newlen > SvLEN(sv))
- newlen += 10 * (newlen - SvCUR(sv)); /* avoid copy each time */
+ newlen += 8 * (newlen - SvCUR(sv)); /* avoid copy each time */
}
else
{
@@ -1560,7 +1560,7 @@ Perl_sv_grow(pTHX_ SV *const sv, STRLEN newlen)
if (newlen > SvLEN(sv)) { /* need more room? */
STRLEN minlen = SvCUR(sv);
- minlen += (minlen >> PERL_STRLEN_EXPAND_SHIFT) + 10;
+ minlen += (minlen >> PERL_STRLEN_EXPAND_SHIFT) + 2;
if (newlen < minlen)
newlen = minlen;
#ifndef PERL_UNWARANTED_CHUMMINESS_WITH_MALLOC
|
From @rurbanno-overalloc.patchdiff --git sv.c sv.c
index 748d3e6..eabe276 100644
--- sv.c
+++ sv.c
@@ -1534,7 +1534,7 @@ Perl_sv_grow(pTHX_ SV *const sv, STRLEN newlen)
sv_backoff(sv);
s = SvPVX_mutable(sv);
if (newlen > SvLEN(sv))
- newlen += 10 * (newlen - SvCUR(sv)); /* avoid copy each time */
+ newlen += 8 * (newlen - SvCUR(sv)); /* avoid copy each time */
}
else
{
@@ -1555,22 +1555,22 @@ Perl_sv_grow(pTHX_ SV *const sv, STRLEN newlen)
#endif
#if defined(PERL_USE_MALLOC_SIZE) && defined(Perl_safesysmalloc_size)
-#define PERL_UNWARANTED_CHUMMINESS_WITH_MALLOC
+#undef PERL_UNWARANTED_CHUMMINESS_WITH_MALLOC
#endif
if (newlen > SvLEN(sv)) { /* need more room? */
+#if 0
STRLEN minlen = SvCUR(sv);
- minlen += (minlen >> PERL_STRLEN_EXPAND_SHIFT) + 10;
+ minlen += (minlen >> PERL_STRLEN_EXPAND_SHIFT) + 2;
if (newlen < minlen)
newlen = minlen;
-#ifndef PERL_UNWARANTED_CHUMMINESS_WITH_MALLOC
-
+/*#ifndef PERL_UNWARANTED_CHUMMINESS_WITH_MALLOC */
+#endif
/* Don't round up on the first allocation, as odds are pretty good that
* the initial request is accurate as to what is really needed */
if (SvLEN(sv)) {
newlen = PERL_STRLEN_ROUNDUP(newlen);
}
-#endif
if (SvLEN(sv) && s) {
s = (char*)saferealloc(s, newlen);
}
|
From @tonycozOn Mon Sep 01 12:43:17 2014, rurban wrote:
That patch also includes a change unrelated to attributes.xs. I plan to apply the attached subset of that patch. That said, that little change did seem to make a difference in performance on both Linux and darwin, but the problem is without knowing why, we can't tell if it will degrade performance on other operating systems, or even on different versions of the same operating systems. As to your original patch, I got the impression from the "All gone" thread that you'd prefer that we only did our own exponential allocation growth on systems that require it - Win32 in particular. Can you think of a Configure test we could do to detect systems that need that? I tried a simple program: int main() { for (; sz < 1000; ++sz) { for (;sz < 10000; ++sz) { printf("s %lu b %lu\n", (unsigned long)schanges, which produced: Win32: s 1 b 7 With larger sizes the results were a bit less slanted towards Win32 being bad, and Cygwin/Linux being good, eg. for start 1000, second 100000, third 1000000: Win32: s 16 b 17 but I suspect the larger sizes gets into sizes where the implementation uses page mapping rather than memcpy() to move memory around when the block needs to be moved. Tony |
From @tonycoz0001-fix-attributes-memcmp-without-len-6-asan-catch.patchFrom 695ce2ba362b7e35d71f857e4e38581dbab936eb Mon Sep 17 00:00:00 2001
From: Reini Urban <rurban@x-ray.at>
Date: Tue, 7 Oct 2014 15:44:27 +1100
Subject: [PATCH] fix attributes memcmp without len<6 (asan catch)
---
ext/attributes/attributes.pm | 2 +-
ext/attributes/attributes.xs | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/ext/attributes/attributes.pm b/ext/attributes/attributes.pm
index ebca214..6ca9ce5 100644
--- a/ext/attributes/attributes.pm
+++ b/ext/attributes/attributes.pm
@@ -1,6 +1,6 @@
package attributes;
-our $VERSION = 0.23;
+our $VERSION = 0.24;
@EXPORT_OK = qw(get reftype);
@EXPORT = ();
diff --git a/ext/attributes/attributes.xs b/ext/attributes/attributes.xs
index 6b36812..c131734 100644
--- a/ext/attributes/attributes.xs
+++ b/ext/attributes/attributes.xs
@@ -97,7 +97,7 @@ modify_SV_attributes(pTHX_ SV *sv, SV **retlist, SV **attrlist, int numattrs)
}
break;
default:
- if (memEQs(name, len, "shared")) {
+ if (len == 6 && memEQs(name, len, "shared")) {
if (negated)
Perl_croak(aTHX_ "A variable may not be unshared");
SvSHARE(sv);
--
1.7.10.4
|
From @iabynOn Mon, Oct 06, 2014 at 10:48:12PM -0700, Tony Cook via RT wrote:
I'm currently working on looking at the whole sv_grow() and COW and -- |
From @tonycozOn Mon Oct 06 22:48:11 2014, tonyc wrote:
I've applied that subset to blead as ff5314c. Tony |
From @doughera88On Tue, Oct 07, 2014 at 04:32:28PM -0700, Tony Cook via RT wrote:
Sorry I didn't catch this sooner. This should be reverted (though it's -- |
From @steve-m-hayOn Tue Oct 07 17:39:43 2014, doughera wrote:
Now reverted in commit 46fa4d9. |
|
@steve-m-hay - Status changed from 'open' to 'pending release' |
From @khwilliamsonThanks for submitting this ticket The issue should be resolved with the release today of Perl v5.22. If you find that the problem persists, feel free to reopen this ticket -- |
|
@khwilliamson - Status changed from 'pending release' to 'resolved' |
From @rurbanHow is this resolved? You still have the problematic code: if (newlen > SvLEN(sv)) Empty COW strings with CUR=0 ended up allocated as LEN=10. Timings: +2 was consistently the best number, and +10 the worst. |
From @iabynOn Tue, Jun 02, 2015 at 01:20:39PM -0700, Reini Urban via RT wrote:
It was marked "pending release" in Jan, which was automatically upgraded I'll re-open the ticket. I still intend to provide a more general fix for this by probing the -- |
|
@iabyn - Status changed from 'resolved' to 'open' |
From @rurban
Great, thanks. My benchmark showed a bigger slowdown caused by this, but I don’t know exactly why. |
Migrated from rt.perl.org#122629 (status was 'open')
Searchable as RT122629$
The text was updated successfully, but these errors were encountered: