834242 - Reduce cost of stack rooting during property read operations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Element and property accesses in the interpreter and stubs are a large source of root construction, at the top level and even more so in the various object calls they make.  The attached patch eliminates almost all of these on SS, cutting the number of roots constructed from 60% of the original total to 39%.

Comment 1

[Terrence Cole [:terrence]]

Comment on attachment 705850 [details] [diff] [review]
patch

Review of attachment 705850 [details] [diff] [review]:
-----------------------------------------------------------------

It appears from our IRC conversation as if you also need to remove the default template parameters. Before you do that, I'd like to request a name change to the AllowGC enum to make the options match the assertion names and to change SHOUTY_CAPS to SuaveCamel to match our style guidelines: https://wiki.mozilla.org/JavaScript:SpiderMonkey:C%2B%2B_Coding_Style#Enums

I'm thinking something like:
enum AllowGC {
    NoGC = false,
    CanGC = true
};

::: js/src/jsinterpinlines.h
@@ +846,3 @@
>  
> +        objArg = obj;
> +    } while (0);

This refactor is sad-making, but I don't see a significantly better way to do it.

@@ +846,5 @@
>  
> +        objArg = obj;
> +    } while (0);
> +
> +    if (op == JSOP_CALLELEM && JS_UNLIKELY(res.isPrimitive()) && wasObject) {

This if should probably still be protected by #if JS_HAS_NO_SUCH_METHOD.

@@ -858,5 @@
>          return false;
> -    if (!GetObjectElementOperation(cx, op, obj, rref, res))
> -        return false;
> -
> -#if JS_HAS_NO_SUCH_METHOD

Nevermind our IRC conversation, it looks like this got moved into GetObjectElementOperation, but isn't protected by the #if anymore.

::: js/src/jsnum.cpp
@@ +1271,1 @@
>      c->dtoaCache.cache(base, d, s);

I think accidentally caching NULL here would not be catastrophic, but would definitely be suboptimal. Lets only cache if |s| is not null.

::: js/src/jsstr.cpp
@@ +3573,4 @@
>  JSFlatString *
>  js_NewStringCopyZ(JSContext *cx, const char *s)
>  {
>      return js_NewStringCopyN(cx, s, strlen(s));

I think we also need to templatize the char* variant of js_NewStringCopyN to keep a DONT_GC here from implicitly turning into an ALLOW_GC on the NewString and NewShortString in the callee. Even if it's not strictly necessary right now, I don't think the compiler will catch this mistake if someone makes it in the future, so we should make it impossible right now.

Comment 2

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6450a26eb74f

Comment 3

[Boris Zbarsky [:bzbarsky]]

This gave a quite nice win on Dromaeo's dom-attr due to making the atomization not require uninlining.  See http://dromaeo.com/?id=188982,188981 for numbers (without this patch on the left, with it on the right).

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6450a26eb74f