1865879 - React completeWork()/precacheFiberNode() spends more time in SetElementMegamorphicYesCache than Chrome

Status: NEW

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Jeff Muizelaar [:jrmuizel]]

59% of the time is in SetNonexistentProperty. With ~80% in SetNonexistentFirstProperty vs 20% in SetNonexistentNextProperty

https://share.firefox.dev/3sLzhPc

Comment 1

[Jeff Muizelaar [:jrmuizel]]

completeWork:
https://github.com/mozilla/Speedometer/blob/dc24852942494ec6e8198e1fc21f2ab4048f60e9//resources/todomvc/architecture-examples/react-redux/dist/app.bundle.js#L4284

Chrome profile: https://share.firefox.dev/49NHxPh

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Looking at a version of react-redux without having the closure minifier doing inlining attributes this to:

function precacheFiberNode(hostInst, node) {
  node[internalInstanceKey] = hostInst;
}

This is setting a property on a freshly created element.

After the call to precacheFiberNode comes a call to updateFiberProps which sets a second property on the node.

function updateFiberProps(node, props) {
  node[internalEventHandlersKey] = props;
}

We are 10x slower on precacheFiberNode:
FF: https://share.firefox.dev/3MUzgiA
Chrome: https://share.firefox.dev/3RbQfQ1

and 5.4x slower on updateFiberProps:
FF: https://share.firefox.dev/3Rd4IeC
Chrome: https://share.firefox.dev/3G9Ic01

Comment 3

[Iain Ireland [:iain]]

I think bug 1844878 should help with updateFiberProps. I don't think it will affect precacheFiberNode.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Safari is also a 8x faster at this:
FF: https://share.firefox.dev/47rEFpS
Safari: https://share.firefox.dev/3sNNTxp

Comment 5

[Iain Ireland [:iain]]

I had a chat with smaug in the sp3 channel about this. Summarizing my understanding:

The DOM side maintains a JSHolders map, containing the set of native DOM objects that keep JS objects alive. It is a map (rather than a list) so that we can quickly remove DOM objects when they die. We preserve a wrapper by finding the corresponding DOM object and adding it to the JSHolders map. (Objects can also be added to the JSHolders map for other reasons.)

Right now SpiderMonkey preserves wrappers in two cases. First, when we generate DOM classes, classes where we need to preserve wrappers get an addProperty hook. When we add any property to an object, we call that hook. If it's the first time we've done so, we preserve the wrapper. Second, there are a few weak-ref related places where we call an embedding-provided preserveWrapper callback.

The first case is where we're spending our time in this bug. My patch in bug 1844878 is designed to reduce the overhead by replacing the addProperty hook with a class flag (to indicate that we want to preserve wrappers for objects with this class) and a shape flag (to indicate that we have not already preserved wrappers for this particular object). This lets us avoid calling the hook in cases where it isn't going to do anything. The main problem with that patch is that it needs some work on the DOM side to let us preserve wrappers. It might seem like we could just call the preserveWrapper callback when we add the first property to an object, but that callback is implemented using the addProperty hook, which is precisely the thing we're trying to eliminate. If we can fix that problem (smaug's suggestion: "we might want to tweak codegen a bit for this. Basically change the _addProperty to some more generic helper and perhaps expose that through DOMJSClass.") then I think we can eliminate almost all overhead for updateFiberProps.

If we also want to speed up precacheFiberNode, then we need a way to make preserving the wrapper less expensive. One option would be to try batching wrapper preservation. Instead of calling the preserveWrapper callback, SM would simply push the wrapper object into a buffer. Before looking anything up in the JSHolders map, the DOM would drain the buffer (by calling TryPreserveWrapper on each object, for example). This would significantly reduce the number of SM<->DOM calls, and probably be a bit better for code/data locality. If we keep things simple enough, we could even consider pushing wrappers into the buffer from jitcode, reducing the number of JIT<->C++ calls. (We'd have to think about the best way to integrate that with the megamorphic property cache).

I don't know the DOM code well enough to confidently write the non-SM parts of this on my own.

Comment 6

[Iain Ireland [:iain]]

Another way to mitigate some of the cost of preserving wrappers is to avoid allocating them in the nursery, which makes minor GC slower. See bug 1892764.