Patchwork [6,of,7,RFC] parsers: inline fm1readmarker() into Obsmarker1_FromData()

login
register
mail settings
Submitter Gregory Szorc
Date March 14, 2017, 5:15 a.m.
Message ID <beda0762692f4252b049.1489468548@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/19322/
State Accepted
Headers show

Comments

Gregory Szorc - March 14, 2017, 5:15 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1489467613 25200
#      Mon Mar 13 22:00:13 2017 -0700
# Node ID beda0762692f4252b049472b7d653abc5e809151
# Parent  b4159d4c3683f03b6962991f318b080d849d6ba3
[RFC] parsers: inline fm1readmarker() into Obsmarker1_FromData()

As promised by the previous commit.

Some moderate refactoring was performed to eliminate the various
PyObject variables and to assign directly into the returned
value.

We see another speedup on `hg perfloadmarkers`:

! result: 130341
! wall 0.085715 comb 0.080000 user 0.060000 sys 0.020000 (best of 100
! wall 0.077677 comb 0.080000 user 0.060000 sys 0.020000 (best of 100)

That is likely due eliminating the temporary tuple instance.

Patch

diff --git a/mercurial/parsers.c b/mercurial/parsers.c
--- a/mercurial/parsers.c
+++ b/mercurial/parsers.c
@@ -2698,24 +2698,27 @@  static void obsmarker1_dealloc(obsmarker
 	PyObject_Del(self);
 }
 
-static PyObject *fm1readmarker(const char *databegin, const char *dataend,
-			       uint32_t *msize);
-
 /**
  * Construct an obsmarker1Object from raw data.
  */
 static obsmarker1Object * Obsmarker1_FromData(const char *databegin,
 					      const char *dataend,
-					      uint32_t *msize) {
-
-	PyObject *tuple;
+					      uint32_t *msize)
+{
 	obsmarker1Object *result = NULL;
-
-	/* For now, we call the existing function to parse to a tuple
-	   and convert to our new type. */
-	tuple = fm1readmarker(databegin, dataend, msize);
-	if (tuple == NULL) {
-		return NULL;
+	const char *data = databegin;
+	const char *meta;
+
+	double mtime;
+	int16_t tz;
+	uint16_t flags;
+	unsigned char nsuccs, nparents, nmetadata;
+	Py_ssize_t hashwidth = 20;
+	PyObject *mtimeo, *tzo;
+	int i;
+
+	if (data + FM1_HEADER_SIZE > dataend) {
+		goto overflow;
 	}
 
 	result = (obsmarker1Object *)PyObject_CallObject(
@@ -2724,24 +2727,126 @@  static obsmarker1Object * Obsmarker1_Fro
 		goto bail;
 	}
 
-	result->precursors = PyTuple_GET_ITEM(tuple, 0);
-	Py_INCREF(result->precursors);
-	result->successors = PyTuple_GET_ITEM(tuple, 1);
-	Py_INCREF(result->successors);
-	result->flags = PyTuple_GET_ITEM(tuple, 2);
-	Py_INCREF(result->flags);
-	result->metadata = PyTuple_GET_ITEM(tuple, 3);
-	Py_INCREF(result->metadata);
-	result->time = PyTuple_GET_ITEM(tuple, 4);
-	Py_INCREF(result->time);
-	result->parents = PyTuple_GET_ITEM(tuple, 5);
-	Py_INCREF(result->parents);
-
-	Py_CLEAR(tuple);
-
+	*msize = getbe32(data);
+	data += 4;
+	mtime = getbefloat64(data);
+	data += 8;
+	tz = getbeint16(data);
+	data += 2;
+	flags = getbeuint16(data);
+	data += 2;
+
+	result->flags = PyLong_FromUnsignedLong(flags);
+	if (result->flags == NULL) {
+		Py_CLEAR(result);
+		goto bail;
+	}
+
+	result->time = PyTuple_New(2);
+	if (result->time == NULL) {
+		Py_CLEAR(result);
+		goto bail;
+	}
+
+	mtimeo = PyFloat_FromDouble(mtime);
+	tzo = PyLong_FromUnsignedLong(tz * 60);
+	if (!mtimeo || !tzo) {
+		Py_XDECREF(mtimeo);
+		Py_XDECREF(tzo);
+		Py_CLEAR(result);
+		goto bail;
+	}
+
+	PyTuple_SET_ITEM(result->time, 0, mtimeo);
+	PyTuple_SET_ITEM(result->time, 1, tzo);
+
+	if (flags & USING_SHA_256) {
+		hashwidth = 32;
+	}
+
+	nsuccs = (unsigned char)(*data++);
+	nparents = (unsigned char)(*data++);
+	nmetadata = (unsigned char)(*data++);
+
+	if (databegin + *msize > dataend) {
+		goto overflow;
+	}
+	dataend = databegin + *msize;  /* narrow down to marker size */
+
+	if (data + hashwidth > dataend) {
+		goto overflow;
+	}
+
+	result->precursors = PyBytes_FromStringAndSize(data, hashwidth);
+	data += hashwidth;
+	if (result->precursors == NULL) {
+		Py_CLEAR(result);
+		goto bail;
+	}
+
+	if (data + nsuccs * hashwidth > dataend) {
+		goto overflow;
+	}
+	result->successors = readshas(data, nsuccs, hashwidth);
+	if (result->successors == NULL) {
+		Py_CLEAR(result);
+		goto bail;
+	}
+	data += nsuccs * hashwidth;
+
+	if (nparents == 1 || nparents == 2) {
+		if (data + nparents * hashwidth > dataend) {
+			goto overflow;
+		}
+		result->parents = readshas(data, nparents, hashwidth);
+		if (result->parents == NULL) {
+			Py_CLEAR(result);
+			goto bail;
+		}
+		data += nparents * hashwidth;
+	} else {
+		result->parents = Py_None;
+		Py_INCREF(Py_None);
+	}
+
+	if (data + 2 * nmetadata > dataend) {
+		goto overflow;
+	}
+	meta = data + (2 * nmetadata);
+	result->metadata = PyTuple_New(nmetadata);
+	if (result->metadata == NULL) {
+		Py_CLEAR(result);
+		goto bail;
+	}
+	for (i = 0; i < nmetadata; i++) {
+		PyObject *tmp, *left = NULL, *right = NULL;
+		Py_ssize_t leftsize = (unsigned char)(*data++);
+		Py_ssize_t rightsize = (unsigned char)(*data++);
+		if (meta + leftsize + rightsize > dataend) {
+			goto overflow;
+		}
+		left = PyBytes_FromStringAndSize(meta, leftsize);
+		meta += leftsize;
+		right = PyBytes_FromStringAndSize(meta, rightsize);
+		meta += rightsize;
+		tmp = PyTuple_New(2);
+		if (!left || !right || !tmp) {
+			Py_XDECREF(left);
+			Py_XDECREF(right);
+			Py_XDECREF(tmp);
+			goto bail;
+		}
+		PyTuple_SET_ITEM(tmp, 0, left);
+		PyTuple_SET_ITEM(tmp, 1, right);
+		PyTuple_SET_ITEM(result->metadata, i, tmp);
+	}
+
+	goto bail;
+
+overflow:
+	Py_CLEAR(result);
+	PyErr_SetString(PyExc_ValueError, "overflow in obsstore");
 bail:
-	Py_XDECREF(tuple);
-
 	return result;
 }
 
@@ -2854,125 +2959,6 @@  PyTypeObject obsmarker1Type = {
 	PyType_GenericNew, /* tp_new */
 };
 
-static PyObject *fm1readmarker(const char *databegin, const char *dataend,
-			       uint32_t *msize)
-{
-	const char *data = databegin;
-	const char *meta;
-
-	double mtime;
-	int16_t tz;
-	uint16_t flags;
-	unsigned char nsuccs, nparents, nmetadata;
-	Py_ssize_t hashwidth = 20;
-
-	PyObject *prec = NULL, *parents = NULL, *succs = NULL;
-	PyObject *metadata = NULL, *ret = NULL;
-	int i;
-
-	if (data + FM1_HEADER_SIZE > dataend) {
-		goto overflow;
-	}
-
-	*msize = getbe32(data);
-	data += 4;
-	mtime = getbefloat64(data);
-	data += 8;
-	tz = getbeint16(data);
-	data += 2;
-	flags = getbeuint16(data);
-	data += 2;
-
-	if (flags & USING_SHA_256) {
-		hashwidth = 32;
-	}
-
-	nsuccs = (unsigned char)(*data++);
-	nparents = (unsigned char)(*data++);
-	nmetadata = (unsigned char)(*data++);
-
-	if (databegin + *msize > dataend) {
-		goto overflow;
-	}
-	dataend = databegin + *msize;  /* narrow down to marker size */
-
-	if (data + hashwidth > dataend) {
-		goto overflow;
-	}
-	prec = PyBytes_FromStringAndSize(data, hashwidth);
-	data += hashwidth;
-	if (prec == NULL) {
-		goto bail;
-	}
-
-	if (data + nsuccs * hashwidth > dataend) {
-		goto overflow;
-	}
-	succs = readshas(data, nsuccs, hashwidth);
-	if (succs == NULL) {
-		goto bail;
-	}
-	data += nsuccs * hashwidth;
-
-	if (nparents == 1 || nparents == 2) {
-		if (data + nparents * hashwidth > dataend) {
-			goto overflow;
-		}
-		parents = readshas(data, nparents, hashwidth);
-		if (parents == NULL) {
-			goto bail;
-		}
-		data += nparents * hashwidth;
-	} else {
-		parents = Py_None;
-		Py_INCREF(parents);
-	}
-
-	if (data + 2 * nmetadata > dataend) {
-		goto overflow;
-	}
-	meta = data + (2 * nmetadata);
-	metadata = PyTuple_New(nmetadata);
-	if (metadata == NULL) {
-		goto bail;
-	}
-	for (i = 0; i < nmetadata; i++) {
-		PyObject *tmp, *left = NULL, *right = NULL;
-		Py_ssize_t leftsize = (unsigned char)(*data++);
-		Py_ssize_t rightsize = (unsigned char)(*data++);
-		if (meta + leftsize + rightsize > dataend) {
-			goto overflow;
-		}
-		left = PyBytes_FromStringAndSize(meta, leftsize);
-		meta += leftsize;
-		right = PyBytes_FromStringAndSize(meta, rightsize);
-		meta += rightsize;
-		tmp = PyTuple_New(2);
-		if (!left || !right || !tmp) {
-			Py_XDECREF(left);
-			Py_XDECREF(right);
-			Py_XDECREF(tmp);
-			goto bail;
-		}
-		PyTuple_SET_ITEM(tmp, 0, left);
-		PyTuple_SET_ITEM(tmp, 1, right);
-		PyTuple_SET_ITEM(metadata, i, tmp);
-	}
-	ret = Py_BuildValue("(OOHO(di)O)", prec, succs, flags,
-			    metadata, mtime, (int)tz * 60, parents);
-	goto bail;  /* return successfully */
-
-overflow:
-	PyErr_SetString(PyExc_ValueError, "overflow in obsstore");
-bail:
-	Py_XDECREF(prec);
-	Py_XDECREF(succs);
-	Py_XDECREF(metadata);
-	Py_XDECREF(parents);
-	return ret;
-}
-
-
 static PyObject *fm1readmarkers(PyObject *self, PyObject *args) {
 	const char *data, *dataend;
 	int datalen;