CV symbol table optimization only works in main:: #15671
Comments
From @maukeCreated by @maukeHere's the merge I'm talking about: Also listed in perl5220delta: Subroutines in packages no longer need to be stored in typeglobs: This works fine for the main script: $ perl -wE 'sub foo { 42 } say $main::{foo}' But not in other packages: $ perl -wE 'package Other; sub foo { 42 } say $Other::{foo}' I.e. this optimization doesn't apply to modules, which is where most ----- Speculation follows. I suspect this is due to the following code in op.c (Perl_newATTRSUB_x): const I32 flags = 'ec' stands for "error count"; this is normally false (unless we're The 'name' check is fine (name = "foo", namlen = 3); what's tripping us PL_curstash is \%X:: in this situation, but CopSTASH(PL_curcop) is Perl Info |
From @maukeOn Wed Oct 19 07:20:42 2016, mauke- wrote:
I think this is the same underlying problem as https://rt-archive.perl.org/perl5/Ticket/Display.html?id=129239. |
From @maukeOn Wed Oct 19 07:20:42 2016, mauke- wrote:
Here's a patch that adds a TODO test for now. |
From @mauke0001-t-op-sub.t-add-a-TODO-test-for-RT-129916.patchFrom 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
|
From @jkeenanOn Wed, 19 Oct 2016 14:20:42 GMT, mauke- wrote:
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: ##### Thank you very much. -- |
|
The RT System itself - Status changed from 'new' to 'open' |
From @toddrOn Fri, 30 Dec 2016 16:42:52 -0800, jkeenan wrote:
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 |
From @cpansproutOn Wed, 19 Oct 2016 07:20:42 -0700, mauke- wrote:
Fixing it causes this to fail: use Test::More tests => 1; package Other; sub f1 { "f1" } 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 |
From @cpansproutOn Mon, 28 Aug 2017 20:33:55 -0700, sprout wrote:
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 |
From @atoomicOn Mon, 28 Aug 2017 20:33:55 -0700, sprout wrote:
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
|
From @cpansproutOn Mon, 18 Sep 2017 09:00:58 -0700, atoomic wrote:
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.
-- Father Chrysostomos |
From @atoomicHi 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. Also, notice that some other OPs might require the same kind of action. sincerely On Thu, 21 Sep 2017 09:34:42 -0700, sprout wrote:
|
From @atoomic0001-Adjust-OP-multideref-for-RV-CV.patchFrom 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
|
From @atoomic |
From @atoomicIn 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: I've attached a fix for the B::Xref one, hope the other are not so hard to fix. nicolas On Thu, 21 Sep 2017 15:52:42 -0700, atoomic wrote:
|
From @atoomic0001-Adjust-B-Xref-to-handle-RV-CV-in-GV-slot.patchFrom 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
|
From @atoomic0001-Adjust-OP-multideref-for-RV-CV.patchFrom 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
|
From @cpansproutOn Thu, 21 Sep 2017 16:22:55 -0700, atoomic wrote:
I have fixed the last two. They are now passing for me locally.
But I am not getting that failure. What is your perl configuration? Mine: $ ./perl -Ilib -V:config_args -- Father Chrysostomos |
From @cpansproutOn Thu, 21 Sep 2017 15:52:42 -0700, atoomic wrote:
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.
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 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);
I think it is acceptable to force the upgrade. -- Father Chrysostomos |
From @cpansproutOn Sun, 24 Sep 2017 14:37:49 -0700, sprout wrote:
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 |
From @cpansproutOn Sun, 24 Sep 2017 14:37:49 -0700, sprout wrote:
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 |
From @atoomicOn Sun, 24 Sep 2017 14:34:35 -0700, sprout wrote:
I was simply running configure with: ./Configure -Dusedevel -Doptimize=-g3 -des |
From @atoomicOn Sun, 24 Sep 2017 16:55:14 -0700, sprout wrote:
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 ? |
From @cpansproutThis bug is now fixed with the branch merged into blead as 1369fd5. -- Father Chrysostomos |
|
@cpansprout - Status changed from 'open' to 'pending release' |
From @cpansproutOn Mon, 25 Sep 2017 08:13:29 -0700, atoomic wrote:
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 |
From @cpansproutOn Sun, 08 Oct 2017 16:52:52 -0700, sprout wrote:
Fixed in 7a388d0. -- Father Chrysostomos |
From @jkeenanRe-opening this ticket, as the final commit referenced appears to have caused large BBC problems (RT 132252). Thank you very much. -- |
|
@jkeenan - Status changed from 'pending release' to 'open' |
From @cpansproutOn Sun, 15 Oct 2017 13:12:28 -0700, jkeenan wrote:
The optimisation was reverted in commit 6eed25e and has now been reinstated in commit d964025. -- Father Chrysostomos |
|
@cpansprout - Status changed from 'open' to 'pending release' |
From @khwilliamsonThank 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 Perl 5.28.0 may be downloaded via: If you find that the problem persists, feel free to reopen this ticket. |
|
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#129916 (status was 'resolved')
Searchable as RT129916$
The text was updated successfully, but these errors were encountered: