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

memory leak in array delete #7428

Closed
p5pRT opened this issue Jul 18, 2004 · 10 comments
Closed

memory leak in array delete #7428

p5pRT opened this issue Jul 18, 2004 · 10 comments

Comments

@p5pRT
Copy link

p5pRT commented Jul 18, 2004

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

Searchable as RT30733$

@p5pRT
Copy link
Author

p5pRT commented Jul 18, 2004

From perl-5.8.0@ton.iguana.be

Created by perl-5.8.0@ton.iguana.be

This leaks about 20 megabytes per second for me​:

#!/usr/bin/perl -w
for (1..1e9) {
  $z = delete $array[0];
  $array[0] = 0;
}

Perl Info

Flags:
    category=core
    severity=medium

Site configuration information for perl v5.8.4:

Configured by ton at Thu Jun  3 13:28:19 CEST 2004.

Summary of my perl5 (revision 5 version 8 subversion 4) configuration:
  Platform:
    osname=linux, osvers=2.6.5, archname=i686-linux-64int-ld
    uname='linux quasar 2.6.5 #8 mon apr 5 05:41:20 cest 2004 i686 gnulinux '
    config_args=''
    hint=recommended, useposix=true, d_sigaction=define
    usethreads=undef use5005threads=undef useithreads=undef usemultiplicity=undef
    useperlio=define d_sfio=undef uselargefiles=define usesocks=undef
    use64bitint=define use64bitall=undef uselongdouble=define
    usemymalloc=y, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-fno-strict-aliasing -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O2 -fomit-frame-pointer',
    cppflags='-fno-strict-aliasing -I/usr/local/include'
    ccversion='', gccversion='3.4.0 20031231 (experimental)', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long long', ivsize=8, nvtype='long double', nvsize=12, Off_t='off_t', lseeksize=8
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib
    libs=-lnsl -ldb -ldl -lm -lcrypt -lutil -lc
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
    libc=/lib/libc-2.3.2.so, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.3.2'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fpic', lddlflags='-shared -L/usr/local/lib'

Locally applied patches:
    


@INC for perl v5.8.4:
    /usr/lib/perl5/5.8.4/i686-linux-64int-ld
    /usr/lib/perl5/5.8.4
    /usr/lib/perl5/site_perl/5.8.4/i686-linux-64int-ld
    /usr/lib/perl5/site_perl/5.8.4
    /usr/lib/perl5/site_perl
    .


Environment for perl v5.8.4:
    HOME=/home/ton
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/ton/bin.Linux:/home/ton/bin:/home/ton/bin.SampleSetup:/opt/schily/bin:/usr/local/bin:/usr/local/sbin:/home/oracle/product/9.0.1/bin:/usr/local/ar/bin:/usr/games/bin:/usr/X11R6/bin:/usr/share/bin:/usr/bin:/usr/sbin:/bin:/sbin:.
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Jul 22, 2004

From @Tux

On 18 Jul 2004 22​:16​:25 -0000, Perl-5 . 8 . 0 @​ Ton . Iguana . Be
<perlbug-followup@​perl.org> wrote​:

This leaks about 20 megabytes per second for me​:

#!/usr/bin/perl -w
for (1..1e9) {
$z = delete $array[0];
$array[0] = 0;
}

even if you add two our's for $z and @​array, it blows up pretty fast (but
at least I ran that under use strict and use warnings

I didn't try with lexicals

--
Enjoy, have FUN! H.Merijn

@p5pRT
Copy link
Author

p5pRT commented Jul 22, 2004

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

@p5pRT
Copy link
Author

p5pRT commented Jul 22, 2004

From @iabyn

On Sun, Jul 18, 2004 at 10​:16​:25PM -0000, perl-5. 8. 0 @​ ton. iguana. be wrote​:

This leaks about 20 megabytes per second for me​:

#!/usr/bin/perl -w
for (1..1e9) {
$z = delete $array[0];
$array[0] = 0;
}

it doesn't leak if you replace $array[0] with $hash{foo}.

This boils down to the fact that hv_delete_ent() mortalizes the returned
sv, while av_delete() doesn't. I presume its a simle bug in av_delete(),
but can anyone think of a valid reason why it shouldn't sv_2mortal()
it's returned value?

Dave.

--
Hofstadter's Law​:
It always takes longer than you expect, even when you take into account
Hofstadter's Law.

@p5pRT
Copy link
Author

p5pRT commented Jul 23, 2004

From @iabyn

On Thu, Jul 22, 2004 at 09​:52​:48PM +0100, Dave Mitchell wrote​:

On Sun, Jul 18, 2004 at 10​:16​:25PM -0000, perl-5. 8. 0 @​ ton. iguana. be wrote​:

This leaks about 20 megabytes per second for me​:

#!/usr/bin/perl -w
for (1..1e9) {
$z = delete $array[0];
$array[0] = 0;
}

it doesn't leak if you replace $array[0] with $hash{foo}.

This boils down to the fact that hv_delete_ent() mortalizes the returned
sv, while av_delete() doesn't. I presume its a simle bug in av_delete(),
but can anyone think of a valid reason why it shouldn't sv_2mortal()
it's returned value?

Now fixed by the change below.

Dave.

--
The perl5 internals are a complete mess. It's like Jenga - to get the
perl5 tower taller and do something new you select a block somewhere in
the middle, with trepidation pull it out slowly, and then carefully
balance it somewhere new, hoping the whole edifice won't collapse as a
result.
  - Nicholas Clark, based on an original by Simon Cozens.

Change 23158 by davem@​davem-percy on 2004/07/23 11​:06​:02

  [perl #30733] memory leak in array delete
  av_delete() wasn't mortalizing the returned value

Affected files ...

... //depot/perl/av.c#78 edit
... //depot/perl/t/op/delete.t#10 edit

Differences ...

==== //depot/perl/av.c#78 (text) ====

@​@​ -864,6 +864,8 @​@​
  SvREFCNT_dec(sv);
  sv = Nullsv;
  }
+ else
+ sv = sv_2mortal(sv);
  return sv;
}

==== //depot/perl/t/op/delete.t#10 (xtext) ====

@​@​ -1,6 +1,6 @​@​
#!./perl

-print "1..37\n";
+print "1..38\n";

# delete() on hash elements

@​@​ -129,3 +129,16 @​@​
  print "not " if defined $y;
  print "ok 37\n";
}
+
+{
+ # [perl #30733] array delete didn't free returned element
+ my $x = 0;
+ sub X​::DESTROY { $x++ }
+ {
+ my @​a;
+ $a[0] = bless [], 'X';
+ my $y = delete $a[0];
+ }
+ print "not " unless $x == 1;
+ print "ok 38\n";
+}

@p5pRT
Copy link
Author

p5pRT commented Jul 23, 2004

@rspier - Status changed from 'open' to 'resolved'

@p5pRT p5pRT closed this as completed Jul 23, 2004
@p5pRT
Copy link
Author

p5pRT commented Jul 23, 2004

From @gsar

On Fri, 23 Jul 2004 12​:27​:31 BST, Dave Mitchell wrote​:

On Thu, Jul 22, 2004 at 09​:52​:48PM +0100, Dave Mitchell wrote​:

On Sun, Jul 18, 2004 at 10​:16​:25PM -0000, perl-5. 8. 0 @​ ton. iguana. be wro
te​:

This leaks about 20 megabytes per second for me​:

#!/usr/bin/perl -w
for (1..1e9) {
$z = delete $array[0];
$array[0] = 0;
}

it doesn't leak if you replace $array[0] with $hash{foo}.

This boils down to the fact that hv_delete_ent() mortalizes the returned
sv, while av_delete() doesn't. I presume its a simle bug in av_delete(),
but can anyone think of a valid reason why it shouldn't sv_2mortal()
it's returned value?

Now fixed by the change below.
[...]
Change 23158 by davem@​davem-percy on 2004/07/23 11​:06​:02

[perl #30733] memory leak in array delete
av_delete() wasn't mortalizing the returned value

I'd suggest mortalizing only if it is AvREAL(av).

FWIW, the previous behavior was probably a result of trying to be
consistent with the semantics of av_pop() and av_shift(), neither
of which adjust the refcount.

Thanks,

Sarathy
gsar@​ActiveState.com

@p5pRT
Copy link
Author

p5pRT commented Jul 23, 2004

From @iabyn

On Fri, Jul 23, 2004 at 09​:22​:45AM -0700, Gurusamy Sarathy wrote​:

I'd suggest mortalizing only if it is AvREAL(av).

Good point.

FWIW, the previous behavior was probably a result of trying to be
consistent with the semantics of av_pop() and av_shift(), neither
of which adjust the refcount.

Ah. I've just noticed that pp_pop() does the mortalizing rather than
av_pop(), in contrast to hv_delete(). I wonder whether my fix is best
moved from av_delete() to pp_delete()?

Neither the docs for hv_delete() nor av_delete() make any statement
about mortalizing and refcounts.

--
"The GPL violates the U.S. Constitution, together with copyright,
antitrust and export control laws"
  -- SCO smoking crack again.

@p5pRT
Copy link
Author

p5pRT commented Jul 23, 2004

From @gsar

On Fri, 23 Jul 2004 19​:50​:26 BST, Dave Mitchell wrote​:

On Fri, Jul 23, 2004 at 09​:22​:45AM -0700, Gurusamy Sarathy wrote​:

FWIW, the previous behavior was probably a result of trying to be
consistent with the semantics of av_pop() and av_shift(), neither
of which adjust the refcount.

Ah. I've just noticed that pp_pop() does the mortalizing rather than
av_pop(), in contrast to hv_delete(). I wonder whether my fix is best
moved from av_delete() to pp_delete()?

Tough call.

Might be a good idea if retaining backward compatibility is
important.

It comes down to a choice between internal consistency of the
av_*() APIs vs consistency between the av_*() and hv_*() APIs.
I know I would be more likely to change an av_pop() to an
av_delete() somewhere and promptly forget to handle the
refcount correctly, if they behaved differently. Others could
obviously say the same about hv_delete() vs av_delete().

Sarathy
gsar@​ActiveState.com

@p5pRT
Copy link
Author

p5pRT commented Jul 24, 2004

From @iabyn

On Fri, Jul 23, 2004 at 12​:21​:47PM -0700, Gurusamy Sarathy wrote​:

Ah. I've just noticed that pp_pop() does the mortalizing rather than
av_pop(), in contrast to hv_delete(). I wonder whether my fix is best
moved from av_delete() to pp_delete()?

Tough call.

Might be a good idea if retaining backward compatibility is
important.

It comes down to a choice between internal consistency of the
av_*() APIs vs consistency between the av_*() and hv_*() APIs.
I know I would be more likely to change an av_pop() to an
av_delete() somewhere and promptly forget to handle the
refcount correctly, if they behaved differently. Others could
obviously say the same about hv_delete() vs av_delete().

One the other hand I just had a go at moving it from av_delete to
pp_delete, and started getting into hot water over tying​: sometimes the
thing returned by av_delete() may already be mortal, so remortalizing it
does nasty things. Which means that it needs to be wrapped in stuff
like
  if (!lval && SvMAGICAL(sv)) sv = mortalcopy(sv) else sv = sv_2mortal(sv).

And pp_delete() calls av_delete() twice, so this needs to be done in two
places.

So all other things being equal, I want to leave the mortalizing in
av_delete().

dave.

--
You live and learn (although usually you just live).

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