530240 - [FIX]Make jsstr's NormalizeThis somewhat faster for String objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I think next to JSString being the this, this is the common case; it's not much code, and speeds up some dromaeo string tests up to 10%.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

One question I had was whether I want OBJ_GET_CLASS or STOBJ_GET_CLASS and whether I need a request check somewhere in here...

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 413762 [details] [diff] [review]
Proposed fix

I don't think you need a request check, since this method is only called from a purely internal macro.


>diff --git a/js/src/jsstr.cpp b/js/src/jsstr.cpp

>+    // js_GetPrimitiveThis seems to to a bunch of work (like calls to

s/to to/to do/


>+    // JS_THIS_OBJECT) which we don't need in the common case (where
>+    // vp[1] is a String object) here.

/*
 *
 */
-style comments


>+    if (!JSVAL_IS_PRIMITIVE(vp[1]) &&
>+        OBJ_GET_CLASS(cx, JSVAL_TO_OBJECT(vp[1])) == &js_StringClass) {

None of the above, it's obj->getClass() now (the others just haven't been terminated with extreme prejudice yet).

r=me if you want it; I have difficulty thinking there's anything else here Brendan might see that I missed.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Address waldo's comments

Would still like a once-over, specifically on the assumption that vp[1] is JSVAL_IS_OBJECT here (because the macro checked for JSVAL_IS_STRING, we already checked for JSVAL_IS_NULL, and I can't see how to get the other primitives in here without them being boxed) that I'm making.  If that assumption is bogus, I'd love to know (and know why).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Er, with working assert

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 413771 [details] [diff] [review]
Er, with working assert

>@@ -644,19 +644,29 @@ JSClass js_StringClass = {
> 
> static JSString *
> NormalizeThis(JSContext *cx, jsval *vp)
> {
>     JSString *str;
> 
>     if (JSVAL_IS_NULL(vp[1]) && JSVAL_IS_NULL(JS_THIS(cx, vp)))
>         return NULL;

Nit: blank line here.

>+    /*
>+     * js_GetPrimitiveThis seems to do a bunch of work (like calls to
>+     * JS_THIS_OBJECT) which we don't need in the common case (where
>+     * vp[1] is a String object) here.
>+     */
>+    JS_ASSERT(!JSVAL_IS_PRIMITIVE(vp[1]));
>+    if (JSVAL_TO_OBJECT(vp[1])->getClass() == &js_StringClass) {
>+        str = JSVAL_TO_STRING(JSVAL_TO_OBJECT(vp[1])->fslots[JSSLOT_PRIMITIVE_THIS]);

This is fine and generates optimal code, but for debugging and conciseness, consider JSObject *obj = JSVAL_TO_OBJECT(vp[1]) and use obj instead of the lengthy JSVAL_TO_OBJECT(vp[1]).

/be

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Nit: blank line here.
> consider JSObject *obj = JSVAL_TO_OBJECT(vp[1])

Both done.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/e254e5dcd2a1

Comment 8

[Boris Zbarsky [:bzbarsky]]

Er... I meant http://hg.mozilla.org/tracemonkey/rev/e254e5dcd2a1

Comment 9

[Boris Zbarsky [:bzbarsky]]

> specifically on the assumption that vp[1] is JSVAL_IS_OBJECT here

jsreftest says this assumption is false.  Testcase:

  eval("x=false; x.__proto__.charAt = String.prototype.charAt; x.charAt(0)") );

gives me JSVAL_FALSE as vp[1].

Pushed http://hg.mozilla.org/tracemonkey/rev/efb6d958c53f to handle that.

Comment 10

[Brendan Eich [:brendan]]

Sorry, I should have remembered JSFUN_THISP_PRIMITIVE better.

/be

Comment 11

[Jeff Walden [:Waldo]]

You weren't the only one.  In any case: huzzah for tests!

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e254e5dcd2a1

Comment 13

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/efb6d958c53f