cmpilato_at_apache.org wrote on Thu, Nov 24, 2011 at 05:45:25 -0000:
> Author: cmpilato
> Date: Thu Nov 24 05:45:24 2011
> New Revision: 1205726
>
> URL: http://svn.apache.org/viewvc?rev=1205726&view=rev
> Log:
> Plug a memory leak in the fs-base deltification logic. Twice in a
> decade I've seen this code swallow all the memory on a server in the
> wild when trying to deltify some fairly large versioned files. Now,
> such a deltification maintains a constant (and relatively low -- 17 MB
> in my verification against the second of these troublesome data sets)
> level of memory usage.
>
> NOTE: This patch appears to pass all the BDB tests on my laptop, but
> of course those aren't known to cover large datasets. I would
> really, really appreciate some extra eyes on this change!
>
Reviewed the scratch pool usage in these functions and their callees,
looks good. I'll go ahead and add my +1 to backport.
> Wondering aloud... if this approach turns out to be correct, should
> the corresponding stream write function in this same file
> (rep_write_contents) use a similarly initialized scratch pool and
> clear it at the start of each invocation, too?
>
I suppose this might be a good idea; datasets that are problematic for
read() should also be problematic for write(), correct?
> * subversion/libsvn_fs_base/reps-strings.c
> (struct rep_read_baton): Rename (and repurpose) 'pool' to
> 'scratch_pool'.
> (rep_read_get_baton): Now initialize scratch_pool (formerly, pool)
> from a subpool of the passed-in pool. This allows us to clear it
> without destroying the baton.
> (txn_body_read_rep): Use the baton's scratch_pool instead of
> trail->pool in the call to rep_read_range().
> (rep_read_contents): Clear the baton's scratch_pool before use.
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_base/reps-strings.c
>
> Modified: subversion/trunk/subversion/libsvn_fs_base/reps-strings.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_base/reps-strings.c?rev=1205726&r1=1205725&r2=1205726&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_base/reps-strings.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_base/reps-strings.c Thu Nov 24 05:45:24 2011
> @@ -674,8 +674,10 @@ struct rep_read_baton
> is digestified. */
> svn_boolean_t checksum_finalized;
>
> - /* Used for temporary allocations, iff `trail' (above) is null. */
> - apr_pool_t *pool;
> + /* Used for temporary allocations. This pool is cleared at the
> + start of each invocation of the relevant stream read function --
> + see rep_read_contents(). */
> + apr_pool_t *scratch_pool;
>
> };
>
> @@ -703,7 +705,7 @@ rep_read_get_baton(struct rep_read_baton
> b->checksum_finalized = FALSE;
> b->fs = fs;
> b->trail = use_trail_for_reads ? trail : NULL;
> - b->pool = pool;
> + b->scratch_pool = svn_pool_create(pool);
> b->rep_key = rep_key;
> b->offset = 0;
>
> @@ -869,7 +871,7 @@ txn_body_read_rep(void *baton, trail_t *
> args->buf,
> args->len,
> trail,
> - trail->pool));
> + args->rb->scratch_pool));
>
> args->rb->offset += *(args->len);
>
> @@ -956,6 +958,9 @@ rep_read_contents(void *baton, char *buf
> struct rep_read_baton *rb = baton;
> struct read_rep_args args;
>
> + /* Clear the scratch pool of the results of previous invocations. */
> + svn_pool_clear(rb->scratch_pool);
> +
> args.rb = rb;
> args.buf = buf;
> args.len = len;
> @@ -974,7 +979,7 @@ rep_read_contents(void *baton, char *buf
> txn_body_read_rep,
> &args,
> TRUE,
> - rb->pool));
> + rb->scratch_pool));
> }
> return SVN_NO_ERROR;
> }
>
>
Received on 2011-11-24 23:05:38 CET