Closed p5pRT closed 4 years ago
I ran across some strange looking code so I investigated. The SPAGAINs and PUTBACKs can be removed since magic calls now swap stacks. But removing the X from XPUSH is challenging. There is
" EXTEND(SP\, HvUSEDKEYS(keys) * (dokeys + dovalues));"
But it DOESNT extend the stack enough to avoid the X being called in XPUSHs when executing
"our %Config = %Config::Config;"
in ExtUtils\MakeMaker\Config.pm . I tried doing ------------------------ use Config; use Data::Dumper; %h = ('a' => 1\, 'b' =>2); print(Dumper(scalar(%h))); print(Dumper(scalar(%Config))); ------------------------ which gave
#############5.12 C:\Documents and Settings\Owner\Desktop>perl n11.pl $VAR1 = "2/8"; $VAR1 = 1; #############5.19 C:\Documents and Settings\Owner\Desktop>perl n11.pl $VAR1 = '2/8'; $VAR1 = 1;
But %Config clearly doesn't have 1 key in it. Config_heavy.pl does not have a SCALAR method\, and it seem optional to have one per Perl_magic_scalarpack(pTHX_ HV *hv\, MAGIC *mg) in mg.c. I found https://rt-archive.perl.org/perl5/Ticket/Display.html?id=18186 and https://rt-archive.perl.org/perl5/Ticket/Display.html?id=24798 which seem related to the fact that SCALAR returns an IMMORTAL SV when there is no SCALAR sub in the package\, not the key and bucket counts. Is this intended behavior or should it be revisted?
So is there a way to fix " EXTEND(SP\, HvUSEDKEYS(keys) * (dokeys + dovalues));" to remove the X from XPUSHs later in the sub and correctly EXTEND for tied HVs? Calling SCALAR doesn't work on %Config.
Also Perl_magic_scalarpack seems to do the same stash lookup twice for no good reason.
------------------------------------------------------------- SV * Perl_magic_scalarpack(pTHX_ HV *hv\, MAGIC *mg) { dVAR; SV *retval; SV * const tied = SvTIED_obj(MUTABLE_SV(hv)\, mg); HV * const pkg = SvSTASH((const SV *)SvRV(tied));
PERL_ARGS_ASSERT_MAGIC_SCALARPACK;
if (!gv_fetchmethod_autoload(pkg\, "SCALAR"\, FALSE)) {\<\<\<\<\<\<\<\<\<\<\<\<\<\<\<FIRST TIME SV *key; if (HvEITER_get(hv)) /* we are in an iteration so the hash cannot be empty */ return &PL_sv_yes; /* no xhv_eiter so now use FIRSTKEY */ key = sv_newmortal(); magic_nextpack(MUTABLE_SV(hv)\, mg\, key); HvEITER_set(hv\, NULL); /* need to reset iterator */ return SvOK(key) ? &PL_sv_yes : &PL_sv_no; }
/* there is a SCALAR method that we can call */ retval = Perl_magic_methcall(aTHX_ MUTABLE_SV(hv)\, mg\, SV_CONST(SCALAR)\, 0\, 0);\<\<\<\<\<\<\<\<\<\<\<\<SECOND TIME if (!retval) retval = &PL_sv_undef; return retval; } -------------------------------------------------------------
I did testing with the following patch. That is how I know the SPAGAIN and PUTBACK can go. The extend check assert were so frequent they were commented out so the stack realloc asserts can trip without noise. harness did pass all tests with no crashes from stack realloc assert. ("DebugBreak()" is a C breakpoint function so harness stops because a child perl proc crashed and I can examine the .t perl process with a C debugger).
On Thu\, Feb 27\, 2014 at 06:50:51PM -0800\, bulk88 wrote:
and PUTBACKs can be removed since magic calls now swap stacks. But removing the X from XPUSH is challenging. There is
" EXTEND(SP\, HvUSEDKEYS(keys) * (dokeys + dovalues));"
But it DOESNT extend the stack enough to avoid the X being called in XPUSHs when executing
"our %Config = %Config::Config;"
So is there a way to fix " EXTEND(SP\, HvUSEDKEYS(keys) * (dokeys + dovalues));" to remove the X from XPUSHs later in the sub and correctly EXTEND for tied HVs? Calling SCALAR doesn't work on %Config.
I think the behaviour is fine the way it is. For the non-tied case\, the initial EXTEND is efficient\, and the extra overhead of the X is trivial. For the tied case\, we don't really care about efficiency (or rather\, any savings from removing the X are completely dwarfed by calling out to HASHNEXT each time).
Calling SCALAR to allow pre-extending would be inappropriate.
-- "You're so sadly neglected\, and often ignored. A poor second to Belgium\, When going abroad." -- Monty Python\, "Finland"
The RT System itself - Status changed from 'new' to 'open'
On Thu\, Feb 27\, 2014 at 06:50:51PM -0800\, bulk88 wrote:
Also Perl_magic_scalarpack seems to do the same stash lookup twice for no good reason.
------------------------------------------------------------- SV * Perl_magic_scalarpack(pTHX_ HV *hv\, MAGIC *mg) { dVAR; SV *retval; SV * const tied = SvTIED_obj(MUTABLE_SV(hv)\, mg); HV * const pkg = SvSTASH((const SV *)SvRV(tied));
PERL\_ARGS\_ASSERT\_MAGIC\_SCALARPACK; if \(\!gv\_fetchmethod\_autoload\(pkg\, "SCALAR"\, FALSE\)\)
{\<\<\<\<\<\<\<\<\<\<\<\<\<\<\<FIRST TIME SV *key; if (HvEITER_get(hv)) /* we are in an iteration so the hash cannot be empty */ return &PL_sv_yes; /* no xhv_eiter so now use FIRSTKEY */ key = sv_newmortal(); magic_nextpack(MUTABLE_SV(hv)\, mg\, key); HvEITER_set(hv\, NULL); /* need to reset iterator */ return SvOK(key) ? &PL_sv_yes : &PL_sv_no; }
/\* there is a SCALAR method that we can call \*/ retval = Perl\_magic\_methcall\(aTHX\_ MUTABLE\_SV\(hv\)\, mg\,
SV_CONST(SCALAR)\, 0\, 0);\<\<\<\<\<\<\<\<\<\<\<\<SECOND TIME if (!retval) retval = &PL_sv_undef; return retval; }
Presumably because the usual API for magic calls\, Perl_magic_methcall() does its own method lookup\, while unusually\, magic_scalarpack has special handling for when the SCALAR method doesn't exist\, so has to do it's own check first. Not 100% efficient\, but trivial compared with the cost that's about to be bourne by calling out to SCALAR.
-- Wesley Crusher gets beaten up by his classmates for being a smarmy git\, and consequently has a go at making some friends of his own age for a change. -- Things That Never Happen in "Star Trek" #18
On Fri Feb 28 03:48:40 2014\, davem wrote:
I think the behaviour is fine the way it is. For the non-tied case\, the initial EXTEND is efficient\, and the extra overhead of the X is trivial. For the tied case\, we don't really care about efficiency (or rather\, any savings from removing the X are completely dwarfed by calling out to HASHNEXT each time).
Calling SCALAR to allow pre-extending would be inappropriate.
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?
-- bulk88 ~ bulk88 at hotmail.com
On Sun\, Mar 09\, 2014 at 01:43:00PM -0700\, bulk88 via RT 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?
Given that it makes no sense for Perl_do_kv() to be called without pushing at least one of keys or values\, a simple assert should suffice.
Encoding the multiplier in the high nibble seems a bit tricksy. Couldn't you just have:
EXTEND(SP\, usedkeys * (1 + (do_kind == (DOKVA_KEYS|DOKVA_VALUES)));
-- I thought I was wrong once\, but I was mistaken.
On Mon Mar 10 06:44:20 2014\, davem wrote:
On Sun\, Mar 09\, 2014 at 01:43:00PM -0700\, bulk88 via RT 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?
Given that it makes no sense for Perl_do_kv() to be called without pushing at least one of keys or values\, a simple assert should suffice.
Encoding the multiplier in the high nibble seems a bit tricksy. Couldn't you just have:
EXTEND\(SP\, usedkeys \* \(1 \+ \(do\_kind == \(DOKVA\_KEYS|DOKVA\_VALUES\)\)\);
New final patch attached.
-- bulk88 ~ bulk88 at hotmail.com
On Fri Mar 14 12:11:56 2014\, bulk88 wrote:
New final patch attached.
Forgot attachment.
-- bulk88 ~ bulk88 at hotmail.com
On Fri Mar 14 12:12:28 2014\, bulk88 wrote:
On Fri Mar 14 12:11:56 2014\, bulk88 wrote:
New final patch attached.
Forgot attachment.
Bump.
-- bulk88 ~ bulk88 at hotmail.com
On Fri Mar 14 12:12:28 2014\, bulk88 wrote:
On Fri Mar 14 12:11:56 2014\, bulk88 wrote:
New final patch attached.
Forgot attachment.
-in G_ARRAY\, simplify op_type to do type conversion\, "dokv ||" logic looked less than ideal for perf
- 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);
+ 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);
I don't think I'd call that simplified. Just moving it and using bool for the do* variables would be preferable.
-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
The ASSUME() you've added will assert on this for DEBUGGING builds.
+ const I32 usedkeys = HvUSEDKEYS(keys);
I32 is the devil\, this should be STRLEN (or maybe SSize_t). HvUSEDKEYS() is the difference between xhv_keys (STRLEN) and Perl_hv_placeholders_get() (I32) (or possibly 0)\, so it may not fit in an I32.
+ 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 */
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$