Closed
Bug 603017
Opened 14 years ago
Closed 14 years ago
speed up js::DefaultValue on String builtin with void hint
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: luke, Assigned: luke)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file)
5.96 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
Doing js::DefaultValue on a js_StringClass object takes the following roundabout path to basically just do obj->getPrimitiveThis().toString(): #0 JSObject::getPrimitiveThis #1 0x080f5f7e in js_GetPrimitiveThis #2 0x08189c08 in js_str_toString #3 0x080fa050 in js::CallJSNative #4 0x080f6908 in js::Invoke #5 0x080f7286 in js::ExternalInvoke #6 0x0810c044 in ExternalInvoke #7 0x0811c6ac in js_TryMethod #8 0x0811c5bd in js_TryValueOf #9 0x0806cf1a in JS_ConvertStub #10 0x0811b917 in js::DefaultValue #11 0x082e073b in DefaultValue It seems like we should be able to take a shortcut somewhere in js::DefaultValue. On date-format-xparb, callgrind shows that we spend 21% of our time under DefaultValue (17M insns), so theoretically this could be up to a 5ms SS win.
Comment 1•14 years ago
|
||
Do note these funtimes: String.prototype.valueOf = function() { return "oh snap"; }; assertEq(new String("foo") + "!", "oh snap!"); String.prototype.valueOf = null; String.prototype.toString = function() { return "oh snap"; }; assertEq(new String("foo") + "!", "oh snap!"); I'm sure you can cache String.prototype's shape or something to require valueOf is its original value, or something similar if it's been removed/redefined in ways such that the DefaultValue algorithm will consult toString -- just to say this isn't just as simple as pulling the value from the primitive-this slot, unfortunately.
Assignee | ||
Comment 2•14 years ago
|
||
(In reply to comment #1) Eww, good point, I'll be wary, thanks!
Assignee | ||
Comment 3•14 years ago
|
||
Without the patch, the timings for date-format-xparb: -m 25ms -j -m 21ms With the patch, the timings are: -m 18ms -j -m 21ms (For reference, jsc gets 20ms) So this depends on tracer-integration to make the right choice. Fortunately, Bill already told me yesterday that his profiler was choosing -m for date-format-xparb which, before the patch, was a problem but, with the patch, is perfect. Btw, I just copied the existing optimized path for DefaultValue with JSTYPE_STRING hint. However, it would be faster (up to 30% for a micro-bench doing (new String('a')) + (new String('b'))) to do a per-thread cache based on shape. This doesn't seem to matter for xparb, though, so I'll leave it as a followup.
Attachment #482574 -
Flags: review?(brendan)
Assignee | ||
Updated•14 years ago
|
Summary: speed up js::DefaultValue on js_StringClass and other builtin classes → speed up js::DefaultValue on String builtin with void hint
Blocks: 580468
Comment 4•14 years ago
|
||
Comment on attachment 482574 [details] [diff] [review] patch >+static bool >+StringMethodIsNative(JSContext *cx, JSObject *obj, jsid methodid, Native native) Wonder if JS_ALWAYS_INLINE would help an aggressive optimizer do something better than the current-best with this patch, enough to matter. > return JS_TRUE; Nit: return true here, it's the only return left that uses JSBool constants. /be
Attachment #482574 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 5•14 years ago
|
||
Thanks! http://hg.mozilla.org/tracemonkey/rev/544c27a914e7
Whiteboard: fixed-in-tracemonkey
Comment 6•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/544c27a914e7
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•