1384562 - Optimize the obj_toString call in OrdinaryToPrimitive

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

Going through js::Call and obj_toString slows us down on Ember. It seems we can short-circuit this in OrdinaryToPrimitive.

André, any thoughts on this? I wrote the patch like this to not make things much slower when objects override toString.

Comment 1

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> Going through js::Call and obj_toString slows us down on Ember. It seems we
> can short-circuit this in OrdinaryToPrimitive.

Do you already have any numbers how much this will improve Ember resp. the part of Ember which repeatedly calls obj_toString?


> André, any thoughts on this? I wrote the patch like this to not make things
> much slower when objects override toString.

Hmm, on the one hand I kind of don't like to have an Ember-specific optimisation in OrdinaryToPrimitive. But on the other hand a single run of Speedometer/Ember calls 1.7M times obj_toString (bug 1383795, comment #1). So then I measured how often OrdinaryToPrimitive (with hint == JSTYPE_STRING) is called for each framework in Speedometer:

           # calls here [1]  # your optimisation applies
Vanilla               7       0
Vanilla2015          22       0
Vanilla-babel         0       0
React                 0       0
React-Redux           0       0
Ember           1692181 1692173
Backbone            204     200
AngularJS           202       0
Angular2              2       0
Vue                   0       0
jQuery              685       0
Preact                0       0
Inferno               0       0
Elm                   0       0
Flight              206       0

And based on these numbers, at least for the frameworks tested in Speedometer, all frameworks except Ember have a negligible number of calls to OrdinaryToPrimitive, so having this specific optimisation shouldn't slow down other parts of Speedometer. 

I'm still a bit undecided on whether or not I'd go with this change, but if we apply the patch, we should probably also file a follow-up bug to investigate a more general solution for this problem. (So we can look into this issue in a year or so. ;-)

Given that I'm still undecided, I'd be great to have a second opinion. Maybe ask :djvj, too, since he's currently also looking into built-in optimisations?


[1] http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/js/src/jsobj.cpp#3075

Comment 2

[Jan de Mooij [:jandem]]

(In reply to André Bargull from comment #1)
> Do you already have any numbers how much this will improve Ember resp. the
> part of Ember which repeatedly calls obj_toString?

It's a measurable improvement on Ember. I don't remember how much exactly but more than a few %, I can get more data on this. I don't think it's that Ember-specific, it also hits on non-Ember-using websites. I'll do some measurements of that too :)

FWIW, Chakra has a similar optimization where they check for Object.prototype.valueOf/toString: https://github.com/Microsoft/ChakraCore/blob/e063e2a6527389c54f3b2dd181f9669c7140fbe4/lib/Runtime/Types/DynamicType.cpp#L395-L418

Comment 3

[André Bargull [:anba]]

Do we still want to look into this optimization? It's been over a week and no one stepped up to veto this approach, so maybe just go ahead and try it out on Speedometer (and other benchmarks), and if it shows improvements (and there are no regressions), we'll take it. WDYT?

Comment 4

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 5

[André Bargull [:anba]]

Comment on attachment 8890346 [details] [diff] [review]
Patch

Clearing f? for now per comment #3.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1384562 - Add OrdinaryToPrimitive fast path for stringifying plain objects. r?iain!

The Ember subtest in Speedometer hits this a lot for some logging code and this is
likely to also affect similar JS code elsewhere.

This optimization is written so that even if it doesn't apply, there's little overhead
in most cases because we do the toString lookup only once.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0e6d2171c5e
Add OrdinaryToPrimitive fast path for stringifying plain objects. r=iain

Comment 8

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/e0e6d2171c5e

Comment 9

[Mayank Bansal]

Huge improvement on AWFY-Speedometer-EmberJS-Debug-TodoMVC*

Comment 10

[Markus Stange [:mstange]]

This caused a 13.4% improvement on "speedometer EmberJS-Debug-TodoMVC/CompletingAllItems opt fission webrender":

https://treeherder.mozilla.org/perfherder/graphs?series=mozilla-central,7ad734d6a47b40b214ab178e1a1c4948c47b313e,1,13&timerange=5184000&series=mozilla-central,7ad734d6a47b40b214ab178e1a1c4948c47b313e,1,13&series=autoland,7c44de1a34d09971755c04b209b45ac8af595f12,1,13