New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
optimizing Perl_do_kv and Perl_magic_scalarpack, tied HV SCALAR method strangeness #13631
Comments
From @bulk88Created by @bulk88I ran across some strange looking code so I investigated. The SPAGAINs " EXTEND(SP, HvUSEDKEYS(keys) * (dokeys + dovalues));" But it DOESNT extend the stack enough to avoid the X being called in "our %Config = %Config::Config;" in ExtUtils\MakeMaker\Config.pm . I tried doing #############5.12 But %Config clearly doesn't have 1 key in it. Config_heavy.pl does not So is there a way to fix " EXTEND(SP, HvUSEDKEYS(keys) * (dokeys + Also Perl_magic_scalarpack seems to do the same stash lookup twice for ------------------------------------------------------------- PERL_ARGS_ASSERT_MAGIC_SCALARPACK; if (!gv_fetchmethod_autoload(pkg, "SCALAR", FALSE)) /* there is a SCALAR method that we can call */ I did testing with the following patch. That is how I know the SPAGAIN Perl Info
|
From @bulk880001-Perl_do_kv-hacking.patchFrom 93b9a6af5395469274297415f4e135b326c08d89 Mon Sep 17 00:00:00 2001
From: bulk88 <bulk88@hotmail.com>
Date: Thu, 27 Feb 2014 21:43:47 -0500
Subject: [PATCH] Perl_do_kv hacking
---
doop.c | 9 ++++++++-
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/doop.c b/doop.c
index 5031af8..2a908d3 100644
--- a/doop.c
+++ b/doop.c
@@ -1230,6 +1230,7 @@ Perl_do_kv(pTHX)
dSP;
HV * const keys = MUTABLE_HV(POPs);
HE *entry;
+ SV ** sprealloc;
const I32 gimme = GIMME_V;
const I32 dokv = (PL_op->op_type == OP_RV2HV || PL_op->op_type == OP_PADHV);
/* op_type is OP_RKEYS/OP_RVALUES if pp_rkeys delegated to here */
@@ -1268,14 +1269,18 @@ Perl_do_kv(pTHX)
EXTEND(SP, HvUSEDKEYS(keys) * (dokeys + dovalues));
PUTBACK; /* hv_iternext and hv_iterval might clobber stack_sp */
- while ((entry = hv_iternext(keys))) {
+ while ((sprealloc = SP), (entry = hv_iternext(keys))) {
SPAGAIN;
+ if(SP != sprealloc) DebugBreak();
if (dokeys) {
SV* const sv = hv_iterkeysv(entry);
+ // "our %Config = %Config::Config;" in ExtUtils\MakeMaker\Config.pm triggers this
+ //if(PL_stack_max - SP < (SSize_t)(1)) DebugBreak();//op/magic.t porting/cmp_version.t porting/utils.t files to run to trigger
XPUSHs(sv); /* won't clobber stack_sp */
}
if (dovalues) {
SV *tmpstr;
+ sprealloc = SP;
PUTBACK;
tmpstr = hv_iterval(keys,entry);
DEBUG_H(Perl_sv_setpvf(aTHX_ tmpstr, "%lu%%%d=%lu",
@@ -1283,6 +1288,8 @@ Perl_do_kv(pTHX)
(int)HvMAX(keys)+1,
(unsigned long)(HeHASH(entry) & HvMAX(keys))));
SPAGAIN;
+ if(SP != sprealloc) DebugBreak();
+ //if(PL_stack_max - SP < (SSize_t)(1)) DebugBreak(); //op/magic.t porting/cmp_version.t porting/utils.t
XPUSHs(tmpstr);
}
PUTBACK;
--
1.7.9.msysgit.0
|
From @iabynOn Thu, Feb 27, 2014 at 06:50:51PM -0800, bulk88 wrote:
I think the behaviour is fine the way it is. For the non-tied case, Calling SCALAR to allow pre-extending would be inappropriate. -- |
The RT System itself - Status changed from 'new' to 'open' |
From @iabynOn Thu, Feb 27, 2014 at 06:50:51PM -0800, bulk88 wrote:
Presumably because the usual API for magic calls, Perl_magic_methcall() -- |
From @bulk88On Fri Feb 28 03:48:40 2014, davem wrote:
Ok, removing the per key EXTEND is impossible. I attached a WIP patch of my cleanup of Perl_do_kv. 1 small issue is, there is a feature I accidentally put in. Read the comment with XXX. IDK whether to delete it without a trace, leave it as comments, or put the ASSUME. Since it might not be obvious to the next person this provision/feature exists. Or this feature makes no sense to remove it and assert it? -- |
From @bulk880001-WIP-Perl_do_kv-refactoring.patchFrom eaa4efa21258323fe01a1187b094e7a22c56dde3 Mon Sep 17 00:00:00 2001
From: bulk88 <bulk88@hotmail.com>
Date: Sun, 9 Mar 2014 16:30:12 -0400
Subject: [PATCH] WIP Perl_do_kv refactoring
-move GIMME_V closet to first and last use
-if G_VOID, don't calculate the type of do, just return
-in G_SCALAR, factor 2 SP++s out into 2, use SETs later on
-move dTARGET closer to first use to save registers, targ is not saved
across any function calls now
-in G_ARRAY, simplify op_type to do type conversion, "dokv ||" logic
looked less than ideal for perf
-unknown op_type gets NOT_REACHED, a smoke by me shows it wasn't reached
so the 6 op_types are the whole list of what will call this
-do_kind constants are 1 char for compact machine code encoding, a 2 byte
litteral in C is often a 4 byte litteral in various machine code b/c
16 bit operands aren't implemented
-the 2 "X" on XPUSH are factored out into 1 EXTEND, the overhead of
choosing 1 vs 2 isn't worth it, because the real extend size is much
larger in Perl_stack_grow
-remove the SPAGAIN/PUTBACK which dates from commit 463ee0b2ac alpha 4,
isn't needed since magic nowadays swaps perl stacks
On my Win32 VC 2003 x86-32 machine code size decreased from 0x282 to 0x1EB
with this patch.
---
doop.c | 89 +++++++++++++++++++++++++++++++++++++++------------------------
1 files changed, 55 insertions(+), 34 deletions(-)
diff --git a/doop.c b/doop.c
index 5031af8..869bfff 100644
--- a/doop.c
+++ b/doop.c
@@ -1228,31 +1228,25 @@ Perl_do_kv(pTHX)
{
dVAR;
dSP;
+ I32 gimme;
HV * const keys = MUTABLE_HV(POPs);
- HE *entry;
- const I32 gimme = GIMME_V;
- const I32 dokv = (PL_op->op_type == OP_RV2HV || PL_op->op_type == OP_PADHV);
- /* op_type is OP_RKEYS/OP_RVALUES if pp_rkeys delegated to here */
- const I32 dokeys = dokv || (PL_op->op_type == OP_KEYS || PL_op->op_type == OP_RKEYS);
- const I32 dovalues = dokv || (PL_op->op_type == OP_VALUES || PL_op->op_type == OP_RVALUES);
-
(void)hv_iterinit(keys); /* always reset iterator regardless */
+ gimme = GIMME_V;
if (gimme == G_VOID)
RETURN;
if (gimme == G_SCALAR) {
+ SP++; /* part of SETs */
if (PL_op->op_flags & OPf_MOD || LVRET) { /* lvalue */
SV * const ret = sv_2mortal(newSV_type(SVt_PVLV)); /* Not TARG RT#67838 */
sv_magic(ret, NULL, PERL_MAGIC_nkeys, NULL, 0);
LvTYPE(ret) = 'k';
LvTARG(ret) = SvREFCNT_inc_simple(keys);
- PUSHs(ret);
+ SETs(ret);
}
else {
IV i;
- dTARGET;
-
if (! SvTIED_mg((const SV *)keys, PERL_MAGIC_tied) ) {
i = HvUSEDKEYS(keys);
}
@@ -1260,34 +1254,61 @@ Perl_do_kv(pTHX)
i = 0;
while (hv_iternext(keys)) i++;
}
- PUSHi( i );
+ {
+ dTARGET;
+ SETs(targ);
+ sv_setiv_mg(targ , i);
+ }
}
- RETURN;
}
-
- EXTEND(SP, HvUSEDKEYS(keys) * (dokeys + dovalues));
-
- PUTBACK; /* hv_iternext and hv_iterval might clobber stack_sp */
- while ((entry = hv_iternext(keys))) {
- SPAGAIN;
- if (dokeys) {
- SV* const sv = hv_iterkeysv(entry);
- XPUSHs(sv); /* won't clobber stack_sp */
+ else {
+/* A = G_ARRAY */
+#define DOKVA_KEYS 0x1
+#define DOKVA_VALUES 0x2
+ HE *entry;
+ /* op_type is OP_RKEYS/OP_RVALUES if pp_rkeys delegated to here */
+ const I32 usedkeys = HvUSEDKEYS(keys);
+ /* do_kind, low nibble is bf, high nibble is extend multiplier,
+ times 3 on stack extend might cause out of memory error in rare case
+ */
+ U8 do_kind = (PL_op->op_type == OP_RV2HV || PL_op->op_type == OP_PADHV)
+ ? 2<<4|DOKVA_KEYS|DOKVA_VALUES
+ : (PL_op->op_type == OP_KEYS || PL_op->op_type == OP_RKEYS)
+ ? 1<<4|DOKVA_KEYS
+ :(PL_op->op_type == OP_VALUES || PL_op->op_type == OP_RVALUES)
+ ? 1<<4|DOKVA_VALUES : (NOT_REACHED,0);
+ {
+ const U32 extend_mul = do_kind & 0xf0;
+ do_kind ^= extend_mul; /* remove extend_mul nibble from bf */
+ EXTEND(SP, usedkeys * extend_mul);
}
- if (dovalues) {
- SV *tmpstr;
- PUTBACK;
- tmpstr = hv_iterval(keys,entry);
- DEBUG_H(Perl_sv_setpvf(aTHX_ tmpstr, "%lu%%%d=%lu",
- (unsigned long)HeHASH(entry),
- (int)HvMAX(keys)+1,
- (unsigned long)(HeHASH(entry) & HvMAX(keys))));
- SPAGAIN;
- XPUSHs(tmpstr);
+
+ while (entry = hv_iternext(keys)) {
+/* XXX remove or comment the next line out, since this "feature" doesn't exist
+ because do_kind can never be set to 0, because branch 0 is NOT_REACHED, or
+ ASSUME(do_kind) it to neutralize the branch, if the code is found to be
+ needed (DEBUGGING failed), then ASSUME can be removed ???? */
+ if(do_kind) {
+ EXTEND(SP,2); /* overextend by 1 sometimes won't hurt */
+ if (do_kind & DOKVA_KEYS) {
+ SV* const sv = hv_iterkeysv(entry);
+ PUSHs(sv);
+ }
+ if (do_kind & DOKVA_VALUES) {
+ SV *tmpstr;
+ tmpstr = hv_iterval(keys,entry);
+ DEBUG_H(Perl_sv_setpvf(aTHX_ tmpstr, "%lu%%%d=%lu",
+ (unsigned long)HeHASH(entry),
+ (int)HvMAX(keys)+1,
+ (unsigned long)(HeHASH(entry) & HvMAX(keys))));
+ PUSHs(tmpstr);
+ }
+ }
}
- PUTBACK;
- }
- return NORMAL;
+#undef DOKVA_KEYS
+#undef DOKVA_VALUES
+ } /* else/G_ARRAY */
+ RETURN;
}
/*
--
1.7.9.msysgit.0
|
From @iabynOn Sun, Mar 09, 2014 at 01:43:00PM -0700, bulk88 via RT wrote:
Given that it makes no sense for Perl_do_kv() to be called without Encoding the multiplier in the high nibble seems a bit tricksy. Couldn't EXTEND(SP, usedkeys * (1 + (do_kind == (DOKVA_KEYS|DOKVA_VALUES))); -- |
From @bulk88On Mon Mar 10 06:44:20 2014, davem wrote:
New final patch attached. -- |
From @bulk88On Fri Mar 14 12:11:56 2014, bulk88 wrote:
Forgot attachment. -- |
From @bulk880001-Perl_do_kv-refactoring.patchFrom c0f0b1497217bd7221bd4f915b41b8005b60bf5d Mon Sep 17 00:00:00 2001
From: bulk88 <bulk88@hotmail.com>
Date: Fri, 14 Mar 2014 14:53:13 -0400
Subject: [PATCH] Perl_do_kv refactoring
-move GIMME_V closet to first and last use
-if G_VOID, don't calculate the type of do, just return
-in G_SCALAR, factor 2 SP++s out into 1, use SETs later on
-move dTARGET closer to first use to save registers, targ is not saved
across any function calls now
-in G_ARRAY, simplify op_type to do type conversion, "dokv ||" logic
looked less than ideal for perf
-unknown op_type gets NOT_REACHED, a smoke by me shows it wasn't reached
so the 6 op_types are the whole list of what will call this
-do_kind constants are 1 char for compact machine code encoding
-the 2 "X" on XPUSH are factored out into 1 EXTEND, the overhead of
choosing 1 vs 2 isn't worth it, because the real extend size is much
larger in Perl_stack_grow
-remove the SPAGAIN/PUTBACK which dates from commit 463ee0b2ac alpha 4,
isn't needed since magic nowadays swaps perl stacks
On my Win32 VC 2003 x86-32 machine code size decreased from 0x282 to 0x1FD
with this patch.
---
doop.c | 80 ++++++++++++++++++++++++++++++++++++----------------------------
1 files changed, 45 insertions(+), 35 deletions(-)
diff --git a/doop.c b/doop.c
index 5031af8..f4c1474 100644
--- a/doop.c
+++ b/doop.c
@@ -1228,31 +1228,25 @@ Perl_do_kv(pTHX)
{
dVAR;
dSP;
+ I32 gimme;
HV * const keys = MUTABLE_HV(POPs);
- HE *entry;
- const I32 gimme = GIMME_V;
- const I32 dokv = (PL_op->op_type == OP_RV2HV || PL_op->op_type == OP_PADHV);
- /* op_type is OP_RKEYS/OP_RVALUES if pp_rkeys delegated to here */
- const I32 dokeys = dokv || (PL_op->op_type == OP_KEYS || PL_op->op_type == OP_RKEYS);
- const I32 dovalues = dokv || (PL_op->op_type == OP_VALUES || PL_op->op_type == OP_RVALUES);
-
(void)hv_iterinit(keys); /* always reset iterator regardless */
+ gimme = GIMME_V;
if (gimme == G_VOID)
RETURN;
if (gimme == G_SCALAR) {
+ SP++; /* part of SETs */
if (PL_op->op_flags & OPf_MOD || LVRET) { /* lvalue */
SV * const ret = sv_2mortal(newSV_type(SVt_PVLV)); /* Not TARG RT#67838 */
sv_magic(ret, NULL, PERL_MAGIC_nkeys, NULL, 0);
LvTYPE(ret) = 'k';
LvTARG(ret) = SvREFCNT_inc_simple(keys);
- PUSHs(ret);
+ SETs(ret);
}
else {
IV i;
- dTARGET;
-
if (! SvTIED_mg((const SV *)keys, PERL_MAGIC_tied) ) {
i = HvUSEDKEYS(keys);
}
@@ -1260,34 +1254,50 @@ Perl_do_kv(pTHX)
i = 0;
while (hv_iternext(keys)) i++;
}
- PUSHi( i );
+ {
+ dTARGET;
+ SETs(targ);
+ sv_setiv_mg(targ , i);
+ }
}
- RETURN;
}
-
- EXTEND(SP, HvUSEDKEYS(keys) * (dokeys + dovalues));
-
- PUTBACK; /* hv_iternext and hv_iterval might clobber stack_sp */
- while ((entry = hv_iternext(keys))) {
- SPAGAIN;
- if (dokeys) {
- SV* const sv = hv_iterkeysv(entry);
- XPUSHs(sv); /* won't clobber stack_sp */
- }
- if (dovalues) {
- SV *tmpstr;
- PUTBACK;
- tmpstr = hv_iterval(keys,entry);
- DEBUG_H(Perl_sv_setpvf(aTHX_ tmpstr, "%lu%%%d=%lu",
- (unsigned long)HeHASH(entry),
- (int)HvMAX(keys)+1,
- (unsigned long)(HeHASH(entry) & HvMAX(keys))));
- SPAGAIN;
- XPUSHs(tmpstr);
+ else {
+/* A = G_ARRAY */
+#define DOKVA_KEYS 0x1
+#define DOKVA_VALUES 0x2
+ HE *entry;
+ /* op_type is OP_RKEYS/OP_RVALUES if pp_rkeys delegated to here */
+ const I32 usedkeys = HvUSEDKEYS(keys);
+ U8 do_kind = (PL_op->op_type == OP_RV2HV || PL_op->op_type == OP_PADHV)
+ ? DOKVA_KEYS|DOKVA_VALUES
+ : (PL_op->op_type == OP_KEYS || PL_op->op_type == OP_RKEYS)
+ ? DOKVA_KEYS
+ :(PL_op->op_type == OP_VALUES || PL_op->op_type == OP_RVALUES)
+ ? DOKVA_VALUES : (NOT_REACHED,0);
+ ASSUME(do_kind != 0);
+ EXTEND(SP, usedkeys * (1+(do_kind == (DOKVA_KEYS|DOKVA_VALUES))));
+
+ while (entry = hv_iternext(keys)) {
+ ASSUME(do_kind != 0);
+ EXTEND(SP,2); /* overextend by 1 sometimes won't hurt */
+ if (do_kind & DOKVA_KEYS) {
+ SV* const sv = hv_iterkeysv(entry);
+ PUSHs(sv);
+ }
+ if (do_kind & DOKVA_VALUES) {
+ SV *tmpstr;
+ tmpstr = hv_iterval(keys,entry);
+ DEBUG_H(Perl_sv_setpvf(aTHX_ tmpstr, "%lu%%%d=%lu",
+ (unsigned long)HeHASH(entry),
+ (int)HvMAX(keys)+1,
+ (unsigned long)(HeHASH(entry) & HvMAX(keys))));
+ PUSHs(tmpstr);
+ }
}
- PUTBACK;
- }
- return NORMAL;
+#undef DOKVA_KEYS
+#undef DOKVA_VALUES
+ } /* else/G_ARRAY */
+ RETURN;
}
/*
--
1.7.9.msysgit.0
|
From @bulk88On Fri Mar 14 12:12:28 2014, bulk88 wrote:
Bump. -- |
From @tonycozOn Fri Mar 14 12:12:28 2014, bulk88 wrote:
+ U8 do_kind = (PL_op->op_type == OP_RV2HV || PL_op->op_type == OP_PADHV) I don't think I'd call that simplified. Just moving it and using bool for the do* variables would be preferable.
The ASSUME() you've added will assert on this for DEBUGGING builds.
I32 is the devil, this should be STRLEN (or maybe SSize_t). HvUSEDKEYS() is the difference between
Why ASSUME(do_kind != 0); twice? Tony |
@bulk88 This patch no longer applies. If you are interested in pursuing it, can you please open a PR for further discussion? |
Migrated from rt.perl.org#121348 (status was 'open')
Searchable as RT121348$
The text was updated successfully, but these errors were encountered: