Patchwork [7,of,7] reachableroots: return list of revisions instead of set

login
register
mail settings
Submitter Yuya Nishihara
Date Aug. 18, 2015, 2:42 p.m.
Message ID <bd9eecaa3c297c59cf0e.1439908979@mimosa>
Download mbox | patch
Permalink /patch/10229/
State Accepted
Delegated to: Augie Fackler
Headers show

Comments

Yuya Nishihara - Aug. 18, 2015, 2:42 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1439535139 -32400
#      Fri Aug 14 15:52:19 2015 +0900
# Node ID bd9eecaa3c297c59cf0e558672b2564ef8268d02
# Parent  9a0ff0541db15438c019d5a3db6184c84c6b4c25
reachableroots: return list of revisions instead of set

Now we don't need a set of reachable revisions, and the caller wants a sorted
list of revisions, so constructing a set is just a waste of time.

  revset #0: 0::tip
  2) 0.002536
  3) 0.001598  63%

PyList_New() should set an appropriate exception on error, so we don't need
to call PyErr_NoMemory() manually.

This patch lacks error handling of PyList_Append() as it was before for
PySet_Add(). It should be fixed later.
Sean Farley - Aug. 18, 2015, 5:32 p.m.
Yuya Nishihara <yuya@tcha.org> writes:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1439535139 -32400
> #      Fri Aug 14 15:52:19 2015 +0900
> # Node ID bd9eecaa3c297c59cf0e558672b2564ef8268d02
> # Parent  9a0ff0541db15438c019d5a3db6184c84c6b4c25
> reachableroots: return list of revisions instead of set
>
> Now we don't need a set of reachable revisions, and the caller wants a sorted
> list of revisions, so constructing a set is just a waste of time.
>
>   revset #0: 0::tip
>   2) 0.002536
>   3) 0.001598  63%
>
> PyList_New() should set an appropriate exception on error, so we don't need
> to call PyErr_NoMemory() manually.
>
> This patch lacks error handling of PyList_Append() as it was before for
> PySet_Add(). It should be fixed later.

This series looks fine to me but I don't know about the question of
renaming the function due to its signature changing.
Pierre-Yves David - Aug. 18, 2015, 7:39 p.m.
On 08/18/2015 07:42 AM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1439535139 -32400
> #      Fri Aug 14 15:52:19 2015 +0900
> # Node ID bd9eecaa3c297c59cf0e558672b2564ef8268d02
> # Parent  9a0ff0541db15438c019d5a3db6184c84c6b4c25
> reachableroots: return list of revisions instead of set
>
> Now we don't need a set of reachable revisions, and the caller wants a sorted
> list of revisions, so constructing a set is just a waste of time.
>
>    revset #0: 0::tip
>    2) 0.002536
>    3) 0.001598  63%
>
> PyList_New() should set an appropriate exception on error, so we don't need
> to call PyErr_NoMemory() manually.
>
> This patch lacks error handling of PyList_Append() as it was before for
> PySet_Add(). It should be fixed later.
>
> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
> --- a/mercurial/parsers.c
> +++ b/mercurial/parsers.c
> @@ -1145,11 +1145,9 @@ static PyObject *reachableroots(indexObj
>   		includepath = 1;
>
>   	/* Initialize return set */
> -	reachable = PySet_New(NULL);
> -	if (reachable == NULL) {
> -		PyErr_NoMemory();
> +	reachable = PyList_New(0);

If we know the size of the list (something I feel like we could know) 
before having to fill it. It might be more efficient to create the list 
at the right size in the first place.

Patch

diff --git a/mercurial/parsers.c b/mercurial/parsers.c
--- a/mercurial/parsers.c
+++ b/mercurial/parsers.c
@@ -1145,11 +1145,9 @@  static PyObject *reachableroots(indexObj
 		includepath = 1;
 
 	/* Initialize return set */
-	reachable = PySet_New(NULL);
-	if (reachable == NULL) {
-		PyErr_NoMemory();
+	reachable = PyList_New(0);
+	if (reachable == NULL)
 		goto bail;
-	}
 
 	/* Initialize internal datastructures */
 	tovisit = (int *)malloc((len + 1) * sizeof(int));
@@ -1202,7 +1200,7 @@  static PyObject *reachableroots(indexObj
 			val = PyInt_FromLong(revnum);
 			if (val == NULL)
 				goto bail;
-			PySet_Add(reachable, val);
+			PyList_Append(reachable, val);
 			Py_DECREF(val);
 			if (includepath == 0)
 				continue;
@@ -1238,12 +1236,13 @@  static PyObject *reachableroots(indexObj
 			if (r < 0)
 				goto bail;
 			for (k = 0; k < 2; k++) {
-				if (revstates[parents[k] + 1] & RS_REACHABLE) {
+				if ((revstates[parents[k] + 1] & RS_REACHABLE)
+				    && !(revstates[i + 1] & RS_REACHABLE)) {
 					revstates[i + 1] |= RS_REACHABLE;
 					val = PyInt_FromLong(i);
 					if (val == NULL)
 						goto bail;
-					PySet_Add(reachable, val);
+					PyList_Append(reachable, val);
 					Py_DECREF(val);
 				}
 			}