Closed Bug 1383644 Opened 3 years ago Closed 3 years ago

ToObject often not inlined in Speedometer/AngularJS

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Calls (not inlined) to intrinsic_ToObject [1] in Speedometer (*)

Vanilla         230
Vanilla2015     489
Vanilla-babel   589
React           1115
React-Redux     1197
Ember           5095
Backbone        962
AngularJS       112401
Angular2        691
Vue             873
jQuery          352
Preact          634
Inferno         810
Elm             524
Flight          1584

Apparently we miss to inline ToObject quite often in AngularJS.

Breaking here [2] with 
    br MCallOptimize.cpp:2906 if callInfo.args_.mBegin[0]->resultType_!=js::jit::MIRType::Object
and 
    br MCallOptimize.cpp:2906 if callInfo.args_.mBegin[0]->resultType_==js::jit::MIRType::Value

we can measure how often ToObject isn't inlined because Arg[0] isn't an object.

ToObject       All       Arg[0] not Object
React-Redux     21       2
Ember           87       0
Backbone         6       0
AngularJS       58      16      <-- Only because callInfo.getArg(0)->type() == MIRType::Value
Angular2         8       0
Vue              6       0
jquery           3       0
Preact           7       0
Inferno         18       0
Elm              7       0
Flight          19       0

I guess we should handle MIRType::Value when it flows into the ToObject call given that it is the second most called intrinsic per bug 1365361?


(*) Measured via gdb breakpoints, so the numbers may not be accurate to the last digit :-)
[1] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/js/src/vm/SelfHosting.cpp#82
[2] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/js/src/jit/MCallOptimize.cpp#2906
[3] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/js/src/jit/MCallOptimize.cpp#2915
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
(In reply to André Bargull from comment #0)
> Calls (not inlined) to intrinsic_ToObject [1] in Speedometer (*)
> 
> Vanilla         230
> Vanilla2015     489
> Vanilla-babel   589
> React           1115
> React-Redux     1197
> Ember           5095
> Backbone        962
> AngularJS       112401
> Angular2        691
> Vue             873
> jQuery          352
> Preact          634
> Inferno         810
> Elm             524
> Flight          1584
> 
> Apparently we miss to inline ToObject quite often in AngularJS.
> 

With the patch to inline ToObject with MIRType::Value as the input, the non-inlined calls to ToObject drop to 3000-5000 for AngularJS.
Attached patch bug1383644.patch (obsolete) — Splinter Review
This is similar to the existing MToObjectOrNull, except we can't make it moveable, because we always need to return a new object when ToObject is called with a primitive value and it'd be observable to return the same primitive wrapper multiple times.
Attachment #8892002 - Flags: review?(jdemooij)
Comment on attachment 8892002 [details] [diff] [review]
bug1383644.patch

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

Inline all the things :)

::: js/src/jit/MCallOptimize.cpp
@@ +2947,5 @@
> +        current->push(object);
> +    } else {
> +        auto* ins = MToObject::New(alloc(), object);
> +        current->add(ins);
> +        current->push(ins);

Here (the else-branch) it would also be good to use pushTypeBarrier because it's likely more precise than any-object.
Attachment #8892002 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #3)
> Comment on attachment 8892002 [details] [diff] [review]
> bug1383644.patch
> 
> Review of attachment 8892002 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Inline all the things :)
> 

And one day :djvj will complain he can no longer use his scripts from bug 1357180/1365361 to sample built-ins. ;-)
Attached patch bug1383644.patchSplinter Review
Updated patch per review comments, carrying r+.
Attachment #8892002 - Attachment is obsolete: true
Attachment #8892573 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4b029e2a35d
Inline ToObject when called with MIRType::Value. r=jandem
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f4b029e2a35d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.