Patchwork [1,of,2] reachableroots: bail if integer object cannot be allocated

login
register
mail settings
Submitter Yuya Nishihara
Date Aug. 14, 2015, 10:25 a.m.
Message ID <949f27368952a03365a8.1439547932@mimosa>
Download mbox | patch
Permalink /patch/10211/
State Accepted
Commit a3d5da8b641ee0a6336d708d839d3dad809fc86f
Headers show

Comments

Yuya Nishihara - Aug. 14, 2015, 10:25 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1439523116 -32400
#      Fri Aug 14 12:31:56 2015 +0900
# Node ID 949f27368952a03365a84ec81d1a6aefe4e8319a
# Parent  0b57b77f9b3ed5c1cc24ce95173c4f7e824df13f
reachableroots: bail if integer object cannot be allocated

This patch also replaces Py_XDECREF() by Py_DECREF() because we known "val"
and "p" are not NULL.

BTW, we can eliminate some of these allocation and error handling of int objects
if the internal "seen" array has more information. For example,

  enum { SEEN = 1, ROOT = 2, REACHABLE = 4 };
  /* ... build ROOT mask from roots argument ... */
  if (seen[revnum + 1] & ROOT) {  /* instead of PySet_Contains(roots, val) */

From my quick hack, it is 2x faster.
Matt Mackall - Aug. 14, 2015, 8:29 p.m.
On Fri, 2015-08-14 at 19:25 +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1439523116 -32400
> #      Fri Aug 14 12:31:56 2015 +0900
> # Node ID 949f27368952a03365a84ec81d1a6aefe4e8319a
> # Parent  0b57b77f9b3ed5c1cc24ce95173c4f7e824df13f
> reachableroots: bail if integer object cannot be allocated
> 
> This patch also replaces Py_XDECREF() by Py_DECREF() because we known "val"
> and "p" are not NULL.
> 
> BTW, we can eliminate some of these allocation and error handling of int objects
> if the internal "seen" array has more information. For example,
> 
>   enum { SEEN = 1, ROOT = 2, REACHABLE = 4 };
>   /* ... build ROOT mask from roots argument ... */
>   if (seen[revnum + 1] & ROOT) {  /* instead of PySet_Contains(roots, val) */
> 
> >From my quick hack, it is 2x faster.

Sounds good.
Pierre-Yves David - Aug. 15, 2015, 5:41 a.m.
On 08/14/2015 03:25 AM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1439523116 -32400
> #      Fri Aug 14 12:31:56 2015 +0900
> # Node ID 949f27368952a03365a84ec81d1a6aefe4e8319a
> # Parent  0b57b77f9b3ed5c1cc24ce95173c4f7e824df13f
> reachableroots: bail if integer object cannot be allocated
>
> This patch also replaces Py_XDECREF() by Py_DECREF() because we known "val"
> and "p" are not NULL.
>
> BTW, we can eliminate some of these allocation and error handling of int objects
> if the internal "seen" array has more information. For example,
>
>    enum { SEEN = 1, ROOT = 2, REACHABLE = 4 };
>    /* ... build ROOT mask from roots argument ... */
>    if (seen[revnum + 1] & ROOT) {  /* instead of PySet_Contains(roots, val) */
>
>  From my quick hack, it is 2x faster.

Same logic probably apply for reachable. If we do not have to check the 
python set for content we can probably be faster.
Yuya Nishihara - Aug. 17, 2015, 2:18 p.m.
On Fri, 14 Aug 2015 22:41:09 -0700, Pierre-Yves David wrote:
> On 08/14/2015 03:25 AM, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1439523116 -32400
> > #      Fri Aug 14 12:31:56 2015 +0900
> > # Node ID 949f27368952a03365a84ec81d1a6aefe4e8319a
> > # Parent  0b57b77f9b3ed5c1cc24ce95173c4f7e824df13f
> > reachableroots: bail if integer object cannot be allocated
> >
> > This patch also replaces Py_XDECREF() by Py_DECREF() because we known "val"
> > and "p" are not NULL.
> >
> > BTW, we can eliminate some of these allocation and error handling of int objects
> > if the internal "seen" array has more information. For example,
> >
> >    enum { SEEN = 1, ROOT = 2, REACHABLE = 4 };
> >    /* ... build ROOT mask from roots argument ... */
> >    if (seen[revnum + 1] & ROOT) {  /* instead of PySet_Contains(roots, val) */
> >
> >  From my quick hack, it is 2x faster.
> 
> Same logic probably apply for reachable. If we do not have to check the 
> python set for content we can probably be faster.

Yes. I'll patchbomb them as two series because the series gets bigger than I
thought. The overall result is

  revset #0: 0::tip
  0) 0.004168
  5) 0.001377

Regards,

Patch

diff --git a/mercurial/parsers.c b/mercurial/parsers.c
--- a/mercurial/parsers.c
+++ b/mercurial/parsers.c
@@ -1185,14 +1185,16 @@  static PyObject *reachableroots(indexObj
 		/* Add the node to reachable if it is a root*/
 		revnum = tovisit[k++];
 		val = PyInt_FromLong(revnum);
+		if (val == NULL)
+			goto bail;
 		if (PySet_Contains(roots, val) == 1) {
 			PySet_Add(reachable, val);
 			if (includepath == 0) {
-				Py_XDECREF(val);
+				Py_DECREF(val);
 				continue;
 			}
 		}
-		Py_XDECREF(val);
+		Py_DECREF(val);
 
 		/* Add its parents to the list of nodes to visit */
 		if (revnum != -1) {
@@ -1223,9 +1225,15 @@  static PyObject *reachableroots(indexObj
 					goto bail;
 				for (k = 0; k < 2; k++) {
 					PyObject *p = PyInt_FromLong(parents[k]);
-					if (PySet_Contains(reachable, p) == 1)
-						PySet_Add(reachable, PyInt_FromLong(i));
-					Py_XDECREF(p);
+					if (p == NULL)
+						goto bail;
+					if (PySet_Contains(reachable, p) == 1) {
+						val = PyInt_FromLong(i);
+						if (val == NULL)
+							goto bail;
+						PySet_Add(reachable, val);
+					}
+					Py_DECREF(p);
 				}
 			}
 		}