[PATCH 2 of 2] revset: make children only look at commits >= minrev

[Olle]

On Wed, Sep 24, 2014 at 1:35 AM, Durham Goode <durham at fb.com> wrote:

>
> On 9/23/14, 11:34 AM, Durham Goode wrote:
>
>>
>> On 9/23/14, 11:27 AM, Pierre-Yves David wrote:
>>
>>>
>>>
>>> On 09/23/2014 11:23 AM, Augie Fackler wrote:
>>>
>>>> On Mon, Sep 22, 2014 at 11:31:09PM -0700, Durham Goode wrote:
>>>>
>>>>> # HG changeset patch
>>>>> # User Durham Goode <durham at fb.com>
>>>>> # Date 1411450748 25200
>>>>> #      Mon Sep 22 22:39:08 2014 -0700
>>>>> # Node ID 14087ac481b1d25ccaa69ed15a014f56c829da65
>>>>> # Parent  e7e0c696c29e1735aad7a77f82bfc987354a98c1
>>>>> revset: make children only look at commits >= minrev
>>>>>
>>>>
>>>> Nice! Both queued, thanks.
>>>>
>>>
>>> Please don't. I really would like that the work on revset focus on
>>> fixing the core mechanism instead of working around its limitation by
>>> making the code more complex. Having more complex code will make it harder
>>> to get the benefit of improving the core mechanism.
>>>
>> I don't think either of these make the code that much more complex, and
>> the performance win is pretty massive.  In particular, patch #2 uses the
>> smartness of the subset to perform it's work more efficiently, which is
>> exactly what the classes are there to allow.
>>
>> As for patch #1, I still think we should be making all our revsets
>> 'localwork & subset' instead of 'subset & localwork' because it logically
>> indicates the better O(...).  For your fullreposet work, you could just
>> have fullreposet.__contains__ return True to provide good performance
>> (similar to your optimization for 'subset & localwork').
>>
> Pierre-Yves and I have come to a stalemate in real life, so perhaps the
> community will have an opinion.
>
> I believe these changes are small enough and impactful enough to go in as
> is. They don't cause any regressions in the revset benchmarks, and are
> unlikely to cause significant regressions in any other cases.  While the
> same performance might be achievable by refactoring the underlying revset
> classes to be smarter, the current change is 3 lines, buys us a 40% perf
> improvement, and doesn't make a future refactor any more difficult.


Push it with an added comment about a future refactor? If we don't get the
full refactored revsets fixed until next release, we would at least have a
faster implementation and a reminder for a refactoring.

>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>



-- 
Olle
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-devel/attachments/20140924/01217e83/attachment-0002.html>