You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2011/07/19 23:51:21 UTC
svn commit: r1148566 - /subversion/trunk/subversion/libsvn_client/merge.c
Author: stsp
Date: Tue Jul 19 21:51:21 2011
New Revision: 1148566
URL: http://svn.apache.org/viewvc?rev=1148566&view=rev
Log:
Fix a memory leak observed during a merge which involves a lot of subtree
mergeinfo on long-lived branches.
Reported by me in #svn-dev today:
[15:31] <stsp> pburba, running svn merge ^/subversion/trunk in a WC of the
gpg-agent-password-store branch @1148391 results in over 300MB of memory
allocated
http://colabti.org/irclogger/irclogger_log/svn-dev?date=2011-07-19#l115
With this fix memory use stays constant at around 20MB throughout the merge.
The merge is also very slow but that is an unrelated problem and will
be fixed later.
* 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().
Modified:
subversion/trunk/subversion/libsvn_client/merge.c
Modified: subversion/trunk/subversion/libsvn_client/merge.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/merge.c?rev=1148566&r1=1148565&r2=1148566&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/merge.c (original)
+++ subversion/trunk/subversion/libsvn_client/merge.c Tue Jul 19 21:51:21 2011
@@ -7889,6 +7889,18 @@ log_noop_revs(void *baton,
apr_hash_index_t *hi;
svn_revnum_t revision;
svn_boolean_t log_entry_rev_required = FALSE;
+ apr_array_header_t *rl1;
+ apr_array_header_t *rl2;
+
+ /* The baton's pool is essentially an iterpool so we must clear it
+ * for each invocation of this function. */
+ rl1 = svn_rangelist_dup(log_gap_baton->operative_ranges, pool);
+ rl2 = svn_rangelist_dup(log_gap_baton->merged_ranges, pool);
+ svn_pool_clear(log_gap_baton->pool);
+ log_gap_baton->operative_ranges = svn_rangelist_dup(rl1,
+ log_gap_baton->pool);
+ log_gap_baton->merged_ranges = svn_rangelist_dup(rl2,
+ log_gap_baton->pool);
revision = log_entry->revision;
@@ -8116,7 +8128,7 @@ remove_noop_subtree_ranges(const char *u
result_pool, scratch_pool));
log_gap_baton.merged_ranges = merged_ranges;
log_gap_baton.operative_ranges = operative_ranges;
- log_gap_baton.pool = scratch_pool;
+ log_gap_baton.pool = svn_pool_create(scratch_pool);
APR_ARRAY_PUSH(log_targets, const char *) = "";
Re: svn commit: r1148566 -
/subversion/trunk/subversion/libsvn_client/merge.c
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Jul 19, 2011 at 08:48:02PM -0400, Greg Stein wrote:
> On Tue, Jul 19, 2011 at 20:29, Greg Stein <gs...@gmail.com> wrote:
> > On Tue, Jul 19, 2011 at 20:10, Greg Stein <gs...@gmail.com> wrote:
> >> On Tue, Jul 19, 2011 at 17:51, <st...@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.
Re: svn commit: r1148566 - /subversion/trunk/subversion/libsvn_client/merge.c
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Jul 19, 2011 at 20:29, Greg Stein <gs...@gmail.com> wrote:
> On Tue, Jul 19, 2011 at 20:10, Greg Stein <gs...@gmail.com> wrote:
>> On Tue, Jul 19, 2011 at 17:51, <st...@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.
>
> That should be fixed. log_context_t should grow an "item_pool" that is
> cleared on each call to push_state(ITEM). Then all the log_entry stuff
> is placed into that pool, and passed to the receiver. That item_pool
> would also be the scratch_pool passed to the receiver.
>
> The log_info_t would lose its pool member. (and it really should be
> called something like log_element_t since it always corresponds
> directly to an XML element)
>
> Thoughts?
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).
We figured to wait for your thoughts before another iteration.
Cheers,
-g
Re: svn commit: r1148566 - /subversion/trunk/subversion/libsvn_client/merge.c
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Jul 19, 2011 at 20:10, Greg Stein <gs...@gmail.com> wrote:
> On Tue, Jul 19, 2011 at 17:51, <st...@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.
That should be fixed. log_context_t should grow an "item_pool" that is
cleared on each call to push_state(ITEM). Then all the log_entry stuff
is placed into that pool, and passed to the receiver. That item_pool
would also be the scratch_pool passed to the receiver.
The log_info_t would lose its pool member. (and it really should be
called something like log_element_t since it always corresponds
directly to an XML element)
Thoughts?
Cheers,
-g
Re: svn commit: r1148566 - /subversion/trunk/subversion/libsvn_client/merge.c
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Jul 19, 2011 at 17:51, <st...@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).
Cheers,
-g