blair_at_tigris.org writes:
> Author: blair
> Date: Fri Nov 14 17:46:41 2008
> New Revision: 34213
>
> Log:
> * subversion/libsvn_fs_fs/tree.c
> (svn_fs_fs__commit_txn):
> Fix excessive memory usage during commit in repositories with high
> commit rates where the transactions modify nodes that have 20,000
> siblings nodes.
>
> Modified:
> trunk/subversion/libsvn_fs_fs/tree.c
>
> Modified: trunk/subversion/libsvn_fs_fs/tree.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_fs_fs/tree.c?pathrev=34213&r1=34212&r2=34213
> ==============================================================================
> --- trunk/subversion/libsvn_fs_fs/tree.c Fri Nov 14 15:40:18 2008 (r34212)
> +++ trunk/subversion/libsvn_fs_fs/tree.c Fri Nov 14 17:46:41 2008 (r34213)
> @@ -1648,10 +1648,16 @@ svn_fs_fs__commit_txn(const char **confl
> * Lather, rinse, repeat.
> */
>
> - svn_error_t *err;
> + svn_error_t *err = SVN_NO_ERROR;
> svn_revnum_t new_rev;
> svn_fs_t *fs = txn->fs;
>
> + /* Limit memory usage when the repository has a high commit rate and
> + needs to run the following while loop multiple times. The memory
> + growth without an iteration pool is very noticeable when the
> + transaction modifies a node that has 20,000 sibling nodes. */
> + apr_pool_t *iterpool = svn_pool_create(pool);
> +
> /* Initialize output params. */
> new_rev = SVN_INVALID_REVNUM;
> if (conflict_p)
> @@ -1662,7 +1668,11 @@ svn_fs_fs__commit_txn(const char **confl
> svn_revnum_t youngish_rev;
> svn_fs_root_t *youngish_root;
> dag_node_t *youngish_root_node;
> - svn_stringbuf_t *conflict = svn_stringbuf_create("", pool);
> + svn_stringbuf_t *conflict;
> +
> + svn_pool_clear(iterpool);
> +
> + conflict = svn_stringbuf_create("", iterpool);
Allocating conflict from iterpool causes fs-test 32 to fail:
valgrind -q subversion/tests/libsvn_fs/.libs/lt-fs-test 32
==17969== Invalid read of size 1
==17969== at 0x4A1C7D1: strcmp (mc_replace_strmem.c:341)
==17969== by 0x40246E: test_commit_txn (fs-test.c:87)
==17969== by 0x412E2B: unordered_txn_dirprops (fs-test.c:4706)
==17969== by 0x4B2499A: do_test_num (svn_test_main.c:192)
==17969== by 0x4B25132: main (svn_test_main.c:395)
==17969== Address 0x8D59D28 is 0 bytes inside a block of size 8 free'd
==17969== at 0x4A1B46D: free (vg_replace_malloc.c:233)
==17969== by 0x50C978B: pool_clear_debug (apr_pools.c:1395)
==17969== by 0x50C989D: pool_destroy_debug (apr_pools.c:1457)
==17969== by 0x5AC3DD3: svn_fs_fs__commit_txn (tree.c:1739)
==17969== by 0x4C2B191: svn_fs_commit_txn (fs-loader.c:628)
==17969== by 0x402394: test_commit_txn (fs-test.c:67)
==17969== by 0x412E2B: unordered_txn_dirprops (fs-test.c:4706)
==17969== by 0x4B2499A: do_test_num (svn_test_main.c:192)
==17969== by 0x4B25132: main (svn_test_main.c:395)
Would it work to create the conflict stringbuf in pool outside the
looop? Or...
>
> /* Get the *current* youngest revision, in one short-lived
> Berkeley transaction. (We don't want the revisions table
> @@ -1670,9 +1680,9 @@ svn_fs_fs__commit_txn(const char **confl
> because new revisions might get committed after we've
> obtained it. */
>
> - SVN_ERR(svn_fs_fs__youngest_rev(&youngish_rev, fs, pool));
> + SVN_ERR(svn_fs_fs__youngest_rev(&youngish_rev, fs, iterpool));
> SVN_ERR(svn_fs_fs__revision_root(&youngish_root, fs, youngish_rev,
> - pool));
> + iterpool));
>
> /* Get the dag node for the youngest revision, also in one
> Berkeley transaction. Later we'll use it as the SOURCE
> @@ -1681,23 +1691,23 @@ svn_fs_fs__commit_txn(const char **confl
> was the target of the merge (but note that the youngest rev
> may have changed by then -- that's why we're careful to get
> this root in its own bdb txn here). */
> - SVN_ERR(get_root(&youngish_root_node, youngish_root, pool));
> + SVN_ERR(get_root(&youngish_root_node, youngish_root, iterpool));
>
> /* Try to merge. If the merge succeeds, the base root node of
> TARGET's txn will become the same as youngish_root_node, so
> any future merges will only be between that node and whatever
> the root node of the youngest rev is by then. */
> - err = merge_changes(NULL, youngish_root_node, txn, conflict, pool);
> + err = merge_changes(NULL, youngish_root_node, txn, conflict, iterpool);
> if (err)
> {
> if ((err->apr_err == SVN_ERR_FS_CONFLICT) && conflict_p)
> *conflict_p = conflict->data;
...copy the data from iterpool into pool here.
> - return err;
> + goto cleanup;
> }
> txn->base_rev = youngish_rev;
>
> /* Try to commit. */
> - err = svn_fs_fs__commit(&new_rev, fs, txn, pool);
> + err = svn_fs_fs__commit(&new_rev, fs, txn, iterpool);
> if (err && (err->apr_err == SVN_ERR_FS_TXN_OUT_OF_DATE))
> {
> /* Did someone else finish committing a new revision while we
> @@ -1706,25 +1716,28 @@ svn_fs_fs__commit_txn(const char **confl
> commit again. Or if that's not what happened, then just
> return the error. */
> svn_revnum_t youngest_rev;
> - SVN_ERR(svn_fs_fs__youngest_rev(&youngest_rev, fs, pool));
> + SVN_ERR(svn_fs_fs__youngest_rev(&youngest_rev, fs, iterpool));
> if (youngest_rev == youngish_rev)
> - return err;
> + goto cleanup;
> else
> svn_error_clear(err);
> }
> else if (err)
> {
> - return err;
> + goto cleanup;
> }
> else
> {
> /* Set the return value -- our brand spankin' new revision! */
> *new_rev_p = new_rev;
> - return SVN_NO_ERROR;
> + err = SVN_NO_ERROR;
> + goto cleanup;
> }
> }
>
> - /* NOTREACHED */
> + cleanup:
> + svn_pool_destroy(iterpool);
> + return err;
> }
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-11-22 22:59:27 CET