421154 - Faster number conversions in the interpreter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Igor Bukanov]

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

Currently to convert non-numeric jsval into a double the interpreter calls:

extern JSBool
js_ValueToNumber(JSContext *cx, jsval v, jsdouble *dp);

where dp an an address of a local variable. To save the generated code size and to allow better optimizations in the the interpreter I suggest to follow the idea from bug 415455 and to change the signature into:

extern jsdouble
js_ValueToNumber(JSContext *cx, jsval* vp);

Here the error reporting is done via setting *vp to JSVAL_NULL.

This also makes the function less GC-hazard friendly as now the conversion function will have an extra root to store its results.

Comment 1

[Igor Bukanov]

Attachment: v1 (quilt patch)

The patch implements the signature change. In few places where I was not sure about root availability, I changes js_ValueToNumber into JS_ValueToNumber which now allows explicitly to pass an unrooted value.

Comment 2

[Igor Bukanov]

Attachment: v2 (quilt patch)

The previous version missed JS_POP_TEMP_ROOT in JS_ValueToInt32.

Comment 3

[Igor Bukanov]

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

This is the results for trunk versus the patch from the comment 2 applied on top of the patch from bug 415455 comment 9.

Given the line:

** TOTAL **:           1.085x as fast  

I suspect that there must be some bugs in the patch. On the other hand it does pass shell and mochi tests...

Comment 4

[Igor Bukanov]

Asking for inclusion in 1.9 based on the preliminary benchmarking results.

Comment 5

[Igor Bukanov]

(In reply to comment #3)
> 
> ** TOTAL **:           1.085x as fast  

With the patch the address of the local variables jsdouble d, d2 is no longer leaked. Perhaps it allowed GCC to do better job at optimizing the code.

Comment 6

[Mike Schroepfer]

(In reply to comment #4)
> Asking for inclusion in 1.9 based on the preliminary benchmarking results.
> 

Yep - that's easy.  You got it.  

Comment 7

[Igor Bukanov]

Comment on attachment 307552 [details] [diff] [review]
v2 (quilt patch)

The patch follows the same idea as implemented in the patch from bug 415455 comment 11 for the integer conversion case.

Comment 8

[Igor Bukanov]

Attachment: diff -b version of v2

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 307552 [details] [diff] [review]
v2 (quilt patch)

>+         * vp roots obj so we can not use it as an extra root for

s/can not/cannot/g

>+ * Convert a value to a number. On exit *vp is JSVAL_NULL if and only if there
>+ * are errors.

s/are errors/was an error/g

and maybe iff for if and only if, throughout.

Another global change that would help future jsval representation experiements. Consider:

$ grep ' == JSVAL_NULL' *.[ch] | wc -l
      10
$ grep JSVAL_IS_NULL *.[ch] | wc -l
      55
$ grep ' == JSVAL_VOID' *.[ch] | wc -l
      11
$ grep JSVAL_IS_VOID *.[ch] | wc -l
      49

Would like JSVAL_IS_NULL and JSVAL_IS_VOID used in this patch, therefore.

r+me and you have approval to land with these picked. Thanks again, the use of already-in-memory root as in/out param to convey error and propagate main result possible to registerized variable is righteous!

/be

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> (From update of attachment 307552 [details] [diff] [review])
> Would like JSVAL_IS_NULL and JSVAL_IS_VOID used in this patch, therefore.

Sorry, I have not get it. Do you suggest to use JSVAL_VOID, not JSVAL_NULL, as an error indicator when converting values to numbers?

Comment 11

[Brendan Eich [:brendan]]

No, I mean use JSVAL_IS_NULL(v) instead of v == JSVAL_NULL, likewise for JSVAL_IS_VOID/JSVAL_VOID. Future jsval representations may allow shortcuts compared to loading a (possibly big) constant and testing equality.

/be

Comment 12

[Igor Bukanov]

Attachment: v3

The new version addresses the nits from the comment 9 and makes sure that code uses JSVAL_IS_(NULL|VOID)(v) and not the explicit compare operation v == JSVAL_(NULL|VOID).

Comment 13

[Igor Bukanov]

Attachment: v2 - v3 delta

Comment 14

[Igor Bukanov]

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

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

Comment 15

[Bob Clary [:bc] (inactive)]

fat fingers.