On Mon, Nov 8, 2010 at 1:23 PM, Paul Burba <ptbu...@gmail.com> wrote:
> On Mon, Nov 8, 2010 at 10:35 AM, Paul Burba <ptbu...@gmail.com> wrote:
>> On Mon, Nov 8, 2010 at 9:13 AM, Paul Burba <ptbu...@gmail.com> wrote:
>>> On Thu, Nov 4, 2010 at 8:59 PM, Chris Tashjian <ct...@thepond.com> wrote:
>>>> I posted this on the users list, but I'm confident that this is a bug.
>>>>
>>>> Background:
>>>> For a while now (off and on for over a year, but more frequently in the
>>>> last
>>>> 6+ months) we've been having problems with svn "crashing", yet there's no
>>>> error in the log file. In talking to someone the users list it sounds like
>>>> svn is actually just hanging. Clients get the following response:
>>>>
>>>> svn: Can't connect to host 'svn': No connection could be made
>>>> because the target machine actively refused it.
>>>>
>>>> Our repository has 129K revisions. The format is "4 layout linear", it was
>>>> created with svnadmin 1.4.x and has since had "svnadmin upgrade" run both
>>>> in
>>>> 1.5 and 1.6. We're currently running SlikSVN 1.6.13 (Win32), but I have
>>>> previously had this problem dating back to versions of 1.5, both stock and
>>>> from CollabNet. The issue now happens numerous times per day and it looks
>>>> like I've discovered why....
>>>>
>>>>
>>>> As a test I ran "svn blame -g" on a file with a bunch of revisions and
>>>> watched memory usage on the server spin up to 2GB.
>>>
>>> Chris,
>>>
>>> By a "bunch of revisions" what exactly do you mean? Many revisions
>>> which were the result of a merge? Or simply many changes made
>>> directly to the file (not the result of a merge)?
>>>
>>>> Paul - I'll see if I can get a test repo up with the error. In the
>>>> meantime, would a copy of the svn:mergeinfo help?
>>>
>>> Any luck?
>>
>> Chris,
>>
>> Don't sweat the replication effort too much, I think I'm on the trail
>> of this problem. Using a copy of the old (pre-ASF) Subversion
>> repository I'm seeing unexpectedly high memory use when using log -g
>> on a file with a "lot" of merge history:
>>
>> 1.6.13.client>svn blame ^/trunk/subversion/tests/cmdline/merge_tests.py
>> 25 MB Peak Working Set Memory svnserve.exe
>>
>> 1.6.13.client>svn blame ^/trunk/subversion/tests/cmdline/merge_tests.py -g
>> 291 MB Peak Working Set Memory svnserve.exe
> ^^^
> That should have been 691 MB! Sorry!
>
>>
>> More soon...
Hi Chris,
Ok, I think I got it. Switching to a Subversion tr...@1032651 (debug)
build and using this repository as a test*:
http://svn.collab.net/tmp/subversion-data-backup/subversion-history-20091115.tar.bz2
The peak working set memory of
'svn blame ^/trunk/subversion/tests/cmdline/merge_tests.py' was 21 MB
'svn blame ^/trunk/subversion/tests/cmdline/merge_tests.py -g' was 574 MB
That's comparable to what you saw with 1.6.13.
Applying some standard pool-fu to
libsvn_repos/rev_hunt.c:(find_merged_revisions|find_interesting_revisions),
see attached patch, and things look a *lot* better:
The peak working set memory of
'svn blame ^/trunk/subversion/tests/cmdline/merge_tests.py' stays at 21 MB
'svn blame ^/trunk/subversion/tests/cmdline/merge_tests.py -g'
drops to 71 MB
A tidy 88% reduction in peak memory usage!
Running the test suite on this patch. Assuming no problems I will be
committing and nominating for backport to 1.6.x.
log:
[[[
Fix blame -g server-side memory leaks.
See http://svn.haxx.se/dev/archive-2010-11/0102.shtml
* subversion/libsvn_repos/rev_hunt.c
(find_interesting_revisions): Implement result/scratch two-pool paradigm.
Don't needlessly allocate path_revision structures until we are sure
we are going to keep it. Rename local variable "iter_pool" to the
de facto standard "iterpool".
(find_merged_revisions): Use a separate iterpool for each nested loop.
Update call to find_interesting_revisions, passing, you guessed it, an
iterpool. Rename local variable "iter_pool" to the de facto standard
"iterpool".
]]]
Paul
* This is the old Tigris Subversion repository, see
http://svn.apache.org/repos/asf/subversion/README.
Index: subversion/libsvn_repos/rev_hunt.c
===================================================================
--- subversion/libsvn_repos/rev_hunt.c (revision 1032800)
+++ subversion/libsvn_repos/rev_hunt.c (working copy)
@@ -1085,47 +1085,49 @@
apr_hash_t *duplicate_path_revs,
svn_repos_authz_func_t authz_read_func,
void *authz_read_baton,
- apr_pool_t *pool)
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
{
- apr_pool_t *iter_pool, *last_pool;
+ apr_pool_t *iterpool, *last_pool;
svn_fs_history_t *history;
svn_fs_root_t *root;
svn_node_kind_t kind;
/* We switch between two pools while looping, since we need information from
the last iteration to be available. */
- iter_pool = svn_pool_create(pool);
- last_pool = svn_pool_create(pool);
+ iterpool = svn_pool_create(result_pool);
+ last_pool = svn_pool_create(result_pool);
/* The path had better be a file in this revision. */
- SVN_ERR(svn_fs_revision_root(&root, repos->fs, end, last_pool));
- SVN_ERR(svn_fs_check_path(&kind, root, path, pool));
+ SVN_ERR(svn_fs_revision_root(&root, repos->fs, end, scratch_pool));
+ SVN_ERR(svn_fs_check_path(&kind, root, path, scratch_pool));
if (kind != svn_node_file)
return svn_error_createf
(SVN_ERR_FS_NOT_FILE, NULL, _("'%s' is not a file in revision %ld"),
path, end);
/* Open a history object. */
- SVN_ERR(svn_fs_node_history(&history, root, path, last_pool));
-
+ SVN_ERR(svn_fs_node_history(&history, root, path, scratch_pool));
while (1)
{
- struct path_revision *path_rev = apr_palloc(pool, sizeof(*path_rev));
+ struct path_revision *path_rev;
+ svn_revnum_t tmp_revnum;
+ const char *tmp_path;
apr_pool_t *tmp_pool;
- svn_pool_clear(iter_pool);
+ svn_pool_clear(iterpool);
/* Fetch the history object to walk through. */
- SVN_ERR(svn_fs_history_prev(&history, history, TRUE, iter_pool));
+ SVN_ERR(svn_fs_history_prev(&history, history, TRUE, iterpool));
if (!history)
break;
- SVN_ERR(svn_fs_history_location(&path_rev->path, &path_rev->revnum,
- history, iter_pool));
+ SVN_ERR(svn_fs_history_location(&tmp_path, &tmp_revnum,
+ history, iterpool));
/* Check to see if we already saw this path (and it's ancestors) */
if (include_merged_revisions
- && is_path_in_hash(duplicate_path_revs, path_rev->path,
- path_rev->revnum, iter_pool))
+ && is_path_in_hash(duplicate_path_revs, tmp_path,
+ tmp_revnum, iterpool))
break;
/* Check authorization. */
@@ -1134,21 +1136,24 @@
svn_boolean_t readable;
svn_fs_root_t *tmp_root;
- SVN_ERR(svn_fs_revision_root(&tmp_root, repos->fs, path_rev->revnum,
- iter_pool));
- SVN_ERR(authz_read_func(&readable, tmp_root, path_rev->path,
- authz_read_baton, iter_pool));
+ SVN_ERR(svn_fs_revision_root(&tmp_root, repos->fs, tmp_revnum,
+ iterpool));
+ SVN_ERR(authz_read_func(&readable, tmp_root, tmp_path,
+ authz_read_baton, iterpool));
if (! readable)
break;
}
- path_rev->path = apr_pstrdup(pool, path_rev->path);
+ /* We didn't break, so we must really want this path-rev. */
+ path_rev = apr_palloc(result_pool, sizeof(*path_rev));
+ path_rev->path = apr_pstrdup(result_pool, tmp_path);
+ path_rev->revnum = tmp_revnum;
path_rev->merged = mark_as_merged;
APR_ARRAY_PUSH(path_revisions, struct path_revision *) = path_rev;
if (include_merged_revisions)
SVN_ERR(get_merged_mergeinfo(&path_rev->merged_mergeinfo, repos,
- path_rev, pool));
+ path_rev, result_pool));
else
path_rev->merged_mergeinfo = NULL;
@@ -1156,7 +1161,7 @@
occurrences of it. We only care about this if including merged
revisions, 'cause that's the only time we can have duplicates. */
apr_hash_set(duplicate_path_revs,
- apr_psprintf(pool, "%s:%ld", path_rev->path,
+ apr_psprintf(result_pool, "%s:%ld", path_rev->path,
path_rev->revnum),
APR_HASH_KEY_STRING, (void *)0xdeadbeef);
@@ -1164,12 +1169,12 @@
break;
/* Swap pools. */
- tmp_pool = iter_pool;
- iter_pool = last_pool;
+ tmp_pool = iterpool;
+ iterpool = last_pool;
last_pool = tmp_pool;
}
- svn_pool_destroy(iter_pool);
+ svn_pool_destroy(iterpool);
return SVN_NO_ERROR;
}
@@ -1198,12 +1203,12 @@
{
const apr_array_header_t *old;
apr_array_header_t *new;
- apr_pool_t *iter_pool, *last_pool;
+ apr_pool_t *iterpool, *last_pool;
apr_array_header_t *merged_path_revisions = apr_array_make(pool, 0,
sizeof(struct path_revision
*));
old = mainline_path_revisions;
- iter_pool = svn_pool_create(pool);
+ iterpool = svn_pool_create(pool);
last_pool = svn_pool_create(pool);
do
@@ -1211,27 +1216,34 @@
int i;
apr_pool_t *temp_pool;
- svn_pool_clear(iter_pool);
- new = apr_array_make(iter_pool, 0, sizeof(struct path_revision *));
+ svn_pool_clear(iterpool);
+ new = apr_array_make(iterpool, 0, sizeof(struct path_revision *));
/* Iterate over OLD, checking for non-empty mergeinfo. If found, gather
path_revisions for any merged revisions, and store those in NEW. */
for (i = 0; i < old->nelts; i++)
{
+ apr_pool_t *iterpool2;
apr_hash_index_t *hi;
struct path_revision *old_pr = APR_ARRAY_IDX(old, i,
struct path_revision *);
if (!old_pr->merged_mergeinfo)
continue;
+ iterpool2 = svn_pool_create(iterpool);
+
/* Determine and trace the merge sources. */
- for (hi = apr_hash_first(iter_pool, old_pr->merged_mergeinfo); hi;
+ for (hi = apr_hash_first(iterpool, old_pr->merged_mergeinfo); hi;
hi = apr_hash_next(hi))
{
+ apr_pool_t *iterpool3;
apr_array_header_t *rangelist;
const char *path;
int j;
+ svn_pool_clear(iterpool2);
+ iterpool3 = svn_pool_create(iterpool2);
+
apr_hash_this(hi, (void *) &path, NULL, (void *) &rangelist);
for (j = 0; j < rangelist->nelts; j++)
@@ -1241,9 +1253,10 @@
svn_node_kind_t kind;
svn_fs_root_t *root;
+ svn_pool_clear(iterpool3);
SVN_ERR(svn_fs_revision_root(&root, repos->fs, range->end,
- iter_pool));
- SVN_ERR(svn_fs_check_path(&kind, root, path, iter_pool));
+ iterpool3));
+ SVN_ERR(svn_fs_check_path(&kind, root, path, iterpool3));
if (kind != svn_node_file)
continue;
@@ -1253,20 +1266,23 @@
TRUE, TRUE,
duplicate_path_revs,
authz_read_func,
- authz_read_baton, pool));
+ authz_read_baton, pool,
+ iterpool3));
}
+ svn_pool_destroy(iterpool3);
}
+ svn_pool_destroy(iterpool2);
}
/* Append the newly found path revisions with the old ones. */
- merged_path_revisions = apr_array_append(iter_pool,
merged_path_revisions,
+ merged_path_revisions = apr_array_append(iterpool, merged_path_revisions,
new);
/* Swap data structures */
old = new;
temp_pool = last_pool;
- last_pool = iter_pool;
- iter_pool = temp_pool;
+ last_pool = iterpool;
+ iterpool = temp_pool;
}
while (new->nelts > 0);
@@ -1277,7 +1293,7 @@
/* Copy to the output array. */
*merged_path_revisions_out = apr_array_copy(pool, merged_path_revisions);
- svn_pool_destroy(iter_pool);
+ svn_pool_destroy(iterpool);
svn_pool_destroy(last_pool);
return SVN_NO_ERROR;
@@ -1413,7 +1429,8 @@
SVN_ERR(find_interesting_revisions(mainline_path_revisions, repos, path,
start, end, include_merged_revisions,
FALSE, duplicate_path_revs,
- authz_read_func, authz_read_baton, pool));
+ authz_read_func, authz_read_baton, pool,
+ pool));
/* If we are including merged revisions, go get those, too. */
if (include_merged_revisions)
