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

Owner: Nobody
Requestors: airwave-dev <dev [at] airwave.com>
Cc:
AdminCc:

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



Subject: Symbol::delete_package does not free certain memory associated with package::ISA
Date: Tue, 18 May 2010 13:25:49 -0700
To: perlbug [...] perl.org
From: Thomas Whaples <twhaples [...] arubanetworks.com>
Download (untitled) / with headers
text/plain 6.9k
This is a bug report for perl from twhaples@airwave.com,
generated with the help of perlbug 1.39 running under perl 5.10.1.


-----------------------------------------------------------------
[Please describe your issue here]

We find it convenient to construct dynamic packages in our code for various purposes, and have created internal tools for manipulating these packages. We have found that the use of these packages seems to be associated with memory leaks which impair our code's operation when it runs for weeks or months at a time, and believe we have tracked the problem to a deficiency in Symbol::delete_package.

When we call Symbol::delete_package, we expect that all traces of the package in question would disappear. (We recognize that the package is not removed from %INC and manage to do so ourselves.) However, it seems that there is still some memory left over which is not freed. The memory appears to be located somewhere in Perl-guts (we have tried to track it with Devel::Leak and found nothing) and seems to be associated with the contents of the magic @ISA variable in the package: somewhere about 80 bytes per package if @ISA contains a single empty string, more if there is a more complicated inheritance pattern, none if @ISA is the empty list. 


A script to reproduce this issue follows. This code not only leaks memory on our internal build of Perl 5.10.1, but we have also found it to leak on stock perl 5.8.8 from CentOS and on stock perl 5.10.1 on Ubuntu 10.04. (The script uses the Linux /proc/ filesystem as a convenient way to display memory usage; I'm sure that it can be adapted to another platform readily enough).


use strict;
use warnings;

use Symbol;
# use Devel::Leak;

@a::ISA = qw(UNIVERSAL);
@b::ISA = qw(a);
@c::ISA = qw(b);

my $leak_more = 1;
my $base = $leak_more ? 'c' : '';

# my $handle;
# Devel::Leak::NoteSV($handle);

for my $count (0..20_000) {
  no strict 'refs';
  *{"Foo::${count}::ISA"} = [$base];
  Symbol::delete_package("Foo::${count}");
  
  memstats() unless $count % 5000;
}

# Devel::Leak::CheckSV($handle);

sub memstats {
  system("cat /proc/$$/status | grep VmRSS");
}

[Please do not change anything below this line]
-----------------------------------------------------------------
---
Flags:
    category=library
    severity=medium
    module=Symbol
---
Site configuration information for perl 5.10.1:

Configured by Red Hat, Inc. at Mon Oct 19 16:15:38 PDT 2009.

Summary of my perl5 (revision 5 version 10 subversion 1) configuration:
   
  Platform:
    osname=linux, osvers=2.6.18-128.4.1.el5pae, archname=i386-linux-thread-multi
    uname='linux langley.corp.airwave.com 2.6.18-128.4.1.el5pae #1 smp tue aug 4 20:58:34 edt 2009 i686 i686 i386 gnulinux '
    config_args='-des -Doptimize=-O2 -g -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -Dversion=5.10.1 -Dmyhostname=localhost -Dperladmin=root@localhost -Dcc=gcc -Dcf_by=Red Hat, Inc. -Dinstallprefix=/opt/airwave -Dprefix=/opt/airwave -Dprivlib=/opt/airwave/lib/perl5/5.10.1 -Dsitelib=/opt/airwave/local/lib/perl5/site_perl/5.10.1 -Dvendorlib=/opt/airwave/lib/perl5/vendor_perl/5.10.1 -Darchlib=/opt/airwave/lib/perl5/5.10.1/i386-linux-thread-multi -Dsitearch=/opt/airwave/local/lib/perl5/site_perl/5.10.1/i386-linux-thread-multi -Dvendorarch=/opt/airwave/lib/perl5/vendor_perl/5.10.1/i386-linux-thread-multi -Darchname=i386-linux-thread-multi -Dvendorprefix=/opt/airwave -Dsiteprefix=/opt/airwave/local -Duseshrplib -Dusethreads -Duseithreads -Duselargefiles -Dd_dosuid -Dd_semctl_semun -Di_db -Ui_ndbm -Di_gdbm -Di_shadow -Di_syslog -Dman3ext=3pm -Duseperlio -Dinstallusrbinperl=n -Ubincompat5005 -Uversiononly -Dpager=/usr/bin/less -isr -Dd_gethostent_r_proto -Ud_endhoste

 nt_r_proto -Ud_sethostent_r_proto -Ud_endprotoent_r_proto -Ud_setprotoent_r_proto -Ud_endservent_r_proto -Ud_setservent_r_proto -Dscriptdir=/opt/airwave/bin'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=define, usemultiplicity=define
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=undef, use64bitall=undef, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='gcc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O2 -g -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.1.2 20080704 (Red Hat 4.1.2-44)', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='gcc', ldflags =' -fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib
    libs=-lresolv -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lpthread -lc
    perllibs=-lresolv -lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
    libc=/lib/libc-2.5.so, so=so, useshrplib=true, libperl=libperl.so
    gnulibc_version='2.5'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E -Wl,-rpath,/opt/airwave/lib/perl5/5.10.1/i386-linux-thread-multi/CORE'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -g -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -L/usr/local/lib -fstack-protector'

Locally applied patches:
    

---
@INC for perl 5.10.1:
    /root/svn/mercury/lib/perl
    /root/svn/devtoys/rally/lib
    /root/svn/devtoys/bug_emails
    /root/svn/devtoys/farm
    /opt/airwave/lib/perl5/5.10.1/i386-linux-thread-multi
    /opt/airwave/lib/perl5/5.10.1
    /opt/airwave/local/lib/perl5/site_perl/5.10.1/i386-linux-thread-multi
    /opt/airwave/local/lib/perl5/site_perl/5.10.1
    /opt/airwave/local/lib/perl5/site_perl/5.10.0/i386-linux-thread-multi
    /opt/airwave/local/lib/perl5/site_perl/5.10.0
    /opt/airwave/local/lib/perl5/site_perl
    /opt/airwave/lib/perl5/vendor_perl/5.10.1/i386-linux-thread-multi
    /opt/airwave/lib/perl5/vendor_perl/5.10.1
    /opt/airwave/lib/perl5/vendor_perl/5.10.0/i386-linux-thread-multi
    /opt/airwave/lib/perl5/vendor_perl/5.10.0
    /opt/airwave/lib/perl5/vendor_perl
    .

---
Environment for perl 5.10.1:
    HOME=/root
    LANG=en_US
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/usr/local/sbin:/usr/sbin:/sbin:/opt/airwave/local/bin:/opt/airwave/bin:/usr/local/sbin:/root/svn/mercury/bin:/usr/local/airwave/bin:/usr/java/jre/bin:/usr/java/jdk/bin:/var/airwave/support:/opt/condor/sbin:/opt/condor/bin:/opt/flex/bin:/opt/ant/bin:/usr/kerberos/sbin:/usr/kerberos/bin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin:/root/bin
    PERL5LIB=/root/svn/mercury/lib/perl:/root/svn/devtoys/rally/lib:/root/svn/devtoys/bug_emails:/root/svn/devtoys/farm
    PERL_BADLANG (unset)
    SHELL=/bin/bash
Subject: Re: [perl #75176] perlbug AutoReply: Symbol::delete_package does not free certain memory associated with package::ISA
Date: Tue, 18 May 2010 15:11:31 -0700
To: perlbug-followup [...] perl.org
From: Thomas Whaples <twhaples [...] arubanetworks.com>
Download (untitled) / with headers
text/plain 200b
For completeness (and to see whether an upgrade would make the problem just "go away") we compiled a version of Perl 5.12.0 and ran the script there as well, but the memory leak was still present.
whoops! i think that's *your* workflow I just clicked.
Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
Date: Sun, 25 Jul 2010 12:57:02 -0700
To: perl5-porters [...] perl.org
From: webmasters [...] ctosonline.org
Download (untitled) / with headers
text/plain 1.1k
This was caused by change 31239/70cd14 which fixes *ISA=[] to call mro_isa_changed_in. The actual underlying cause is explained in mro’s manpage, under mro::get_isarev: Currently, this list only grows, it never shrinks. This was a performance consideration (properly tracking and deleting isarev entries when someone removes an entry from an @ISA is costly, and it doesn't happen often anyways). If it doesn’t happen often, then the cost will not be incurred very often. Normally, the list of parent classes is cached, so Perl_mro_isa_changed_in can be made to compare it with the new list and delete isarev entries as appropriate. This may not be very efficient, but, again, it won’t happen very often, as the existing cached list will normally be empty. S_hfreeentries is the only other routine that clears the cache, so I can make that remove isarev entries, too. If I write a patch for this, is there any remote possibility of its being applied, or would it be a waste of time? Another possibility for fixing this particular case is to add a function to mro:: for deleting isarev entries for particular classes. Then Symbol::delete_package could be made to use that.
CC: perl5-porters [...] perl.org
Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
Date: Tue, 27 Jul 2010 15:25:07 +0100
To: webmasters [...] ctosonline.org
From: Nicholas Clark <nick [...] ccl4.org>
Download (untitled) / with headers
text/plain 1.6k
On Sun, Jul 25, 2010 at 12:57:02PM -0700, webmasters@ctosonline.org wrote: Show quoted text
> This was caused by change 31239/70cd14 which fixes *ISA=[] to call mro_isa_changed_in. > > The actual underlying cause is explained in mro?s manpage, under mro::get_isarev: > > Currently, this list only grows, it never shrinks. This was a > performance consideration (properly tracking and deleting isarev > entries when someone removes an entry from an @ISA is costly, and it > doesn't happen often anyways). > > If it doesn?t happen often, then the cost will not be incurred very often. > > Normally, the list of parent classes is cached, so Perl_mro_isa_changed_in can be made to compare it with the new list and delete isarev entries as appropriate. This may not be very efficient, but, again, it won?t happen very often, as the existing cached list will normally be empty. > > S_hfreeentries is the only other routine that clears the cache, so I can make that remove isarev entries, too. > > If I write a patch for this, is there any remote possibility of its being applied, or would it be a waste of time?
The documentation you quoted continues with: The fact that a class which no longer truly "isa" this class at runtime remains on the list should be considered a quirky implementation detail which is subject to future change. It shouldn't be an issue as long as you're looking at this list for the same reasons the core code does: as a performance optimization over having to search every class in existence. Yes, finding an efficient way to fix this would be useful and welcomed. Nicholas Clark
CC: perl5-porters [...] perl.org
Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
Date: Sun, 1 Aug 2010 12:17:36 -0700
To: Nicholas Clark <nick [...] ccl4.org>
From: Father Chrysostomos <sprout [...] cpan.org>
Download (untitled) / with headers
text/plain 3.1k
On Jul 27, 2010, at 7:25 AM, Nicholas Clark wrote: Show quoted text
> On Sun, Jul 25, 2010 at 12:57:02PM -0700, webmasters@ctosonline.org wrote:
>> This was caused by change 31239/70cd14 which fixes *ISA=[] to call mro_isa_changed_in. >> >> The actual underlying cause is explained in mro?s manpage, under mro::get_isarev: >> >> Currently, this list only grows, it never shrinks. This was a >> performance consideration (properly tracking and deleting isarev >> entries when someone removes an entry from an @ISA is costly, and it >> doesn't happen often anyways). >> >> If it doesn?t happen often, then the cost will not be incurred very often. >> >> Normally, the list of parent classes is cached, so Perl_mro_isa_changed_in can be made to compare it with the new list and delete isarev entries as appropriate. This may not be very efficient, but, again, it won?t happen very often, as the existing cached list will normally be empty. >> >> S_hfreeentries is the only other routine that clears the cache, so I can make that remove isarev entries, too. >> >> If I write a patch for this, is there any remote possibility of its being applied, or would it be a waste of time?
> > The documentation you quoted continues with: > > The fact that a class which no longer truly "isa" this class at > runtime remains on the list should be considered a quirky > implementation detail which is subject to future change. It shouldn't > be an issue as long as you're looking at this list for the same > reasons the core code does: as a performance optimization over having > to search every class in existence. > > > Yes, finding an efficient way to fix this would be useful and welcomed.
OK, now for the next set of questions: This script shows a regression in 5.10, with regard to stash assignments. I can rearrange packages to my heart’s content without triggering mro_isa_changed_in. @a'ISA = 'b'; @b'ISA = 'c'; sub c'bark { warn "Woof!" } sub d'bark { warn "Bow-wow!" } $a = bless [], a; $a->bark; # Woof! @e'ISA = 'd'; $b = delete $::{'b::'}; $::{'b::'} = delete $::{'e::'}; $a->bark; # Woof! in 5.10; Bow-wow! in 5.8 undef $b; $a->bark; # Bow-wow! print *{"b::"},"\n"; # Shows that it still thinks it is *e:: For my proposed isarev changes to work, every stash (or the glob containing the stash) needs to know whether it is still part of the main stash hierarchy, or whether it has been orphaned, so that it won’t delete isarev entries that do not belong to it. In the script above, ‘delete $::{'b::'}’ needs to trigger mro_isa_changed_in and the return value flagged such that it knows not to remove isarev entries when its @ISA is changed. For instance, c and d could both inherit from f. Without such a flag, @{*{$$b{'ISA::'}}{ARRAY}} = () will remove a from f’s isarev, which shouldn’t happen. The flags on SVs seem to be a rather crowded space. Am I right in thinking hashes cannot have get-magic? If that is the case, I can re-use SVs_GMG for this purpose. And then each stash needs to know its effective name, as opposed to its original name (see the last line of the script above). What would be the best way to do this? Store it in magic?
CC: perl5-porters [...] perl.org
Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
Date: Mon, 2 Aug 2010 10:34:14 +0100
To: Father Chrysostomos <sprout [...] cpan.org>
From: Nicholas Clark <nick [...] ccl4.org>
Download (untitled) / with headers
text/plain 1.7k
On Sun, Aug 01, 2010 at 12:17:36PM -0700, Father Chrysostomos wrote: [snip pathological but interesting example] Show quoted text
> The flags on SVs seem to be a rather crowded space. Am I right in thinking hashes cannot have get-magic? If that is the case, I can re-use SVs_GMG for this purpose.
I don't know with 100% certainty, but I see tests on SvGMAGICAL() in hv.c, this in mg.c: if (vtbl->svt_get && !(mg->mg_flags & MGf_GSKIP)) SvGMAGICAL_on(sv); and magic vtables in perl.h which have the svt_get function pointer set, such as MGVTBL_SET( PL_vtbl_sigelem, MEMBER_TO_FPTR(Perl_magic_getsig), MEMBER_TO_FPTR(Perl_magic_setsig), 0, MEMBER_TO_FPTR(Perl_magic_clearsig), 0, 0, 0, 0 ); so I'm assuming that yes, it is used. To the best of my knowledge there are no free flag bits on PVHV, and no more that can be scavenged. Show quoted text
> And then each stash needs to know its effective name, as opposed to its original name (see the last line of the script above). What would be the best way to do this? Store it in magic?
Having any magic on all stashes is a measurable performance hit. For a while in 5.9.x there was magic on stashes to store weak reference backreferences (no vtable actions for get or set) and that was enough to slow things down. That's why there's all the "extra" code to allow hashes to store backreferences in the hv_aux structure. I believe that that's the logical place to hang any extra data needed for this. Also, if it's easier to implement, I don't see a problem with using the global sub generation counter to flag when someone has directly manipulated the tree of stashes. It's not common, and I don't see an absolute need to calculate the strict set of packages affected, and call mro_changed_in() for just them. Nicholas Clark
CC: perl5-porters [...] perl.org
Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
Date: Sun, 8 Aug 2010 12:22:13 -0700
To: Nicholas Clark <nick [...] ccl4.org>
From: Father Chrysostomos <sprout [...] cpan.org>
Download (untitled) / with headers
text/plain 1.3k
On Aug 2, 2010, at 2:34 AM, Nicholas Clark wrote: Show quoted text
> That's why there's all the "extra" code to allow hashes to store backreferences > in the hv_aux structure. I believe that that's the logical place to hang any > extra data needed for this.
If you are suggesting adding new fields to the xpvhv_aux struct, what do I need to know about alignment issues? (After doing some research, it now looks as though I just need one more field; viz., a HEK* to store an alternate name.) Show quoted text
> Also, if it's easier to implement, I don't see a problem with using the global > sub generation counter to flag when someone has directly manipulated the tree > of stashes.
Doesn’t that just affect method caches? What about isa caches? Looking through the isa code, I haven’t yet seen anything that checks PL_sub_generation, but I may have missed it. If PL_sub_generation does not invalidate isa caches, would it be appropriate to add a new PL_isa_generation variable? Show quoted text
> It's not common, and I don't see an absolute need to calculate the > strict set of packages affected, and call mro_changed_in() for just them.
And I now realise that such would be very inefficient anyway, as nested classes have nothing to do with inheritance. (*p:: = *PPI:: would have to iterate through 94 packages, recalculating the linear isa cache, etc. *p:: = delete $::{"PPI"} would do that iteration twice.) Father Chrysostomos
CC: perl5-porters [...] perl.org
Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
Date: Sun, 8 Aug 2010 12:42:35 -0700
From: Father Chrysostomos <sprout [...] cpan.org>
Download (untitled) / with headers
text/plain 380b
On Aug 8, 2010, at 12:22 PM, Father Chrysostomos wrote: Show quoted text
> And I now realise that such would be very inefficient anyway, as nested classes have nothing to do with inheritance. (*p:: = *PPI:: would have to iterate through 94 packages, recalculating the linear isa cache, etc. *p:: = delete $::{"PPI"} would do that iteration twice.)
Oops. I meant delete $::{"PPI::"}, of course.
CC: Nicholas Clark <nick [...] ccl4.org>, perl5-porters [...] perl.org
Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
Date: Wed, 18 Aug 2010 17:47:52 +0100
To: Father Chrysostomos <sprout [...] cpan.org>
From: Dave Mitchell <davem [...] iabyn.com>
Download (untitled) / with headers
text/plain 537b
On Sun, Aug 08, 2010 at 12:22:13PM -0700, Father Chrysostomos wrote: Show quoted text
> If you are suggesting adding new fields to the xpvhv_aux struct, what do > I need to know about alignment issues? > > (After doing some research, it now looks as though I just need one more > field; viz., a HEK* to store an alternate name.)
Note that hv_clear and hv_undef may remove the aux struct, so you may not be able to rely on it always remaining. -- In England there is a special word which means the last sunshine of the summer. That word is "spring".
CC: Nicholas Clark <nick [...] ccl4.org>, perl5-porters [...] perl.org
Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
Date: Sun, 22 Aug 2010 12:11:27 -0700
To: Dave Mitchell <davem [...] iabyn.com>
From: Father Chrysostomos <sprout [...] cpan.org>
Download (untitled) / with headers
text/plain 565b
On Aug 18, 2010, at 9:47 AM, Dave Mitchell wrote: Show quoted text
> On Sun, Aug 08, 2010 at 12:22:13PM -0700, Father Chrysostomos wrote:
>> If you are suggesting adding new fields to the xpvhv_aux struct, what do >> I need to know about alignment issues? >> >> (After doing some research, it now looks as though I just need one more >> field; viz., a HEK* to store an alternate name.)
> > Note that hv_clear and hv_undef may remove the aux struct, so you may not > be able to rely on it always remaining.
What happens to the name currently stored in there when that happens?
CC: Nicholas Clark <nick [...] ccl4.org>, perl5-porters [...] perl.org
Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
Date: Sun, 22 Aug 2010 17:39:15 -0700
From: Father Chrysostomos <sprout [...] cpan.org>
Download (untitled) / with headers
text/plain 843b
On Aug 8, 2010, at 12:22 PM, Father Chrysostomos wrote: Show quoted text
> If PL_sub_generation does not invalidate isa caches, would it be appropriate to add a new PL_isa_generation variable? >
>> It's not common, and I don't see an absolute need to calculate the >> strict set of packages affected, and call mro_changed_in() for just them.
> > And I now realise that such would be very inefficient anyway, as nested classes have nothing to do with inheritance. (*p:: = *PPI:: would have to iterate through 94 packages, recalculating the linear isa cache, etc. *p:: = delete $::{"PPI"} would do that iteration twice.)
I think we will have to live with this inefficiency. Anyway, most of the time I alias packages, it’s just for a few. The most common example in my code is ‘use DDS’ which loads DDS.pm, which aliases DDS:: to Data'Dump'Streamer::.

Message body is not shown because sender requested not to inline it.

Message body is not shown because sender requested not to inline it.

CC: Nicholas Clark <nick [...] ccl4.org>, perl5-porters [...] perl.org
Subject: Fwd: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
Date: Sun, 22 Aug 2010 17:46:29 -0700
From: Father Chrysostomos <sprout [...] cpan.org>
Download (untitled) / with headers
text/plain 2.4k
Oops. I forgot the main part of the message and just sent the postscript. Here it is: Begin forwarded message: Show quoted text
> From: Father Chrysostomos > Date: August 22, 2010 5:39:15 PM PDT > Cc: Nicholas Clark, p5p > Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA > > > On Aug 8, 2010, at 12:22 PM, Father Chrysostomos wrote: >
>> If PL_sub_generation does not invalidate isa caches, would it be appropriate to add a new PL_isa_generation variable?
I tried PL_sub_generation, and found that it doesn’t work. I tried adding PL_isa_generation (attached, in case you want to see it; not for application), which worked, but then I realised that it would mean iterating through *all* packages afterwards to reconstruct PL_isarev. So I’ve used a different approach with the other patch attached (1. reset isa...). When a stash is manipulated, it calls mro_isa_changed_in on the affected packages and iterates through their subpackages, doing the same. It does not yet take into account all methods of manipulating the stashes. More patches will follow. What does this have to do with the original bug? Initially I planned to fix the isarev leaks by making appropriate adjustments to it whenever an @ISA is modified or a stash is freed. But, since stashes can be detached from the main tree of stashes ($stash = delete $::{"it::"}), they need to know whether they are still attached, so detached stashes don’t muddle up isarev and delete entries that are still needed (which would be worse than leaking). In the process of experimenting with that, I found that such stash manipulations already fail to update isa caches, which, too, would cause my proposed isarev changes to introduce regressions. So that’s what this patch is for. Show quoted text
>>
>>> It's not common, and I don't see an absolute need to calculate the >>> strict set of packages affected, and call mro_changed_in() for just them.
>> >> And I now realise that such would be very inefficient anyway, as nested classes have nothing to do with inheritance. (*p:: = *PPI:: would have to iterate through 94 packages, recalculating the linear isa cache, etc. *p:: = delete $::{"PPI"} would do that iteration twice.)
> > I think we will have to live with this inefficiency. Anyway, most of the time I alias packages, it’s just for a few. The most common example in my code is ‘use DDS’ which loads DDS.pm, which aliases DDS:: to Data'Dump'Streamer::.

Message body is not shown because sender requested not to inline it.

Message body is not shown because sender requested not to inline it.

Show quoted text
>
CC: Nicholas Clark <nick [...] ccl4.org>, perl5-porters [...] perl.org
Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
Date: Sun, 22 Aug 2010 20:20:56 -0700
From: Father Chrysostomos <sprout [...] cpan.org>
Download (untitled) / with headers
text/plain 357b
On Aug 22, 2010, at 5:46 PM, Father Chrysostomos wrote: Show quoted text
> So I’ve used a different approach with the other patch attached (1. reset isa...). When a stash is manipulated, it calls mro_isa_changed_in on the affected packages and iterates through their subpackages, doing the same.
And I forgot to add the new test to MANIFEST. Here’s a patch for that.

Message body is not shown because sender requested not to inline it.

CC: Nicholas Clark <nick [...] ccl4.org>, perl5-porters [...] perl.org
Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
Date: Mon, 23 Aug 2010 12:50:20 +0100
To: Father Chrysostomos <sprout [...] cpan.org>
From: Dave Mitchell <davem [...] iabyn.com>
Download (untitled) / with headers
text/plain 934b
On Sun, Aug 22, 2010 at 12:11:27PM -0700, Father Chrysostomos wrote: Show quoted text
> On Aug 18, 2010, at 9:47 AM, Dave Mitchell wrote: >
> > On Sun, Aug 08, 2010 at 12:22:13PM -0700, Father Chrysostomos wrote:
> >> If you are suggesting adding new fields to the xpvhv_aux struct, what do > >> I need to know about alignment issues? > >> > >> (After doing some research, it now looks as though I just need one more > >> field; viz., a HEK* to store an alternate name.)
> > > > Note that hv_clear and hv_undef may remove the aux struct, so you may not > > be able to rely on it always remaining.
> > What happens to the name currently stored in there when that happens?
See S_hfreeentries: it special-cases adding name back at the end. -- Wesley Crusher gets beaten up by his classmates for being a smarmy git, and consequently has a go at making some friends of his own age for a change. -- Things That Never Happen in "Star Trek" #18
CC: Nicholas Clark <nick [...] ccl4.org>, perl5-porters [...] perl.org
Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
Date: Mon, 23 Aug 2010 14:07:06 +0100
To: Father Chrysostomos <sprout [...] cpan.org>
From: Dave Mitchell <davem [...] iabyn.com>
Download (untitled) / with headers
text/plain 468b
On Mon, Aug 23, 2010 at 12:50:20PM +0100, Dave Mitchell wrote: Show quoted text
> See S_hfreeentries: it special-cases adding name back at the end.
In fact I'm tempted to modify this code so that the aux structure is always maintained (ecvne for hv_undef) when there's something that nedds to be kept in it, so that back-references no longer have to be shuffled betwween hv_aux and magic. -- In economics, the exam questions are the same every year. They just change the answers.
CC: Nicholas Clark <nick [...] ccl4.org>, perl5-porters [...] perl.org
Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
Date: Sun, 29 Aug 2010 14:09:07 -0700
From: Father Chrysostomos <sprout [...] cpan.org>
Download (untitled) / with headers
text/plain 1.7k

On Aug 22, 2010, at 5:46 PM, Father Chrysostomos wrote:

Show quoted text
Initially I planned to fix the isarev leaks by making appropriate adjustments to it whenever an @ISA is modified or a stash is freed. But, since stashes can be detached from the main tree of stashes ($stash = delete $::{"it::"}), they need to know whether they are still attached, so detached stashes don’t muddle up isarev and delete entries that are still needed (which would be worse than leaking).

In the process of experimenting with that, I found that such stash manipulations already fail to update isa caches, which, too, would cause my proposed isarev changes to introduce regressions. So that’s what this patch is for.

Here is a patch that deals with a couple more ways of manipulating stashes.

I plan to work on assignment to empty stash elements next (or any stash elements that are not globs). I can do this by using PVLVs. But there are two approaches:

1. Do it just for access from Perl by adding it to pp_helem.
2. Do it for XS access as well, by modifying hv_fetch_ent instead (or hv_common, or wherever the actual code is).

Number 1 makes it too easy for XS code to make changes without triggering mro_package_moved. mro_package_moved will also have to be made public (and probably given a better name).
Number 2 is probably more reliable. perl’s internals can pass a flag to avoid the PVLVs. XS code that assumes that stash elements are never magical will be broken (code that specifically calls the _nomg forms to avoid the magic checks). Most XS code will continue to work.

So in both cases, XS code will have to be modified (if there is any that plays with stashes). But in case 2 only code that is currently potentially buggy will have to change.

I prefer number 2.

Message body is not shown because sender requested not to inline it.

CC: Nicholas Clark <nick [...] ccl4.org>, perl5-porters [...] perl.org
Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
Date: Mon, 30 Aug 2010 00:20:08 +0200
To: Father Chrysostomos <sprout [...] cpan.org>
From: demerphq <demerphq [...] gmail.com>
Download (untitled) / with headers
text/plain 2.3k
On 29 August 2010 23:09, Father Chrysostomos <sprout@cpan.org> wrote: Show quoted text
> > On Aug 22, 2010, at 5:46 PM, Father Chrysostomos wrote: > > Initially I planned to fix the isarev leaks by making appropriate > adjustments to it whenever an @ISA is modified or a stash is freed. But, > since stashes can be detached from the main tree of stashes ($stash = delete > $::{"it::"}), they need to know whether they are still attached, so detached > stashes don’t muddle up isarev and delete entries that are still needed > (which would be worse than leaking). > > In the process of experimenting with that, I found that such stash > manipulations already fail to update isa caches, which, too, would cause my > proposed isarev changes to introduce regressions. So that’s what this patch > is for. > > Here is a patch that deals with a couple more ways of manipulating stashes. > I plan to work on assignment to empty stash elements next (or any stash > elements that are not globs). I can do this by using PVLVs. But there are > two approaches: > 1. Do it just for access from Perl by adding it to pp_helem. > 2. Do it for XS access as well, by modifying hv_fetch_ent instead (or > hv_common, or wherever the actual code is). > Number 1 makes it too easy for XS code to make changes without triggering > mro_package_moved. mro_package_moved will also have to be made public (and > probably given a better name). > Number 2 is probably more reliable. perl’s internals can pass a flag to > avoid the PVLVs. XS code that assumes that stash elements are never magical > will be broken (code that specifically calls the _nomg forms to avoid the > magic checks). Most XS code will continue to work. > So in both cases, XS code will have to be modified (if there is any that > plays with stashes). But in case 2 only code that is currently potentially > buggy will have to change. > I prefer number 2.
+ if (stype == SVt_PVHV) { + const char * const name = GvNAME((GV*)dstr); + const STRLEN len = GvNAMELEN(dstr); + if (len > 1 && name[len-2] == ':' && name[len-1] == ':') + if(HvNAME(dref)) mro_package_moved((HV *)dref); + if(HvNAME(sref)) mro_package_moved((HV *)sref); + } Did you miss a { } there? From the indenting it looks like the if () applies to both of the following ifs, but it is an if (EXPR) STMT; not an IF (EXPR) BLOCK yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
CC: Nicholas Clark <nick [...] ccl4.org>, perl5-porters [...] perl.org
Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
Date: Sun, 29 Aug 2010 17:13:28 -0700
To: demerphq <demerphq [...] gmail.com>
From: Father Chrysostomos <sprout [...] cpan.org>
Download (untitled) / with headers
text/plain 645b
On Aug 29, 2010, at 3:20 PM, demerphq wrote: Show quoted text
> On 29 August 2010 23:09, Father Chrysostomos <sprout@cpan.org> wrote: > + if (stype == SVt_PVHV) { > + const char * const name = GvNAME((GV*)dstr); > + const STRLEN len = GvNAMELEN(dstr); > + if (len > 1 && name[len-2] == ':' && name[len-1] == ':') > + if(HvNAME(dref)) mro_package_moved((HV *)dref); > + if(HvNAME(sref)) mro_package_moved((HV *)sref); > + } > > Did you miss a { } there? From the indenting it looks like the if () > applies to both of the following ifs, but it is an if (EXPR) STMT; not > an IF (EXPR) BLOCK
Thank you for catching that. Here is a fixed version.

Message body is not shown because sender requested not to inline it.

CC: Nicholas Clark <nick [...] ccl4.org>, perl5-porters [...] perl.org
Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
Date: Sun, 5 Sep 2010 13:14:55 -0700
From: Father Chrysostomos <sprout [...] cpan.org>
Download (untitled) / with headers
text/plain 1.2k
On Aug 22, 2010, at 5:46 PM, Father Chrysostomos wrote: Show quoted text
> Oops. I forgot the main part of the message and just sent the postscript. Here it is: > > Begin forwarded message: >
>> From: Father Chrysostomos >> Date: August 22, 2010 5:39:15 PM PDT >> Cc: Nicholas Clark, p5p >> Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA >> >> >> On Aug 8, 2010, at 12:22 PM, Father Chrysostomos wrote: >>
>>> If PL_sub_generation does not invalidate isa caches, would it be appropriate to add a new PL_isa_generation variable?
> > I tried PL_sub_generation, and found that it doesn’t work. I tried adding PL_isa_generation (attached, in case you want to see it; not for application), which worked, but then I realised that it would mean iterating through *all* packages afterwards to reconstruct PL_isarev. > > So I’ve used a different approach with the other patch attached (1. reset isa...). When a stash is manipulated, it calls mro_isa_changed_in on the affected packages and iterates through their subpackages, doing the same.
That patch can cause a bus error, so here is a new version. The third patch no longer applies with this version, so here is a new version of that, too (3b).

Message body is not shown because sender requested not to inline it.

Message body is not shown because sender requested not to inline it.

CC: Nicholas Clark <nick [...] ccl4.org>, perl5-porters [...] perl.org
Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
Date: Sun, 5 Sep 2010 13:17:30 -0700
From: Father Chrysostomos <sprout [...] cpan.org>
Download (untitled) / with headers
text/plain 2.3k

On Aug 29, 2010, at 2:09 PM, Father Chrysostomos wrote:

Show quoted text

On Aug 22, 2010, at 5:46 PM, Father Chrysostomos wrote:

Initially I planned to fix the isarev leaks by making appropriate adjustments to it whenever an @ISA is modified or a stash is freed. But, since stashes can be detached from the main tree of stashes ($stash = delete $::{"it::"}), they need to know whether they are still attached, so detached stashes don’t muddle up isarev and delete entries that are still needed (which would be worse than leaking).

In the process of experimenting with that, I found that such stash manipulations already fail to update isa caches, which, too, would cause my proposed isarev changes to introduce regressions. So that’s what this patch is for.

Here is a patch that deals with a couple more ways of manipulating stashes.

I plan to work on assignment to empty stash elements next (or any stash elements that are not globs). I can do this by using PVLVs. But there are two approaches:

1. Do it just for access from Perl by adding it to pp_helem.
2. Do it for XS access as well, by modifying hv_fetch_ent instead (or hv_common, or wherever the actual code is).

Number 1 makes it too easy for XS code to make changes without triggering mro_package_moved. mro_package_moved will also have to be made public (and probably given a better name).
Number 2 is probably more reliable. perl’s internals can pass a flag to avoid the PVLVs. XS code that assumes that stash elements are never magical will be broken (code that specifically calls the _nomg forms to avoid the magic checks). Most XS code will continue to work.

So in both cases, XS code will have to be modified (if there is any that plays with stashes). But in case 2 only code that is currently potentially buggy will have to change.

I prefer number 2.

Here is a patch for this. I used approach number 1 because number 2 proved to be far more complicated than I had thought, and probably too risky. I am not so sure now that mro_package_moved needs to be made public. But we could change that later if necessary.

I did not bother with a mathom for mro_isa_changed_in, since this patch relies on the fix for #77362, which changes behaviour in a way that is not suitable for maint. (There are many other bugs that can occur more often as a result of that fix.)

Message body is not shown because sender requested not to inline it.

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 434b
On Sun Sep 05 13:15:28 2010, sprout wrote: Show quoted text
> > On Aug 22, 2010, at 5:46 PM, Father Chrysostomos wrote:
> > So I’ve used a different approach with the other patch attached (1.
> reset isa...). When a stash is manipulated, it calls > mro_isa_changed_in on the affected packages and iterates through > their subpackages, doing the same. > > That patch can cause a bus error, so here is a new version.
Applied as c8bbf675c.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 540b
On Sun Aug 22 20:21:25 2010, sprout wrote: Show quoted text
> > On Aug 22, 2010, at 5:46 PM, Father Chrysostomos wrote: >
> > So I’ve used a different approach with the other patch attached (1.
> reset isa...). When a stash is manipulated, it calls > mro_isa_changed_in on the affected packages and iterates through > their subpackages, doing the same. > > And I forgot to add the new test to MANIFEST. Here’s a patch for that.
I’m now putting the tests in package_alias.t or whatever it’s called, so that patch is no longer applicable.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 255b
On Sun Sep 05 13:15:28 2010, sprout wrote: Show quoted text
> That patch can cause a bus error, so here is a new version. The third > patch no longer applies with this version, so here is a new version > of that, too (3b).
3b has been applied as 1c93aaac75354eeef.
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 337b
On Sat Oct 09 23:06:49 2010, sprout wrote: Show quoted text
> On Sun Sep 05 13:15:28 2010, sprout wrote:
> > That patch can cause a bus error, so here is a new version. The third > > patch no longer applies with this version, so here is a new version > > of that, too (3b).
> > 3b has been applied as 1c93aaac75354eeef.
Er, actually 3e79609f389.
RT-Send-CC: perl5-porters [...] perl.org
On Sun Sep 05 13:18:02 2010, sprout wrote: Show quoted text
> > I plan to work on assignment to empty stash elements next (or any
> stash elements that are not globs). I can do this by using PVLVs. > But there are two approaches:
> > > > 1. Do it just for access from Perl by adding it to pp_helem. > > 2. Do it for XS access as well, by modifying hv_fetch_ent instead
> (or hv_common, or wherever the actual code is).
> > > > Number 1 makes it too easy for XS code to make changes without
> triggering mro_package_moved. mro_package_moved will also have to > be made public (and probably given a better name).
> > Number 2 is probably more reliable. perl’s internals can pass a flag
> to avoid the PVLVs. XS code that assumes that stash elements are > never magical will be broken (code that specifically calls the > _nomg forms to avoid the magic checks). Most XS code will continue > to work.
> > > > So in both cases, XS code will have to be modified (if there is any
> that plays with stashes). But in case 2 only code that is currently > potentially buggy will have to change.
> > > > I prefer number 2.
> > Here is a patch for this. I used approach number 1 because number 2 > proved to be far more complicated than I had thought, and probably > too risky. I am not so sure now that mro_package_moved needs to be > made public. But we could change that later if necessary.
That fourth patch is no good. Not only does it have mistakes, but the whole concept is flawed. Here are some of the problems: • It only works with top-level packages • References to stash elements that are not globs no longer reference the SVs themselves, but the named stash elements. • Actually, those references would probably stringify with LVALUE, not GLOB/SCALAR. • Two references to the same element would not compare equal. • References to stash elements containing glob copies--well, let::s not get into that. And I now think that assignments to empty stash elements should not be done by user code anyway. See #78074.
Now fixed by 80ebaca and a couple dozen patches leading up to it.


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