Closed Bug 834242 Opened 7 years ago Closed 7 years ago

Reduce cost of stack rooting during property read operations


(Core :: JavaScript Engine, defect)

Other Branch
Not set





(Reporter: bhackett, Unassigned)




(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]

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:

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;
> -

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,188981 for numbers (without this patch on the left, with it on the right).
Duplicate of this bug: 829791
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.