Closed Bug 914132 Opened 6 years ago Closed 6 years ago

IonMonkey: typeof improvements

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(4 files)

No description provided.
Bug 839027 removed the typeof hook, so typeof is no longer effectful.

This improves test10b in bug 606648 from 584 to 505 ms.
Attachment #801555 - Flags: review?(evilpies)
The TypeOf functions are infallible now, so this patch removes an unnecessary root and cx arguments. Also adds TypeOfObjectOperation and uses callWithABI instead of the more heavyweight callVM machinery to call it.

On top of the previous patch, we're now down to 298 ms, d8 is at 375 ms, but there's more we can do.
Attachment #801578 - Flags: review?(evilpies)
Attachment #801555 - Flags: review?(evilpies) → review+
Comment on attachment 801578 [details] [diff] [review]
Part 2 - Use infallible calls

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

::: js/src/jit/CodeGenerator.cpp
@@ +6422,5 @@
> +
> +    saveVolatile(output);
> +    masm.setupUnalignedABICall(2, output);
> +    masm.passABIArg(obj);
> +    masm.loadJSContext(output);

You can just use
movePtr(ImmWord(GetIonContext()->runtime), output);
after you made the other changes. One load less ;)

::: js/src/vm/Interpreter-inl.h
@@ +469,5 @@
>      return GetObjectElementOperation(cx, op, obj, isObject, rref, res);
>  }
>  
>  static JS_ALWAYS_INLINE JSString *
> +TypeOfOperation(const Value &v, JSContext *cx)

These two functions could take a JSRuntime pointer. You then we could try removing the TypeName version with JSContext as well. This depends if that make a lot more code worse or better.

::: js/src/vm/Interpreter.h
@@ +310,5 @@
>  extern JSType
> +TypeOfObject(JSObject *obj);
> +
> +extern JSType
> +TypeOfValue(const Value &v);

I feel like these two functions should live in jsobj(inlines).h
Attachment #801578 - Flags: review?(evilpies) → review+
Don't use an OOL call for "typeof object" if we know the operand is not callable and does not emulate undefined.

This brings our time down to 139 ms (from 584 ms without these patches), just 12 ms slower than the version without typeof (bug 606648 comment 15).
Attachment #801609 - Flags: review?(bhackett1024)
Attachment #801609 - Flags: review?(bhackett1024)
Attachment #801609 - Flags: review?(bhackett1024)
Attachment #801609 - Flags: review?(bhackett1024) → review+
Forgot this part yesterday. This brings our time down to 97 ms, faster than the version without typeof.
Attachment #802163 - Flags: review?(bhackett1024)
Attachment #802163 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fd5f21d4663
https://hg.mozilla.org/integration/mozilla-inbound/rev/61824642543a
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c452ca6d416
https://hg.mozilla.org/integration/mozilla-inbound/rev/e112a8245e29

(In reply to Tom Schuster [:evilpie] from comment #3)
> These two functions could take a JSRuntime pointer. You then we could try
> removing the TypeName version with JSContext as well. This depends if that
> make a lot more code worse or better.

Good idea, thanks! After doing that there was only one caller of TypeName(.., JSContext), so I changed it to pass cx->runtime() and removed the TypeName overload that takes a JSContext.

> ::: js/src/vm/Interpreter.h
> I feel like these two functions should live in jsobj(inlines).h

Moving them to jsobjinlines.h requires an extra #include of jsboolinlines.h for EmulatesUndefined... I kept them in Interpreter* for now so that they can be inlined in the interpreter loop (without requiring link-time optimization etc).
Depends on: 915167
Blocks: 607202
You need to log in before you can comment on or make changes to this bug.