Patchwork [2,of,2] reachableroots: unroll loop that checks if one of parents is reachable

login
register
mail settings
Submitter Yuya Nishihara
Date Aug. 22, 2015, 12:53 p.m.
Message ID <7737721288cbade4ab72.1440248027@mimosa>
Download mbox | patch
Permalink /patch/10252/
State Accepted
Headers show

Comments

Yuya Nishihara - Aug. 22, 2015, 12:53 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1439685037 -32400
#      Sun Aug 16 09:30:37 2015 +0900
# Node ID 7737721288cbade4ab725ac87d8a57d2a99fa649
# Parent  32fd293a3f4709841d1e555b9fe3040c086281a3
reachableroots: unroll loop that checks if one of parents is reachable

The difference is small, but fewer loops should be better in general:

  revset #0: 0::tip
  0) 0.001609
  1) 0.001510  93%
Sean Farley - Aug. 22, 2015, 10:25 p.m.
Yuya Nishihara <yuya@tcha.org> writes:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1439685037 -32400
> #      Sun Aug 16 09:30:37 2015 +0900
> # Node ID 7737721288cbade4ab725ac87d8a57d2a99fa649
> # Parent  32fd293a3f4709841d1e555b9fe3040c086281a3
> reachableroots: unroll loop that checks if one of parents is reachable
>
> The difference is small, but fewer loops should be better in general:
>
>   revset #0: 0::tip
>   0) 0.001609
>   1) 0.001510  93%

Series looks good to me.
Augie Fackler - Aug. 24, 2015, 1:40 p.m.
On Sat, Aug 22, 2015 at 03:25:24PM -0700, Sean Farley wrote:
>
> Yuya Nishihara <yuya@tcha.org> writes:
>
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1439685037 -32400
> > #      Sun Aug 16 09:30:37 2015 +0900
> > # Node ID 7737721288cbade4ab725ac87d8a57d2a99fa649
> > # Parent  32fd293a3f4709841d1e555b9fe3040c086281a3
> > reachableroots: unroll loop that checks if one of parents is reachable
> >
> > The difference is small, but fewer loops should be better in general:
> >
> >   revset #0: 0::tip
> >   0) 0.001609
> >   1) 0.001510  93%
>
> Series looks good to me.

Agreed, queued. Thanks!

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Sept. 8, 2015, 8:45 p.m.
On 08/22/2015 05:53 AM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1439685037 -32400
> #      Sun Aug 16 09:30:37 2015 +0900
> # Node ID 7737721288cbade4ab725ac87d8a57d2a99fa649
> # Parent  32fd293a3f4709841d1e555b9fe3040c086281a3
> reachableroots: unroll loop that checks if one of parents is reachable
>
> The difference is small, but fewer loops should be better in general:
>
>    revset #0: 0::tip
>    0) 0.001609
>    1) 0.001510  93%

I'm kinda surprise the compiler is not doing this for us.
Yuya Nishihara - Sept. 9, 2015, 1:26 a.m.
On Tue, 08 Sep 2015 13:45:19 -0700, Pierre-Yves David wrote:
> > The difference is small, but fewer loops should be better in general:
> >
> >    revset #0: 0::tip
> >    0) 0.001609
> >    1) 0.001510  93%
> 
> I'm kinda surprise the compiler is not doing this for us.

Perhaps it isn't easy to detect that
  revstates[i + 1] is RS_REACHABLE at k == 1
  if revstates[parents[k] + 1] is RS_REACHABLE at k == 0.

gcc 5.2.1 could generate optimized code if we had "break".

> for (k = 0; k < 2; k++) {
> -				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;
> -					r = PyList_Append(reachable, val);
> -					Py_DECREF(val);
> -					if (r < 0)
> -						goto bail;
					break;  /* revision i is reachable */
> -				}

Patch

diff --git a/mercurial/parsers.c b/mercurial/parsers.c
--- a/mercurial/parsers.c
+++ b/mercurial/parsers.c
@@ -1240,18 +1240,17 @@  static PyObject *reachableroots2(indexOb
 			 * index_get_parents */
 			if (r < 0)
 				goto bail;
-			for (k = 0; k < 2; k++) {
-				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;
-					r = PyList_Append(reachable, val);
-					Py_DECREF(val);
-					if (r < 0)
-						goto bail;
-				}
+			if (((revstates[parents[0] + 1] |
+			      revstates[parents[1] + 1]) & RS_REACHABLE)
+			    && !(revstates[i + 1] & RS_REACHABLE)) {
+				revstates[i + 1] |= RS_REACHABLE;
+				val = PyInt_FromLong(i);
+				if (val == NULL)
+					goto bail;
+				r = PyList_Append(reachable, val);
+				Py_DECREF(val);
+				if (r < 0)
+					goto bail;
 			}
 		}
 	}