Ion-inline Reflect.getPrototypeOf

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

(Blocks: 1 bug)

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
Posted patch bug1385802.patch (obsolete) — Splinter Review
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.
Attachment #8891998 - Flags: review?(jdemooij)
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
Attachment #8891998 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 3

2 years ago
(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!
(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).
(Assignee)

Comment 5

2 years ago
Updated patch per review comments, carrying r+ from jandem.
Attachment #8891998 - Attachment is obsolete: true
Attachment #8892575 - Flags: review+

Comment 7

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99dfa13672c4
Add ion-inline path for Reflect.getPrototypeOf(). r=jandem
Keywords: checkin-needed

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/99dfa13672c4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.