[PATCH 1 of 2 V2] revset: use smartset minus operator

[Durham Goode]

Taking a minus operator, then converting it to "and-not", then converting it back to minus just seems strange.  I can see people screwing up minus operator perf while trying to improve 'and' performance and not even realizing it.  Seems cleaner to just let minus be minus and be done with it.


From: Martin von Zweigbergk <martinvonz at google.com<mailto:martinvonz at google.com>>
Date: Tuesday, February 23, 2016 at 1:44 PM
To: Durham Goode <durham at fb.com<mailto:durham at fb.com>>
Cc: "mercurial-devel at mercurial-scm.org<mailto:mercurial-devel at mercurial-scm.org>" <mercurial-devel at mercurial-scm.org<mailto:mercurial-devel at mercurial-scm.org>>
Subject: Re: [PATCH 1 of 2 V2] revset: use smartset minus operator


Could you elaborate on why? It results in less duplication, which is usually good for maintainability.

The only thing I disliked about my patch was the name "minusset". I would perhaps rename that to "difference". Otherwise it seemed simple and clean to me.

On Tue, Feb 23, 2016, 12:54 Durham Goode <durham at fb.com<mailto:durham at fb.com>> wrote:
Your patch routes all minus revsets through the and-not operator optimizer, which seemed inelegant and prone to future maintenance mistakes.

From: Martin von Zweigbergk <martinvonz at google.com<mailto:martinvonz at google.com>>
Date: Tuesday, February 23, 2016 at 12:45 PM
To: Durham Goode <durham at fb.com<mailto:durham at fb.com>>
Cc: "mercurial-devel at mercurial-scm.org<mailto:mercurial-devel at mercurial-scm.org>" <mercurial-devel at mercurial-scm.org<mailto:mercurial-devel at mercurial-scm.org>>
Subject: Re: [PATCH 1 of 2 V2] revset: use smartset minus operator


I think my patch also took care of all the improvements these two patches address. Are there any cases where my patch would be worse?

On Tue, Feb 23, 2016, 12:30 Durham Goode <durham at fb.com<mailto:durham at fb.com>> wrote:





On 2/19/16, 11:39 PM, "Martin von Zweigbergk" <martinvonz at google.com<mailto:martinvonz at google.com>> wrote:

>On Thu, Feb 18, 2016 at 11:38 AM, Durham Goode <durham at fb.com<mailto:durham at fb.com>> wrote:
>> # HG changeset patch
>> # User Durham Goode <durham at fb.com<mailto:durham at fb.com>>
>> # Date 1455824115 28800
>> #      Thu Feb 18 11:35:15 2016 -0800
>> # Node ID dc8d72c3df7dfcd7b67fa71d679cc7c3ad90f25c
>> # Parent  a036e1ae1fbe88ab99cb861ebfc2e4da7a3912ca
>> revset: use smartset minus operator
>>
>> Previously, revsets like 'X - Y' were translated to be 'X and not Y'. This can
>> be expensive, since if Y is a single commit then 'not Y' becomes a huge set and
>> sometimes the query optimizer doesn't account for it well.
>>
>> This patch changes revsets to use the built in smartset minus operator, which is
>> often smarter than 'X and not Y'.
>
>How does this patch compare to my patch in
>https://urldefense.proofpoint.com/v2/url?u=http-3A__thread.gmane.org_gmane.comp.version-2Dcontrol.mercurial.devel_89348_focus-3D89391-3F&d=CwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=Ic3AKLl02HHMU_yM5NlmBLJc3oZ8MUkGiPj-TnPQQiI&s=NoBhM7NiNWrsbHqoCgMbe-t_MuPSdYkIqB5W2go53mY&e=
>I did not include the test case changes there, but it only affects the
>first one among the ones affected by this patch, so there is no need
>to revert that behavior in a second patch like here. It also makes
>"(20000::) and not (20000)" faster (which these two patches do not).
>What are the disadvantages?

We could add your patch as well, but it seems like it would have to be a separate patch anyway (one adds minus and avoids the minus->and-not translation, then another patch takes advantage of it for and not).  I did not add the optimization to 'and' because none of the benchmark tests showed any perf improvement.  I suppose "(20000::) and not (20000)" might show an improvement, but that probably doesn't stop these from going.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20160223/8cf140e4/attachment.html>