Symbol::delete_package does not free certain memory associated with package::ISA

[p5pRT]

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

Searchable as RT75176$

[p5pRT]

From dev@airwave.com

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");
}

Perl Info

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

[p5pRT]

From dev@airwave.com

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.

[p5pRT]

dev@airwave.com - Status changed from 'new' to 'open'

[p5pRT]

From dev@airwave.com

whoops! i think that's *your* workflow I just clicked.

[p5pRT]

dev@airwave.com - Status changed from 'open' to 'new'

[p5pRT]

From webmasters@ctosonline.org

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.

[p5pRT]

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

[p5pRT]

From @nwc10

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.

Nicholas Clark

[p5pRT]

From @cpansprout

On Jul 27, 2010, at 7​:25 AM, Nicholas Clark wrote​:

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?

[p5pRT]

From @nwc10

On Sun, Aug 01, 2010 at 12​:17​:36PM -0700, Father Chrysostomos wrote​:

[snip pathological but interesting example]

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.

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

[p5pRT]

From @cpansprout

On Aug 2, 2010, at 2​:34 AM, Nicholas Clark wrote​:

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.)

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?

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

[p5pRT]

From @cpansprout

On Aug 8, 2010, at 12​:22 PM, Father Chrysostomos wrote​:

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.

[p5pRT]

From @iabyn

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.

--
In England there is a special word which means the last sunshine
of the summer. That word is "spring".

[p5pRT]

From @cpansprout

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?

[p5pRT]

From @cpansprout

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?

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​::.