419632 - Avoiding weak roots for doubles

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Igor Bukanov]

+++ This bug was initially created as a clone of Bug #400902 +++

The initial versions of the patch from bug 400902 contained en extra optimization to avoid storing the double numbers in weak roots. To minimize the risk of regression that feature was removed.

It would be nice to reconsider it and provide really fast path for allocation of doubles since in most cases we do have a strong root to store the allocated result.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch changes js_NewDoubleGCThing into a function that does not use weak roots:

/*
 * Allocate a new double jsval and store the result in *vp. vp must be a root.
 * The function does not copy the result into any weak root.
 */
extern JSBool
js_NewDoubleGCThing(JSContext *cx, jsdouble d, jsval *vp);
 
The function that allocates weakly rooted doubles is js_NewWeaklyRootedDouble.

With the patch js_NewNumberValue(cx, d, vp) requires that vp must be a root. In few places where it was possible to have unrooted vp, I replaced the calls by JS_NewNumberValue.

I also considered renaming js_NewNumberValue into something that emphasizes the rooting requirement, but I could not find a good name.

Comment 2

[Igor Bukanov]

Attachment: sunspider results (Pentium-M 1.1 GHz, Fedora-8, threadsafe jsshell)

The patch from comment 1 speeds up the benchmark by 1% where the wins comes from numeric-intensive benchmarks which now runs 3-4% faster.

The patch passes shell and mochi tests without regressions.

Comment 3

[Igor Bukanov]

Asking for 1.9 blocking based on benchmark results.

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #1)
> I also considered renaming js_NewNumberValue into something that emphasizes the
> rooting requirement, but I could not find a good name.  

Suggest js_NewNumberInRootedValue, or something akin to differentiate this from the JS_NewNumberValue API, which differs significantly now.

IIRC there's already a bug on file to split types so that jsrval or some such type is used for the rooted jsval out parameters. Could you remind me of it? Thx,

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 308058 [details] [diff] [review]
v1

>+            /*
>+             * Here and in two other cases we use JS_NewNumberValue and
>+             * JS_NewDoubleValue, not js_NewNumberValue and
>+             * js_NewDoubleGCThing, as sp may point to an unrooted location.
>+             */

Suggest:

            /*
             * Use JS_NewNumberValue and JS_NewDoubleValue here and in the
             * next two cases, not js_NewNumberValue and js_NewDoubleGCThing,
             * as sp may point to an unrooted location.
             */

>+/*
>  * Return a pointer to a new GC-allocated and weakly rooted jsdouble number

s/weakly rooted/weakly-rooted/

Suggest comma after "number"

>- * or null when the allocation fails. The caller must initialize the returned

r=me with these and js_NewNumberValue name suggestion if it works for you, thanks.

/be

Comment 6

[Mike Schroepfer]

Got blocking.  cleared to land with appropriate reviews.  

Comment 7

[Igor Bukanov]

(In reply to comment #4)
> IIRC there's already a bug on file to split types so that jsrval or some such
> type is used for the rooted jsval out parameters. Could you remind me of it?

There is no such bug.

Comment 8

[Igor Bukanov]

Attachment: v2

The new version, besides addressing the nits and using the js_New(Number|Double)InRooted names for number allocators, contains the following changes:

1. The Number constructed is fixed to always pass rooted value to js_NewNumberInRooted.

2. js_ValueToNumber is changed to set *vp to JSVAL_TRUE when the resulting double is not represented by vp. This way the caller of the function can avoid allocating new jsdoubles when it needs to store the resulting number as jsval. 

3. The code added missing SAVE_SP_AND_PC to jsinterp.c to couple recently introduced code fragments. At this point I really hope that the bug 421274 would lead to elimination of SAVE_SP_AND_PC without hurting the performance.

Comment 9

[Igor Bukanov]

Attachment: v1 - v2 delta

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 308144 [details] [diff] [review]
v2

This patch is just a diff error from CVS. :)

Comment 11

[Igor Bukanov]

Attachment: v2 for real

I should remember to check if not the patch itself but at least the size of the resulting attachment.

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 308147 [details] [diff] [review]
v2 for real

>+            /*
>+             * Use JS_NewNumberValue and JS_NewDoubleValue here and in the
>+             * next two cases, not js_NewNumberInRootedValue and
>+             * js_NewDoubleInRootedValue, as sp may point to an unrooted
>+             * location.

Tempting to use csh-style meta-braces:

            /*
             * Use JS_NewNumberValue and JS_NewDoubleValue here and in the
             * next two cases, not js_New{Double,Number}InRootedValue, as sp
             * may point to an unrooted location.
             */

r=me with that, thanks!

/be

Comment 13

[Igor Bukanov]

Attachment: v2b

The new version of the patch addresses the nit from the comment 12:

             /*
-             * Use JS_NewNumberValue and JS_NewDoubleValue here and in the
-             * next two cases, not js_NewNumberInRootedValue and
-             * js_NewDoubleInRootedValue, as sp may point to an unrooted
-             * location.
+             * Use JS_New{Double,Number}Value here and in the next two cases,
+             * not js_New{Double,Number}InRootedValue, as sp may point to an
+             * unrooted location.
              */

Comment 14

[Igor Bukanov]

I checked in the patch from the comment 13 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1205177220&maxdate=1205177282&who=igor%25mir2.org