Re: Re: big memleak in svn 1.5

On Nov 14, 2007 6:49 PM, David Glasser <glasser@davidglasser.net> wrote:
>
> On Nov 14, 2007 6:30 PM, David Glasser <glasser@davidglasser.net> wrote:
> >
> > On Nov 14, 2007 6:28 PM, David Glasser <glasser@davidglasser.net> wrote:
> > >
> > > On Nov 14, 2007 3:29 PM, David Glasser <glasser@davidglasser.net> wrote:
> > > >
> > > > On Jun 7, 2007 8:23 AM, Paul Burba <pburba@collab.net> wrote:
> > > > > > -----Original Message-----
> > > > > > From: Daniel Rall [mailto:dlr@collab.net]
> > > > > > Sent: Wednesday, June 06, 2007 8:33 PM
> > > > > > To: Ben Collins-Sussman
> > > > > > Cc: dev
> > > > > > Subject: Re: big memleak in svn 1.5
> > > > > >
> > > > > > On Wed, 06 Jun 2007, Daniel Rall wrote:
> > > > > >
> > > > > > > On Wed, 06 Jun 2007, Ben Collins-Sussman wrote:
> > > > >
> > > > > <snip>
> > > > >
> > > > > > > The main differences are for:
> > > > > > >
> > > > > > > a) Eliding of merge info in the WC
> > > > > > > b) Depth argument handling
> > > > > > > c) Preserved file extension handling
> > > > > > >
> > > > > > > (b) isn't obviously suspect in this function, but there's potential
> > > > > > > for a leak buried in the changes to the reporter code. (c)
> > > > > > is fairly
> > > > > > > simple, and also seems pretty unlikely.
> > > > > > >
> > > > > > > WRT (a), I notice two potential scalability issues, the second
> > > > > > > somewhat dependent upon the first:
> > > > > > >
> > > > > > > 1) children_with_mergeinfo_hash could potentially grow to be large.
> > > > > > > 2) We iterate over children with merge info, but don't use
> > > > > > a sub-pool.
> > > > > > >
> > > > > > > I'm attaching a patch for #2, but I don't think we can
> > > > > > easily fix #1.
> > > > > > > Assuming Google's tree doesn't have much/any merge info, I
> > > > > > doubt this
> > > > > > > will have much impact on the memory footprint, unless the leak is
> > > > > > > buried under svn_client__elide_mergeinfo().
> > > > > >
> > > > > > Patch wasn't quite right, attaching a corrected revision.
> > > > >
> > > > > As we discussed on IRC this patch isn't going to solve this particular
> > > > > memory leak, but it might solve some future ones where there actually
> > > > > are a lot of paths under the update target with mergeinfo.
> > > > >
> > > > > Added similar code for switch.c:svn_client__switch_internal and
> > > > > committed r25322.
> > > >
> > > > I'm looking at this leak again. It definitely still exists, and it is
> > > > definitely in the elision code: when I commented that out the
> > > > explosion didn't happen.
> > >
> > > My two friends gdb and top have told me that it happens during the
> > > svn_client__get_prop_from_wc call in svn_client__update_internal.
> > > Note that this call returns an empty hash, so it's not just "oh well,
> > > there's tons of mergeinfo". Investigating more.
> >
> > You can also reproduce just by running
> >
> > $ svn pg --depth=infinity asdf /path/to/enormous/wc
>
> Fixed in r27822.
>
> (Tip: "top -b -d 0.1 | grep svn" is so awesome.)

I did a cursory audit of all uses of svn_wc_walk_entries3. I did not
see any similar abuses.

--dave

-- 
David Glasser | glasser_at_davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org