On Tue, Jul 19, 2011 at 08:48:02PM -0400, Greg Stein wrote:
> On Tue, Jul 19, 2011 at 20:29, Greg Stein <gstein_at_gmail.com> wrote:
> > On Tue, Jul 19, 2011 at 20:10, Greg Stein <gstein_at_gmail.com> wrote:
> >> On Tue, Jul 19, 2011 at 17:51, <stsp_at_apache.org> wrote:
> >>>...
> >>> * subversion/libsvn_client/merge.c
> >>> (log_noop_revs): The baton's pool is used in an iterpool pattern so it
> >>> must be cleared on each invocation of this function.
> >>> (remove_noop_subtree_ranges): Make the baton's pool a proper subpool
> >>> to avoid clearing unrelated data in log_noop_revs().
> >>
> >> Why does the baton have a pool at all? The pool passed to
> >> log_noop_revs() *is* already a scratch_pool. If that pool is not
> >> getting cleared on each iteration, then we need to fix the caller.
> >>
> >> The log_baton shouldn't even have a pool, I think.
> >>
> >> I believe the above change is the wrong fix for this issue. The
> >> caller(s) should be corrected (and remove that spurious pool).
> >
> > Ugh. There isn't a single svn_pool_clear() in libsvn_ra_serf/log.c.
> Bert solved the underlying problem in r1148588. However, I think that
> we can iterate on that change to clarify pool usage, and clear a pool
> (rather than create/destroy). e.g my suggestions above.
>
> Note that Bert's change fixes all log usage: rather than just 'svn
> merge', it also handles 'svn log' (dunno where else we fetch logs).
Great, thanks Bert!
That commit also answers my question from IRC.
> We figured to wait for your thoughts before another iteration.
The baton pool is used to slowly builds up a mergeinfo rangelist,
across multiple invocations of log_noop_revs(). Where would
log_noop_revs() store the rangelist it is building if it only had
a scratch pool that is valid for the current invocation?
So the callback needs a result pool of sorts. But also note that a new
rangelist is allocated for each call to svn_rangelist_merge() which is
used to expand the rangelist as it is being built. If we don't ever
free the temporary rangelists from the result pool we can still run
out of memory.
So log_noop_revs() really needs three pools:
1) a pool to store the final result
2) a pool to store temp data until the next invocation
3) a pool to store temp data during the current invocation
With my fix it is using the baton pool for purposes 1 and 2.
It could make better use of the temp store pool 3.
I tried this but ran into the ra_serf issue which Bert fixed.
We could now make the function use this pool more agressively but in
practice this wouldn't save a lot more memory than we already do.
The real issue was that we never freed the temp data that needs to
live from one invocation to the next.
Maybe there is a better way to fix this. But I have no good idea.
Received on 2011-07-20 11:30:50 CEST