Skip Menu |
Report information
Id: 129916
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: mauke- <l.mai [at] web.de>
Cc:
AdminCc:

Operating System: Linux
PatchStatus: (no value)
Severity: low
Type: core
Perl Version: 5.24.0
Fixed In: (no value)



From: l.mai [...] web.de
Subject: CV symbol table optimization only works in main::
To: perlbug [...] perl.org
Date: Wed, 19 Oct 2016 16:20:17 +0200
Download (untitled) / with headers
text/plain 4.3k
This is a bug report for perl from l.mai@web.de, generated with the help of perlbug 1.40 running under perl 5.24.0. ----------------------------------------------------------------- [Please describe your issue here] Here's the merge I'm talking about: http://perl5.git.perl.org/perl.git/commitdiff/f9d9e965 Also listed in perl5220delta: Subroutines in packages no longer need to be stored in typeglobs: declaring a subroutine will now put a simple sub reference directly in the stash if possible, saving memory. This works fine for the main script: $ perl -wE 'sub foo { 42 } say $main::{foo}' CODE(0xa06f668) But not in other packages: $ perl -wE 'package Other; sub foo { 42 } say $Other::{foo}' *Other::foo I.e. this optimization doesn't apply to modules, which is where most subroutines come from. ----- Speculation follows. I suspect this is due to the following code in op.c (Perl_newATTRSUB_x): const I32 flags = ec ? GV_NOADD_NOINIT : PL_curstash != CopSTASH(PL_curcop) || memchr(name, ':', namlen) || memchr(name, '\'', namlen) ? gv_fetch_flags : GV_ADDMULTI | GV_NOINIT | GV_NOTQUAL; gv = gv_fetchsv(cSVOPo->op_sv, flags, SVt_PVCV); 'ec' stands for "error count"; this is normally false (unless we're parsing past a syntax error). The 'name' check is fine (name = "foo", namlen = 3); what's tripping us up is the stash check. PL_curstash is \%X:: in this situation, but CopSTASH(PL_curcop) is \%main::. So the stashes differ and we land in the gv_fetch_flags path, which adds a full typeglob. [Please do not change anything below this line] ----------------------------------------------------------------- --- Flags: category=core severity=low --- Site configuration information for perl 5.24.0: Configured by mauke at Mon May 9 21:21:33 CEST 2016. Summary of my perl5 (revision 5 version 24 subversion 0) configuration: Platform: osname=linux, osvers=4.4.5-1-arch, archname=i686-linux uname='linux simplicio 4.4.5-1-arch #1 smp preempt thu mar 10 07:54:30 cet 2016 i686 gnulinux ' config_args='' hint=previous, useposix=true, d_sigaction=define useithreads=undef, usemultiplicity=undef use64bitint=undef, use64bitall=undef, uselongdouble=undef usemymalloc=n, bincompat5005=undef Compiler: cc='cc', ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64', optimize='-O2 -flto', cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include' ccversion='', gccversion='6.1.1 20160501', gccosandvers='' intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234, doublekind=3 d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12, longdblkind=3 ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8 alignbytes=4, prototype=define Linker and Libraries: ld='cc', ldflags ='-fstack-protector-strong -L/usr/local/lib -flto' libpth=/usr/local/lib /usr/lib/gcc/i686-pc-linux-gnu/6.1.1/include-fixed /usr/lib /lib /usr/local/lib /usr/lib/gcc/i686-pc-linux-gnu/6.1.1/include-fixed /usr/lib libs=-lpthread -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc libc=libc-2.23.so, so=so, useshrplib=false, libperl=libperl.a gnulibc_version='2.23' Dynamic Linking: dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E' cccdlflags='-fPIC', lddlflags='-shared -O2 -flto -L/usr/local/lib -fstack-protector-strong' --- @INC for perl 5.24.0: /home/mauke/usr/lib/perl5/site_perl/5.24.0/i686-linux /home/mauke/usr/lib/perl5/site_perl/5.24.0 /home/mauke/usr/lib/perl5/5.24.0/i686-linux /home/mauke/usr/lib/perl5/5.24.0 . --- Environment for perl 5.24.0: HOME=/home/mauke LANG=en_US.UTF-8 LANGUAGE=en_US LC_COLLATE=C LC_MONETARY=de_DE.UTF-8 LC_TIME=de_DE.UTF-8 LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/home/mauke/perl5/perlbrew/bin:/home/mauke/bin:/usr/local/sbin:/usr/local/bin:/usr/bin:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl PERLBREW_BASHRC_VERSION=0.73 PERLBREW_HOME=/home/mauke/.perlbrew PERLBREW_ROOT=/home/mauke/perl5/perlbrew PERL_BADLANG (unset) PERL_UNICODE=SAL SHELL=/bin/bash
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 316b
On Wed Oct 19 07:20:42 2016, mauke- wrote: Show quoted text
> PL_curstash is \%X:: in this situation, but CopSTASH(PL_curcop) is > \%main::. So the stashes differ and we land in the gv_fetch_flags > path, > which adds a full typeglob.
I think this is the same underlying problem as https://rt.perl.org/Ticket/Display.html?id=129239.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 724b
On Wed Oct 19 07:20:42 2016, mauke- wrote: Show quoted text
> Here's the merge I'm talking about: > http://perl5.git.perl.org/perl.git/commitdiff/f9d9e965 > > Also listed in perl5220delta: > > Subroutines in packages no longer need to be stored in typeglobs: > declaring a subroutine will now put a simple sub reference directly in > the stash if possible, saving memory. > > This works fine for the main script: > > $ perl -wE 'sub foo { 42 } say $main::{foo}' > CODE(0xa06f668) > > But not in other packages: > > $ perl -wE 'package Other; sub foo { 42 } say $Other::{foo}' > *Other::foo > > I.e. this optimization doesn't apply to modules, which is where most > subroutines come from.
Here's a patch that adds a TODO test for now.
Subject: 0001-t-op-sub.t-add-a-TODO-test-for-RT-129916.patch
From e4b2a8e44465712094f2d4e76fe75f70bd3e1b49 Mon Sep 17 00:00:00 2001 From: Lukas Mai <l.mai@web.de> Date: Fri, 21 Oct 2016 15:04:39 +0200 Subject: [PATCH] t/op/sub.t: add a TODO test for RT #129916 --- t/op/sub.t | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/t/op/sub.t b/t/op/sub.t index 07fa033..5c501b1 100644 --- a/t/op/sub.t +++ b/t/op/sub.t @@ -6,7 +6,7 @@ BEGIN { set_up_inc('../lib'); } -plan(tests => 63); +plan(tests => 65); sub empty_sub {} @@ -416,6 +416,17 @@ sub curpm { "a" =~ /(.)/; is(curpm(), 'c', 'return and PL_curpm'); +sub rt_129916 { 42 } +is ref($main::{rt_129916}), 'CODE', 'simple sub stored as CV in stash (main::)'; +{ + package RT129916; + sub foo { 42 } +} +{ + local $TODO = "CV symbol table optimization only works in main:: [perl #129916]"; + is ref($RT129916::{foo}), 'CODE', 'simple sub stored as CV in stash (non-main::)'; +} + # [perl #129090] Crashes and hangs watchdog 10; { no warnings; -- 2.10.0
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.9k
On Wed, 19 Oct 2016 14:20:42 GMT, mauke- wrote: Show quoted text
> > This is a bug report for perl from l.mai@web.de, > generated with the help of perlbug 1.40 running under perl 5.24.0. > > > ----------------------------------------------------------------- > [Please describe your issue here] > > Here's the merge I'm talking about: > http://perl5.git.perl.org/perl.git/commitdiff/f9d9e965 > > Also listed in perl5220delta: > > Subroutines in packages no longer need to be stored in typeglobs: > declaring a subroutine will now put a simple sub reference directly in > the stash if possible, saving memory. > > This works fine for the main script: > > $ perl -wE 'sub foo { 42 } say $main::{foo}' > CODE(0xa06f668) > > But not in other packages: > > $ perl -wE 'package Other; sub foo { 42 } say $Other::{foo}' > *Other::foo > > I.e. this optimization doesn't apply to modules, which is where most > subroutines come from. > > ----- > > Speculation follows. > > I suspect this is due to the following code in op.c > (Perl_newATTRSUB_x): > > const I32 flags = > ec ? GV_NOADD_NOINIT > : PL_curstash != CopSTASH(PL_curcop) > || memchr(name, ':', namlen) || memchr(name, '\'', namlen) > ? gv_fetch_flags > : GV_ADDMULTI | GV_NOINIT | GV_NOTQUAL; > gv = gv_fetchsv(cSVOPo->op_sv, flags, SVt_PVCV); > > 'ec' stands for "error count"; this is normally false (unless we're > parsing past a syntax error). > > The 'name' check is fine (name = "foo", namlen = 3); what's tripping > us > up is the stash check. > > PL_curstash is \%X:: in this situation, but CopSTASH(PL_curcop) is > \%main::. So the stashes differ and we land in the gv_fetch_flags > path, > which adds a full typeglob. >
Is this something where, for the short run, we would be best off simply noting the phenomenon in the documentation? I.e., something along the lines of: ##### Contrary to what was stated in pod/perl5220delta.pod, ... ##### Thank you very much. -- James E Keenan (jkeenan@cpan.org)
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 474b
On Fri, 30 Dec 2016 16:42:52 -0800, jkeenan wrote: Show quoted text
> Is this something where, for the short run, we would be best off > simply noting the phenomenon in the documentation? I.e., something > along the lines of: > > ##### > Contrary to what was stated in pod/perl5220delta.pod, ... > ##### > > Thank you very much.
Unnecessarily allocating a GV when an RV will do can lead to program bloat. (160 bytes vs 24 bytes on 64 bit). This would be a very good thing to fix. Todd
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.2k
On Wed, 19 Oct 2016 07:20:42 -0700, mauke- wrote: Show quoted text
> > This is a bug report for perl from l.mai@web.de, > generated with the help of perlbug 1.40 running under perl 5.24.0. > > > ----------------------------------------------------------------- > [Please describe your issue here] > > Here's the merge I'm talking about: > http://perl5.git.perl.org/perl.git/commitdiff/f9d9e965 > > Also listed in perl5220delta: > > Subroutines in packages no longer need to be stored in typeglobs: > declaring a subroutine will now put a simple sub reference directly in > the stash if possible, saving memory. > > This works fine for the main script: > > $ perl -wE 'sub foo { 42 } say $main::{foo}' > CODE(0xa06f668) > > But not in other packages: > > $ perl -wE 'package Other; sub foo { 42 } say $Other::{foo}' > *Other::foo > > I.e. this optimization doesn't apply to modules, which is where most > subroutines come from.
Fixing it causes this to fail: use Test::More tests => 1; package Other; sub f1 { "f1" } local $Other::{"f1"} = sub { "h1" }; ::is(f1(), "h1", "localised sub via stash"); Does this matter? This is pretty obscure code (from t/op/local.t) that should probably be doing local *Other::f1 = sub { "h1" } instead (if it were production code). -- Father Chrysostomos
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.8k
On Mon, 28 Aug 2017 20:33:55 -0700, sprout wrote: Show quoted text
> On Wed, 19 Oct 2016 07:20:42 -0700, mauke- wrote:
> > > > This is a bug report for perl from l.mai@web.de, > > generated with the help of perlbug 1.40 running under perl 5.24.0. > > > > > > ----------------------------------------------------------------- > > [Please describe your issue here] > > > > Here's the merge I'm talking about: > > http://perl5.git.perl.org/perl.git/commitdiff/f9d9e965 > > > > Also listed in perl5220delta: > > > > Subroutines in packages no longer need to be stored in typeglobs: > > declaring a subroutine will now put a simple sub reference directly > > in > > the stash if possible, saving memory. > > > > This works fine for the main script: > > > > $ perl -wE 'sub foo { 42 } say $main::{foo}' > > CODE(0xa06f668) > > > > But not in other packages: > > > > $ perl -wE 'package Other; sub foo { 42 } say $Other::{foo}' > > *Other::foo > > > > I.e. this optimization doesn't apply to modules, which is where most > > subroutines come from.
> > Fixing it causes this to fail: > > use Test::More tests => 1; > > package Other; > > sub f1 { "f1" } > local $Other::{"f1"} = sub { "h1" }; > ::is(f1(), "h1", "localised sub via stash"); > > Does this matter? This is pretty obscure code (from t/op/local.t) > that should probably be doing local *Other::f1 = sub { "h1" } instead > (if it were production code).
It’s possible to add a special case to save_helem_flags such that it treats stashes differently. Basically, the rv2cv op in the f1() call above references the actual scalar that $Other::{f1} points to, so if localization swaps out a different scalar (which happens with local $Other::{f1}) the stash no longer points to the scalar that the op points to. GVs in stashes are already treated specially. Maybe subrefs in stashes should be treated the same way. -- Father Chrysostomos
RT-Send-CC: perl5-porters [...] perl.org, sprout [...] cpan.org
Download (untitled) / with headers
text/plain 1.5k
On Mon, 28 Aug 2017 20:33:55 -0700, sprout wrote: Show quoted text
> On Wed, 19 Oct 2016 07:20:42 -0700, mauke- wrote:
> > > > This is a bug report for perl from l.mai@web.de, > > generated with the help of perlbug 1.40 running under perl 5.24.0. > > > > > > ----------------------------------------------------------------- > > [Please describe your issue here] > > > > Here's the merge I'm talking about: > > http://perl5.git.perl.org/perl.git/commitdiff/f9d9e965 > > > > Also listed in perl5220delta: > > > > Subroutines in packages no longer need to be stored in typeglobs: > > declaring a subroutine will now put a simple sub reference directly > > in > > the stash if possible, saving memory. > > > > This works fine for the main script: > > > > $ perl -wE 'sub foo { 42 } say $main::{foo}' > > CODE(0xa06f668) > > > > But not in other packages: > > > > $ perl -wE 'package Other; sub foo { 42 } say $Other::{foo}' > > *Other::foo > > > > I.e. this optimization doesn't apply to modules, which is where most > > subroutines come from.
> > Fixing it causes this to fail:
Could you precise the 'fixing it' part. Where/what is the patch with your fix candidate ? is there any commit id / topic branch where we could read your attempt to fix it ? thanks Show quoted text
> > use Test::More tests => 1; > > package Other; > > sub f1 { "f1" } > local $Other::{"f1"} = sub { "h1" }; > ::is(f1(), "h1", "localised sub via stash"); > > Does this matter? This is pretty obscure code (from t/op/local.t) > that should probably be doing local *Other::f1 = sub { "h1" } instead > (if it were production code).
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.8k
On Mon, 18 Sep 2017 09:00:58 -0700, atoomic wrote: Show quoted text
> On Mon, 28 Aug 2017 20:33:55 -0700, sprout wrote:
> > On Wed, 19 Oct 2016 07:20:42 -0700, mauke- wrote:
> > > > > > This is a bug report for perl from l.mai@web.de, > > > generated with the help of perlbug 1.40 running under perl 5.24.0. > > > > > > > > > ----------------------------------------------------------------- > > > [Please describe your issue here] > > > > > > Here's the merge I'm talking about: > > > http://perl5.git.perl.org/perl.git/commitdiff/f9d9e965 > > > > > > Also listed in perl5220delta: > > > > > > Subroutines in packages no longer need to be stored in typeglobs: > > > declaring a subroutine will now put a simple sub reference directly > > > in > > > the stash if possible, saving memory. > > > > > > This works fine for the main script: > > > > > > $ perl -wE 'sub foo { 42 } say $main::{foo}' > > > CODE(0xa06f668) > > > > > > But not in other packages: > > > > > > $ perl -wE 'package Other; sub foo { 42 } say $Other::{foo}' > > > *Other::foo > > > > > > I.e. this optimization doesn't apply to modules, which is where > > > most > > > subroutines come from.
> > > > Fixing it causes this to fail:
> > Could you precise the 'fixing it' part. Where/what is the patch with > your fix candidate ? is there any commit id / topic branch where we > could read your attempt to fix it ?
Due to test failures, I had not actually committed it yet. But now I have. It is the ‘unfinished’ commit on the sprout/cv-in-stash branch. Show quoted text
>
> > > > use Test::More tests => 1; > > > > package Other; > > > > sub f1 { "f1" } > > local $Other::{"f1"} = sub { "h1" }; > > ::is(f1(), "h1", "localised sub via stash"); > > > > Does this matter? This is pretty obscure code (from t/op/local.t) > > that should probably be doing local *Other::f1 = sub { "h1" } instead > > (if it were production code).
-- Father Chrysostomos
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 2.6k
Hi Father Chrysostomos, Todd R. and I checked your branch sprout/cv-in-stash and had a look at the issue from the extract from t/op/local.t, which is failing. Hope this patch could help, but looks like the OP multideref is not calling the Perl_mro_method_changed_in via a regular save_gp call. We came with this lazy solution (view attached patch) which force to upgrade the RV/CV to a GV. As these cases are probably uncommon, I think it's a solution we could consider when using local on a CV. Also, notice that some other OPs might require the same kind of action. I’ve also attached to this case a simple test (with some comments) to show the problem. sincerely nicolas On Thu, 21 Sep 2017 09:34:42 -0700, sprout wrote: Show quoted text
> On Mon, 18 Sep 2017 09:00:58 -0700, atoomic wrote:
> > On Mon, 28 Aug 2017 20:33:55 -0700, sprout wrote:
> > > On Wed, 19 Oct 2016 07:20:42 -0700, mauke- wrote:
> > > > > > > > This is a bug report for perl from l.mai@web.de, > > > > generated with the help of perlbug 1.40 running under perl > > > > 5.24.0. > > > > > > > > > > > > ----------------------------------------------------------------- > > > > [Please describe your issue here] > > > > > > > > Here's the merge I'm talking about: > > > > http://perl5.git.perl.org/perl.git/commitdiff/f9d9e965 > > > > > > > > Also listed in perl5220delta: > > > > > > > > Subroutines in packages no longer need to be stored in typeglobs: > > > > declaring a subroutine will now put a simple sub reference > > > > directly > > > > in > > > > the stash if possible, saving memory. > > > > > > > > This works fine for the main script: > > > > > > > > $ perl -wE 'sub foo { 42 } say $main::{foo}' > > > > CODE(0xa06f668) > > > > > > > > But not in other packages: > > > > > > > > $ perl -wE 'package Other; sub foo { 42 } say $Other::{foo}' > > > > *Other::foo > > > > > > > > I.e. this optimization doesn't apply to modules, which is where > > > > most > > > > subroutines come from.
> > > > > > Fixing it causes this to fail:
> > > > Could you precise the 'fixing it' part. Where/what is the patch with > > your fix candidate ? is there any commit id / topic branch where we > > could read your attempt to fix it ?
> > Due to test failures, I had not actually committed it yet. But now I > have. It is the ‘unfinished’ commit on the sprout/cv-in-stash branch. > >
> >
> > > > > > use Test::More tests => 1; > > > > > > package Other; > > > > > > sub f1 { "f1" } > > > local $Other::{"f1"} = sub { "h1" }; > > > ::is(f1(), "h1", "localised sub via stash"); > > > > > > Does this matter? This is pretty obscure code (from t/op/local.t) > > > that should probably be doing local *Other::f1 = sub { "h1" } > > > instead > > > (if it were production code).
Subject: 0001-Adjust-OP-multideref-for-RV-CV.patch
From 77ab33315c43b3a61a588d1f9ba5adfa9cf0914d Mon Sep 17 00:00:00 2001 From: Nicolas R <atoomic@cpan.org> Date: Thu, 21 Sep 2017 17:30:06 -0500 Subject: [PATCH] Adjust OP multideref for RV/CV When the sv is not a GV but one reference to a CV, force to coerce it to a GV using CvGV then treat it as a regular GV. --- pp_hot.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pp_hot.c b/pp_hot.c index 7fc48e1a79..d7db038fff 100644 --- a/pp_hot.c +++ b/pp_hot.c @@ -3000,6 +3000,9 @@ PP(pp_multideref) } else { if (localizing) { + if ( SvROK(sv) && SvTYPE(SvRV(sv)) == SVt_PVCV && CvNAMED(SvRV(*svp)) ) { + sv = (SV*) CvGV( SvRV(sv) ); + } if (HvNAME_get(hv) && isGV(sv)) save_gp(MUTABLE_GV(sv), !(PL_op->op_flags & OPf_SPECIAL)); -- 2.14.1
Subject: test-extract.pl
Download test-extract.pl
text/x-perl 349b
#!/bin/env perl package Other; sub f1 { "not ok - global\n" } #eval q[$f1 = 5]; # uncommenting this line force the CV to be attached to a GV - and also fixes the issue local $Other::{"f1"} = sub { "ok - local mocked\n" }; print f1(); # pointing to the incorrect CV eval 'print f1()'; # using an eval string hide it from the compiler and also works
RT-Send-CC: sprout [...] cpan.org, perl5-porters [...] perl.org, sprout [...] cpan.org
Download (untitled) / with headers
text/plain 3.3k
In addition to my previous emaul, you would found attached to this message a minor update of the multideref patch which uses 'sv' instead of '*svp'. After running the tests from 'sprout/cv-in-stash' with this additional patch only 3 tests fail: ../ext/B/t/xref.t ../ext/XS-APItest/t/gv_fetchmeth.t ../lib/B/Deparse.t I've attached a fix for the B::Xref one, hope the other are not so hard to fix. That should leave us with only failures nicolas On Thu, 21 Sep 2017 15:52:42 -0700, atoomic wrote: Show quoted text
> Hi Father Chrysostomos, Todd R. and I checked your branch sprout/cv- > in-stash and had a look at the issue from the extract from > t/op/local.t, which is failing. > > Hope this patch could help, but looks like the OP multideref is not > calling the Perl_mro_method_changed_in via a regular save_gp call. > > We came with this lazy solution (view attached patch) which force to > upgrade the RV/CV to a GV. > As these cases are probably uncommon, I think it's a solution we could > consider when using local on a CV. > > Also, notice that some other OPs might require the same kind of > action. > I’ve also attached to this case a simple test (with some comments) to > show the problem. > > sincerely > nicolas > > On Thu, 21 Sep 2017 09:34:42 -0700, sprout wrote:
> > On Mon, 18 Sep 2017 09:00:58 -0700, atoomic wrote:
> > > On Mon, 28 Aug 2017 20:33:55 -0700, sprout wrote:
> > > > On Wed, 19 Oct 2016 07:20:42 -0700, mauke- wrote:
> > > > > > > > > > This is a bug report for perl from l.mai@web.de, > > > > > generated with the help of perlbug 1.40 running under perl > > > > > 5.24.0. > > > > > > > > > > > > > > > ----------------------------------------------------------------- > > > > > [Please describe your issue here] > > > > > > > > > > Here's the merge I'm talking about: > > > > > http://perl5.git.perl.org/perl.git/commitdiff/f9d9e965 > > > > > > > > > > Also listed in perl5220delta: > > > > > > > > > > Subroutines in packages no longer need to be stored in > > > > > typeglobs: > > > > > declaring a subroutine will now put a simple sub reference > > > > > directly > > > > > in > > > > > the stash if possible, saving memory. > > > > > > > > > > This works fine for the main script: > > > > > > > > > > $ perl -wE 'sub foo { 42 } say $main::{foo}' > > > > > CODE(0xa06f668) > > > > > > > > > > But not in other packages: > > > > > > > > > > $ perl -wE 'package Other; sub foo { 42 } say $Other::{foo}' > > > > > *Other::foo > > > > > > > > > > I.e. this optimization doesn't apply to modules, which is where > > > > > most > > > > > subroutines come from.
> > > > > > > > Fixing it causes this to fail:
> > > > > > Could you precise the 'fixing it' part. Where/what is the patch > > > with > > > your fix candidate ? is there any commit id / topic branch where we > > > could read your attempt to fix it ?
> > > > Due to test failures, I had not actually committed it yet. But now I > > have. It is the ‘unfinished’ commit on the sprout/cv-in-stash > > branch. > > > >
> > >
> > > > > > > > use Test::More tests => 1; > > > > > > > > package Other; > > > > > > > > sub f1 { "f1" } > > > > local $Other::{"f1"} = sub { "h1" }; > > > > ::is(f1(), "h1", "localised sub via stash"); > > > > > > > > Does this matter? This is pretty obscure code (from > > > > t/op/local.t) > > > > that should probably be doing local *Other::f1 = sub { "h1" } > > > > instead > > > > (if it were production code).
Subject: 0001-Adjust-B-Xref-to-handle-RV-CV-in-GV-slot.patch
From bc37ef11206d52137523c78644279f61bd2652e0 Mon Sep 17 00:00:00 2001 From: Nicolas R <atoomic@cpan.org> Date: Thu, 21 Sep 2017 18:20:56 -0500 Subject: [PATCH] Adjust B::Xref to handle RV/CV in GV slot --- ext/B/B/Xref.pm | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ext/B/B/Xref.pm b/ext/B/B/Xref.pm index 255ee890bd..1e6bb290bb 100644 --- a/ext/B/B/Xref.pm +++ b/ext/B/B/Xref.pm @@ -143,7 +143,7 @@ Malcolm Beattie, mbeattie@sable.ox.ac.uk. use strict; use Config; use B qw(peekop class comppadlist main_start svref_2object walksymtable - OPpLVAL_INTRO SVf_POK OPpOUR_INTRO cstring + OPpLVAL_INTRO SVf_POK SVf_ROK OPpOUR_INTRO cstring ); sub UNKNOWN { ["?", "?", "?"] } @@ -331,7 +331,8 @@ sub pp_gv { } else { $gv = $op->gv; - $top = [$gv->STASH->NAME, "*", $gv->SAFENAME]; + $gv = $gv->RV->GV if $gv->FLAGS & SVf_ROK && ref $gv->RV eq 'B::CV'; + $top = [$gv->STASH->NAME, "*", $gv->SAFENAME]; } process($top, $op->private & OPpLVAL_INTRO ? "intro" : "used"); } -- 2.14.1
Subject: 0001-Adjust-OP-multideref-for-RV-CV.patch
From 9efcac990caf890b2daa86ebcf45877067423a5b Mon Sep 17 00:00:00 2001 From: Nicolas R <atoomic@cpan.org> Date: Thu, 21 Sep 2017 17:30:06 -0500 Subject: [PATCH] Adjust OP multideref for RV/CV When the sv is not a GV but one reference to a CV, force to coerce it to a GV using CvGV then treat it as a regular GV. --- pp_hot.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pp_hot.c b/pp_hot.c index 7fc48e1a79..19d0693501 100644 --- a/pp_hot.c +++ b/pp_hot.c @@ -3000,6 +3000,9 @@ PP(pp_multideref) } else { if (localizing) { + if ( SvROK(sv) && SvTYPE(SvRV(sv)) == SVt_PVCV && CvNAMED(SvRV(sv)) ) { + sv = (SV*) CvGV( SvRV(sv) ); + } if (HvNAME_get(hv) && isGV(sv)) save_gp(MUTABLE_GV(sv), !(PL_op->op_flags & OPf_SPECIAL)); -- 2.14.1
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 807b
On Thu, 21 Sep 2017 16:22:55 -0700, atoomic wrote: Show quoted text
> In addition to my previous emaul, you would found attached to this > message a minor update of the multideref patch which uses 'sv' instead > of '*svp'. > > After running the tests from 'sprout/cv-in-stash' with this additional > patch only 3 tests fail: > ../ext/B/t/xref.t > ../ext/XS-APItest/t/gv_fetchmeth.t > ../lib/B/Deparse.t
I have fixed the last two. They are now passing for me locally. Show quoted text
> > I've attached a fix for the B::Xref one,
But I am not getting that failure. What is your perl configuration? Mine: $ ./perl -Ilib -V:config_args config_args='-de -DDEBUGGING -Dusedevel -Duseithreads -Doptimize=-O0 -Aoptimize=-g -Accflags=-DPERL_BOOL_AS_CHAR -Dcc=g++ -Accflags=-DPERL_POISON'; -- Father Chrysostomos
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.2k
On Thu, 21 Sep 2017 15:52:42 -0700, atoomic wrote: Show quoted text
> Hi Father Chrysostomos, Todd R. and I checked your branch sprout/cv- > in-stash and had a look at the issue from the extract from > t/op/local.t, which is failing.
Thank you. I forgot to mention that I had already gotten local.t to pass, by passing CV refs straight to save_gp if they occur inside stashes. I did not touch multideref, though. Show quoted text
> Hope this patch could help, but looks like the OP multideref is not > calling the Perl_mro_method_changed_in via a regular save_gp call.
Your test-extract.pl does still fail for me. So now I do not know why multideref is not kicking in for me in local.t, whereas it is for you. So I need to dig into that, since I do not feel comfortably committing a change I cannot test properly. Show quoted text
> We came with this lazy solution (view attached patch) which force to > upgrade the RV/CV to a GV.
I still need to look into it, but I think your patch forces the upgrade even when localising the entry of a regular hash. We don’t want this to do save_gp on *baz: %foo = (bar => \&baz); local $foo{bar}; Show quoted text
> As these cases are probably uncommon, I think it's a solution we could > consider when using local on a CV.
I think it is acceptable to force the upgrade. -- Father Chrysostomos
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.3k
On Sun, 24 Sep 2017 14:37:49 -0700, sprout wrote: Show quoted text
> On Thu, 21 Sep 2017 15:52:42 -0700, atoomic wrote:
> > Hi Father Chrysostomos, Todd R. and I checked your branch sprout/cv- > > in-stash and had a look at the issue from the extract from > > t/op/local.t, which is failing.
> > Thank you. > > I forgot to mention that I had already gotten local.t to pass, by > passing CV refs straight to save_gp if they occur inside stashes. I > did not touch multideref, though. >
> > Hope this patch could help, but looks like the OP multideref is not > > calling the Perl_mro_method_changed_in via a regular save_gp call.
> > Your test-extract.pl does still fail for me. So now I do not know why > multideref is not kicking in for me in local.t, whereas it is for you. > So I need to dig into that, since I do not feel comfortably committing > a change I cannot test properly. >
> > We came with this lazy solution (view attached patch) which force to > > upgrade the RV/CV to a GV.
> > I still need to look into it, but I think your patch forces the > upgrade even when localising the entry of a regular hash. We don’t > want this to do save_gp on *baz: > > %foo = (bar => \&baz); > local $foo{bar};
No, I was wrong. Your patch does not do that. Still, since I have save_gp handling sub refs, I will apply a simpler patch. Thank you for bringing multideref to my attention. -- Father Chrysostomos
RT-Send-CC: perl5-porters [...] perl.org
On Sun, 24 Sep 2017 14:37:49 -0700, sprout wrote: Show quoted text
> On Thu, 21 Sep 2017 15:52:42 -0700, atoomic wrote:
> > Hi Father Chrysostomos, Todd R. and I checked your branch sprout/cv- > > in-stash and had a look at the issue from the extract from > > t/op/local.t, which is failing.
> > Thank you. > > I forgot to mention that I had already gotten local.t to pass, by > passing CV refs straight to save_gp if they occur inside stashes. I > did not touch multideref, though. >
> > Hope this patch could help, but looks like the OP multideref is not > > calling the Perl_mro_method_changed_in via a regular save_gp call.
> > Your test-extract.pl does still fail for me. So now I do not know why > multideref is not kicking in for me in local.t, whereas it is for you. > So I need to dig into that, since I do not feel comfortably committing > a change I cannot test properly.
I see that local.t does use multideref, but a previous test vivifies the glob, which is why it was not failing. So I have added more tests to local.t and fixed multideref, on the newly-updated sprout/cv-in-stash branch. -- Father Chrysostomos
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 963b
On Sun, 24 Sep 2017 14:34:35 -0700, sprout wrote: Show quoted text
> On Thu, 21 Sep 2017 16:22:55 -0700, atoomic wrote:
> > In addition to my previous emaul, you would found attached to this > > message a minor update of the multideref patch which uses 'sv' > > instead > > of '*svp'. > > > > After running the tests from 'sprout/cv-in-stash' with this > > additional > > patch only 3 tests fail: > > ../ext/B/t/xref.t > > ../ext/XS-APItest/t/gv_fetchmeth.t > > ../lib/B/Deparse.t
> > I have fixed the last two. They are now passing for me locally. >
> > > > I've attached a fix for the B::Xref one,
> > But I am not getting that failure. What is your perl configuration? > Mine: > > $ ./perl -Ilib -V:config_args > config_args='-de -DDEBUGGING -Dusedevel -Duseithreads -Doptimize=-O0 > -Aoptimize=-g -Accflags=-DPERL_BOOL_AS_CHAR -Dcc=g++ -Accflags=- > DPERL_POISON';
I was simply running configure with: ./Configure -Dusedevel -Doptimize=-g3 -des
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.4k
On Sun, 24 Sep 2017 16:55:14 -0700, sprout wrote: Show quoted text
> On Sun, 24 Sep 2017 14:37:49 -0700, sprout wrote:
> > On Thu, 21 Sep 2017 15:52:42 -0700, atoomic wrote:
> > > Hi Father Chrysostomos, Todd R. and I checked your branch > > > sprout/cv- > > > in-stash and had a look at the issue from the extract from > > > t/op/local.t, which is failing.
> > > > Thank you. > > > > I forgot to mention that I had already gotten local.t to pass, by > > passing CV refs straight to save_gp if they occur inside stashes. I > > did not touch multideref, though. > >
> > > Hope this patch could help, but looks like the OP multideref is not > > > calling the Perl_mro_method_changed_in via a regular save_gp call.
> > > > Your test-extract.pl does still fail for me. So now I do not know > > why > > multideref is not kicking in for me in local.t, whereas it is for > > you. > > So I need to dig into that, since I do not feel comfortably > > committing > > a change I cannot test properly.
> > I see that local.t does use multideref, but a previous test vivifies > the glob, which is why it was not failing. So I have added more tests > to local.t and fixed multideref, on the newly-updated sprout/cv-in- > stash branch.
Excellent, indeed your patch with the isGV_or_RVCV is even simpler & nicer :-) Do you think we could also extend this work to simple SVs: PV/IV..., to store them in stash when possible and avoid a GV GV when not needed ? or this would require much more work ?
RT-Send-CC: perl5-porters [...] perl.org
This bug is now fixed with the branch merged into blead as 1369fd508. -- Father Chrysostomos
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.2k
On Mon, 25 Sep 2017 08:13:29 -0700, atoomic wrote: Show quoted text
> On Sun, 24 Sep 2017 14:34:35 -0700, sprout wrote:
> > On Thu, 21 Sep 2017 16:22:55 -0700, atoomic wrote:
> > > In addition to my previous emaul, you would found attached to this > > > message a minor update of the multideref patch which uses 'sv' > > > instead > > > of '*svp'. > > > > > > After running the tests from 'sprout/cv-in-stash' with this > > > additional > > > patch only 3 tests fail: > > > ../ext/B/t/xref.t > > > ../ext/XS-APItest/t/gv_fetchmeth.t > > > ../lib/B/Deparse.t
> > > > I have fixed the last two. They are now passing for me locally. > >
> > > > > > I've attached a fix for the B::Xref one,
> > > > But I am not getting that failure. What is your perl configuration? > > Mine: > > > > $ ./perl -Ilib -V:config_args > > config_args='-de -DDEBUGGING -Dusedevel -Duseithreads -Doptimize=-O0 > > -Aoptimize=-g -Accflags=-DPERL_BOOL_AS_CHAR -Dcc=g++ -Accflags=- > > DPERL_POISON';
> > I was simply running configure with: ./Configure -Dusedevel > -Doptimize=-g3 -des
Apparently James Keenan is having the same issue. I forgot to look at this last issue before I merged my fixes. I am building with your configuration arguments now. I hope I can get it fixed tonight before someone does a big revert! -- Father Chrysostomos
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.4k
On Sun, 08 Oct 2017 16:52:52 -0700, sprout wrote: Show quoted text
> On Mon, 25 Sep 2017 08:13:29 -0700, atoomic wrote:
> > On Sun, 24 Sep 2017 14:34:35 -0700, sprout wrote:
> > > On Thu, 21 Sep 2017 16:22:55 -0700, atoomic wrote:
> > > > In addition to my previous emaul, you would found attached to > > > > this > > > > message a minor update of the multideref patch which uses 'sv' > > > > instead > > > > of '*svp'. > > > > > > > > After running the tests from 'sprout/cv-in-stash' with this > > > > additional > > > > patch only 3 tests fail: > > > > ../ext/B/t/xref.t > > > > ../ext/XS-APItest/t/gv_fetchmeth.t > > > > ../lib/B/Deparse.t
> > > > > > I have fixed the last two. They are now passing for me locally. > > >
> > > > > > > > I've attached a fix for the B::Xref one,
> > > > > > But I am not getting that failure. What is your perl > > > configuration? > > > Mine: > > > > > > $ ./perl -Ilib -V:config_args > > > config_args='-de -DDEBUGGING -Dusedevel -Duseithreads -Doptimize=- > > > O0 > > > -Aoptimize=-g -Accflags=-DPERL_BOOL_AS_CHAR -Dcc=g++ -Accflags=- > > > DPERL_POISON';
> > > > I was simply running configure with: ./Configure -Dusedevel > > -Doptimize=-g3 -des
> > Apparently James Keenan is having the same issue. > > I forgot to look at this last issue before I merged my fixes. I am > building with your configuration arguments now. I hope I can get it > fixed tonight before someone does a big revert!
Fixed in 7a388d0f14. -- Father Chrysostomos
Download (untitled) / with headers
text/plain 170b
Re-opening this ticket, as the final commit referenced appears to have caused large BBC problems (RT 132252). Thank you very much. -- James E Keenan (jkeenan@cpan.org)
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 295b
On Sun, 15 Oct 2017 13:12:28 -0700, jkeenan wrote: Show quoted text
> Re-opening this ticket, as the final commit referenced appears to have > caused large BBC problems (RT 132252).
The optimisation was reverted in commit 6eed25e2537 and has now been reinstated in commit d96402565cc. -- Father Chrysostomos
Download (untitled) / with headers
text/plain 317b
Thank you for filing this report. You have helped make Perl better. With the release yesterday of Perl 5.28.0, this and 185 other issues have been resolved. Perl 5.28.0 may be downloaded via: https://metacpan.org/release/XSAWYERX/perl-5.28.0 If you find that the problem persists, feel free to reopen this ticket.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org