svn commit: r1205726 - /subversion/trunk/subversion/libsvn_fs_base/reps-strings.c

[cmpilato]

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!

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?

* 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;
 }