[PATCH 6 of 7 V4] delta: have a native implementation of _findsnapshot

[Yuya Nishihara]

On Sun, 30 Dec 2018 19:43:53 +0100, Boris Feld wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld at octobus.net>
> # Date 1545297320 -3600
> #      Thu Dec 20 10:15:20 2018 +0100
> # Node ID a6bb015a9fb1a2c23d73f1c55e7d4160a9701191
> # Parent  5aff42c50dd1bc1e7c2e533a8e052fa42c6f9304
> # EXP-Topic sparse-revlog
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r a6bb015a9fb1
> delta: have a native implementation of _findsnapshot
> 
> The function might traverse a lot of revision, a native implementation get
> significantly faster.
> 
> example affected manifest write
> before: 0.114989
> after:  0.067141 (-42%)
> 
> diff --git a/mercurial/cext/revlog.c b/mercurial/cext/revlog.c
> --- a/mercurial/cext/revlog.c
> +++ b/mercurial/cext/revlog.c
> @@ -1050,6 +1050,80 @@ static PyObject *index_issnapshot(indexO
>  	return PyBool_FromLong((long)issnap);
>  }
>  
> +static PyObject *index_findsnapshots(indexObject *self, PyObject *args)
> +{
> +
> +	Py_ssize_t start_rev;
> +	PyObject *cache;
> +	Py_ssize_t base;
> +	Py_ssize_t rev;
> +	int issnap;

> +	PyObject *key;
> +	bool newallvalues;
> +	PyObject *allvalues;
> +	PyObject *value;

These variables have to be initialized since we do XDECREF() on failure.

> +	if (!PyArg_ParseTuple(args, "On", &cache, &start_rev)) {
> +		return NULL;
> +	}
> +	if (!PyDict_Check(cache)) {
> +		PyErr_SetString(PyExc_TypeError,
> +		                "cache argument must be a dict");
> +		return NULL;
> +	}

Nit: maybe this can be "O!" with &PyDict_Type.

> +	for (rev = start_rev; rev < length; rev++) {
> +		issnap = index_issnapshotrev(self, rev);
> +		if (issnap < 0) {
> +			goto bail;
> +		}
> +		if (issnap == 0) {
> +			continue;
> +		}
> +		base = (Py_ssize_t)index_baserev(self, rev);
> +		if (base == rev) {
> +			base = -1;
> +		}
> +		if (base == -2) {
> +			assert(PyErr_Occurred());
> +			goto bail;
> +		}
> +		key = PyInt_FromSsize_t(base);
> +		newallvalues = false;
> +		allvalues = PyDict_GetItem(cache, key);
> +		if (allvalues == NULL && PyErr_Occurred()) {
> +			goto bail;
> +		}
> +		if (allvalues == NULL) {
> +			allvalues = PyList_New(0);

PyList_New() may fail in theory.

> +			newallvalues = true;
> +			if (PyDict_SetItem(cache, key, allvalues) < 0) {
> +				goto bail;
> +			}

I prefer cleaning up the allvalues reference here for this particular case,
so that the scope of the allvalues can be narrowed and the newallvalues can
be eliminated.

  for (rev ...) {
      PyObject *allvalues; /* borrowed */
      ...
      if (allvalues == NULL) {
          int r;
          allvalues = PyList_New(0);
          if (!allvalues)
              goto bail;
          r = PyDict_SetItem(cache, key, allvalues);
          Py_DECREF(allvalues);
          if (r < 0)
              goto bail;
      }

> +		Py_XDECREF(key);
> +		if (newallvalues) {
> +			Py_XDECREF(allvalues);
> +		}
> +		Py_XDECREF(value);
> +		key = NULL;
> +		allvalues = NULL;
> +		value = NULL;

Nit: Py_CLEAR() could be used in place of XDECREF() + = NULL.

> +	}
> +	Py_INCREF(Py_None);
> +	return Py_None;

Nit: can be Py_RETURN_NONE;