Re: svn commit: r1101578 [2/2] - /subversion/trunk/subversion/libsvn_client/merge.c

On Wed, May 11, 2011 at 3:25 AM, Greg Stein <gstein_at_gmail.com> wrote:
> On Tue, May 10, 2011 at 14:14,  <pburba_at_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_at_REVISION1:URL2_at_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 *) = &range;
>>     }
>>
>> -  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
>
Received on 2011-05-11 18:34:00 CEST