Re: svn commit: r31504 - trunk/subversion/libsvn_client

Karl Fogel wrote:
> cmpilato_at_tigris.org writes:
>> Log:
>> Optimize file merges performed with ancestrally related sources by
>> using the svn_ra_get_log2() interface to figure out in which revisions
>> the source really changed, and using that information to filter out
>> no-op revision ranges prior to merging. We still *record* the
>> original set of revision ranges, of course.
>>
>> * subversion/libsvn_client/merge.c
>> (log_changed_revs, remove_noop_merge_ranges): New functions.
>> (do_file_merge): Selective use remove_noop_merge_ranges() to reduce
>> the amount of work this function needs to do.
>>
>> --- trunk/subversion/libsvn_client/merge.c (r31503)
>> +++ trunk/subversion/libsvn_client/merge.c (r31504)
>> @@ -3543,6 +3543,89 @@ get_mergeinfo_paths(apr_array_header_t *
>> }
>>
>>
>> +/* Implements the svn_log_entry_receiver_t interface. */
>> +static svn_error_t *
>> +log_changed_revs(void *baton,
>> + svn_log_entry_t *log_entry,
>> + apr_pool_t *pool)
>> +{
>> + apr_array_header_t *revs = baton;
>> + svn_revnum_t *revision = apr_palloc(revs->pool, sizeof(*revision));
>> + *revision = log_entry->revision;
>> + APR_ARRAY_PUSH(revs, svn_revnum_t *) = revision;
>> + return SVN_NO_ERROR;
>> +}
>
> We should also document what the function actually does -- the fact that
> it implements a type is important, but that's not all there is to say
> (we have to say what BATON is, for example). I committed a doc string
> tweak in r31845.
>
>> +/* Set *OPERATIVE_RANGES_P to an array of svn_merge_range_t * merge
>> + range objects copied wholesale from RANGES which have the property
>> + that in some revision within that range the object identified by
>> + RA_SESSION was modified (if by "modified" we mean "'svn log' would
>> + return that revision). *OPERATIVE_RANGES_P is allocated from the
>> + same pool as RANGES, and the ranges within it are shared with
>> + RANGES, too. Use POOL for temporary allocations. */
>> +static svn_error_t *
>> +remove_noop_merge_ranges(apr_array_header_t **operative_ranges_p,
>> + svn_ra_session_t *ra_session,
>> + apr_array_header_t *ranges,
>> + apr_pool_t *pool)
>> +{
>> + int i;
>> + svn_revnum_t oldest_rev = SVN_INVALID_REVNUM;
>> + svn_revnum_t youngest_rev = SVN_INVALID_REVNUM;
>> + apr_array_header_t *changed_revs =
>> + apr_array_make(pool, ranges->nelts, sizeof(svn_revnum_t *));
>> + apr_array_header_t *operative_ranges =
>> + apr_array_make(ranges->pool, ranges->nelts, ranges->elt_size);
>> + apr_array_header_t *log_targets =
>> + apr_array_make(pool, 1, sizeof(const char *));
>> + APR_ARRAY_PUSH(log_targets, const char *) = "";
>> +
>> + /* Find the revision extremes of the RANGES we have. */
>> + for (i = 0; i < ranges->nelts; i++)
>> + {
>> + svn_merge_range_t *r = APR_ARRAY_IDX(ranges, i, svn_merge_range_t *);
>> + svn_revnum_t max_rev = MAX(r->start, r->end);
>> + svn_revnum_t min_rev = MIN(r->start, r->end) + 1;
>> +
>> + if ((! SVN_IS_VALID_REVNUM(youngest_rev)) || (max_rev > youngest_rev))
>> + youngest_rev = max_rev;
>> + if ((! SVN_IS_VALID_REVNUM(oldest_rev)) || (min_rev < oldest_rev))
>> + oldest_rev = min_rev;
>> + }
>> +
>> + /* Get logs across those ranges, recording which revisions hold
>> + changes to our object's history. */
>> + SVN_ERR(svn_ra_get_log2(ra_session, log_targets, youngest_rev,
>> + oldest_rev, 0, FALSE, FALSE, FALSE,
>> + apr_array_make(pool, 0, sizeof(const char *)),
>> + log_changed_revs, changed_revs, pool));
>> +
>> + /* Now, copy from RANGES to *OPERATIVE_RANGES, filtering out ranges
>> + that aren't operative (by virtue of not having any revisions
>> + represented in the CHANGED_REVS array). */
>> + for (i = 0; i < ranges->nelts; i++)
>> + {
>> + svn_merge_range_t *range = APR_ARRAY_IDX(ranges, i, svn_merge_range_t *);
>> + int j;
>> +
>> + for (j = 0; j < changed_revs->nelts; j++)
>> + {
>> + svn_revnum_t *changed_rev =
>> + APR_ARRAY_IDX(changed_revs, j, svn_revnum_t *);
>> + if ((*changed_rev > MIN(range->start, range->end))
>> + && (*changed_rev <= MAX(range->start, range->end)))
>> + {
>> + APR_ARRAY_PUSH(operative_ranges, svn_merge_range_t *) = range;
>> + break;
>> + }
>> + }
>> + }
>> + *operative_ranges_p = operative_ranges;
>> + return SVN_NO_ERROR;
>> +}
>
> Hmmm, a question about that final (innermost) for-loop:
>
> for (j = 0; j < changed_revs->nelts; j++)
> {
> ...
> }
>
> Do we really have to loop over (on average) half of changed_revs each
> time to evaluate whether or not the range in question is operative?
> After all, changed_revs is in sorted order already -- I don't know which
> way, but svn_ra_get_log2()'s doc string has details on that. So instead
> of looping every time, we could just compare
>
> APR_ARRAY_IDX(changed_revs, 0, svn_revnum_t *)
> APR_ARRAY_IDX(changed_revs, changed_revs->nelts, svn_revnum_t *)

changed_revs->nelts-1 here, but yes, I see what you're saying.

-- 
C. Michael Pilato <cmpilato_at_collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand