+++ 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.

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.

The previous version missed JS_POP_TEMP_ROOT in JS_ValueToInt32.

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...

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

(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.

(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 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 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

(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?

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

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).

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

fat fingers.