1385802 - Ion-inline Reflect.getPrototypeOf

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[André Bargull [:anba]]

It's called 1.5M times in Speedometer (through Object.getPrototypeOf) per bug 1365361, so why not inline the call!?


Calls to js::Reflect_getPrototypeOf
Vanilla             0
Vanilla2015         0
Vanilla-babel       0
React               0
React-Redux         301
Ember               2486
Backbone            6
AngularJS           73187
Angular2            38
Vue                 0
jQuery              6
Preact              0
Inferno             0
Elm                 0
Flight              0


After Patch:
AngularJS           1000-2000

Comment 1

[André Bargull [:anba]]

Attachment: bug1385802.patch

This inlines Reflect.getPrototypeOf to retrieve the prototype of an object, and when a lazy prototype was found, performs an OOL-call to the VM. At first I tried to restrict the inlining to non-Proxy objects (using types->forAllClasses(constraints(), IsProxyClass), but that didn't really work out, because the TypeSets often had the TYPE_FLAG_ANYOBJECT flag set, so a forAllClasses(...) query was always returning ForAllResult::MIXED.


And some questions to help me understand the code I've copied from other inlined instructions. :-)

- Does it actually make a difference to call setGuard() in the MIR-node if the instruction is non-moveable and uses the default AliasSet::Store(AliasSet::Any) for getAliasSet()?
- And I was wondering when it is necessary to call pushTypeBarrier(...) when inlining a function call in MCallOptimize, because I can't spot a clear rule when comparing the existing callers to pushTypeBarrier(...) in MCallOptimize.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8891998 [details] [diff] [review]
bug1385802.patch

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

Nice!

::: js/src/jit/MCallOptimize.cpp
@@ +1809,5 @@
> +    auto* ins = MGetPrototypeOf::New(alloc(), target);
> +    current->add(ins);
> +    current->push(ins);
> +
> +    MOZ_TRY(resumeAfter(ins));

Yes, here we should use pushTypeBarrier, because the MGetPrototypeOf MIR instruction has very imprecise type information (MIRType::Value without a resultTypeSet is the worst case because we know nothing about the value).

pushTypeBarrier will look at the types we observed here and if they are more specific than |ins|'s types, it will insert a type barrier to guard on these observed types. The main advantage of this is that other code that uses the getPrototypeOf return value can then use the improved type information.

In general, pushTypeBarrier is not needed when a MIR instruction already has a very specific type, for instance in inlineObjectToString we check getInlineReturnType is MIRType::String, and MObjectClassToString also returns a string, so we know a type barrier is not going to give us better type info in that case.

::: js/src/jit/MIR.h
@@ +13918,5 @@
> +    explicit MGetPrototypeOf(MDefinition* target)
> +      : MUnaryInstruction(target)
> +    {
> +        setResultType(MIRType::Value);
> +        setGuard(); // May throw if target is a proxy.

To answer your question: this won't hurt but it's not strictly necessary as the instruction isEffectful() already so it's not going to be eliminated anyway.

::: js/src/jit/shared/LIR-shared.h
@@ +9352,5 @@
> +{
> +  public:
> +    LIR_HEADER(GetPrototypeOf)
> +
> +    LGetPrototypeOf(const LAllocation& target)

Nit: explicit

Comment 3

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> ::: js/src/jit/MCallOptimize.cpp
> @@ +1809,5 @@
> > +    auto* ins = MGetPrototypeOf::New(alloc(), target);
> > +    current->add(ins);
> > +    current->push(ins);
> > +
> > +    MOZ_TRY(resumeAfter(ins));
> 
> Yes, here we should use pushTypeBarrier, because the MGetPrototypeOf MIR
> instruction has very imprecise type information (MIRType::Value without a
> resultTypeSet is the worst case because we know nothing about the value).
> 
> pushTypeBarrier will look at the types we observed here and if they are more
> specific than |ins|'s types, it will insert a type barrier to guard on these
> observed types. The main advantage of this is that other code that uses the
> getPrototypeOf return value can then use the improved type information.
> 
> In general, pushTypeBarrier is not needed when a MIR instruction already has
> a very specific type, for instance in inlineObjectToString we check
> getInlineReturnType is MIRType::String, and MObjectClassToString also
> returns a string, so we know a type barrier is not going to give us better
> type info in that case.
> 

So let's see if I understand correctly: The pushTypeBarrier(...) for MRegExpSearcher (http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/js/src/jit/MCallOptimize.cpp#2032) isn't actually needed, because MRegExpSearcher already has a precise type (http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/js/src/jit/MIR.h#8293). Is that correct?


> ::: js/src/jit/MIR.h
> @@ +13918,5 @@
> > +    explicit MGetPrototypeOf(MDefinition* target)
> > +      : MUnaryInstruction(target)
> > +    {
> > +        setResultType(MIRType::Value);
> > +        setGuard(); // May throw if target is a proxy.
> 
> To answer your question: this won't hurt but it's not strictly necessary as
> the instruction isEffectful() already so it's not going to be eliminated
> anyway.
> 

Okay, great!

Comment 4

[Jan de Mooij [:jandem]]

(In reply to André Bargull from comment #3)
> So let's see if I understand correctly: The pushTypeBarrier(...) for
> MRegExpSearcher
> (http://searchfox.org/mozilla-central/rev/
> 09c065976fd4f18d4ad764d7cb4bbc684bf56714/js/src/jit/MCallOptimize.cpp#2032)
> isn't actually needed, because MRegExpSearcher already has a precise type
> (http://searchfox.org/mozilla-central/rev/
> 09c065976fd4f18d4ad764d7cb4bbc684bf56714/js/src/jit/MIR.h#8293). Is that
> correct?

MRegExpSearcher returns MIRType::Int32, so I agree that barrier isn't going to improve our type information.

There's another thing to consider: we don't check getInlineReturnType() == MIRType::Int32 there, so if the call never executed before we'll have an empty observed TypeSet (getInlineReturnType() == MIRType::Value). If we use pushTypeBarrier, it will ensure we bail out immediately after we execute MRegExpSearcher. If we don't use pushTypeBarrier, we will continue running in Ion (although we're probably going to bail soon anyway). AFAIK this difference doesn't matter much in practice (both correctness and perf).

Comment 5

[André Bargull [:anba]]

Attachment: bug1385802.patch

Updated patch per review comments, carrying r+ from jandem.

Comment 6

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8997b7d045fca1be174dfedb8d14aa563e09a8bf

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99dfa13672c4
Add ion-inline path for Reflect.getPrototypeOf(). r=jandem

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/99dfa13672c4