Re: svnsync crashes on a huge commit

[Ben Reser <be...@reser.org>]

On 7/28/13 1:52 AM, Ben Reser wrote:
> Even if the change that is resolving the problem on trunk isn't viable
> for backport, that still doesn't mean we won't fix the problem till
> 1.9.x, it just means a fix that will work for 1.8.x needs to be
> identified.

I looked into merging the trunk changes.  It doesn't look pretty.  There
are changes to the update report and inherited properties code that
create conflicts when merging the changes required to get use the
transition based parser code.  If Bert wants to nominate the change to
the transition based parser I wouldn't object, but I'm not comfortable
figuring out the backport on this code myself.  On the upside all of the
changes should be in private namespace so we shouldn't be prevented from
backporting.

I'm inclined to take a more conservative approach.  The following patch
should fix most of this problem on 1.8.x without pulling the new XML
parsing implementation onto 1.8.x.

[[[
Index: subversion/libsvn_ra_serf/replay.c
===================================================================
--- subversion/libsvn_ra_serf/replay.c	(revision 1513303)
+++ subversion/libsvn_ra_serf/replay.c	(working copy)
@@ -147,10 +147,16 @@
       state == OPEN_FILE || state == ADD_FILE)
     {
       replay_info_t *info;
+      apr_pool_t *pool;

-      info = apr_palloc(replay_ctx->dst_rev_pool, sizeof(*info));
+      if (state == OPEN_FILE || state == ADD_FILE)
+        pool = replay_ctx->file_pool;
+      else
+        pool = replay_ctx->dst_rev_pool;

-      info->pool = replay_ctx->dst_rev_pool;
+      info = apr_palloc(pool, sizeof(*info));
+
+      info->pool = pool;
       info->parent = parser->state->private;
       info->baton = NULL;
       info->stream = NULL;
]]]

Not sure if this is really complete.  I don't think the directory case
or the property case really should be using the dst_rev_pool.   In fact
I really don't understand why the replay_ctx->file_pool exists as well
as the info->pool.  I think we should probably rip out the
replay_ctx->file_pool, produce appropriate child pools on the info
struct and then use that.  But I haven't done that work to see if it
works properly.

I used the following script to reproduce the problem:
[[[
#!/bin/bash

set -e

LOCAL_REPO=file://`pwd`/repo
REMOTE_REPO=https://src.chromium.org/chrome
SVNADMIN=$HOME/builds/svn-1.8.x/subversion/svnadmin/svnadmin
SVNSYNC=$HOME/builds/svn-1.8.x/subversion/svnsync/svnsync
SVNRDUMP=$HOME/builds/svn-1.8.x/subversion/svnrdump/svnrdump

if [ ! -e base.dump -a ! -d base.repo ]; then
  $SVNRDUMP dump -r0:17 "$REMOTE_REPO" > base.dump
fi

rm -rf repo
if [ ! -d base.repo ]; then
  $SVNADMIN create repo
  echo $'#!/bin/sh\nexit 0\n' > repo/hooks/pre-revprop-change
  chmod a+x repo/hooks/pre-revprop-change
  $SVNADMIN load repo < base.dump
  $SVNSYNC init --allow-non-empty "$LOCAL_REPO" "$REMOTE_REPO"
  cp -a repo base.repo
else
  cp -a base.repo repo
fi

$SVNSYNC sync "$LOCAL_REPO"
]]]

I used dump and saved a repo so I could simply repeat r18 and look at
the memory usage.  Without the above patch the code used 400-500MB peak
for r18.  With the patch it stayed under 50MB, with most of the peak
memory usage coming as the commit is built.

I'll look into further cleanup tomorrow unless Bert really wants to run
with the backport of the transition based code.

Re: svnsync crashes on a huge commit

[Ben Reser <be...@reser.org>]

On 8/15/13 8:15 PM, Ben Reser wrote:
> Not sure if this is really complete.  I don't think the directory case
> or the property case really should be using the dst_rev_pool.   In fact
> I really don't understand why the replay_ctx->file_pool exists as well
> as the info->pool.  I think we should probably rip out the
> replay_ctx->file_pool, produce appropriate child pools on the info
> struct and then use that.  But I haven't done that work to see if it
> works properly.

I finished this up and have nominated the fix for 1.8.x.

I tried continuing to use the shared file_pool and generating new pools
for each file.  I didn't find any significant difference between the two
methods in memory usage or speed.  The separate pool code I committed
may be slightly faster but the difference is so small it may just be
random differences.  I went ahead with this code since it reduced some
code complexity in having to choose which pool to use since all code
paths just always use info->pool.

The previous patch would continue to scale memory based on total
directories opened and the number of properties changed.  So it only
fixed part of the problem.

I compared the results of the various strategies using the r18-20 of the
chromium repo using the GNU time command's -v option (not the bash builtin).

1.8.1:
	Maximum resident set size (kbytes): 1828144
r1515249 on 1.8.x-svnsync-serf-memory branch:
	Maximum resident set size (kbytes): 210080
r1515269 on 1.8.x-svnsync-serf-memory branch:
	Maximum resident set size (kbytes): 196544

I realize these are different than the previous numbers everyone was
looking at.  I know when I looked before I just used the RES column of
top and I got tired of staring at top.  In comparison to that I went
from ~500MB for the 1.8.1 code syncing r18 of the chromium repo, ~50MB
for the r1515249 code and ~36MB for the r1515269 code.

Please review this change so we can get it in 1.8.2 when I roll the
tarballs tomorrow.

Reviewers may want to consider the details of the delta editor:
http://subversion.apache.org/docs/api/latest/structsvn__delta__editor__t.html#_details

Re: svnsync crashes on a huge commit

[Philip Martin <ph...@wandisco.com>]

Ben Reser <be...@reser.org> writes:

> I'm inclined to take a more conservative approach.  The following patch
> should fix most of this problem on 1.8.x without pulling the new XML
> parsing implementation onto 1.8.x.
>
> [[[
> Index: subversion/libsvn_ra_serf/replay.c
> ===================================================================
> --- subversion/libsvn_ra_serf/replay.c	(revision 1513303)
> +++ subversion/libsvn_ra_serf/replay.c	(working copy)
> @@ -147,10 +147,16 @@
>        state == OPEN_FILE || state == ADD_FILE)
>      {
>        replay_info_t *info;
> +      apr_pool_t *pool;
>
> -      info = apr_palloc(replay_ctx->dst_rev_pool, sizeof(*info));
> +      if (state == OPEN_FILE || state == ADD_FILE)
> +        pool = replay_ctx->file_pool;
> +      else
> +        pool = replay_ctx->dst_rev_pool;
>
> -      info->pool = replay_ctx->dst_rev_pool;
> +      info = apr_palloc(pool, sizeof(*info));
> +
> +      info->pool = pool;
>        info->parent = parser->state->private;
>        info->baton = NULL;
>        info->stream = NULL;

I've tested this by running syncs under valgrind, including the first 75
revisions of the chromium repository, and it doesn't trigger any
warnings.

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*