[PATCH 1 of 5] revlog: make commonancestorsheads accept revision numbers

Pierre-Yves David pierre-yves.david at ens-lyon.org
Thu Nov 3 15:11:31 EDT 2016



On 11/02/2016 03:59 AM, Jun Wu wrote:
> Excerpts from Gregory Szorc's message of 2016-11-01 19:47:07 -0700:
>> On Tue, Nov 1, 2016 at 7:25 PM, Jun Wu <quark at fb.com> wrote:
>>
>>> Excerpts from Pierre-Yves David's message of 2016-11-02 00:05:32 +0100:
>>>> I think I would rather see a two methods, one for revs and one for nodes
>>>> that would keep the signature of each function clearer. One of the
>>>> function would probably all into the other one to keep the code common.
>>>>
>>>> What do you think?
>>>
>>> If we have two "commonancestorsheads"s, we will have to write two
>>> "isancestor"s. I'd avoid the contagious pattern that enforces the caller
>>> ("isancestor" in this case) to do things otherwise unnecessary.
>>>
>>> Given the fact that "repo.rev(x)" takes both node and rev. And repo[x]
>>> takes
>>> everything. I'd prefer shorter code. It can be seen as a reasonable
>>> function
>>> overloading in C++.

The two commands you cite are high level function that live on a top 
level function, the repository. This series is touching code at the 
changelog/revlog level something much lower level were we try to have 
simpler signature and more channeled usage. I was once tempted by having 
more flexible argument at this level but over time things emerged as 
cleaner the current way (with adjusting functions from time to time). On 
such low level object, keeping function simpler and clear is also 
helpful to not oversight anything regarding performance.

So I really would like to avoid this flexible argument types here. Can 
you submit a new version with either two methods or a single one (the 
simplest) and the few caller converted as Greg suggested ?

>> I agree with Pierre-Yves here: it is too easy to fall into performance
>> traps where functions accept either node or rev and needlessly perform type
>> checking or dip into the index for unneeded lookups. Let's standardize on
>> one. With list comprehensions, it's easy enough to ensure arguments are
>> either all revs or all nodes.
>>
>> I do disagree about the need for 2 methods. Just standardize on whatever
>> one makes sense and have the caller convert to that type. This may entail a
>
> The convert is not free (coverting from node -> rev requires walking through
> the whole index for the first 2 times). This patch is all about avoiding
> converting from node -> rev -> node (commonancestorsheads), and the return
> value is a bool (isancestor). Comparing with converting between node and
> rev, I think "isinstance" is a cheaper operation. It should be even much
> faster comparing with commonancestorsheads's C implementation. When the
> commonancestorsheads C implementation is the bottleneck, I don't think
> it's necessary to optimize those "isinstance" out is necessary.
>
>> `revlog.revstonodes(revs)` or `revlog.nodestorevs(nodes)` to facilitate
>> bulk converting iterables of 1 type to the other.

-- 
Pierre-Yves David


More information about the Mercurial-devel mailing list