603017 - speed up js::DefaultValue on String builtin with void hint

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

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

[Jeff Walden [:Waldo]]

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.

Comment 2

[Luke Wagner [:luke]]

(In reply to comment #1)
Eww, good point, I'll be wary, thanks!

Comment 3

[Luke Wagner [:luke]]

Attachment: patch

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.

Comment 4

[Brendan Eich [:brendan]]

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

Comment 5

[Luke Wagner [:luke]]

Thanks!

http://hg.mozilla.org/tracemonkey/rev/544c27a914e7

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/544c27a914e7