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

On Wed, May 6, 2009 at 5:55 AM, Bert Huijben <bert_at_qqmail.nl> wrote:
>> -----Original Message-----
>> From: Greg Stein [mailto:gstein_at_gmail.com]
>> Sent: woensdag 6 mei 2009 1:06
>> To: dev_at_subversion.tigris.org
>> Subject: Re: svn commit: r37491 - trunk/subversion/libsvn_client
>>
>> On Tue, Apr 28, 2009 at 01:16, Paul T. Burba <pburba_at_collab.net> wrote:
>> >...
>> > +++ trunk/subversion/libsvn_client/merge.c Mon Apr 27 16:16:50 2009
>> (r37491)
>> >...
>> > @@ -2847,7 +2902,8 @@ get_full_mergeinfo(svn_mergeinfo_t *reco
>> > function must have previously been called for PARENT.
>> > */
>> > static svn_error_t *
>> > -filter_merged_revisions(svn_client__merge_path_t *child,
>> > +filter_merged_revisions(svn_client__merge_path_t *parent,
>> > + svn_client__merge_path_t *child,
>> > const svn_wc_entry_t *entry,
>> > const char *mergeinfo_path,
>> > svn_mergeinfo_t target_mergeinfo,
>> > @@ -2942,12 +2998,33 @@ filter_merged_revisions(svn_client__merg
>> > /* If we haven't already found CHILD->IMPLICIT_MERGEINFO then
>> > contact the server to get it. */
>> > if (! (child->implicit_mergeinfo))
>> > - SVN_ERR(get_full_mergeinfo(NULL, &(child->implicit_mergeinfo),
>> > - entry, NULL,
>> svn_mergeinfo_inherited,
>> > - ra_session, child->path,
>> > - MAX(revision1, revision2),
>> > - MIN(revision1, revision2),
>> > - adm_access, ctx, subpool));
>> > + {
>> > + /* If CHILD has explicit mergeinfo only because its parent
>> has
>> > + non-inheritable mergeinfo (see criteria 3 in
>> > + get_mergeinfo_paths() then CHILD can inherit PARENT's
>> > + implicit mergeinfo and we can avoid contacting the server.
>> > +
>> > + If child->child_of_noninheritable is true, it implies that
>> > + PARENT must exist per the rules of get_mergeinfo_paths().
>> */
>> > + if (child->child_of_noninheritable)
>> > + SVN_ERR(inherit_implicit_mereginfo_form_parent(parent,
>> > + child,
>> > + revision1,
>> > + revision2,
>> > + ra_session,
>> > + adm_access,
>> > + ctx,
>> > + pool));
>> > + else
>> > + SVN_ERR(get_full_mergeinfo(NULL,
>> > + &(child->implicit_mergeinfo),
>> > + entry, NULL,
>> > + svn_mergeinfo_inherited,
>> > + ra_session, child->path,
>> > + MAX(revision1, revision2),
>> > + MIN(revision1, revision2),
>> > + adm_access, ctx, subpool));
>> > + }
>> >
>> > target_implicit_rangelist = apr_hash_get(child-
>> >implicit_mergeinfo,
>> > mergeinfo_path,
>> > @@ -3029,12 +3106,33 @@ filter_merged_revisions(svn_client__merg
>> > ranges are represented there. If we don't already have
>> > CHILD->IMPLICIT_MERGEINFO then get it now. */
>> > if (! (child->implicit_mergeinfo))
>> > - SVN_ERR(get_full_mergeinfo(NULL, &(child->implicit_mergeinfo),
>> > - entry, NULL,
>> svn_mergeinfo_inherited,
>> > - ra_session, child->path,
>> > - MAX(revision1, revision2),
>> > - MIN(revision1, revision2),
>> > - adm_access, ctx, pool));
>> > + {
>> > + /* If CHILD has explicit mergeinfo only because its parent
>> has
>> > + non-inheritable mergeinfo (see criteria 3 in
>> > + get_mergeinfo_paths() then CHILD can inherit PARENT's
>> > + implicit mergeinfo and we can avoid contacting the server.
>> > +
>> > + If child->child_of_noninheritable is true, it implies that
>> > + PARENT must exist per the rules of get_mergeinfo_paths().
>> */
>> > + if (child->child_of_noninheritable)
>> > + SVN_ERR(inherit_implicit_mereginfo_form_parent(parent,
>> > + child,
>> > + revision1,
>> > + revision2,
>> > + ra_session,
>> > + adm_access,
>> > + ctx,
>> > + pool));
>> > + else
>> > + SVN_ERR(get_full_mergeinfo(NULL,
>> > + &(child->implicit_mergeinfo),
>> > + entry, NULL,
>> > + svn_mergeinfo_inherited,
>> > + ra_session, child->path,
>> > + MAX(revision1, revision2),
>> > + MIN(revision1, revision2),
>> > + adm_access, ctx, pool));
>> > + }
>>
>> The above couple blocks of code are *exactly* the same.
>>
>> I see no rationale for condoning this for a backport. I don't know if
>> this is a single instance of the merge code bloat, or if it is
>> indicative. But we shouldn't see duplicative code like this.
>
> Working on an initial patch to address these concerns (to hopefully get the fix in 1.6.2) I found the 2 calls are /not/ identical.
>
>
> The first call stores the result of get_full_mergeinfo in subpool, the second in pool.
>
> As child lives longer than subpool, I think both should be stored in pool.
>
>
> Paul, could you take a look at this patch? (and this pool lifetime issue).
>
>
> Bert
>
> [[[
> Stop the bloat.
>
> * subversion/libsvn_client/merge.c
> (ensure_implicit_mergeinfo): New helper function.
> (filter_merged_revisions): Use ensure_implicit_mergeinfo instead of duplicating code.
>
> Patch by: rhuijben
> ]]]

In IRC this morning:

[[[
<Bert> gstein: Patch for the 'Stop the bloat.' on list :)

<gstein> Bert: your patch. might be clearer to rename that pool to
result_pool or scratch_pool. What is the intended usage? Is it for
allocating a return value? or just scratch space. If it is used for a
return value, then add another scratch_pool argument, even if it isn't
used. We could then use it in the future. (and it sounds like subpool
could be used for it)

<Bert> The result is stored in child.. which most likely lives longer
than subpool.. (as it is also passed as parent to the children)

<gstein> oh dear
<gstein> don't tell me...

<Bert> I tried to store the result in subpool, but that broke a few dozen tests

<gstein> child is carrying its own pool?

<Bert> I don't think so.. but I think it should after a code cleanup

<gstein> Bert: structures should not carry their own pool. that way
lies madness.

<Bert> gstein: No.. some fields in parent can be unavailable when
walking the first child.. (It should probably walk the ancestors
instead of just the parent)

<Bert> Maybe we should move this to the list (or wait on Paul).. Paul
knows much more about this code than I do.. I know how several parts
work, but I don't have a total overview :(

<gstein> Bert: meh. not too concerned. these are just some simple
transforms on your patch...
]]]

Hi Greg and Bert,

To clarify, the "child" structures, svn_client__merge_path_t, do not
carry their own pools.

And no, we don't want to allocate CHILD->IMPLICIT_MERGEINFO in the
subpool, these children svn_client__merge_path_t elements in the
CHILDREN_WITH_MERGEINFO array need to live as long as the subpool that
libsvn_client/merge.c:do_merge() passes to do_directory_merge(). The
fact that I was passing the subpool to the get_full_mergeinfo() call
when processing reverse merges is definitely wrong, our test suite
simply didn't expose it (as Bert noted, using a subpool for forward
merges will cause many tests to fail).

Bert -- Re your comment 'It should probably walk the ancestors instead
of just the parent': inherit_implicit_mergeinfo_from_parent() will get
PARENT->IMPLICIT_MERGEINFO if that was deferred, so we only need look
to the parent (unless I am not correctly understanding your concern?).

Greg -- I see now that we are using the double pool ('result_pool' and
'scratch_pool') approach quite extensively. I have not been doing
this, but I adjusted Bert's patch to use this approach and will use it
myself going forward.

Other than the double pool, the only other changes I've made to Bert's
patch are to the doc string for ensure_implicit_mergeinfo().

Paul

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2083002