[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