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

Augie Fackler raf at durin42.com
Mon Aug 24 13:52:01 CDT 2015


On Mon, Aug 24, 2015 at 11:46:16AM -0700, Sean Farley wrote:
>
> Augie Fackler <raf at durin42.com> writes:
>
> > # HG changeset patch
> > # User Augie Fackler <augie at 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 at selenic.com
> > https://selenic.com/mailman/listinfo/mercurial-devel
>


More information about the Mercurial-devel mailing list