Optimize the obj_toString call in OrdinaryToPrimitive

NEW
Unassigned

Status

()

enhancement
P3
normal
2 years ago
Last year

People

(Reporter: jandem, Unassigned)

Tracking

(Blocks 1 bug, {perf})

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 wontfix, firefox59 ?)

Details

(Whiteboard: [qf:p3])

Attachments

(1 attachment)

Reporter

Description

2 years ago
Posted patch PatchSplinter Review
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.
Attachment #8890346 - Flags: feedback?(andrebargull)
(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
Reporter

Comment 2

2 years ago
(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
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?
Keywords: perf
Priority: -- → P3
Whiteboard: [qf:p3]
Comment on attachment 8890346 [details] [diff] [review]
Patch

Clearing f? for now per comment #3.
Attachment #8890346 - Flags: feedback?(andrebargull)
Reporter

Updated

Last year
Assignee: jdemooij → nobody
Status: ASSIGNED → NEW
Reporter

Updated

Last year
See Also: → 1376948
You need to log in before you can comment on or make changes to this bug.