Skip to content
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

AUTOLOAD is not called when DESTROY is missing in 5.20.1 - but this is not documented #14673

Closed
p5pRT opened this issue Apr 25, 2015 · 18 comments

Comments

@p5pRT
Copy link

p5pRT commented Apr 25, 2015

Migrated from rt.perl.org#124387 (status was 'resolved')

Searchable as RT124387$

@p5pRT
Copy link
Author

p5pRT commented Apr 25, 2015

From @szabgab

In earlier versions of perl if AUTOLOAD is defined in a module but DESTROY
is not defined, the AUTOLOAD would be executed when the object goes out of
scope and one would have had to add an empty DESTROY to eliminate the
problem caused by this.

In 5.20.1 this does not seem to be the case. In 5.20.1 it seems if AUTOLOAD
won't be called when the object goes out of scope.

However I think the documentation in perlobj still indicates the old
behavior
http​://perldoc.perl.org/perlobj.html#Destructors
and I could not find any indication in the perldelta docs that would imply
this change.

So I wonder if this was an unplanned change in perl,
a lack of documentation, or something else.

The issue was discussed here as well​:

http​://www.perlmonks.org/?node_id=1124504

@p5pRT
Copy link
Author

p5pRT commented Apr 25, 2015

From @iabyn

On Fri, Apr 24, 2015 at 11​:16​:23PM -0700, Gabor Szabo wrote​:

# New Ticket Created by Gabor Szabo
# Please include the string​: [perl #124387]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=124387 >

In earlier versions of perl if AUTOLOAD is defined in a module but DESTROY
is not defined, the AUTOLOAD would be executed when the object goes out of
scope and one would have had to add an empty DESTROY to eliminate the
problem caused by this.

In 5.20.1 this does not seem to be the case. In 5.20.1 it seems if AUTOLOAD
won't be called when the object goes out of scope.

However I think the documentation in perlobj still indicates the old
behavior
http​://perldoc.perl.org/perlobj.html#Destructors
and I could not find any indication in the perldelta docs that would imply
this change.

So I wonder if this was an unplanned change in perl,
a lack of documentation, or something else.

The issue was discussed here as well​:

http​://www.perlmonks.org/?node_id=1124504

The change bisects to this, which leads to the suspicion that it was
unintended.

commit 8c34e50
Author​: Father Chrysostomos <sprout@​cpan.org>
AuthorDate​: Fri Nov 16 10​:00​:50 2012 -0800
Commit​: Father Chrysostomos <sprout@​cpan.org>
CommitDate​: Sat Nov 17 10​:13​:43 2012 -0800

  [perl #114864] Don’t use amt for DESTROY
 
  DESTROY has been cached in overload tables since
  perl-5.6.0-2080-g32251b2, making it 4 times faster than before (over-
  load tables are faster than method lookup).
 
  But it slows down symbol lookup on stashes with overload tables,
  because overload tables use magic, and SvRMAGICAL results in calls to
  mg_find on every hash lookup.
 
  By reusing SvSTASH(stash) to cache the DESTROY method (if the stash
  is unblessed, of course, as most stashes are), we can avoid making
  all destroyable stashes magical and also speed up DESTROY lookup
  slightly more.
 
  The results​:
 
  • 10% increase in stash lookup speed after destructors. That was just
  testing $Foo​::{x}. Other stash lookups will have other overheads
  that make the difference less impressive.
 
  • 5% increase in DESTROY lookup speed. I was using an empty DESTROY
  method to test this, so, again, real DESTROY methods will have more
  overhead and less speedup.

--
"There's something wrong with our bloody ships today, Chatfield."
  -- Admiral Beatty at the Battle of Jutland, 31st May 1916.

@p5pRT
Copy link
Author

p5pRT commented Apr 25, 2015

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented May 26, 2015

From @rjbs

* Dave Mitchell <davem@​iabyn.com> [2015-04-25T06​:25​:34]

On Fri, Apr 24, 2015 at 11​:16​:23PM -0700, Gabor Szabo wrote​:

In earlier versions of perl if AUTOLOAD is defined in a module but DESTROY
is not defined, the AUTOLOAD would be executed when the object goes out of
scope and one would have had to add an empty DESTROY to eliminate the
problem caused by this.
[...]
The change bisects to this, which leads to the suspicion that it was
unintended.

commit 8c34e50

That does look unintended. I don't see discussion of the change on p5p or
changes to tests, either.

I know I've added special casing to AUTOLOADs to skip DESTROY, which makes me
feel certain that people have used AUTOLOAD to handle DESTROY. Sure, it's been
broken for three years now, but unless there is a compelling reason against it,
I think we should restore this behavior and add a regression test.

The downside is breaking code only ever written or tested on 5.18.0, or later.
In other words, we'd be fixing a regression introduced before the earliest
supported version of perl. Still, I think it should be fixed.

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented May 26, 2015

From @ikegami

On Sat, Apr 25, 2015 at 2​:16 AM, Gabor Szabo <perlbug-followup@​perl.org>
wrote​:

# New Ticket Created by Gabor Szabo
# Please include the string​: [perl #124387]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=124387 >

In earlier versions of perl if AUTOLOAD is defined in a module but DESTROY
is not defined, the AUTOLOAD would be executed when the object goes out of
scope and one would have had to add an empty DESTROY to eliminate the
problem caused by this.

In 5.20.1 this does not seem to be the case. In 5.20.1 it seems if AUTOLOAD
won't be called when the object goes out of scope.

Ow! The bug isn't the documentation; it's that AUTOLOAD isn't being called
when it should be!

@p5pRT
Copy link
Author

p5pRT commented Oct 20, 2015

From @VadimPushtaev

The perldoc says​:

If you define an AUTOLOAD in your class, then Perl will call your AUTOLOAD to handle the DESTROY method. You can prevent this by defining an empty DESTROY, like we did in the autoloading example. You can also check the value of $AUTOLOAD and return without doing anything when called to handle DESTROY .

However, my AUTOLOAD is not called in some versions of Perl (including the current stable 5.22.0).

Here is what I tried​:

# 5.8.8
perl -e 'package A; sub AUTOLOAD {print "D\n"} bless {}, "A"'
D

# 5.14.2
perl -e 'package A; sub AUTOLOAD {print "D\n"} bless {}, "A"'
D

# 5.18.2
perl -e 'package A; sub AUTOLOAD {print "D\n"} bless {}, "A"'
# nothing

# 5.20.2
perl -e 'package A; sub AUTOLOAD {print "D\n"} bless {}, "A"'
# nothing

# 5.22.0
perl -e 'package A; sub AUTOLOAD {print "D\n"} bless {}, "A"'
# nothing

I'm not sure whether it's a documentation issue or it should not behave this way at all.

@p5pRT
Copy link
Author

p5pRT commented Oct 21, 2015

From @tonycoz

On Tue Oct 20 02​:25​:43 2015, pushtaev.vm@​gmail.com wrote​:

The perldoc says​:

If you define an AUTOLOAD in your class, then Perl will call your
AUTOLOAD to handle the DESTROY method. You can prevent this by
defining an empty DESTROY, like we did in the autoloading example. You
can also check the value of $AUTOLOAD and return without doing
anything when called to handle DESTROY .

However, my AUTOLOAD is not called in some versions of Perl (including
the current stable 5.22.0).

Looks like this changed in 8c34e50.

Tony

@p5pRT
Copy link
Author

p5pRT commented Oct 21, 2015

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Oct 22, 2015

From @VadimPushtaev

Hello, thanks for you reply!

I googled that commmit identifier you provided and found this​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=124387

Looks like it has already been reported and isn't fixed yet.

Is there a way I mark this ticket as duplicate or something?

--
Vadim Pushtaev

On Tue Oct 20 18​:42​:26 2015, tonyc wrote​:

On Tue Oct 20 02​:25​:43 2015, pushtaev.vm@​gmail.com wrote​:

The perldoc says​:

If you define an AUTOLOAD in your class, then Perl will call your
AUTOLOAD to handle the DESTROY method. You can prevent this by
defining an empty DESTROY, like we did in the autoloading example. You
can also check the value of $AUTOLOAD and return without doing
anything when called to handle DESTROY .

However, my AUTOLOAD is not called in some versions of Perl (including
the current stable 5.22.0).

Looks like this changed in 8c34e50.

Tony

@p5pRT
Copy link
Author

p5pRT commented Oct 22, 2015

From @tonycoz

On Thu Oct 22 01​:56​:47 2015, pushtaev.vm@​gmail.com wrote​:

Hello, thanks for you reply!

I googled that commmit identifier you provided and found this​:
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=124387

Looks like it has already been reported and isn't fixed yet.

Is there a way I mark this ticket as duplicate or something?

Thanks, I've merged the tickets.

Tony

@p5pRT
Copy link
Author

p5pRT commented Oct 22, 2015

From @toddr

On Oct 20, 2015, at 8​:42 PM, Tony Cook via RT <perlbug-followup@​perl.org> wrote​:

On Tue Oct 20 02​:25​:43 2015, pushtaev.vm@​gmail.com wrote​:

The perldoc says​:

If you define an AUTOLOAD in your class, then Perl will call your
AUTOLOAD to handle the DESTROY method. You can prevent this by
defining an empty DESTROY, like we did in the autoloading example. You
can also check the value of $AUTOLOAD and return without doing
anything when called to handle DESTROY .

However, my AUTOLOAD is not called in some versions of Perl (including
the current stable 5.22.0).

Looks like this changed in 8c34e50.

That makes this sound related to https​://rt.perl.org/Ticket/Display.html?id=126410 <https://rt-archive.perl.org/perl5/Ticket/Display.html?id=126410>. Which is the third time (overload changes also broke B​::C in bee7c57) this week I've been fighting with these sets of changes introduced in 5.17.

As I look at the commits, I'm forced to ask why we would want these speedup improvements when the commit messages say​:

10% increase in stash lookup ... Other stash lookups will have other overheads that make the difference less impressive.

So basically it's a 10% speedup on trivial cases. Which were probably billisecond improvements to begin with?

I promise I'm not trying to trivialize the effort that was put in. It seems like we changed some fundamental things about how Perl checks and uses some of the magic. However in doing so made the code potentially more complex and introduced several bugs we're now starting to notice.

It's possible I'm misunderstanding these changes and they're not as related as they seem. However if they are, I'm wondering if it's worth reverting the improvements to fix these problems? I admit that at this point it may be more complex than some other approach.

Todd.

@p5pRT
Copy link
Author

p5pRT commented Jan 19, 2016

From @tonycoz

On Fri Apr 24 23​:16​:23 2015, gabor@​szabgab.com wrote​:

In earlier versions of perl if AUTOLOAD is defined in a module but DESTROY
is not defined, the AUTOLOAD would be executed when the object goes out of
scope and one would have had to add an empty DESTROY to eliminate the
problem caused by this.

The attached, along with the two patches on #126410, fixes this for me.

I needed to change one of the existing tests in op/method.t to allow for DESTROY being called on the stdio objects.

I don't think these patches can be applied as-is to maint, though a different patch could fix this problem in maint.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jan 19, 2016

From @tonycoz

0003-perl-124387-TODO-test-for-AUTOLOAD-on-DESTROY.patch
From 8e91b8fcc8f1c911ccd80dc5bef1639a8a51d2f4 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Tue, 19 Jan 2016 11:39:48 +1100
Subject: [perl #124387] TODO test for AUTOLOAD on DESTROY

---
 t/op/method.t | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/t/op/method.t b/t/op/method.t
index 0d7f254..bdac5bb 100644
--- a/t/op/method.t
+++ b/t/op/method.t
@@ -13,7 +13,7 @@ BEGIN {
 use strict;
 no warnings 'once';
 
-plan(tests => 148);
+plan(tests => 149);
 
 @A::ISA = 'B';
 @B::ISA = 'C';
@@ -460,6 +460,17 @@ is $kalled, 1, 'calling a class method via a magic variable';
    { bless {}, "NoSub"; }
 }
 
+{
+    # [perl #124387]
+    local $::TODO = "AUTOLOAD not being called for DESTROY";
+    my $autoloaded;
+    package AutoloadDestroy;
+    sub AUTOLOAD { $autoloaded = 1 }
+    package main;
+    bless {}, "AutoloadDestroy";
+    ok($autoloaded, "AUTOLOAD called for DESTROY");
+}
+
 eval { () = 3; new {} };
 like $@,
      qr/^Can't call method "new" without a package or object reference/,
-- 
2.1.4

@p5pRT
Copy link
Author

p5pRT commented Jan 19, 2016

From @tonycoz

0004-perl-124387-call-AUTOLOAD-when-DESTROY-isn-t-defined.patch
From 4e3b4ec09e4002c0b74eca141e662b1403d65aaa Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Tue, 19 Jan 2016 11:42:21 +1100
Subject: [perl #124387] call AUTOLOAD when DESTROY isn't defined

---
 sv.c          | 3 ++-
 t/op/method.t | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/sv.c b/sv.c
index 0a5ffe8..9c4fed6 100644
--- a/sv.c
+++ b/sv.c
@@ -6738,6 +6738,7 @@ S_curse(pTHX_ SV * const sv, const bool check_refcnt) {
 	  if (HvNAME(stash)) {
 	    CV* destructor = NULL;
             struct mro_meta *meta;
+
 	    assert (SvOOK(stash));
 
             DEBUG_o( deb(aTHX_ "Looking for DESTROY method for %s\n",
@@ -6753,7 +6754,7 @@ S_curse(pTHX_ SV * const sv, const bool check_refcnt) {
             }
             else {
 		GV * const gv =
-		    gv_fetchmeth_autoload(stash, "DESTROY", 7, 0);
+		    gv_fetchmethod_pvn_flags(stash, "DESTROY", 7, GV_AUTOLOAD);
 		if (gv) destructor = GvCV(gv);
                 HvMROMETA(stash)->destroy_gen = PL_sub_generation;
                 HvMROMETA(stash)->destroy = destructor;
diff --git a/t/op/method.t b/t/op/method.t
index bdac5bb..d11a703 100644
--- a/t/op/method.t
+++ b/t/op/method.t
@@ -361,6 +361,7 @@ for my $meth (['Bar', 'Foo::Bar'],
 {
     fresh_perl_is(<<EOT,
 package UNIVERSAL; sub AUTOLOAD { my \$c = shift; print "\$c \$AUTOLOAD\\n" }
+sub DESTROY {} # prevent AUTOLOAD being called on DESTROY
 package Xyz;
 package main; Foo->$meth->[0]();
 EOT
@@ -462,7 +463,6 @@ is $kalled, 1, 'calling a class method via a magic variable';
 
 {
     # [perl #124387]
-    local $::TODO = "AUTOLOAD not being called for DESTROY";
     my $autoloaded;
     package AutoloadDestroy;
     sub AUTOLOAD { $autoloaded = 1 }
-- 
2.1.4

@p5pRT
Copy link
Author

p5pRT commented Feb 8, 2016

From @tonycoz

On Mon Jan 18 16​:53​:50 2016, tonyc wrote​:

On Fri Apr 24 23​:16​:23 2015, gabor@​szabgab.com wrote​:

In earlier versions of perl if AUTOLOAD is defined in a module but
DESTROY
is not defined, the AUTOLOAD would be executed when the object goes
out of
scope and one would have had to add an empty DESTROY to eliminate the
problem caused by this.

The attached, along with the two patches on #126410, fixes this for
me.

I needed to change one of the existing tests in op/method.t to allow
for DESTROY being called on the stdio objects.

Applied as f05081b and 000814d.

Tony

@p5pRT
Copy link
Author

p5pRT commented Feb 8, 2016

@tonycoz - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Author

p5pRT commented May 13, 2016

From @khwilliamson

Thank you for submitting this report. You have helped make Perl better.
 
With the release of Perl 5.24.0 on May 9, 2016, this and 149 other issues have been resolved.

Perl 5.24.0 may be downloaded via https://metacpan.org/release/RJBS/perl-5.24.0

@p5pRT
Copy link
Author

p5pRT commented May 13, 2016

@khwilliamson - Status changed from 'pending release' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant