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

refcount for anonymous sub is 2 instead of 1 #11818

Closed
p5pRT opened this issue Dec 22, 2011 · 17 comments
Closed

refcount for anonymous sub is 2 instead of 1 #11818

p5pRT opened this issue Dec 22, 2011 · 17 comments

Comments

@p5pRT
Copy link

p5pRT commented Dec 22, 2011

Migrated from rt.perl.org#106864 (status was 'open')

Searchable as RT106864$

@p5pRT
Copy link
Author

p5pRT commented Dec 22, 2011

From @dolmen

This is a bug report for perl from dolmen@​cpan.org,
generated with the help of perlbug 1.39 running under perl 5.10.1.


With the following expression​:

  my $x = sub { };

the refcount of $x is 2 instead of 1.
There is no such problem if the sub, instead of behind a simple sub,
is a closure.

Here is a testcase. Tests 1,2,3 fail at least on perl 5.10.1 and 5.14.2.

use Test​::More;
use Scalar​::Util 'weaken';
use B;

# Simple sub
my $a = sub { };
is B​::svref_2object($a)->REFCNT, 1, 'refcount 1';
weaken $a;
SKIP​: {
  is $a, undef, 'weaken on a simple anonymous sub' and skip 'refcount 0',
1;
  is B​::svref_2object($a)->REFCNT, 0, 'refcount 0';
}

# Closure
my $b = do { my $foo; sub { $foo } };
is B​::svref_2object($b)->REFCNT, 1, 'refcount 1';
weaken $b, 'weaken on a closure';
SKIP​: {
  is $b, undef, 'weaken on a closure' and skip 'refcount 0', 1;
  is B​::svref_2object($b)->REFCNT, 0, 'refcount 0';
}

done_testing;



Flags​:
  category=core
  severity=low


Site configuration information for perl 5.10.1​:

Configured by Debian Project at Tue Apr 26 15​:53​:22 UTC 2011.

Summary of my perl5 (revision 5 version 10 subversion 1) configuration​:

  Platform​:
  osname=linux, osvers=2.6.24-29-server,
archname=x86_64-linux-gnu-thread-multi
  uname='linux crested 2.6.24-29-server #1 smp wed mar 16 19​:04​:28 utc
2011 x86_64 x86_64 x86_64 gnulinux '
  config_args='-Dusethreads -Duselargefiles -Dccflags=-DDEBIAN
-Dcccdlflags=-fPIC -Darchname=x86_64-linux-gnu -Dprefix=/usr
-Dprivlib=/usr/share/perl/5.10 -Darchlib=/usr/lib/perl/5.10
-Dvendorprefix=/usr -Dvendorlib=/usr/share/perl5
-Dvendorarch=/usr/lib/perl5 -Dsiteprefix=/usr/local
-Dsitelib=/usr/local/share/perl/5.10.1
-Dsitearch=/usr/local/lib/perl/5.10.1 -Dman1dir=/usr/share/man/man1
-Dman3dir=/usr/share/man/man3 -Dsiteman1dir=/usr/local/man/man1
-Dsiteman3dir=/usr/local/man/man3 -Dman1ext=1 -Dman3ext=3perl
-Dpager=/usr/bin/sensible-pager -Uafs -Ud_csh -Ud_ualarm -Uusesfio -Uusenm
-DDEBUGGING=-g -Doptimize=-O2 -Dplibpth=/lib/x86_64-linux-gnu
/usr/lib/x86_64-linux-gnu -Duseshrplib -Dlibperl=libperl.so.5.10.1
-Dd_dosuid -des'
  hint=recommended, useposix=true, d_sigaction=define
  useithreads=define, usemultiplicity=define
  useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
  use64bitint=define, use64bitall=define, uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN
-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include
-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
  optimize='-O2 -g',
  cppflags='-D_REENTRANT -D_GNU_SOURCE -DDEBIAN -fno-strict-aliasing
-pipe -fstack-protector -I/usr/local/include'
  ccversion='', gccversion='4.5.2', gccosandvers=''
  intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
  d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
  ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t',
lseeksize=8
  alignbytes=8, prototype=define
  Linker and Libraries​:
  ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
  libpth=/usr/local/lib /lib/x86_64-linux-gnu /usr/lib/x86_64-linux-gnu
/lib /usr/lib /lib64 /usr/lib64
  libs=-lgdbm -lgdbm_compat -ldb -ldl -lm -lpthread -lc -lcrypt
  perllibs=-ldl -lm -lpthread -lc -lcrypt
  libc=, so=so, useshrplib=true, libperl=libperl.so.5.10.1
  gnulibc_version='2.13'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
  cccdlflags='-fPIC', lddlflags='-shared -O2 -g -L/usr/local/lib
-fstack-protector'

Locally applied patches​:
  DEBPKG​:debian/arm_thread_stress_timeout -
http​://bugs.debian.org/501970Raise the timeout of
ext/threads/shared/t/stress.t to accommodate slower
build hosts
  DEBPKG​:debian/cpan_config_path - Set location of CPAN​::Config to
/etc/perl as /usr may not be writable.
  DEBPKG​:debian/cpan_definstalldirs - Provide a sensible INSTALLDIRS
default for modules installed from CPAN.
  DEBPKG​:debian/db_file_ver - http​://bugs.debian.org/340047 Remove overly
restrictive DB_File version check.
  DEBPKG​:debian/doc_info - Replace generic man(1) instructions with
Debian-specific information.
  DEBPKG​:debian/enc2xs_inc - http​://bugs.debian.org/290336 Tweak enc2xs
to follow symlinks and ignore missing @​INC directories.
  DEBPKG​:debian/errno_ver - http​://bugs.debian.org/343351 Remove Errno
version check due to upgrade problems with long-running processes.
  DEBPKG​:debian/extutils_hacks - Various debian-specific ExtUtils changes
  DEBPKG​:debian/fakeroot - Postpone LD_LIBRARY_PATH evaluation to the
binary targets.
  DEBPKG​:debian/instmodsh_doc - Debian policy doesn't install .packlist
files for core or vendor.
  DEBPKG​:debian/ld_run_path - Remove standard libs from LD_RUN_PATH as
per Debian policy.
  DEBPKG​:debian/libnet_config_path - Set location of libnet.cfg to
/etc/perl/Net as /usr may not be writable.
  DEBPKG​:debian/m68k_thread_stress -
http​://bugs.debian.org/495826Disable some threads tests on m68k for
now due to missing TLS.
  DEBPKG​:debian/mod_paths - Tweak @​INC ordering for Debian
  DEBPKG​:debian/module_build_man_extensions -
http​://bugs.debian.org/479460 Adjust Module​::Build manual page extensions
for the Debian Perl policy
  DEBPKG​:debian/perl_synopsis - http​://bugs.debian.org/278323 Rearrange
perl.pod
  DEBPKG​:debian/prune_libs - http​://bugs.debian.org/128355 Prune the list
of libraries wanted to what we actually need.
  DEBPKG​:debian/use_gdbm - Explicitly link against -lgdbm_compat in
ODBM_File/NDBM_File.
  DEBPKG​:fixes/assorted_docs - http​://bugs.debian.org/443733 [384f06a]
Math​::BigInt​::CalcEmu documentation grammar fix
  DEBPKG​:fixes/net_smtp_docs - http​://bugs.debian.org/100195
[rt.cpan.org#36038] Document the Net​::SMTP 'Port' option
  DEBPKG​:fixes/processPL - http​://bugs.debian.org/357264
[rt.cpan.org#17224] Always use PERLRUNINST when building perl modules.
  DEBPKG​:debian/perlivp - http​://bugs.debian.org/510895 Make perlivp skip
include directories in /usr/local
  DEBPKG​:fixes/pod2man-index-backslash -
http​://bugs.debian.org/521256Escape backslashes in .IX entries
  DEBPKG​:debian/disable-zlib-bundling - Disable zlib bundling in
Compress​::Raw​::Zlib
  DEBPKG​:fixes/kfreebsd_cppsymbols -
http​://bugs.debian.org/533098[3b910a0] Add gcc predefined macros to
$Config{cppsymbols} on GNU/kFreeBSD.
  DEBPKG​:debian/cpanplus_definstalldirs -
http​://bugs.debian.org/533707Configure CPANPLUS to use the site
directories by default.
  DEBPKG​:debian/cpanplus_config_path - Save local versions of
CPANPLUS​::Config​::System into /etc/perl.
  DEBPKG​:fixes/kfreebsd-filecopy-pipes -
http​://bugs.debian.org/537555[16f708c] Fix File​::Copy​::copy with pipes
on GNU/kFreeBSD
  DEBPKG​:fixes/anon-tmpfile-dir - http​://bugs.debian.org/528544 [perl
#66452] Honor TMPDIR when open()ing an anonymous temporary file
  DEBPKG​:fixes/abstract-sockets - http​://bugs.debian.org/329291 [89904c0]
Add support for Abstract namespace sockets.
  DEBPKG​:fixes/hurd_cppsymbols - http​://bugs.debian.org/544307 [eeb92b7]
Add gcc predefined macros to $Config{cppsymbols} on GNU/Hurd.
  DEBPKG​:fixes/autodie-flock - http​://bugs.debian.org/543731 Allow for
flock returning EAGAIN instead of EWOULDBLOCK on linux/parisc
  DEBPKG​:fixes/archive-tar-instance-error - http​://bugs.debian.org/539355[
rt.cpan.org #48879] Separate Archive​::Tar instance error strings from each
other
  DEBPKG​:fixes/positive-gpos - http​://bugs.debian.org/545234 [perl
#69056] [c584a96] Fix \\G crash on first match
  DEBPKG​:debian/devel-ppport-ia64-optim -
http​://bugs.debian.org/548943Work around an ICE on ia64
  DEBPKG​:fixes/trie-logic-match - http​://bugs.debian.org/552291 [perl
#69973] [0abd0d7] Fix a DoS in Unicode processing [CVE-2009-3626]
  DEBPKG​:fixes/hppa-thread-eagain - http​://bugs.debian.org/554218 make
the threads-shared test suite more robust, fixing failures on hppa
  DEBPKG​:fixes/crash-on-undefined-destroy -
http​://bugs.debian.org/564074[perl #71952] [1f15e67] Fix a NULL
pointer dereference when looking for a
DESTROY method
  DEBPKG​:fixes/tainted-errno - http​://bugs.debian.org/574129 [perl
#61976] [be1cf43] fix an errno stringification bug in taint mode
  DEBPKG​:fixes/safe-upgrade - http​://bugs.debian.org/582978 Upgrade
Safe.pm to 2.25, fixing CVE-2010-1974
  DEBPKG​:fixes/tell-crash - http​://bugs.debian.org/578577 [f4817f3] Fix a
tell() crash on bad arguments.
  DEBPKG​:fixes/format-write-crash - http​://bugs.debian.org/579537 [perl
#22977] [421f30e] Fix a crash in format/write
  DEBPKG​:fixes/arm-alignment - http​://bugs.debian.org/289884 [f1c7503]
Prevent gcc from optimizing the alignment test away on armel
  DEBPKG​:fixes/fcgi-test - Fix a failure in CGI/t/fast.t when FCGI is
installed
  DEBPKG​:fixes/hurd-ccflags - http​://bugs.debian.org/587901 Make
hints/gnu.sh append to $ccflags rather than overriding them
  DEBPKG​:debian/squelch-locale-warnings -
http​://bugs.debian.org/508764Squelch locale warnings in Debian package
maintainer scripts
  DEBPKG​:fixes/lc-numeric-docs - http​://bugs.debian.org/379329 [perl
#78452] [903eb63] LC_NUMERIC documentation fixes
  DEBPKG​:fixes/lc-numeric-sprintf - http​://bugs.debian.org/601549 [perl
#78632] [b3fd614] Fix sprintf not to ignore LC_NUMERIC with constants
  DEBPKG​:fixes/concat-stack-corruption -
http​://bugs.debian.org/596105[perl #78674] [e3393f5] Fix stack pointer
corruption in pp_concat() with
'use encoding'
  DEBPKG​:fixes/cgi-multiline-header -
http​://bugs.debian.org/606995[CVE-2010-2761 CVE-2010-4410
CVE-2010-4411] CGI.pm MIME boundary and
multiline header vulnerabilities
  DEBPKG​:fixes/h2ph-gcc-4.5 - http​://bugs.debian.org/599933 [8d66b3f] Fix
h2ph and test
  DEBPKG​:fixes/threads-tmps-crash - [perl #70411] [24855df] Conditionally
compile tmps stack cleanup code
  DEBPKG​:patchlevel - http​://bugs.debian.org/567489 List packaged patches
for 5.10.1-17ubuntu1 in patchlevel.h

@p5pRT
Copy link
Author

p5pRT commented Dec 24, 2011

From @ikegami

2011/12/22 Olivier Mengué <perlbug-followup@​perl.org>

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

This is a bug report for perl from dolmen@​cpan.org,
generated with the help of perlbug 1.39 running under perl 5.10.1.

-----------------------------------------------------------------

With the following expression​:

my $x = sub { };

the refcount of $x is 2 instead of 1.

There is no such problem if the sub, instead of behind a simple sub,

is a closure.

What problem?

- Eric

@p5pRT
Copy link
Author

p5pRT commented Dec 24, 2011

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

@p5pRT
Copy link
Author

p5pRT commented Dec 24, 2011

From @ikegami

2011/12/22 Olivier Mengué <perlbug-followup@​perl.org>

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

This is a bug report for perl from dolmen@​cpan.org,
generated with the help of perlbug 1.39 running under perl 5.10.1.

-----------------------------------------------------------------

With the following expression​:

my $x = sub { };

the refcount of $x is 2 instead of 1.
There is no such problem if the sub, instead of behind a simple sub,
is a closure.

perl -MDevel​::Peek -e"for (1..3) { push @​a, sub {}; } Dump($a[0]);"
SV = IV(0x57b650) at 0x57b654
  REFCNT = 1
  FLAGS = (ROK)
  RV = 0x57b624
  SV = PVCV(0x4a9dd4) at 0x57b624
  REFCNT = 4
  FLAGS = (PADMY,ANON,WEAKOUTSIDE,CVGV_RC)
  COMP_STASH = 0x57b444 "main"
  START = 0x4d2cb8 ===> 0
  ROOT = 0x4d2c9c
  GVGV​::GV = 0x4a961c "main" :​: "__ANON__"
  FILE = "-e"
  DEPTH = 0
  FLAGS = 0x490
  OUTSIDE_SEQ = 96
  PADLIST = 0x4a954c
  PADNAME = 0x4a960c(0x4a12f4) PAD = 0x4a98fc(0x4a0c64)
  OUTSIDE = 0x57b6b4 (MAIN)

4, one for each element in @​a, and one to the sub alive until sub {} needs
it next.

With closures, there would be 4 CVs with one reference each instead of 1 CV
with 4 reference.

@p5pRT
Copy link
Author

p5pRT commented Dec 24, 2011

@ikegami - Status changed from 'open' to 'rejected'

@p5pRT p5pRT closed this as completed Dec 24, 2011
@p5pRT
Copy link
Author

p5pRT commented Dec 25, 2011

From @cpansprout

On Sat Dec 24 15​:55​:23 2011, ikegami@​adaelis.com wrote​:

2011/12/22 Olivier Mengu� <perlbug-followup@​perl.org>

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

This is a bug report for perl from dolmen@​cpan.org,
generated with the help of perlbug 1.39 running under perl 5.10.1.

-----------------------------------------------------------------

With the following expression​:

my $x = sub { };

the refcount of $x is 2 instead of 1.
There is no such problem if the sub, instead of behind a simple sub,
is a closure.

perl -MDevel​::Peek -e"for (1..3) { push @​a, sub {}; } Dump($a[0]);"
SV = IV(0x57b650) at 0x57b654
REFCNT = 1
FLAGS = (ROK)
RV = 0x57b624
SV = PVCV(0x4a9dd4) at 0x57b624
REFCNT = 4
FLAGS = (PADMY,ANON,WEAKOUTSIDE,CVGV_RC)
COMP_STASH = 0x57b444 "main"
START = 0x4d2cb8 ===> 0
ROOT = 0x4d2c9c
GVGV​::GV = 0x4a961c "main" :​: "__ANON__"
FILE = "-e"
DEPTH = 0
FLAGS = 0x490
OUTSIDE_SEQ = 96
PADLIST = 0x4a954c
PADNAME = 0x4a960c(0x4a12f4) PAD = 0x4a98fc(0x4a0c64)
OUTSIDE = 0x57b6b4 (MAIN)

4, one for each element in @​a, and one to the sub alive until sub {} needs
it next.

With closures, there would be 4 CVs with one reference each instead of
1 CV
with 4 reference.

I think this is a bug, in that mentioning a variable in a sub shouldn’t
change whether $x has the only reference after ‘$x = sub{}’.

It also causes the blessing bug in ticket #3306.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Dec 25, 2011

@cpansprout - Status changed from 'rejected' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Dec 25, 2011

From @doy

On Sat, Dec 24, 2011 at 06​:55​:03PM -0800, Father Chrysostomos via RT wrote​:

On Sat Dec 24 15​:55​:23 2011, ikegami@​adaelis.com wrote​:

2011/12/22 Olivier Mengu� <perlbug-followup@​perl.org>

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

This is a bug report for perl from dolmen@​cpan.org,
generated with the help of perlbug 1.39 running under perl 5.10.1.

-----------------------------------------------------------------

With the following expression​:

my $x = sub { };

the refcount of $x is 2 instead of 1.
There is no such problem if the sub, instead of behind a simple sub,
is a closure.

perl -MDevel​::Peek -e"for (1..3) { push @​a, sub {}; } Dump($a[0]);"
SV = IV(0x57b650) at 0x57b654
REFCNT = 1
FLAGS = (ROK)
RV = 0x57b624
SV = PVCV(0x4a9dd4) at 0x57b624
REFCNT = 4
FLAGS = (PADMY,ANON,WEAKOUTSIDE,CVGV_RC)
COMP_STASH = 0x57b444 "main"
START = 0x4d2cb8 ===> 0
ROOT = 0x4d2c9c
GVGV​::GV = 0x4a961c "main" :​: "__ANON__"
FILE = "-e"
DEPTH = 0
FLAGS = 0x490
OUTSIDE_SEQ = 96
PADLIST = 0x4a954c
PADNAME = 0x4a960c(0x4a12f4) PAD = 0x4a98fc(0x4a0c64)
OUTSIDE = 0x57b6b4 (MAIN)

4, one for each element in @​a, and one to the sub alive until sub {} needs
it next.

With closures, there would be 4 CVs with one reference each instead of
1 CV
with 4 reference.

I think this is a bug, in that mentioning a variable in a sub shouldn’t
change whether $x has the only reference after ‘$x = sub{}’.

It also causes the blessing bug in ticket #3306.

Oh good, that's already reported - I just ran into that a couple months
ago, and forgot to report it.

-doy

@p5pRT
Copy link
Author

p5pRT commented Dec 25, 2011

From @leonerd

On Thu, Dec 22, 2011 at 11​:59​:15AM -0800, Olivier Mengué wrote​:

With the following expression​:

my $x = sub \{ \};

the refcount of $x is 2 instead of 1.
There is no such problem if the sub, instead of behind a simple sub,
is a closure.

One reference in the RV, one in the optree of the program.

Since the anonymous function is not a closure it does not need to be
cloned with its pad (environment).

I run into similar problems in the unit tests for Test​::Refcount.

To create a trivial coderef with refcount 1 you need to make it a
lexical closure.

my $x = do { my $dummy; sub { undef $dummy; } };

HTH

--
Paul "LeoNerd" Evans

leonerd@​leonerd.org.uk
ICQ# 4135350 | Registered Linux# 179460
http​://www.leonerd.org.uk/

@p5pRT
Copy link
Author

p5pRT commented Dec 30, 2011

From @dolmen

Le Sam. Déc. 24 18​:55​:03 2011, sprout a écrit :

I think this is a bug, in that mentioning a variable in a sub shouldn’t
change whether $x has the only reference after ‘$x = sub{}’.

Well, it does.

In my use case, I use weaken on closures to break circular references
(an object method that stores as a member a closure that references
$self, and I want the closure to disappear when $self is not anymore
referenced). This is quite common when working with AnyEvent.

The bug I had in my code was that I did use weaken "as usual" on the
sub. So my mistake was to do a weaken on something which was not a
closure​: there was no circular reference problem in the first place. I
was trying to use 'weaken' to workaround a problem which didn't exist.

To avoid such mistakes in the future, would it be possible to detect and
report such cases of usage of weaken on non-closure subs?

It also causes the blessing bug in ticket #3306.

Thanks for that reference. I submitted an idea on how to fix that one too.

--
Olivier

@p5pRT
Copy link
Author

p5pRT commented Dec 30, 2011

From [Unknown Contact. See original ticket]

Le Sam. Déc. 24 18​:55​:03 2011, sprout a écrit :

I think this is a bug, in that mentioning a variable in a sub shouldn’t
change whether $x has the only reference after ‘$x = sub{}’.

Well, it does.

In my use case, I use weaken on closures to break circular references
(an object method that stores as a member a closure that references
$self, and I want the closure to disappear when $self is not anymore
referenced). This is quite common when working with AnyEvent.

The bug I had in my code was that I did use weaken "as usual" on the
sub. So my mistake was to do a weaken on something which was not a
closure​: there was no circular reference problem in the first place. I
was trying to use 'weaken' to workaround a problem which didn't exist.

To avoid such mistakes in the future, would it be possible to detect and
report such cases of usage of weaken on non-closure subs?

It also causes the blessing bug in ticket #3306.

Thanks for that reference. I submitted an idea on how to fix that one too.

--
Olivier

@p5pRT
Copy link
Author

p5pRT commented Dec 30, 2011

From @cpansprout

On Fri Dec 30 06​:31​:34 2011, dolmen wrote​:

Le Sam. Déc. 24 18​:55​:03 2011, sprout a écrit :

I think this is a bug, in that mentioning a variable in a sub shouldn’t
change whether $x has the only reference after ‘$x = sub{}’.

Well, it does.

In my use case, I use weaken on closures to break circular references
(an object method that stores as a member a closure that references
$self, and I want the closure to disappear when $self is not anymore
referenced). This is quite common when working with AnyEvent.

The bug I had in my code was that I did use weaken "as usual" on the
sub. So my mistake was to do a weaken on something which was not a
closure​: there was no circular reference problem in the first place. I
was trying to use 'weaken' to workaround a problem which didn't exist.

To avoid such mistakes in the future, would it be possible to detect and
report such cases of usage of weaken on non-closure subs?

But holding a weak reference to a non-closure sub is not necessarily a
bug. Weak references are not just for breaking circularities, you know.
:-) Sometimes they are useful for maintaining mappings between
references. See, e.g., Tie​::RefHash​::Weak.

It also causes the blessing bug in ticket #3306.

Thanks for that reference. I submitted an idea on how to fix that one too.

--
Olivier

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jan 3, 2012

From zefram@fysh.org

Olivier Mengu?? via RT wrote​:

The bug I had in my code was that I did use weaken "as usual" on the
sub. So my mistake was to do a weaken on something which was not a
closure

You might want to make your weakening conditional on the sub being
a closure​:

  use Scalar​::Util qw(weaken);
  use Sub​::Mutate qw(sub_closure_role);

  sub weaken_closure($) {
  weaken($_[0]) if sub_closure_role($_[0]) eq "CLOSURE";
  }

Use weaken_closure() everywhere that you currently use weaken() for
this purpose.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Jan 4, 2012

From @cpansprout

On Fri Dec 30 06​:31​:34 2011, dolmen wrote​:

Le Sam. Déc. 24 18​:55​:03 2011, sprout a écrit :

I think this is a bug, in that mentioning a variable in a sub shouldn’t
change whether $x has the only reference after ‘$x = sub{}’.

Well, it does.

One way to fix this, which would make sub{} do what most people think
(create a new sub), would be to check the reference count whenever sub{}
is evaluated. If it has gone up, then it can be cloned.

I don’t fully understand, though, how CvCLONE and CvCLONED work. Do
both subs (the original and the clone) hold reference counts on the op
tree? Would we leave CvCLONE and CvCLONED off in the non-closure case?
Presumably the clone should be stored in the op/pad, and the original
relinquished, in case it is blessed, or used in field hashes, etc.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jan 8, 2012

From @iabyn

On Tue, Jan 03, 2012 at 11​:16​:54PM -0800, Father Chrysostomos via RT wrote​:

On Fri Dec 30 06​:31​:34 2011, dolmen wrote​:

Le Sam. Déc. 24 18​:55​:03 2011, sprout a écrit :

I think this is a bug, in that mentioning a variable in a sub shouldn’t
change whether $x has the only reference after ‘$x = sub{}’.

Well, it does.

One way to fix this, which would make sub{} do what most people think
(create a new sub), would be to check the reference count whenever sub{}
is evaluated. If it has gone up, then it can be cloned.

I don't understand how that helps. Can you elaborate?

I don’t fully understand, though, how CvCLONE and CvCLONED work. Do
both subs (the original and the clone) hold reference counts on the op
tree? Would we leave CvCLONE and CvCLONED off in the non-closure case?
Presumably the clone should be stored in the op/pad, and the original
relinquished, in case it is blessed, or used in field hashes, etc.

The logic is like this. In an idealised world, a prototype anon sub is
stored in the pad. Every call to sub{} (i.e. pp_anoncode) makes a copy of
that prototype, with its own pad, but sharing the optree. The cloned copy
of the CV has the CvCLONED flag set to distinguish it from the prototype.
This flag is needed in some circumstances, for example with nested anon
subs, we need need to know whether CvOUTSIDE is pointing to the proto or a
real sub before we go haring off trying capture lexicals.

In the real world, as an optimisation, if the anon sub doesn't close over
any external lexical vars (CvCLONE not set), then the prototype CV is just
shared. This is a lot faster, but is buggy with regards to things like
blessing.

--
In my day, we used to edit the inodes by hand. With magnets.

@p5pRT
Copy link
Author

p5pRT commented Jan 11, 2012

From @nwc10

On Sun, Jan 08, 2012 at 03​:41​:20PM +0000, Dave Mitchell wrote​:

The logic is like this. In an idealised world, a prototype anon sub is
stored in the pad. Every call to sub{} (i.e. pp_anoncode) makes a copy of
that prototype, with its own pad, but sharing the optree. The cloned copy
of the CV has the CvCLONED flag set to distinguish it from the prototype.
This flag is needed in some circumstances, for example with nested anon
subs, we need need to know whether CvOUTSIDE is pointing to the proto or a
real sub before we go haring off trying capture lexicals.

I note the above represents the idea, and that this is the optimisation​:

In the real world, as an optimisation, if the anon sub doesn't close over
any external lexical vars (CvCLONE not set), then the prototype CV is just
shared. This is a lot faster, but is buggy with regards to things like
blessing.

and that I don't understand the implications enough to know if something
is obvious wrong with this​:

What I've been wondering for a while is, what happens if PVCV is split
into two structures (sort of like PVGV points to a GP)?

PVCV would be a light(er)weight head, pointing to the heavy weight stuff
in the ancillary structure. The intent is that the head is cheap to copy,
so is copied, so every sub reference becomes a unique PVCV, and can be
distinguished, blessed, etc

I sort of wondered about copying PVCV directly for the "optimised" case
above, ("just") bumping the reference count on PADLIST etc to keep everything
consistent, but it's bugging me that it's not sane to copy xcv_xsubany
because one has no idea what's in it. Which matters, because all XSUBs
fall into the optimised case.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Jan 11, 2012

From @iabyn

On Wed, Jan 11, 2012 at 11​:30​:04AM +0000, Nicholas Clark wrote​:

What I've been wondering for a while is, what happens if PVCV is split
into two structures (sort of like PVGV points to a GP)?

PVCV would be a light(er)weight head, pointing to the heavy weight stuff
in the ancillary structure. The intent is that the head is cheap to copy,
so is copied, so every sub reference becomes a unique PVCV, and can be
distinguished, blessed, etc

Interesting. I suppose the con side is how much will it slow down normal
CV access?

I sort of wondered about copying PVCV directly for the "optimised" case
above, ("just") bumping the reference count on PADLIST etc to keep everything
consistent, but it's bugging me that it's not sane to copy xcv_xsubany
because one has no idea what's in it. Which matters, because all XSUBs
fall into the optimised case.

But the cloning (and its optimised skipping thereof) should only apply to
anon CVs, which shouldn't be XS anyway. A quick look at the source shows
that cv_clone is called from pp_anoncode and S_doform (as I would expect),
but also from pp_rv2cv, which I didn't expect. However, in this latter
function, when I put an assert(0) in the if '(CvCLONE(cv))' branch,
nothing in the test suite fails. So I don't understand what it's for
there.

One possible approach would be to initially just copy the whole CV
structure, with, as you say, bumping the reference count on PADLIST etc,
and see how much of slowdown you get on sub { not a closure }, and if its
noticeable, then investigate splitting the CV.

--
Britain, Britain, Britain! Discovered by Sir Henry Britain in
sixteen-oh-ten. Sold to Germany a year later for a pfennig and the promise
of a kiss. Destroyed in eighteen thirty-forty two, and rebuilt a week
later by a man. This we know. Hello. But what of the people of Britain?
Who they? What do? And why? -- "Little Britain"

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

No branches or pull requests

1 participant