Patchwork [1,of,2,obs-speedup] obsolete: add C implementation of _addsuccessors

login
register
mail settings
Submitter Augie Fackler
Date Aug. 24, 2015, 6:25 p.m.
Message ID <3aecfdbfe3ca54134cdb.1440440755@arthedain.pit.corp.google.com>
Download mbox | patch
Permalink /patch/10266/
State Rejected
Headers show

Comments

Augie Fackler - Aug. 24, 2015, 6:25 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1440439282 14400
#      Mon Aug 24 14:01:22 2015 -0400
# Node ID 3aecfdbfe3ca54134cdb0e98944e8fca53bd46f1
# Parent  207e95720308518aa40c2a91ce20fdf52dfa8550
obsolete: add C implementation of _addsuccessors

Before, best of 10 runs:
hg sl > /dev/null  0.75s user 0.07s system 99% cpu 0.828 total

After, best of 10 runs:
./hg sl > /dev/null  0.65s user 0.09s system 99% cpu 0.744 total

where
alias.sl=log -Gr smart -Tsl
revsetalias.smart=(parents(not public()) or not public() or . or head()) and (not obsolete() or unstable()^)

It's a straight-line port to C code, but still a 10% win on walltime
for the command. I can live with that.
Sean Farley - Aug. 24, 2015, 6:46 p.m.
Augie Fackler <raf@durin42.com> writes:

> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1440439282 14400
> #      Mon Aug 24 14:01:22 2015 -0400
> # Node ID 3aecfdbfe3ca54134cdb0e98944e8fca53bd46f1
> # Parent  207e95720308518aa40c2a91ce20fdf52dfa8550
> obsolete: add C implementation of _addsuccessors
>
> Before, best of 10 runs:
> hg sl > /dev/null  0.75s user 0.07s system 99% cpu 0.828 total
>
> After, best of 10 runs:
> ./hg sl > /dev/null  0.65s user 0.09s system 99% cpu 0.744 total
>
> where
> alias.sl=log -Gr smart -Tsl
> revsetalias.smart=(parents(not public()) or not public() or . or head()) and (not obsolete() or unstable()^)
>
> It's a straight-line port to C code, but still a 10% win on walltime
> for the command. I can live with that.
>
> diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
> --- a/mercurial/obsolete.py
> +++ b/mercurial/obsolete.py
> @@ -470,10 +470,16 @@ class marker(object):
>          return self._data[2]
>  
>  @util.nogc
> -def _addsuccessors(successors, markers):
> +def _addsuccessors_pure(successors, markers):
>      for mark in markers:
>          successors.setdefault(mark[0], set()).add(mark)
>  
> +def _addsuccessors(successors, markers):
> +    global _addsuccessors
> +    fn = getattr(parsers, 'addsuccessors', _addsuccessors_pure)
> +    _addsuccessors = fn
> +    return fn(successors, markers)
> +

Nit: is this how we do C function stuff? With globals? Honestly asking
here.

>  @util.nogc
>  def _addprecursors(precursors, markers):
>      for mark in markers:
> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
> --- a/mercurial/parsers.c
> +++ b/mercurial/parsers.c
> @@ -2700,6 +2700,46 @@ bail:
>  	return NULL;
>  }
>  
> +static PyObject *addsuccessors(PyObject *self, PyObject *args) {
> +	Py_ssize_t i, len;
> +	PyObject *succs = NULL, *marks = NULL;
> +
> +	if (!PyArg_ParseTuple(args, "O!O!", &PyDict_Type, &succs,
> +			      &PyList_Type, &marks)) {

Minor nit: if my calculations are correct, this line has one too many
tabs.

> +		return NULL;
> +	}
> +	len = PyList_Size(marks);
> +	for (i = 0; i < len; i++) {
> +		PyObject *mark, *key, *dest;
> +		mark = PyList_GetItem(marks, i);
> +		if (!mark) {
> +			return NULL;
> +		}
> +		key = PyTuple_GetItem(mark, 0);
> +		if (!key) {
> +			return NULL;
> +		}
> +		if (PyDict_Contains(succs, key)) {
> +			dest = PyDict_GetItem(succs, key);
> +		} else {
> +			int r;
> +			dest = PySet_New(NULL);
> +			if (!dest)
> +				return NULL;
> +			r = PyDict_SetItem(succs, key, dest);
> +			Py_DECREF(dest);
> +			if (r)
> +				return NULL;
> +		}
> +		if (!dest)
> +			return NULL;
> +		if (PySet_Add(dest, mark))
> +			return NULL;
> +	}
> +	Py_INCREF(Py_None);
> +	return Py_None;
> +}
> +
>  static char parsers_doc[] = "Efficient content parsing.";
>  
>  PyObject *encodedir(PyObject *self, PyObject *args);
> @@ -2722,6 +2762,8 @@ static PyMethodDef methods[] = {
>  	{"lowerencode", lowerencode, METH_VARARGS, "lower-encode a path\n"},
>  	{"fm1readmarkers", fm1readmarkers, METH_VARARGS,
>  			"parse v1 obsolete markers\n"},
> +	{"addsuccessors", addsuccessors, METH_VARARGS,
> +	 "add successors to a dict\n"},
>  	{NULL, NULL}
>  };
>  
> diff --git a/mercurial/revset.py b/mercurial/revset.py
> --- a/mercurial/revset.py
> +++ b/mercurial/revset.py
> @@ -2945,6 +2945,7 @@ class abstractsmartset(object):
>  
>          This is part of the mandatory API for smartset."""
>          c = other.__contains__
> +        # this is slow?
>          return self.filter(lambda r: not c(r), cache=False)
>  
>      def filter(self, condition, cache=True):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Augie Fackler - Aug. 24, 2015, 6:52 p.m.
On Mon, Aug 24, 2015 at 11:46:16AM -0700, Sean Farley wrote:
>
> Augie Fackler <raf@durin42.com> writes:
>
> > # HG changeset patch
> > # User Augie Fackler <augie@google.com>
> > # Date 1440439282 14400
> > #      Mon Aug 24 14:01:22 2015 -0400
> > # Node ID 3aecfdbfe3ca54134cdb0e98944e8fca53bd46f1
> > # Parent  207e95720308518aa40c2a91ce20fdf52dfa8550
> > obsolete: add C implementation of _addsuccessors
> >
> > Before, best of 10 runs:
> > hg sl > /dev/null  0.75s user 0.07s system 99% cpu 0.828 total
> >
> > After, best of 10 runs:
> > ./hg sl > /dev/null  0.65s user 0.09s system 99% cpu 0.744 total
> >
> > where
> > alias.sl=log -Gr smart -Tsl
> > revsetalias.smart=(parents(not public()) or not public() or . or head()) and (not obsolete() or unstable()^)
> >
> > It's a straight-line port to C code, but still a 10% win on walltime
> > for the command. I can live with that.
> >
> > diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
> > --- a/mercurial/obsolete.py
> > +++ b/mercurial/obsolete.py
> > @@ -470,10 +470,16 @@ class marker(object):
> >          return self._data[2]
> >
> >  @util.nogc
> > -def _addsuccessors(successors, markers):
> > +def _addsuccessors_pure(successors, markers):
> >      for mark in markers:
> >          successors.setdefault(mark[0], set()).add(mark)
> >
> > +def _addsuccessors(successors, markers):
> > +    global _addsuccessors
> > +    fn = getattr(parsers, 'addsuccessors', _addsuccessors_pure)
> > +    _addsuccessors = fn
> > +    return fn(successors, markers)
> > +
>
> Nit: is this how we do C function stuff? With globals? Honestly asking
> here.

It might be too clever, but basically I'm causing the loader stub to
disappear once we know (at runtime) which implementation we're going
to use.

>
> >  @util.nogc
> >  def _addprecursors(precursors, markers):
> >      for mark in markers:
> > diff --git a/mercurial/parsers.c b/mercurial/parsers.c
> > --- a/mercurial/parsers.c
> > +++ b/mercurial/parsers.c
> > @@ -2700,6 +2700,46 @@ bail:
> >     return NULL;
> >  }
> >
> > +static PyObject *addsuccessors(PyObject *self, PyObject *args) {
> > +	Py_ssize_t i, len;
> > +	PyObject *succs = NULL, *marks = NULL;
> > +
> > +	if (!PyArg_ParseTuple(args, "O!O!", &PyDict_Type, &succs,
> > +                         &PyList_Type, &marks)) {
>
> Minor nit: if my calculations are correct, this line has one too many
> tabs.

My emacs with vaguely-kernel-ish settings produced this. My personal
opinion is that this line should have exactly one tab and then spaces,
but I'm in the minority here.

>
> > +		return NULL;
> > +	}
> > +	len = PyList_Size(marks);
> > +	for (i = 0; i < len; i++) {
> > +		PyObject *mark, *key, *dest;
> > +		mark = PyList_GetItem(marks, i);
> > +		if (!mark) {
> > +			return NULL;
> > +		}
> > +		key = PyTuple_GetItem(mark, 0);
> > +		if (!key) {
> > +			return NULL;
> > +		}
> > +		if (PyDict_Contains(succs, key)) {
> > +			dest = PyDict_GetItem(succs, key);
> > +		} else {
> > +			int r;
> > +			dest = PySet_New(NULL);
> > +			if (!dest)
> > +				return NULL;
> > +			r = PyDict_SetItem(succs, key, dest);
> > +			Py_DECREF(dest);
> > +			if (r)
> > +				return NULL;
> > +		}
> > +		if (!dest)
> > +			return NULL;
> > +		if (PySet_Add(dest, mark))
> > +			return NULL;
> > +	}
> > +	Py_INCREF(Py_None);
> > +	return Py_None;
> > +}
> > +
> >  static char parsers_doc[] = "Efficient content parsing.";
> >
> >  PyObject *encodedir(PyObject *self, PyObject *args);
> > @@ -2722,6 +2762,8 @@ static PyMethodDef methods[] = {
> >     {"lowerencode", lowerencode, METH_VARARGS, "lower-encode a path\n"},
> >     {"fm1readmarkers", fm1readmarkers, METH_VARARGS,
> >                     "parse v1 obsolete markers\n"},
> > +	{"addsuccessors", addsuccessors, METH_VARARGS,
> > +    "add successors to a dict\n"},
> >     {NULL, NULL}
> >  };
> >
> > diff --git a/mercurial/revset.py b/mercurial/revset.py
> > --- a/mercurial/revset.py
> > +++ b/mercurial/revset.py
> > @@ -2945,6 +2945,7 @@ class abstractsmartset(object):
> >
> >          This is part of the mandatory API for smartset."""
> >          c = other.__contains__
> > +        # this is slow?
> >          return self.filter(lambda r: not c(r), cache=False)
> >
> >      def filter(self, condition, cache=True):
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@selenic.com
> > https://selenic.com/mailman/listinfo/mercurial-devel
>
Sean Farley - Aug. 24, 2015, 6:54 p.m.
Augie Fackler <raf@durin42.com> writes:

> On Mon, Aug 24, 2015 at 11:46:16AM -0700, Sean Farley wrote:
>>
>> Augie Fackler <raf@durin42.com> writes:
>>
>> > # HG changeset patch
>> > # User Augie Fackler <augie@google.com>
>> > # Date 1440439282 14400
>> > #      Mon Aug 24 14:01:22 2015 -0400
>> > # Node ID 3aecfdbfe3ca54134cdb0e98944e8fca53bd46f1
>> > # Parent  207e95720308518aa40c2a91ce20fdf52dfa8550
>> > obsolete: add C implementation of _addsuccessors
>> >
>> > Before, best of 10 runs:
>> > hg sl > /dev/null  0.75s user 0.07s system 99% cpu 0.828 total
>> >
>> > After, best of 10 runs:
>> > ./hg sl > /dev/null  0.65s user 0.09s system 99% cpu 0.744 total
>> >
>> > where
>> > alias.sl=log -Gr smart -Tsl
>> > revsetalias.smart=(parents(not public()) or not public() or . or head()) and (not obsolete() or unstable()^)
>> >
>> > It's a straight-line port to C code, but still a 10% win on walltime
>> > for the command. I can live with that.
>> >
>> > diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
>> > --- a/mercurial/obsolete.py
>> > +++ b/mercurial/obsolete.py
>> > @@ -470,10 +470,16 @@ class marker(object):
>> >          return self._data[2]
>> >
>> >  @util.nogc
>> > -def _addsuccessors(successors, markers):
>> > +def _addsuccessors_pure(successors, markers):
>> >      for mark in markers:
>> >          successors.setdefault(mark[0], set()).add(mark)
>> >
>> > +def _addsuccessors(successors, markers):
>> > +    global _addsuccessors
>> > +    fn = getattr(parsers, 'addsuccessors', _addsuccessors_pure)
>> > +    _addsuccessors = fn
>> > +    return fn(successors, markers)
>> > +
>>
>> Nit: is this how we do C function stuff? With globals? Honestly asking
>> here.
>
> It might be too clever, but basically I'm causing the loader stub to
> disappear once we know (at runtime) which implementation we're going
> to use.
>
>>
>> >  @util.nogc
>> >  def _addprecursors(precursors, markers):
>> >      for mark in markers:
>> > diff --git a/mercurial/parsers.c b/mercurial/parsers.c
>> > --- a/mercurial/parsers.c
>> > +++ b/mercurial/parsers.c
>> > @@ -2700,6 +2700,46 @@ bail:
>> >     return NULL;
>> >  }
>> >
>> > +static PyObject *addsuccessors(PyObject *self, PyObject *args) {
>> > +	Py_ssize_t i, len;
>> > +	PyObject *succs = NULL, *marks = NULL;
>> > +
>> > +	if (!PyArg_ParseTuple(args, "O!O!", &PyDict_Type, &succs,
>> > +                         &PyList_Type, &marks)) {
>>
>> Minor nit: if my calculations are correct, this line has one too many
>> tabs.
>
> My emacs with vaguely-kernel-ish settings produced this. My personal
> opinion is that this line should have exactly one tab and then spaces,
> but I'm in the minority here.

I would champion this style as well.
Yuya Nishihara - Aug. 25, 2015, 2:10 p.m.
On Mon, 24 Aug 2015 14:25:55 -0400, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1440439282 14400
> #      Mon Aug 24 14:01:22 2015 -0400
> # Node ID 3aecfdbfe3ca54134cdb0e98944e8fca53bd46f1
> # Parent  207e95720308518aa40c2a91ce20fdf52dfa8550
> obsolete: add C implementation of _addsuccessors

> +static PyObject *addsuccessors(PyObject *self, PyObject *args) {
> +	Py_ssize_t i, len;
> +	PyObject *succs = NULL, *marks = NULL;
> +
> +	if (!PyArg_ParseTuple(args, "O!O!", &PyDict_Type, &succs,
> +			      &PyList_Type, &marks)) {
> +		return NULL;
> +	}
> +	len = PyList_Size(marks);
> +	for (i = 0; i < len; i++) {
> +		PyObject *mark, *key, *dest;
> +		mark = PyList_GetItem(marks, i);
> +		if (!mark) {
> +			return NULL;
> +		}
> +		key = PyTuple_GetItem(mark, 0);
> +		if (!key) {
> +			return NULL;
> +		}
> +		if (PyDict_Contains(succs, key)) {
> +			dest = PyDict_GetItem(succs, key);

The doc says PyDict_Contains() may return -1 on error, though I'm not sure if
it can really happen.

> +		} else {
> +			int r;
> +			dest = PySet_New(NULL);
> +			if (!dest)
> +				return NULL;
> +			r = PyDict_SetItem(succs, key, dest);
> +			Py_DECREF(dest);
> +			if (r)
> +				return NULL;
> +		}
> +		if (!dest)
> +			return NULL;
> +		if (PySet_Add(dest, mark))
> +			return NULL;
> +	}
> +	Py_INCREF(Py_None);
> +	return Py_None;

There's a macro, Py_RETURN_NONE.
Martin von Zweigbergk - Aug. 25, 2015, 4:55 p.m.
When I sent the same patches on Feb 3, we decided the speedup wasn't worth
it then and deferred the decision. Perhaps we do want this now (it perhaps
your version is faster), but we should at least have a pointer to that
discussion from here:
http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/76081/focus=76079

On Mon, Aug 24, 2015 at 11:26 AM Augie Fackler <raf@durin42.com> wrote:

> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1440439282 14400
> #      Mon Aug 24 14:01:22 2015 -0400
> # Node ID 3aecfdbfe3ca54134cdb0e98944e8fca53bd46f1
> # Parent  207e95720308518aa40c2a91ce20fdf52dfa8550
> obsolete: add C implementation of _addsuccessors
>
> Before, best of 10 runs:
> hg sl > /dev/null  0.75s user 0.07s system 99% cpu 0.828 total
>
> After, best of 10 runs:
> ./hg sl > /dev/null  0.65s user 0.09s system 99% cpu 0.744 total
>
> where
> alias.sl=log -Gr smart -Tsl
> revsetalias.smart=(parents(not public()) or not public() or . or head())
> and (not obsolete() or unstable()^)
>
> It's a straight-line port to C code, but still a 10% win on walltime
> for the command. I can live with that.
>
> diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
> --- a/mercurial/obsolete.py
> +++ b/mercurial/obsolete.py
> @@ -470,10 +470,16 @@ class marker(object):
>          return self._data[2]
>
>  @util.nogc
> -def _addsuccessors(successors, markers):
> +def _addsuccessors_pure(successors, markers):
>      for mark in markers:
>          successors.setdefault(mark[0], set()).add(mark)
>
> +def _addsuccessors(successors, markers):
> +    global _addsuccessors
> +    fn = getattr(parsers, 'addsuccessors', _addsuccessors_pure)
> +    _addsuccessors = fn
> +    return fn(successors, markers)
> +
>  @util.nogc
>  def _addprecursors(precursors, markers):
>      for mark in markers:
> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
> --- a/mercurial/parsers.c
> +++ b/mercurial/parsers.c
> @@ -2700,6 +2700,46 @@ bail:
>         return NULL;
>  }
>
> +static PyObject *addsuccessors(PyObject *self, PyObject *args) {
> +       Py_ssize_t i, len;
> +       PyObject *succs = NULL, *marks = NULL;
> +
> +       if (!PyArg_ParseTuple(args, "O!O!", &PyDict_Type, &succs,
> +                             &PyList_Type, &marks)) {
> +               return NULL;
> +       }
> +       len = PyList_Size(marks);
> +       for (i = 0; i < len; i++) {
> +               PyObject *mark, *key, *dest;
> +               mark = PyList_GetItem(marks, i);
> +               if (!mark) {
> +                       return NULL;
> +               }
> +               key = PyTuple_GetItem(mark, 0);
> +               if (!key) {
> +                       return NULL;
> +               }
> +               if (PyDict_Contains(succs, key)) {
> +                       dest = PyDict_GetItem(succs, key);
> +               } else {
> +                       int r;
> +                       dest = PySet_New(NULL);
> +                       if (!dest)
> +                               return NULL;
> +                       r = PyDict_SetItem(succs, key, dest);
> +                       Py_DECREF(dest);
> +                       if (r)
> +                               return NULL;
> +               }
> +               if (!dest)
> +                       return NULL;
> +               if (PySet_Add(dest, mark))
> +                       return NULL;
> +       }
> +       Py_INCREF(Py_None);
> +       return Py_None;
> +}
> +
>  static char parsers_doc[] = "Efficient content parsing.";
>
>  PyObject *encodedir(PyObject *self, PyObject *args);
> @@ -2722,6 +2762,8 @@ static PyMethodDef methods[] = {
>         {"lowerencode", lowerencode, METH_VARARGS, "lower-encode a
> path\n"},
>         {"fm1readmarkers", fm1readmarkers, METH_VARARGS,
>                         "parse v1 obsolete markers\n"},
> +       {"addsuccessors", addsuccessors, METH_VARARGS,
> +        "add successors to a dict\n"},
>         {NULL, NULL}
>  };
>
> diff --git a/mercurial/revset.py b/mercurial/revset.py
> --- a/mercurial/revset.py
> +++ b/mercurial/revset.py
> @@ -2945,6 +2945,7 @@ class abstractsmartset(object):
>
>          This is part of the mandatory API for smartset."""
>          c = other.__contains__
> +        # this is slow?
>          return self.filter(lambda r: not c(r), cache=False)
>
>      def filter(self, condition, cache=True):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>
Matt Mackall - Aug. 25, 2015, 8:52 p.m.
On Mon, 2015-08-24 at 14:25 -0400, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1440439282 14400
> #      Mon Aug 24 14:01:22 2015 -0400
> # Node ID 3aecfdbfe3ca54134cdb0e98944e8fca53bd46f1
> # Parent  207e95720308518aa40c2a91ce20fdf52dfa8550
> obsolete: add C implementation of _addsuccessors
> 
> Before, best of 10 runs:
> hg sl > /dev/null  0.75s user 0.07s system 99% cpu 0.828 total
> 
> After, best of 10 runs:
> ./hg sl > /dev/null  0.65s user 0.09s system 99% cpu 0.744 total
> 
> where
> alias.sl=log -Gr smart -Tsl
> revsetalias.smart=(parents(not public()) or not public() or . or head()) and (not obsolete() or unstable()^)
> 
> It's a straight-line port to C code, but still a 10% win on walltime
> for the command. I can live with that.

I'm kind of loathe to accept C code that isn't at least 4x or so faster
than what it's replacing, but this 10% doesn't measure that directly.
Can I get you to tweak perf.py to do that?

Patch

diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
--- a/mercurial/obsolete.py
+++ b/mercurial/obsolete.py
@@ -470,10 +470,16 @@  class marker(object):
         return self._data[2]
 
 @util.nogc
-def _addsuccessors(successors, markers):
+def _addsuccessors_pure(successors, markers):
     for mark in markers:
         successors.setdefault(mark[0], set()).add(mark)
 
+def _addsuccessors(successors, markers):
+    global _addsuccessors
+    fn = getattr(parsers, 'addsuccessors', _addsuccessors_pure)
+    _addsuccessors = fn
+    return fn(successors, markers)
+
 @util.nogc
 def _addprecursors(precursors, markers):
     for mark in markers:
diff --git a/mercurial/parsers.c b/mercurial/parsers.c
--- a/mercurial/parsers.c
+++ b/mercurial/parsers.c
@@ -2700,6 +2700,46 @@  bail:
 	return NULL;
 }
 
+static PyObject *addsuccessors(PyObject *self, PyObject *args) {
+	Py_ssize_t i, len;
+	PyObject *succs = NULL, *marks = NULL;
+
+	if (!PyArg_ParseTuple(args, "O!O!", &PyDict_Type, &succs,
+			      &PyList_Type, &marks)) {
+		return NULL;
+	}
+	len = PyList_Size(marks);
+	for (i = 0; i < len; i++) {
+		PyObject *mark, *key, *dest;
+		mark = PyList_GetItem(marks, i);
+		if (!mark) {
+			return NULL;
+		}
+		key = PyTuple_GetItem(mark, 0);
+		if (!key) {
+			return NULL;
+		}
+		if (PyDict_Contains(succs, key)) {
+			dest = PyDict_GetItem(succs, key);
+		} else {
+			int r;
+			dest = PySet_New(NULL);
+			if (!dest)
+				return NULL;
+			r = PyDict_SetItem(succs, key, dest);
+			Py_DECREF(dest);
+			if (r)
+				return NULL;
+		}
+		if (!dest)
+			return NULL;
+		if (PySet_Add(dest, mark))
+			return NULL;
+	}
+	Py_INCREF(Py_None);
+	return Py_None;
+}
+
 static char parsers_doc[] = "Efficient content parsing.";
 
 PyObject *encodedir(PyObject *self, PyObject *args);
@@ -2722,6 +2762,8 @@  static PyMethodDef methods[] = {
 	{"lowerencode", lowerencode, METH_VARARGS, "lower-encode a path\n"},
 	{"fm1readmarkers", fm1readmarkers, METH_VARARGS,
 			"parse v1 obsolete markers\n"},
+	{"addsuccessors", addsuccessors, METH_VARARGS,
+	 "add successors to a dict\n"},
 	{NULL, NULL}
 };
 
diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -2945,6 +2945,7 @@  class abstractsmartset(object):
 
         This is part of the mandatory API for smartset."""
         c = other.__contains__
+        # this is slow?
         return self.filter(lambda r: not c(r), cache=False)
 
     def filter(self, condition, cache=True):