Closed
Bug 524346
Opened 15 years ago
Closed 15 years ago
using jsval, not jsdouble *, for JSRuntime::js(Nan|PositiveInfinity|NegativeInfinity)
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: igor, Assigned: igor)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 2 obsolete files)
36.30 KB,
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•15 years ago
|
||
The patch converts double constants in JSRuntime into jsval. It also replaces few NewWeaklyRootedDouble calls with js_NewDoubleInRootedValue.
Attachment #408271 -
Flags: review?(mrbkap)
Comment 2•15 years ago
|
||
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 3•15 years ago
|
||
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).
Attachment #408271 -
Flags: review?(mrbkap) → review+
Comment 4•15 years ago
|
||
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.
Comment 5•15 years ago
|
||
(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
Assignee | ||
Comment 6•15 years ago
|
||
The new version uses NaNValue, negativeInfinityValue, positiveInfinityValue for JSRuntime:: fields.
Attachment #408271 -
Attachment is obsolete: true
Attachment #408572 -
Flags: review+
Comment 7•15 years ago
|
||
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
Assignee | ||
Comment 8•15 years ago
|
||
fixing a bad typo in GetAndCacheLocalTime
Attachment #408572 -
Attachment is obsolete: true
Attachment #408627 -
Flags: review+
Comment 9•15 years ago
|
||
Igor backed out the original check in.
merge: http://hg.mozilla.org/tracemonkey/rev/caee8544bb5e
backout: http://hg.mozilla.org/tracemonkey/rev/2909091fb254
Assignee | ||
Comment 10•15 years ago
|
||
Whiteboard: fixed-in-tracemonkey
Updated•15 years ago
|
Flags: in-testsuite-
Comment 11•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•