[PATCH] refactor and fix leak in const_av_xsub #13510
Comments
From @bulk88Created by @bulk88This patch might need more refinement, specifically I think the number This ML thread also covers this patch Perl Info |
From @bulk880001-refactor-and-fix-leak-in-const_av_xsub.patchFrom f7b29e4d38e71e3a6df27fc557c161333883d8e8 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Sun, 5 Jan 2014 22:21:04 -0500
Subject: [PATCH] refactor and fix leak in const_av_xsub
-calculating items takes more opcodes and memreads than using the mark
that is already there, we don't care how many incoming args there are
-replace XSRETURN since SP is already 0 ret args, make the function
var ax and ST() free, just goto the PUTBACK and return
-read AvFILLp only once, save it to var
-do only 1 EXTEND call, either with 1, or 1+av's last index, branchfree
-both the Copy needs a SP++ed, PUSHs will also SP++, factor it out, then
then use TOPs in G_SCALAR branch
-use TARG in the G_SCALAR branch, write TARG to Perl stack before any calls
so TARG doesn't use a non-vol reg
-in the original code in G_SCALAR, newSViv was leaked because it wasn't
mortaled
-in the G_ARRAY branch, remove the multiple AvFILLp reads
See also "leak in const_av_xsub?"
Message ID: BLU0-SMTP1979C3A6BA69B12AADC7936DFB40@phx.gbl
---
op.c | 43 +++++++++++++++++++++++++++++++++----------
1 files changed, 33 insertions(+), 10 deletions(-)
diff --git a/op.c b/op.c
index fc5742f..4135eb2 100644
--- a/op.c
+++ b/op.c
@@ -12333,23 +12333,46 @@ const_av_xsub(pTHX_ CV* cv)
dVAR;
dXSARGS;
AV * const av = MUTABLE_AV(XSANY.any_ptr);
- SP -= items;
+ U32 gimme_v;
+ assert((SP - items) == MARK);
+ SP = MARK; /* remove all incoming args, more efficient than items */
assert(av);
#ifndef DEBUGGING
if (!av) {
- XSRETURN(0);
+ goto end;
}
#endif
if (SvRMAGICAL(av))
Perl_croak(aTHX_ "Magical list constants are not supported");
- if (GIMME_V != G_ARRAY) {
- EXTEND(SP, 1);
- ST(0) = newSViv((IV)AvFILLp(av)+1);
- XSRETURN(1);
- }
- EXTEND(SP, AvFILLp(av)+1);
- Copy(AvARRAY(av), &ST(0), AvFILLp(av)+1, SV *);
- XSRETURN(AvFILLp(av)+1);
+ gimme_v = GIMME_V;
+ if(gimme_v != G_VOID) {
+ I32 stack_grow = 1;
+ const I32 avlastindex = AvFILLp(av); /* 0 based */
+ I32 avitems; /* 1 based */
+ stack_grow += -(gimme_v == G_ARRAY) & avlastindex; /* branch free */
+ if(gimme_v == G_ARRAY) assert(stack_grow == AvFILLp(av)+1);
+ EXTEND(SP, stack_grow);
+ SP += 1;
+ avitems = avlastindex+1;
+ if(gimme_v != G_ARRAY) {
+ dXSTARG;
+ SETs(TARG);
+ sv_setiv(TARG, (IV)avitems);
+ assert(ST(0) == TARG);
+ }
+ else {
+ SV ** const dstsp = SP;
+ assert(gimme_v == G_ARRAY);
+ assert(&ST(0) == dstsp);
+ assert((AvFILLp(av)+1) == avlastindex+1);
+ SP += avlastindex;
+ assert(SP == (PL_stack_base + ax + ((AvFILLp(av)+1) - 1)));
+ Copy(AvARRAY(av), dstsp, avitems, SV *);
+ }
+ }
+ end:
+ PUTBACK;
+ return;
}
/*
--
1.7.9.msysgit.0
|
From @cpansproutOn Sun Jan 05 19:35:47 2014, bulk88 wrote:
Thank you for using TARG. As an infrequent XS user, I often forget about it. Though it may make const_av_xsub slightly bigger, it should speed things up overall, since I image dXSTARG is faster than newSV. Please use SSize_t, not I32, for stack and array offsets. (I know there are still I32s here and there for stack offsets. It’s an incremental task.) As for the number of assertions, while I agree there may be too many, it doesn’t actually hurt. There is no general rule for how many to add. Usually I add them when the code feels a bit fragile and they make me feel more comfortable, or if they document the code’s assumptions. I leave it to you.
Pure paranoia. It should never happen (hence the assert), but it is an easy mistake to make. So I chose not to be pedantic about it under non-debugging builds. -- Father Chrysostomos |
|
The RT System itself - Status changed from 'new' to 'open' |
From @tonycozOn Sun Jan 05 19:35:47 2014, bulk88 wrote:
I'm fairly happy with this patch, but I'm not sure that: + I32 stack_grow = 1; is a speed or maintenance win (it is a space win on x86_64 and presumably x86_32). On 32/64-bit x86 this avoids a branch by using the setCC instruction, but then we later check the exact same condition again. Tony |
From @bulk88On Sun Jan 12 14:26:57 2014, tonyc wrote:
Where specifically is it checked again? -- |
From @tonycozOn Fri Jan 17 00:38:41 2014, bulk88 wrote:
+ gimme_v = GIMME_V; It's checked again here: Tony |
From @bulk88On Sun Jan 12 14:26:57 2014, tonyc wrote:
I tried saving the result of gimme_v == G_ARRAY into a C auto and then testing truthfulness of the C auto later, the byte difference was 2-5 more asm bytes and an opcode or 2 more. bool was a byte or 2 more than using I32 for the saved result, since there was a movsx op if I used a bool. At the branch I saw no difference in the asm opcode between testing for literal 0 (saved gimme_v == G_ARRAY) and testing for literal 3 (G_ARRAY). It is a 3 byte cmp op with last byte being a literal byte in both cases. Therefore saving the result of gimme_v == G_ARRAY into a C auto is useless. I slightly cleaned up the asserts in the new patch. -- |
From @bulk880001-refactor-and-fix-leak-in-const_av_xsub.patchFrom 6348869c9b4cf36c158f9b5d9e44a3be63468d2f Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Wed, 22 Jan 2014 04:22:38 -0500
Subject: [PATCH] refactor and fix leak in const_av_xsub
-calculating items takes more opcodes and memreads than using the mark
that is already there, we don't care how many incoming args there are
-replace XSRETURN since SP is already 0 ret args, make the function
var ax and ST() free, just goto the PUTBACK and return
-read AvFILLp only once, save it to var
-do only 1 EXTEND call, either with 1, or 1+av's last index, branchfree
-both the Copy needs a SP++ed, PUSHs will also SP++, factor it out, then
then use TOPs in G_SCALAR branch
-use TARG in the G_SCALAR branch, write TARG to Perl stack before any calls
so TARG doesn't use a non-vol reg
-in the original code in G_SCALAR, newSViv was leaked because it wasn't
mortaled
-in the G_ARRAY branch, remove the multiple AvFILLp reads
See also "leak in const_av_xsub?"
Message ID: BLU0-SMTP1979C3A6BA69B12AADC7936DFB40@phx.gbl
---
op.c | 41 +++++++++++++++++++++++++++++++----------
1 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/op.c b/op.c
index a31e1af..c09c011 100644
--- a/op.c
+++ b/op.c
@@ -12349,23 +12349,44 @@ const_av_xsub(pTHX_ CV* cv)
dVAR;
dXSARGS;
AV * const av = MUTABLE_AV(XSANY.any_ptr);
- SP -= items;
+ U32 gimme_v;
+ assert((SP - items) == MARK);
+ SP = MARK; /* remove all incoming args, more efficient than items */
assert(av);
#ifndef DEBUGGING
if (!av) {
- XSRETURN(0);
+ goto end;
}
#endif
if (SvRMAGICAL(av))
Perl_croak(aTHX_ "Magical list constants are not supported");
- if (GIMME_V != G_ARRAY) {
- EXTEND(SP, 1);
- ST(0) = newSViv((IV)AvFILLp(av)+1);
- XSRETURN(1);
- }
- EXTEND(SP, AvFILLp(av)+1);
- Copy(AvARRAY(av), &ST(0), AvFILLp(av)+1, SV *);
- XSRETURN(AvFILLp(av)+1);
+ gimme_v = GIMME_V;
+ if(gimme_v != G_VOID) {
+ I32 stack_grow = 1;
+ const I32 avlastindex = AvFILLp(av); /* 0 based */
+ I32 avitems; /* 1 based */
+ stack_grow += -(gimme_v == G_ARRAY) & avlastindex; /* branch free */
+ if(gimme_v == G_ARRAY) assert(stack_grow == AvFILLp(av)+1);
+ EXTEND(SP, stack_grow);
+ SP += 1;
+ avitems = avlastindex+1;
+ if(gimme_v != G_ARRAY) {
+ dXSTARG;
+ SETs(TARG);
+ assert(ST(0) == TARG);
+ sv_setiv(TARG, (IV)avitems);
+ }
+ else {
+ SV ** const dstsp = SP;
+ assert(gimme_v == G_ARRAY && &ST(0) == dstsp);
+ SP += avlastindex;
+ assert(SP == (PL_stack_base + ax + ((AvFILLp(av)+1) - 1)));
+ Copy(AvARRAY(av), dstsp, avitems, SV *);
+ }
+ }
+ end:
+ PUTBACK;
+ return;
}
/*
--
1.7.9.msysgit.0
|
From @tonycozOn Wed Jan 22 01:31:11 2014, bulk88 wrote:
Smaller code is nice, but it's not what we generally optimize for. I think (and I welcome dissenting views) that something like the following would be better: static void Differences from yours: - use SSize_t for avitems, xav_fill is now SSize_t - use UNLIKELY so the scalar case is put with the other unlikely code (and optimize for branch predition) - avoid doing the same gimme test twice, which could confuse a reader Tony |
From @bulk88On Tue Feb 11 22:16:17 2014, tonyc wrote:
fine
fine
Now there are 2 EXTENDs which are quite fat (for C) to do, scaling, 4 pushes, comparison, reading interp globals. My code has just 1 EXTEND call, which I like. Since you fixed the leak a while ago, the last patch of mine fails to apply, I updated it to continue to discussion and see how your changed appear ontop, so it isn't the final one. I also made a patch with your changed dropped ontop of my work to see how it looks. I'll incorporating some of your changes later. -- |
From @bulk880001-refactor-and-fix-leak-in-const_av_xsub.patchFrom 33ad0766ba32471307a83e78caeffa3087498692 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Wed, 22 Jan 2014 04:22:38 -0500
Subject: [PATCH 1/2] refactor and fix leak in const_av_xsub
-calculating items takes more opcodes and memreads than using the mark
that is already there, we don't care how many incoming args there are
-replace XSRETURN since SP is already 0 ret args, make the function
var ax and ST() free, just goto the PUTBACK and return
-read AvFILLp only once, save it to var
-do only 1 EXTEND call, either with 1, or 1+av's last index, branchfree
-both the Copy needs a SP++ed, PUSHs will also SP++, factor it out, then
then use TOPs in G_SCALAR branch
-use TARG in the G_SCALAR branch, write TARG to Perl stack before any calls
so TARG doesn't use a non-vol reg
-in the original code in G_SCALAR, newSViv was leaked because it wasn't
mortaled
-in the G_ARRAY branch, remove the multiple AvFILLp reads
See also "leak in const_av_xsub?"
Message ID: BLU0-SMTP1979C3A6BA69B12AADC7936DFB40@phx.gbl
---
op.c | 41 +++++++++++++++++++++++++++++++----------
1 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/op.c b/op.c
index a6488b0..c7b846c 100644
--- a/op.c
+++ b/op.c
@@ -12531,23 +12531,44 @@ const_av_xsub(pTHX_ CV* cv)
dVAR;
dXSARGS;
AV * const av = MUTABLE_AV(XSANY.any_ptr);
- SP -= items;
+ U32 gimme_v;
+ assert((SP - items) == MARK);
+ SP = MARK; /* remove all incoming args, more efficient than items */
assert(av);
#ifndef DEBUGGING
if (!av) {
- XSRETURN(0);
+ goto end;
}
#endif
if (SvRMAGICAL(av))
Perl_croak(aTHX_ "Magical list constants are not supported");
- if (GIMME_V != G_ARRAY) {
- EXTEND(SP, 1);
- ST(0) = sv_2mortal(newSViv((IV)AvFILLp(av)+1));
- XSRETURN(1);
- }
- EXTEND(SP, AvFILLp(av)+1);
- Copy(AvARRAY(av), &ST(0), AvFILLp(av)+1, SV *);
- XSRETURN(AvFILLp(av)+1);
+ gimme_v = GIMME_V;
+ if(gimme_v != G_VOID) {
+ I32 stack_grow = 1;
+ const I32 avlastindex = AvFILLp(av); /* 0 based */
+ I32 avitems; /* 1 based */
+ stack_grow += -(gimme_v == G_ARRAY) & avlastindex; /* branch free */
+ if(gimme_v == G_ARRAY) assert(stack_grow == AvFILLp(av)+1);
+ EXTEND(SP, stack_grow);
+ SP += 1;
+ avitems = avlastindex+1;
+ if(gimme_v != G_ARRAY) {
+ dXSTARG;
+ SETs(TARG);
+ assert(ST(0) == TARG);
+ sv_setiv(TARG, (IV)avitems);
+ }
+ else {
+ SV ** const dstsp = SP;
+ assert(gimme_v == G_ARRAY && &ST(0) == dstsp);
+ SP += avlastindex;
+ assert(SP == (PL_stack_base + ax + ((AvFILLp(av)+1) - 1)));
+ Copy(AvARRAY(av), dstsp, avitems, SV *);
+ }
+ }
+ end:
+ PUTBACK;
+ return;
}
/*
--
1.7.9.msysgit.0
|
From @bulk880002-B88-2-TC.patchFrom f98823ff1ddc21557404a8eef22de0cf80d67f26 Mon Sep 17 00:00:00 2001
From: bulk88 <bulk88@hotmail.com>
Date: Mon, 10 Mar 2014 01:19:44 -0400
Subject: [PATCH 2/2] B88 2 TC
---
op.c | 21 ++++++++-------------
1 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/op.c b/op.c
index c7b846c..3ac573f 100644
--- a/op.c
+++ b/op.c
@@ -12531,9 +12531,8 @@ const_av_xsub(pTHX_ CV* cv)
dVAR;
dXSARGS;
AV * const av = MUTABLE_AV(XSANY.any_ptr);
+ SP = MARK;
U32 gimme_v;
- assert((SP - items) == MARK);
- SP = MARK; /* remove all incoming args, more efficient than items */
assert(av);
#ifndef DEBUGGING
if (!av) {
@@ -12543,17 +12542,12 @@ const_av_xsub(pTHX_ CV* cv)
if (SvRMAGICAL(av))
Perl_croak(aTHX_ "Magical list constants are not supported");
gimme_v = GIMME_V;
- if(gimme_v != G_VOID) {
- I32 stack_grow = 1;
- const I32 avlastindex = AvFILLp(av); /* 0 based */
- I32 avitems; /* 1 based */
- stack_grow += -(gimme_v == G_ARRAY) & avlastindex; /* branch free */
- if(gimme_v == G_ARRAY) assert(stack_grow == AvFILLp(av)+1);
- EXTEND(SP, stack_grow);
- SP += 1;
- avitems = avlastindex+1;
- if(gimme_v != G_ARRAY) {
+ if (LIKELY(gimme_v != G_VOID)) {
+ const SSize_t avitems = AvFILLp(av)+1;
+ if (UNLIKELY(gimme_v != G_ARRAY)) {
dXSTARG;
+ EXTEND(SP, 1);
+ SP += 1;
SETs(TARG);
assert(ST(0) == TARG);
sv_setiv(TARG, (IV)avitems);
@@ -12561,7 +12555,8 @@ const_av_xsub(pTHX_ CV* cv)
else {
SV ** const dstsp = SP;
assert(gimme_v == G_ARRAY && &ST(0) == dstsp);
- SP += avlastindex;
+ EXTEND(SP, avitems)
+ SP += avitems;
assert(SP == (PL_stack_base + ax + ((AvFILLp(av)+1) - 1)));
Copy(AvARRAY(av), dstsp, avitems, SV *);
}
--
1.7.9.msysgit.0
|
From @bulk88New patch attached with changes from Tonyc. -- |
From @bulk880001-refactor-const_av_xsub.patchFrom 0341825153c48747531787daea8c27dfa3cd4a37 Mon Sep 17 00:00:00 2001
From: bulk88 <bulk88@hotmail.com>
Date: Wed, 19 Mar 2014 17:11:31 -0400
Subject: [PATCH] refactor const_av_xsub
-calculating items takes more opcodes and memreads than using the mark
that is already there, we don't care how many incoming args there are
-replace XSRETURN since SP is already 0 ret args, make the function
var ax and ST() free, just goto the PUTBACK and return
-read AvFILLp only once, save it to var
-do only 1 EXTEND call, either with 1, or 1+av's last index, branchfree
-both the Copy needs a SP++ed, PUSHs will also SP++, factor it out, then
then use TOPs in G_SCALAR branch
-use TARG in the G_SCALAR branch
-in the G_ARRAY branch, remove the multiple AvFILLp reads
See also "leak in const_av_xsub?"
Message ID: BLU0-SMTP1979C3A6BA69B12AADC7936DFB40@phx.gbl and [#120939]
---
op.c | 41 +++++++++++++++++++++++++++++++----------
1 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/op.c b/op.c
index a6488b0..4b689a3 100644
--- a/op.c
+++ b/op.c
@@ -12531,23 +12531,44 @@ const_av_xsub(pTHX_ CV* cv)
dVAR;
dXSARGS;
AV * const av = MUTABLE_AV(XSANY.any_ptr);
- SP -= items;
+ U32 gimme_v;
+ assert((SP - items) == MARK);
+ SP = MARK; /* remove all incoming args, more efficient than items */
assert(av);
#ifndef DEBUGGING
if (!av) {
- XSRETURN(0);
+ goto end;
}
#endif
if (SvRMAGICAL(av))
Perl_croak(aTHX_ "Magical list constants are not supported");
- if (GIMME_V != G_ARRAY) {
- EXTEND(SP, 1);
- ST(0) = sv_2mortal(newSViv((IV)AvFILLp(av)+1));
- XSRETURN(1);
- }
- EXTEND(SP, AvFILLp(av)+1);
- Copy(AvARRAY(av), &ST(0), AvFILLp(av)+1, SV *);
- XSRETURN(AvFILLp(av)+1);
+ gimme_v = GIMME_V;
+ if (LIKELY(gimme_v != G_VOID)) {
+ const SSize_t avlastindex = AvFILLp(av); /* 0 based */
+ SSize_t avitems; /* 1 based */
+ const SSize_t stack_grow = 1 + (-(gimme_v == G_ARRAY) & avlastindex); /* branch free */
+ if(gimme_v == G_ARRAY) assert(stack_grow == AvFILLp(av)+1);
+ EXTEND(SP, stack_grow);
+ SP += 1;
+ avitems = avlastindex+1;
+ if (UNLIKELY(gimme_v != G_ARRAY)) {
+ dXSTARG;
+ SETs(TARG);
+ assert(gimme_v == G_SCALAR && ST(0) == TARG);
+ sv_setiv(TARG, (IV)avitems);
+ SvSETMAGIC(TARG);
+ }
+ else {
+ SV ** const dstsp = SP;
+ assert(gimme_v == G_ARRAY && &ST(0) == dstsp);
+ SP += avlastindex;
+ assert(SP == (PL_stack_base + ax + ((AvFILLp(av)+1) - 1)));
+ Copy(AvARRAY(av), dstsp, avitems, SV *);
+ }
+ }
+ end:
+ PUTBACK;
+ return;
}
/*
--
1.7.9.msysgit.0
|
From @tonycozOn Sun Mar 09 22:28:10 2014, bulk88 wrote:
The perl implementation strikes the speed vs size balance heavily on the speed side - using macros and inline functions which produce faster but significantly larger code. So I think adding code like: const SSize_t stack_grow = 1 + (-(gimme_v == G_ARRAY) & avlastindex); to save a few bytes from the extra EXTEND() expansion (while slighly slowing it down) doesn't match how perl's implementation has balanced speed vs size.
Yes, I wanted to make sure the fix at least made it into 5.20. Tony |
From @bulk88On Sun Mar 23 16:38:17 2014, tonyc wrote:
Which slows you down when you have a 1 KB in the machine code function to execute. Or 25 calls to hv_common_key() when someone put it inside SvTRUE.
That is trading size for speed. I can write that as a traditional if() {} if you want me to. It will be less machine code, but then its a branch/conditional jump (slower). -- |
From @tonycozOn Thu Apr 03 23:57:57 2014, bulk88 wrote:
In this specific case, on a compiler we support UNLIKELY() with, the unlikely code will be moved out of the flow of execution and will have little to no effect on speed.
That won't help enough for me to accept this patch. BTW: if(gimme_v == G_ARRAY) assert(stack_grow == AvFILLp(av)+1); should probably be: assert(gimme_v != G_ARRAY || stack_grow == AvFILLp(av)+1); The first causes a compiler warning on non-DEBUGGING builds about the empty if() body. The code also introduces an unused variable warning for items on non-DEBUGGING builds. Tony |
|
The leak itself was fixed in d06ddc7. |
Migrated from rt.perl.org#120939 (status was 'open')
Searchable as RT120939$
The text was updated successfully, but these errors were encountered: