Closed Bug 834242 Opened 7 years ago Closed 7 years ago

Reduce cost of stack rooting during property read operations

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: bhackett, Unassigned)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
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%.
Attachment #705850 - Flags: review?(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.
Attachment #705850 - Flags: review?(terrence) → review+
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).
Duplicate of this bug: 829791
https://hg.mozilla.org/mozilla-central/rev/6450a26eb74f
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Depends on: 841956
You need to log in before you can comment on or make changes to this bug.