Currently jsNan, jsPositiveInfinity, jsNegativeInfinity are defined as jsdouble * fields in JSRuntime. On the other hand, in most cases when code uses them, the constants are converted to jsval. This suggests to declare the constants as jsval to avoid an extra bloat due to jsval tag manipulations.

The patch converts double constants in JSRuntime into jsval. It also replaces few NewWeaklyRootedDouble calls with js_NewDoubleInRootedValue.

Not to say this is necessarily bad, but I recall someone recently benchmarking manipulating tag bits on a value just read from memory and finding that modern CPUs coalesce the load and tag/de-tag into essentially the same underlying cycle. Are you sure the cost to tag these bits every time is actually observable?

Comment on attachment 408271 [details] [diff] [review] v1 I don't feel strongly about this. I think that igor's point here is that having all of those DOUBLE_TO_JSVALs pollutes the code (even if the actual generated code is equally fast). While this change is inconsistent with JSRuntime::emptyString, from reading through the patch, this is clearly the directly we want (as opposed to emptyString, where the majority of uses use the JSString * instead of the jsval).

Less code noise is good, that's far more sensible than tagging cost. Might it make sense to rename to NaNValue, negativeInfinityValue, positiveInfinityValue to follow the convention of including v/val/value in jsval-typed variables? This would also get rid of a js prefix that seems unneeded now.

(In reply to comment #4) > Less code noise is good, that's far more sensible than tagging cost. > > Might it make sense to rename to NaNValue, negativeInfinityValue, > positiveInfinityValue to follow the convention of including v/val/value in > jsval-typed variables? This would also get rid of a js prefix that seems > unneeded now. +1 -- or more ;-). /be

The new version uses NaNValue, negativeInfinityValue, positiveInfinityValue for JSRuntime:: fields.

http://hg.mozilla.org/tracemonkey/rev/14c76164f4c2 http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1256639539.1256644303.21754.gz&fulltext=1 did this cause: TEST-UNEXPECTED-FAIL | trace-test.py | /builds/slave/tracemonkey-macosx-debug/build/js/src/trace-test/tests/sunspider/check-date-format-tofte.js: Assertion failure: JSVAL_IS_DOUBLE(v), at /builds/slave/tracemonkey-macosx-debug/build/js/src/jsapi.h:190 TEST-UNEXPECTED-FAIL | trace-test.py | /builds/slave/tracemonkey-macosx-debug/build/js/src/trace-test/tests/sunspider/check-date-format-xparb.js: Assertion failure: JSVAL_IS_DOUBLE(v), at /builds/slave/tracemonkey-macosx-debug/build/js/src/jsapi.h:190

fixing a bad typo in GetAndCacheLocalTime

Igor backed out the original check in. merge: http://hg.mozilla.org/tracemonkey/rev/caee8544bb5e backout: http://hg.mozilla.org/tracemonkey/rev/2909091fb254