On Tue, May 10, 2011 at 14:14, <pbu...@apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_client/merge.c Tue May 10 18:14:22 2011
>...
> @@ -2989,14 +2997,15 @@ fix_deleted_subtree_ranges(const char *u
> svn_ra_session_t *ra_session,
> notification_receiver_baton_t *notify_b,
> merge_cmd_baton_t *merge_b,
> - apr_pool_t *pool)
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> {
> int i;
> const char *source_root_url;
> - apr_pool_t *iterpool = svn_pool_create(pool);
> + apr_pool_t *iterpool = svn_pool_create(scratch_pool);
> svn_boolean_t is_rollback = revision2 < revision1;
>
> - SVN_ERR(svn_ra_get_repos_root2(ra_session, &source_root_url, pool));
> + SVN_ERR(svn_ra_get_repos_root2(ra_session, &source_root_url,
> scratch_pool));
>
> /* NOTIFY_B->CHILDREN_WITH_MERGEINFO is sorted in depth-first order, so
> start at index 1 to examine only subtrees. */
> @@ -3090,7 +3099,8 @@ fix_deleted_subtree_ranges(const char *u
> revision1, revision2,
> child_primary_source_url,
> ra_session,
> - merge_b->ctx, pool));
> + merge_b->ctx, result_pool,
> + iterpool));
> }
> }
>
iterpool is constructured and used, but never cleared or destroyed.
>...
> @@ -4086,7 +4108,7 @@ populate_remaining_ranges(apr_array_head
> svn_client__merge_path_t *parent;
> int parent_index;
>
> - iterpool = svn_pool_create(pool);
> + iterpool = svn_pool_create(scratch_pool);
>
> /* If we aren't honoring mergeinfo or this is a --record-only merge,
> we'll make quick work of this by simply adding dummy REVISION1:REVISION2
> @@ -4115,7 +4137,8 @@ populate_remaining_ranges(apr_array_head
> child->abspath,
> MAX(revision1, revision2),
> MIN(revision1, revision2),
> - merge_b->ctx, pool, iterpool));
> + merge_b->ctx, result_pool,
> + iterpool));
> }
> else
> {
> @@ -4135,12 +4158,13 @@ populate_remaining_ranges(apr_array_head
> child_inherits_implicit,
> revision1, revision2,
> ra_session, merge_b->ctx,
> - pool, iterpool));
> + result_pool, iterpool));
> }
>
> child->remaining_ranges = svn_rangelist__initialize(revision1,
> revision2,
> - TRUE, pool);
> + TRUE,
> + result_pool);
> }
> return SVN_NO_ERROR;
> }
> @@ -4168,7 +4192,7 @@ populate_remaining_ranges(apr_array_head
> recording mergeinfo describing this merge. */
> if (SVN_IS_VALID_REVNUM(gap_start) && SVN_IS_VALID_REVNUM(gap_end))
> merge_b->implicit_src_gap = svn_rangelist__initialize(gap_start, gap_end,
> - TRUE, pool);
> + TRUE, result_pool);
>
> for (i = 0; i < children_with_mergeinfo->nelts; i++)
> {
> @@ -4209,7 +4233,7 @@ populate_remaining_ranges(apr_array_head
> child->abspath,
> MAX(revision1, revision2),
> 0, /* Get all implicit mergeinfo */
> - merge_b->ctx, pool, pool));
> + merge_b->ctx, result_pool, scratch_pool));
>
> /* If CHILD isn't the merge target find its parent. */
> if (i > 0)
> @@ -4239,7 +4263,8 @@ populate_remaining_ranges(apr_array_head
> merge_b->implicit_src_gap,
> child_inherits_implicit,
> ra_session,
> - merge_b->ctx, pool));
> + merge_b->ctx, result_pool,
> + iterpool));
>
> /* Deal with any gap in URL1@REVISION1:URL2@REVISION2's natural history.
>
> @@ -4297,12 +4322,12 @@ populate_remaining_ranges(apr_array_head
> if (overlaps_or_adjoins)
> SVN_ERR(svn_rangelist_merge(&(child->remaining_ranges),
> merge_b->implicit_src_gap,
> - pool));
> + result_pool));
> else /* equals == TRUE */
> SVN_ERR(svn_rangelist_remove(&(child->remaining_ranges),
> merge_b->implicit_src_gap,
> child->remaining_ranges, FALSE,
> - pool));
> + result_pool));
> }
>
> if (revision1 > revision2) /* Reverse merge */
iterpool is constructed and used, but never cleared or destroyed. Also
note that if you defer its destruction (and maybe an earlier creation,
too), then you can use it as a scratch_pool. (of course, being wary
not to use it for things that need to survive the loop, like a hash
iter).
>...
> @@ -6242,7 +6268,8 @@ normalize_merge_sources(apr_array_header
> const apr_array_header_t *ranges_to_merge,
> svn_ra_session_t *ra_session,
> svn_client_ctx_t *ctx,
> - apr_pool_t *pool)
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> {
> svn_revnum_t youngest_rev = SVN_INVALID_REVNUM;
> svn_revnum_t peg_revnum;
> @@ -6251,30 +6278,32 @@ normalize_merge_sources(apr_array_header
> svn_revnum_t trim_revision = SVN_INVALID_REVNUM;
> apr_array_header_t *merge_range_ts, *segments;
> const char *source_abspath_or_url;
> - apr_pool_t *subpool;
> int i;
> + apr_pool_t *iterpool;
>
> if(!svn_path_is_url(source))
> - SVN_ERR(svn_dirent_get_absolute(&source_abspath_or_url, source, pool));
> + SVN_ERR(svn_dirent_get_absolute(&source_abspath_or_url, source,
> + result_pool));
> else
> source_abspath_or_url = source;
>
> /* Initialize our return variable. */
> - *merge_sources_p = apr_array_make(pool, 1, sizeof(merge_source_t *));
> + *merge_sources_p = apr_array_make(result_pool, 1, sizeof(merge_source_t
> *));
>
> /* Resolve our PEG_REVISION to a real number. */
> SVN_ERR(svn_client__get_revision_number(&peg_revnum, &youngest_rev,
> ctx->wc_ctx,
> source_abspath_or_url,
> - ra_session, peg_revision, pool));
> + ra_session, peg_revision,
> + scratch_pool));
> if (! SVN_IS_VALID_REVNUM(peg_revnum))
> return svn_error_create(SVN_ERR_CLIENT_BAD_REVISION, NULL, NULL);
>
> /* Create a list to hold svn_merge_range_t's. */
> - merge_range_ts = apr_array_make(pool, ranges_to_merge->nelts,
> + merge_range_ts = apr_array_make(scratch_pool, ranges_to_merge->nelts,
> sizeof(svn_merge_range_t *));
>
> - subpool = svn_pool_create(pool);
> + iterpool = svn_pool_create(scratch_pool);
If you created the iterpool earlier, then you could use it as a
scratch_pool for many of the above calls. The memory will get free'd
on the first iteration of the loop, rather than be returned in the
caller's scratch_pool (again: minimize the high-water mark).
>...
> @@ -6318,6 +6348,8 @@ normalize_merge_sources(apr_array_header
> }
> }
>
> + svn_pool_destroy(iterpool);
> +
> /* No ranges to merge? No problem. */
> if (merge_range_ts->nelts == 0)
> return SVN_NO_ERROR;
Again, you may be able to defer the destruction of the iterpool for
some benefit. (tho it complicates the early-exit a bit)
>...
> @@ -6661,12 +6695,11 @@ do_file_merge(svn_mergeinfo_catalog_t re
> APR_ARRAY_PUSH(remaining_ranges, svn_merge_range_t *) = ⦥
> }
>
> - subpool = svn_pool_create(scratch_pool);
> -
> if (!merge_b->record_only)
> {
> apr_array_header_t *ranges_to_merge = remaining_ranges;
> int i;
> + apr_pool_t *iterpool;
>
> /* If we have ancestrally related sources and more than one
> range to merge, eliminate no-op ranges before going through
> @@ -6677,16 +6710,18 @@ do_file_merge(svn_mergeinfo_catalog_t re
> const char *old_sess_url = NULL;
> SVN_ERR(svn_client__ensure_ra_session_url(&old_sess_url,
> merge_b->ra_session1,
> - primary_url, subpool));
> + primary_url,
> + scratch_pool));
> SVN_ERR(remove_noop_merge_ranges(&ranges_to_merge,
> merge_b->ra_session1,
> - remaining_ranges, subpool));
> + remaining_ranges, scratch_pool));
> if (old_sess_url)
> SVN_ERR(svn_ra_reparent(merge_b->ra_session1, old_sess_url,
> - subpool));
> - svn_pool_clear(subpool);
> + scratch_pool));
> }
>
> + iterpool = svn_pool_create(scratch_pool);
Again: earlier construction of the pool could have some benefit for
several of the calls above. It looks like you already had a clear()
for the old subpool... by using iterpool, you'll still get that
clear() on the first iteration.
>...
> @@ -8090,7 +8127,7 @@ do_directory_merge(svn_mergeinfo_catalog
> if (honor_mergeinfo && !merge_b->reintegrate_merge)
> {
> svn_revnum_t new_range_start, start_rev, end_rev;
> - apr_pool_t *iterpool = svn_pool_create(pool);
> + apr_pool_t *iterpool = svn_pool_create(scratch_pool);
>
> /* The merge target TARGET_ABSPATH and/or its subtrees may not need all
> of REVISION1:REVISION2 applied. So examine
> @@ -8112,11 +8149,12 @@ do_directory_merge(svn_mergeinfo_catalog
> SVN_ERR(remove_noop_subtree_ranges(url1, revision1, url2, revision2,
> ra_session,
> notify_b, merge_b,
> - pool, iterpool));
> + scratch_pool, iterpool));
>
> /* Adjust subtrees' remaining_ranges to deal with issue #3067 */
> SVN_ERR(fix_deleted_subtree_ranges(url1, revision1, url2, revision2,
> - ra_session, notify_b, merge_b,
> pool));
> + ra_session, notify_b, merge_b,
> + scratch_pool, iterpool));
>
> /* remove_noop_subtree_ranges() and/or fix_deleted_subtree_range()
> may have further refined the starting revision for our editor
I see iterpool being created and used, but not cleared or destroyed.
>...
> @@ -8465,6 +8505,7 @@ do_merge(apr_hash_t **modified_subtrees,
> svn_boolean_t checked_mergeinfo_capability = FALSE;
> svn_ra_session_t *ra_session1 = NULL, *ra_session2 = NULL;
> svn_node_kind_t target_kind;
> + apr_pool_t *iterpool;
>
> SVN_ERR_ASSERT(svn_dirent_is_absolute(target_abspath));
>
> @@ -8491,7 +8532,7 @@ do_merge(apr_hash_t **modified_subtrees,
> }
>
> SVN_ERR(svn_wc_read_kind(&target_kind, ctx->wc_ctx, target_abspath, FALSE,
> - pool));
> + scratch_pool));
>
> if (target_kind != svn_node_dir && target_kind != svn_node_file)
> return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
> @@ -8509,7 +8550,7 @@ do_merge(apr_hash_t **modified_subtrees,
> SVN_CONFIG_OPTION_DIFF3_CMD, NULL);
>
> if (diff3_cmd != NULL)
> - SVN_ERR(svn_path_cstring_to_utf8(&diff3_cmd, diff3_cmd, pool));
> + SVN_ERR(svn_path_cstring_to_utf8(&diff3_cmd, diff3_cmd, scratch_pool));
>
> /* Build the merge context baton (or at least the parts of it that
> don't need to be reset for each merge source). */
> @@ -8527,8 +8568,10 @@ do_merge(apr_hash_t **modified_subtrees,
> SVN_ERR(svn_wc__node_get_repos_info(&merge_cmd_baton.repos_root_url, NULL,
> ctx->wc_ctx,
> merge_cmd_baton.target_abspath,
> - pool, subpool));
> - merge_cmd_baton.pool = subpool;
> + result_pool,
> + scratch_pool));
> + iterpool = svn_pool_create(scratch_pool);
Again, you may see some benefit from creating iterpool sooner, and
destroying it later.
>...
> @@ -8702,7 +8749,7 @@ merge_cousins_and_supplement_mergeinfo(c
> const apr_array_header_t
> *merge_options,
> svn_boolean_t *use_sleep,
> svn_client_ctx_t *ctx,
> - apr_pool_t *pool)
> + apr_pool_t *scratch_pool)
> {
> svn_opt_revision_range_t *range;
> apr_array_header_t *remove_sources, *add_sources, *ranges;
> @@ -8710,6 +8757,12 @@ merge_cousins_and_supplement_mergeinfo(c
> svn_boolean_t same_repos;
> apr_hash_t *modified_subtrees = NULL;
>
> + /* Sure we could use SCRATCH_POOL througout this function, but since this
> + is a wrapper around three separate merges we'll create a subpool we can
> + clear between each of the three. If the merge target has a lot of
> + subtree mergeinfo, then this will help keep memory use in check. */
> + apr_pool_t *subpool = svn_pool_create(scratch_pool);
> +
> SVN_ERR_ASSERT(svn_dirent_is_absolute(target_abspath));
>
> if (strcmp(wc_repos_root, source_repos_root) != 0)
> @@ -8717,9 +8770,10 @@ merge_cousins_and_supplement_mergeinfo(c
> const char *source_repos_uuid;
> const char *wc_repos_uuid;
>
> - SVN_ERR(svn_ra_get_uuid2(URL1_ra_session, &source_repos_uuid, pool));
> + SVN_ERR(svn_ra_get_uuid2(URL1_ra_session, &source_repos_uuid,
> + scratch_pool));
> SVN_ERR(svn_client_uuid_from_path2(&wc_repos_uuid, target_abspath,
> - ctx, pool, pool));
> + ctx, scratch_pool, scratch_pool));
> same_repos = (strcmp(wc_repos_uuid, source_repos_uuid) == 0);
> }
Looks like these UUIDs have a very short lifetime. Could they use the subpool?
>...
> @@ -10052,26 +10131,28 @@ calculate_left_hand_side(const char **ur
> path_rel_to_session,
> target_rev, target_rev,
> SVN_INVALID_REVNUM,
> - ctx, subpool));
> + ctx, scratch_pool));
> apr_hash_set(segments_hash,
> - apr_pstrdup(subpool, path_rel_to_root),
> + apr_pstrdup(scratch_pool, path_rel_to_root),
> APR_HASH_KEY_STRING, segments);
> }
>
> + svn_pool_destroy(iterpool);
You'd probably get benefit from delaying this destruction.
>...
>
> [... 36 lines stripped ...]
>
I guess that's the end of the review :-P
Cheers,
-g
