Re: memory leak in DBI XS bootstrap code
am 13.07.2006 01:10:09 von Tim.BunceOn Wed, Jul 12, 2006 at 10:12:26AM -0700, Ephraim Dan wrote:
> > You could also try another change a few lines further up where it says
> >
> > #ifdef sv_rvweaken
> > if (1) {
> >
> > Change the 1 to a 0 to disable the use of weakrefs which was added in
> > 1.49.
>
> [edan] This seems to have done it! It fixes all the leaks! Hurrah!
Did you try the other suggestions?
How much improvement did each give? (when testing by destroying the interpreter)
> So... do I need to keep this patched myself, at the risk of breaking
> when the code changes in some future release, or can you somehow make
> this optional, or fix it,
I'd probably accept a patch that adds an option to Makefile.PL that then
passes a -DDBI_NO_WEAKREFS to the compiler which, with a suitable
#ifndef in DBI.xs, would disable that code. Or a new per-handle
flag could be added.
But I'm reluctant to do either till I know more about the cause.
> or explain to me what I can do to make the
> leak go away using the current code?
I need more info - see above.
Tim.
> Thanks so much,
> edan
>
> > > > -----Original Message-----
> > > > From: Tim Bunce [mailto:Tim.Bunce@pobox.com]
> > > > Sent: Tuesday, July 11, 2006 16:59
> > > > To: Ephraim Dan
> > > > Cc: Tim Bunce; dbi-users@perl.org
> > > > Subject: Re: memory leak in DBI XS bootstrap code
> > > >
> > > > I'm sorry but I simply don't have the time at the moment.
> > > >
> > > > Some random suggestions:
> > > > - try using different sets of methods to isolate the minimum
> > > > requirements.
> > > > for example:
> > > > call fetchrow_array just once.
> > > > don't call fetchrow_array at all.
> > > > call fetchrow_array in loop but don't call finish
> > > > don't execute, fetch, finish.
> > > > etc etc etc
> > > > - try with other drivers to see if it's the driver or the DBI that's
> > > > leaking
> > > >
> > > > Tim.
> > > >
> > > > On Tue, Jul 11, 2006 at 03:59:05AM -0700, Ephraim Dan wrote:
> > > > > Tim,
> > > > >
> > > > > I'm experiencing further memory leaks which are causing me more
> > grief.
> > > > I put together the following test script which illustrates the
> > leakage:
> > > > >
> > > > > #/usr/bin/perl
> > > > > # dbi.pl
> > > > >
> > > > > use DBI;
> > > > > use Devel::Leak;
> > > > > my $leak_handle;
> > > > >
> > > > > for ( 1 .. 1000 ) {
> > > > > my $dbh = DBI->connect("DBI:mysql:MYDB","user","secret",
> > > > {RaiseError=>1});
> > > > > my $sth = $dbh->prepare("SELECT COUNT(*) FROM TABLE");
> > > > > $sth->execute();
> > > > > while ( my ($c) = $sth->fetchrow_array() ) {
> > > > > # print "$c\n";
> > > > > }
> > > > > $sth->finish;
> > > > > $dbh->disconnect;
> > > > >
> > > > > system("ps -aux | fgrep dbi.pl | fgrep -v fgrep");
> > > > > test_leak();
> > > > > }
> > > > >
> > > > > sub test_leak {
> > > > > if ( $handle ) {
> > > > > Devel::Leak::CheckSV($handle);
> > > > > }
> > > > > print STDERR "count: " . Devel::Leak::NoteSV($handle) . "\n";
> > > > > }
> > > > >
> > > > > Devel::Leak shows that it is leaking 4 "things" every iteration:
> > > > >
> > > > > count: 12128
> > > > > root 17720 17.8 0.3 6840 4220 pts/0 S 09:55 0:00 perl
> > > > dbi.pl
> > > > > new 0x8aad3e8 : SV = PVHV(0x8a68b30) at 0x8aad3e8
> > > > > REFCNT = 1
> > > > > FLAGS = (PADBUSY,PADMY,SHAREKEYS)
> > > > > IV = 0
> > > > > NV = 0
> > > > > ARRAY = 0x0
> > > > > KEYS = 0
> > > > > FILL = 0
> > > > > MAX = 7
> > > > > RITER = -1
> > > > > EITER = 0x0
> > > > > new 0x8aad3f4 : SV = NULL(0x0) at 0x8aad3f4
> > > > > REFCNT = 1
> > > > > FLAGS = (PADBUSY,PADMY)
> > > > > new 0x8aad400 : SV = NULL(0x0) at 0x8aad400
> > > > > REFCNT = 1
> > > > > FLAGS = (PADBUSY,PADMY)
> > > > > new 0x8aad40c : SV = NULL(0x0) at 0x8aad40c
> > > > > REFCNT = 1
> > > > > FLAGS = (PADBUSY,PADMY)
> > > > > count: 12132
> > > > >
> > > > > I think there might be several leaks here. If I move the connect()
> > out
> > > > of the loop, it's leaking, but less - just one of those NULL SV guys.
> > > > >
> > > > > Can you please help me track this down? I'm still a little scared
> > of
> > > > the DBI.xs internals, despite my recent exposure...
> > > > >
> > > > > Thanks a lot,
> > > > > edan
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Tim Bunce [mailto:Tim.Bunce@pobox.com]
> > > > > > Sent: Monday, July 03, 2006 12:16
> > > > > > To: Ephraim Dan
> > > > > > Cc: Tim Bunce; dbi-users@perl.org
> > > > > > Subject: Re: memory leak in DBI XS bootstrap code
> > > > > >
> > > > > > On Sat, Jul 01, 2006 at 10:42:58PM -0700, Ephraim Dan wrote:
> > > > > > > Thanks for the help Tim! You've really done me a great service.
> > > > > > >
> > > > > > > I have created the following patch, which appears to fix all of
> > the
> > > > > > leaks. Can you please review it, and give it your stamp of
> > approval?
> > > > I
> > > > > > am willing to use this patch if you approve it, until a new DBI is
> > > > > > available with the fixes.
> > > > > >
> > > > > > Looks okay at first sight, except that Newz returns memory that's
> > been
> > > > > > reset to zero bytes. You need to add the memzero I suggested
> > > > previously.
> > > > > > Might as well go into malloc_using_sv since it's not performance
> > > > critical.
> > > > > >
> > > > > > Um, looks like CLONE plus _clone_dbis plus dbi_bootinit already do
> > the
> > > > > > right thing (your patch also fixes a leak on thread creation) so
> > > > that's
> > > > > > okay. INIT_PERINTERP is harder to be sure about, but I think it's
> > > > okay.
> > > > > >
> > > > > > I'll apply the patch and add the memzero. Thanks edan.
> > > > > >
> > > > > > Tim.
> > > > > >
> > > > > > > Thanks again,
> > > > > > > edan
> > > > > > >
> > > > > > > The patch:
> > > > > > >
> > > > > > > --- DBI.xs.orig 2006-06-30 10:20:10.000000000 -0400
> > > > > > > +++ DBI.xs 2006-07-02 04:59:32.000000000 -0400
> > > > > > > @@ -171,7 +171,7 @@
> > > > > > > dPERINTERP_SV; dPERINTERP_PTR(PERINTERP_t *, PERINTERP)
> > > > > > > # define INIT_PERINTERP \
> > > > > > > dPERINTERP; \
> > > > > > > - Newz(0,PERINTERP,1,PERINTERP_t); \
> > > > > > > + PERINTERP = malloc_using_sv(sizeof(PERINTERP_t)); \
> > > > > > > sv_setiv(perinterp_sv, PTR2IV(PERINTERP))
> > > > > > >
> > > > > > > # undef DBIS
> > > > > > > @@ -209,13 +209,29 @@
> > > > > > > stc_s, sizeof(dbih_stc_t), fdc_s,
> > > > sizeof(dbih_fdc_t),
> > > > > > msg);
> > > > > > > }
> > > > > > >
> > > > > > > +static void *
> > > > > > > +malloc_using_sv(STRLEN len)
> > > > > > > +{
> > > > > > > + dTHX;
> > > > > > > + SV *sv = newSV(len);
> > > > > > > + return SvPVX(sv);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static char *
> > > > > > > +savepv_using_sv(char *str)
> > > > > > > +{
> > > > > > > + char *buf = malloc_using_sv(strlen(str));
> > > > > > > + strcpy(buf, str);
> > > > > > > + return buf;
> > > > > > > +}
> > > > > > > +
> > > > > > > static void
> > > > > > > dbi_bootinit(dbistate_t * parent_dbis)
> > > > > > > {
> > > > > > > dTHX;
> > > > > > > INIT_PERINTERP;
> > > > > > >
> > > > > > > - Newz(dummy, DBIS, 1, dbistate_t);
> > > > > > > + DBIS = (struct dbistate_st*)malloc_using_sv(sizeof(struct
> > > > > > dbistate_st));
> > > > > > >
> > > > > > > /* store version and size so we can spot DBI/DBD version
> > > > mismatch
> > > > > > */
> > > > > > > DBIS->check_version = check_version;
> > > > > > > @@ -3793,7 +3809,7 @@
> > > > > > > ima->minargs = (U8)SvIV(*av_fetch(av, 0, 1));
> > > > > > > ima->maxargs = (U8)SvIV(*av_fetch(av, 1, 1));
> > > > > > > svp = av_fetch(av, 2, 0);
> > > > > > > - ima->usage_msg = savepv( (svp) ? SvPV(*svp,lna) :
> > "");
> > > > > > > + ima->usage_msg = (svp) ? savepv_using_sv(SvPV(*svp,
> > > > lna)) :
> > > > > > "";
> > > > > > > ima->flags |= IMA_HAS_USAGE;
> > > > > > > if (trace_msg && DBIS_TRACE_LEVEL >= 11)
> > > > > > > sv_catpvf(trace_msg, ",\n usage: min %d, max
> > %d,
> > > > > > '%s'",
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Tim Bunce [mailto:Tim.Bunce@pobox.com]
> > > > > > > > Sent: Friday, June 30, 2006 16:33
> > > > > > > > To: Ephraim Dan
> > > > > > > > Cc: Tim Bunce; dbi-users@perl.org
> > > > > > > > Subject: Re: memory leak in DBI XS bootstrap code
> > > > > > > >
> > > > > > > > On Fri, Jun 30, 2006 at 04:08:46AM -0700, Ephraim Dan wrote:
> > > > > > > > > This is why I love the perl community - you're willing to
> > > > help
> > > > > > even
> > > > > > > > though you don't have time, since
> > > > > > > > > you care about your code, and that other people can
> > benefit
> > > > from
> > > > > > it.
> > > > > > > > >
> > > > > > > > > But ... you sort of left a dangling "But" in your last
> > post.
> > > > Do
> > > > > > you
> > > > > > > > have a suggestion to fix the
> > > > > > > > > _install_method problem?
> > > > > > > > >
> > > > > > > > > I just ran valgrind with your fix - there seems to still
> > be a
> > > > > > leak.
> > > > > > > > It is reported on DBI.xs line 216:
> > > > > > > > > INIT_PERINTERP;
> > > > > > > > > That looks to also have a Newz in it - is that what
> > leaks?
> > > > > > > >
> > > > > > > > Yes. It's basically a plain malloc and there's no mechanism to
> > > > free it
> > > > > > > > later.
> > > > > > > >
> > > > > > > > > Can you suggest how to fix that one?
> > > > > > > >
> > > > > > > > The trick is to use newSV to alloc memory in the form of an SV
> > > > > > > > so that when the interpreter is destroyed the SV memory gets
> > freed
> > > > > > > > automatically.
> > > > > > > >
> > > > > > > > > The _install_method leak is reported on line 3800 (in my
> > > > modified
> > > > > > > > DBI.xs) which validates your theory:
> > > > > > > > > ima->usage_msg = savepv( (svp) ? SvPV(*svp,lna) : "");
> > > > > > > > > If you can fix these, I'll be forever in your debt...
> > > > > > > >
> > > > > > > > As above, and as outlined previously, use an SV.
> > > > > > > > Something like this (completely untested):
> > > > > > > >
> > > > > > > > static void *
> > > > > > > > malloc_using_sv(STRLEN len) {
> > > > > > > > SV *sv = newSV(len);
> > > > > > > > return SvPVX(sv);
> > > > > > > > }
> > > > > > > >
> > > > > > > > static char *
> > > > > > > > savepv_using_sv(char *str) {
> > > > > > > > char *buf = malloc_using_sv(strlen(str));
> > > > > > > > strcpy(buf, str);
> > > > > > > > return buf;
> > > > > > > > }
> > > > > > > >
> > > > > > > > Tim.
> > > > > > > >
> > > > > > > > > --edan
> > > > > > > > >
> > > > > > > > > ---------------------------------------------------------
> > ----
> > > > ----
> > > > > > ----
> > > > > > > > -----------------------------------
> > > > > > > > >
> > > > > > > > > From: Tim Bunce [mailto:Tim.Bunce@pobox.com]
> > > > > > > > > Sent: Fri 6/30/2006 11:35
> > > > > > > > > To: Ephraim Dan
> > > > > > > > > Cc: Tim Bunce; dbi-users@perl.org
> > > > > > > > > Subject: Re: memory leak in DBI XS bootstrap code
> > > > > > > > >
> > > > > > > > > On Fri, Jun 30, 2006 at 12:24:33AM -0700, Ephraim Dan
> > wrote:
> > > > > > > > > > > > I don't see what you mean in the "INSTALL" that
> > comes
> > > > with
> > > > > > perl
> > > > > > > > 5.8.0 (that's what we're using).
> > > > > > > > > > > The file called INSTALL in the perl source code
> > > > directory.
> > > > > > > > > >
> > > > > > > > > > That I knew. What are the special instructions that
> > I'm
> > > > > > supposed
> > > > > > > > to
> > > > > > > > > > find there? Am I supposed to build it with debugging?
> > Can
> > > > you
> > > > > > > > > > specify what special configuration exactly you meant
> > that I
> > > > > > should
> > > > > > > > use?
> > > > > > > > >
> > > > > > > > > I'm sorry. I should have checked myself first. You can
> > build
> > > > perl
> > > > > > > > with
> > > > > > > > > -DPURIFY to enable more precise leak detection.
> > > > > > > > >
> > > > > > > > > > It looks like the leak in boot_DBI is on purpose:
> > > > > > > > > > /* Remember the last handle used. BEWARE! Sneaky
> > stuff
> > > > > > here!
> > > > > > > > */
> > > > > > > > > > /* We want a handle reference but we don't want to
> > > > > > increment
> > > > > > > > */
> > > > > > > > > > /* the handle's reference count and we don't want
> > perl
> > > > to
> > > > > > try
> > > > > > > > */
> > > > > > > > > > /* to destroy it during global destruction. Take
> > care!
> > > > > > > > */
> > > > > > > > > > DBI_UNSET_LAST_HANDLE; /* ensure setup the
> > correct
> > > > way
> > > > > > > > */
> > > > > > > > > >
> > > > > > > > > > Why is this being done, and does anyone have a way to
> > fix
> > > > it?
> > > > > > Why
> > > > > > > > don't we want perl to destroy it?
> > > > > > > > > Me, that's exactly what I want.
> > > > > > > > >
> > > > > > > > > I don't believe that's a leak. It's just using an SV as a
> > > > > > reference
> > > > > > > > > without telling perl its a reference. If perl knew it was
> > a
> > > > > > reference
> > > > > > > > > we'd get double-free warnings as the referenced value
> > would
> > > > be
> > > > > > freed
> > > > > > > > twice.
> > > > > > > > >
> > > > > > > > > The leak is probably the DBIS structure itself:
> > > > > > > > >
> > > > > > > > > Newz(dummy, DBIS, 1, dbistate_t);
> > > > > > > > >
> > > > > > > > > That should probably be using along the lines of:
> > > > > > > > >
> > > > > > > > > sv = newSV(sizeof(struct dbistate_st));
> > > > > > > > > DBIS = (struct dbistate_st*)SvPVX(sv);
> > > > > > > > > memzero(DBIS, sizeof(struct dbistate_st));
> > > > > > > > >
> > > > > > > > > So the memory is allocated within an SV so gets freed on
> > > > global
> > > > > > > > destruction.
> > > > > > > > >
> > > > > > > > > > Still looking into the other leak in _install_method...
> > any
> > > > > > > > pointers still appreciated... Thanks for
> > > > > > > > > your help so far, Tim.
> > > > > > > > >
> > > > > > > > > I've taken a look at the code. I'd guess that it's due to
> > > > > > savepnv:
> > > > > > > > >
> > > > > > > > > ima->usage_msg = savepv( (svp) ? SvPV(*svp,lna) :
> > "");
> > > > > > > > >
> > > > > > > > > But
> > > > > > > > >
> > > > > > > > > Tim.
> > > > > > > > >
> > > > > > > > > > -edan
> > > > > > > > > >
> > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > From: Tim Bunce [[1]mailto:Tim.Bunce@pobox.com]
> > > > > > > > > > > > Sent: Thursday, June 29, 2006 14:47
> > > > > > > > > > > > To: Ephraim Dan
> > > > > > > > > > > > Cc: dbi-users@perl.org
> > > > > > > > > > > > Subject: Re: memory leak in DBI XS bootstrap code
> > > > > > > > > > > >
> > > > > > > > > > > > Try building perl with options to make valgrind
> > leak
> > > > > > tracing
> > > > > > > > more
> > > > > > > > > > > > effective (see perl's INSTALL file). That may help
> > you
> > > > > > pinpoint
> > > > > > > > > > > > the problem.
> > > > > > > > > > > >
> > > > > > > > > > > > Tim.
> > > > > > > > > > > >
> > > > > > > > > > > > On Thu, Jun 29, 2006 at 04:33:40AM -0700, Ephraim
> > Dan
> > > > > > wrote:
> > > > > > > > > > > > > I am experiencing what I believe to be a memory
> > leak
> > > > in
> > > > > > the
> > > > > > > > DBI
> > > > > > > > > > > > bootstrap code. This is a problem for me because I
> > am
> > > > > > > > embedding perl in a
> > > > > > > > > > > > long-running program, and DBI is being loaded over
> > and
> > > > > > over, so
> > > > > > > > my program
> > > > > > > > > > > > grows and grows.
> > > > > > > > > > > > >
> > > > > > > > > > > > > The problem appears to be in the following
> > routines:
> > > > > > > > > > > > >
> > > > > > > > > > > > > boot_DBI (in
> > /usr/lib/perl5/vendor_perl/5.8.0/i386-
> > > > linux-
> > > > > > > > thread-
> > > > > > > > > > > > multi/auto/DBI/DBI.so)
> > > > > > > > > > > > > XS_DBI__install_method (in
> > > > > > > > /usr/lib/perl5/vendor_perl/5.8.0/i386-linux-
> > > > > > > > > > > > thread-multi/auto/DBI/DBI.so)
> > > > > > > > > > > > >
> > > > > > > > > > > > > I am using DBI 1.51
> > > > > > > > > > > > >
> > > > > > > > > > > > > The tool "valgrind" ([2]http://valgrind.org
> > > > > > > > <[3]http://valgrind.org/> ) can be used to reproduce
> > > > > > > > > the
> > > > > > > > > > > > leak using the following code:
> > > > > > > > > > > > >
> > > > > > > > > > > > > ---
> > > > > > > > > > > > > File: embed_test.c
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >
> > > > > > > > > > > > > #include
> > Perl
> > > > > > > > distribution */
> > > > > > > > > > > > > #include
> > Perl
> > > > > > > > distribution */
> > > > > > > > > > > > >
> > > > > > > > > > > > > static PerlInterpreter *my_perl; /*** The
> > Perl
> > > > > > > > interpreter ***/
> > > > > > > > > > > > > EXTERN_C void xs_init (pTHX); /*** init
> > dyn.
> > > > > > loading
> > > > > > > > ***/
> > > > > > > > > > > > >
> > > > > > > > > > > > > int main(int argc, char **argv, char **env)
> > > > > > > > > > > > > {
> > > > > > > > > > > > > char *embedding[] = { "", "-e", "0" };
> > > > > > > > > > > > > my_perl = perl_alloc();
> > > > > > > > > > > > > perl_construct(my_perl);
> > > > > > > > > > > > > perl_parse(my_perl, xs_init, 3, embedding,
> > (char
> > > > > > > > **)NULL);
> > > > > > > > > > > > > PL_exit_flags |= PERL_EXIT_DESTRUCT_END;
> > > > > > > > > > > > > perl_run(my_perl);
> > > > > > > > > > > > > eval_pv("use DBI", TRUE);
> > > > > > > > > > > > > perl_destruct(my_perl);
> > > > > > > > > > > > > perl_free(my_perl);
> > > > > > > > > > > > > }
> > > > > > > > > > > > >
> > > > > > > > > > > > > ---
> > > > > > > > > > > > > File: Makefile
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >
> > > > > > > > > > > > > CC_OPTS = $(shell perl -MExtUtils::Embed -e
> > ccopts)
> > > > > > > > > > > > > LD_OPTS = $(shell perl -MExtUtils::Embed -e
> > ldopts)
> > > > > > > > > > > > >
> > > > > > > > > > > > > EXE = embed_test
> > > > > > > > > > > > >
> > > > > > > > > > > > > $(EXE): xsinit.o embed_test.o
> > > > > > > > > > > > > gcc -o $(EXE) embed_test.o xsinit.o
> > > > $(LD_OPTS)
> > > > > > > > > > > > >
> > > > > > > > > > > > > embed_test.o: embed_test.c
> > > > > > > > > > > > > gcc -c embed_test.c $(CC_OPTS)
> > > > > > > > > > > > >
> > > > > > > > > > > > > xsinit.o: xsinit.c
> > > > > > > > > > > > > gcc -c xsinit.c $(CC_OPTS)
> > > > > > > > > > > > >
> > > > > > > > > > > > > xsinit.c:
> > > > > > > > > > > > > perl -MExtUtils::Embed -e xsinit -- -o
> > > > xsinit.c
> > > > > > > > > > > > >
> > > > > > > > > > > > > clean:
> > > > > > > > > > > > > rm -f *.o xsinit.c $(EXE)
> > > > > > > > > > > > >
> > > > > > > > > > > > > ---
> > > > > > > > > > > > > EOF
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >
> > > > > > > > > > > > > Can anyone suggest a fix for this? I'd be more
> > than
> > > > > > willing
> > > > > > > > to take a
> > > > > > > > > > > > patch to DBI 1.51 as soon as someone has one.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Thanks,
> > > > > > > > > > > > > Ephraim Dan
> > > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > References
> > > > > > > > >
> > > > > > > > > Visible links
> > > > > > > > > 1. mailto:Tim.Bunce@pobox.com
> > > > > > > > > 2. http://valgrind.org/
> > > > > > > > > 3. http://valgrind.org/