Ion-inline Reflect.getPrototypeOf

RESOLVED FIXED in Firefox 56

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 months ago
4 months 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

4 months 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

4 months ago
Created attachment 8891998 [details] [diff] [review]
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.
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

4 months 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

4 months ago
Created attachment 8892575 [details] [diff] [review]
bug1385802.patch

Updated patch per review comments, carrying r+ from jandem.
Attachment #8891998 - Attachment is obsolete: true
Attachment #8892575 - Flags: review+
(Assignee)

Comment 6

4 months ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8997b7d045fca1be174dfedb8d14aa563e09a8bf
Keywords: checkin-needed

Comment 7

4 months 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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/99dfa13672c4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months 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.