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

[Durham Goode]

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/cda11ac2/attachment.html>