1878158 - Slow DOM property access in React's SyntheticEvent constructor

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Markus Stange [:mstange]]

React has this loop in its SyntheticEvent constructor:

for (var propName in Interface) {
  if (!Interface.hasOwnProperty(propName)) {
    continue;
  }
  var normalize = Interface[propName];
  if (normalize) {
    this[propName] = normalize(nativeEvent);
  } else if (propName === "target") {
    this.target = nativeEventTarget;
  } else {
    this[propName] = nativeEvent[propName];
  }
}

It copies over properties from the native DOM object and puts them into this. (Most properties on Interface are null, which means "Copy this property and don't apply any normalization".)

Firefox profile (1205 samples)
Chrome profile (794 samples, 1.52x faster)

Firefox seems to take a long trip to get to places like mozilla::dom::MouseEvent_Binding::get_clientX. Chrome appears to have a more generic stub that takes a more direct path into blink::(anonymous namespace)::v8_mouse_event::ClientYAttributeGetCallback etc. via Builtin:KeyedLoadIC_Megamorphic -> Builtin:CallApiCallbackGeneric

Comment 1

[Iain Ireland [:iain]]

Our megamorphic get-prop handler currently doesn't support getters. One reason for this is that we would need to change the alias set of the MIR node to indicate that it could have arbitrary side effects, which impedes optimization, but in theory we could make that decision based on whether we'd ever actually seen a getter.

Other than that, Alex says it's doable (and she should know). Markus, do you have a sense of how highly we should prioritize this?

Comment 2

[Markus Stange [:mstange]]

(In reply to Iain Ireland [:iain] from comment #1)

Markus, do you have a sense of how highly we should prioritize this?

In the latest React-Redux report this is in spot #6, with a difference of 0.9%, and it's the highest ranking JS function in that report. So priority-wise I'd group it with the rest of the "~1% on React subtests" issues.

Comment 3

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1878158 - Allow cached getters in megamorphic GetProp/Elem r?iain

This patch is doing roughly three things:

• Adds a boolean field through the IR pipeline representing whether we want to
  allow getters or not. This will allow us to do a VM call and all that jazz
  within the op in order to actually call the getter
• Updates existing megamorphic handling to write information to the megamorphic
  cache indicating that the property is a getter. Existing megamorphic handlers
  will treat this as a cache miss and fail / bail
• Allows the tolerant VM calls to look up the cached getter in the megamorphic
  cache and call it without having to do the full expensive property lookup

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e706163e41b4
Allow cached getters in megamorphic GetProp/Elem r=iain

Comment 5

[Cristian Tuns]

Backed out for causing spidermonekey bustages in PortableBaselineInterpret.cpp

• Backout link
• Push with failures
• Failure Log
• Failure line: /builds/worker/checkouts/gecko/js/src/vm/PortableBaselineInterpret.cpp:430:20: error: use of undeclared label 'cacheop_MegamorphicLoadSlotPermissiveResult'
  /builds/worker/checkouts/gecko/js/src/vm/PortableBaselineInterpret.cpp:430:20: error: use of undeclared label 'cacheop_MegamorphicLoadSlotByValuePermissiveResult'

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd5c3f94549d
Allow cached getters in megamorphic GetProp/Elem r=iain

Comment 7

[pstanciu]

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

Comment 8

[Natalia Csoregi [:nataliaCs]]

Backed out for causing multiple regressions (several website failures).

• backout: https://hg.mozilla.org/integration/autoland/rev/5c3e0ef9b916432dfc42f800c3c7997f58ae5a63

Comment 9

[Sandor Molnar[:smolnar]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/5c3e0ef9b916

Comment 10

[Andra Esanu (needinfo me)]

(In reply to Natalia Csoregi [:nataliaCs] from comment #8)

Backed out for causing multiple regressions (several website failures).

• backout: https://hg.mozilla.org/integration/autoland/rev/5c3e0ef9b916432dfc42f800c3c7997f58ae5a63

Perfherder has detected a browsertime performance change from push 5c3e0ef9b916432dfc42f800c3c7997f58ae5a63.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
2%	speedometer3 TodoMVC-React-Redux/CompletingAllItems/Sync	macosx1400-64-shippable-qr	fission webrender	9.26 -> 9.46	Before/After

As author of one of the patches included in that push, we need your help to address this regression.
Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the patch(es) may be backed out in accordance with our regression policy.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 190

For more information on performance sheriffing please see our FAQ.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:alexical, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0de62812a3d6
Allow cached getters in megamorphic GetProp/Elem r=iain

Comment 13

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/0de62812a3d6