On Wed, May 11, 2011 at 3:25 AM, Greg Stein <gst...@gmail.com> wrote:
> 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.
Hi Greg,
Which iterpool are you referring to above? The one in
fix_deleted_subtree_ranges I assume? That is cleared on line 3025 and
destroyed right before the function returns on line 3107.
>>...
>> @@ -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.
Hmmm, something is up. The iterpool in populate_remaining_ranges *is*
cleared: First on line 4128 for the iteration when not honoring
mergeinfo doing a --record-only merge. It is also cleared on line
4215 for the "normal" merge-tracking iteration. The pool is destroyed
at the end of the function on line 4342. There is a long-standing
leak in the first case, where we return early without destroying the
iterpool. I'll fix that, but I don't suspect that was what you were
talking about.
> Also
> note that if you defer its destruction (and maybe an earlier creation,
> too),
The iterpool is actually created at the start of the function. The
patch is a bit misleading because iterpool is needlessly declared and
then constructed on separate lines, I'll fix that.
> 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).
I do use it as a scratch pool. The only exception is the call to
get_full_mergeinfo on line 4231, which can use the iterpool as a
scratch pool (I'll change that too).
>>...
>> @@ -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.
Well only svn_client__get_revision_number can benefit in that way, but
it's something, done r1101957
> 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)
Done r1101957
>>...
>> @@ -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.
Ok, there is a lot of opportunity to do as you suggest in this
function. Done r1101957
>>...
>> @@ -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.
And once again it is cleared on line 8248 and destroyed on line 8345.
I feel like we are looking at different code!
>>...
>> @@ -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.
Done r1101957
>>...
>> @@ -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?
They can, done r1101957
>>...
>> @@ -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.
Done, r1101957
Paul
>>...
>>
>> [... 36 lines stripped ...]
>>
>
> I guess that's the end of the review :-P
>
> Cheers,
> -g
>
