cmpil...@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; > } > >
