subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Shahaf <danie...@elego.de>
Subject Re: svn commit: r1205726 - /subversion/trunk/subversion/libsvn_fs_base/reps-strings.c
Date Thu, 24 Nov 2011 22:04:51 GMT
cmpilato@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;
>  }
> 
> 

Mime
View raw message