ToObject often not inlined in Speedometer/AngularJS

RESOLVED FIXED in Firefox 56

Status

()

enhancement
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
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)

Updated

2 years ago
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
(Assignee)

Comment 1

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

Comment 2

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

Comment 5

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

Comment 6

2 years ago
Updated patch per review comments, carrying r+.
Attachment #8892002 - Attachment is obsolete: true
Attachment #8892573 - Flags: review+

Comment 7

2 years ago
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

Comment 8

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