React: Possible regression in dev mode in v16.3

[gregberge]

This example presents a huge performance drop between React 16.2 and React 16.3.

In investigated it and noticed several things:

• Perfomance drop is only visible using development bundle
• This is not related to styled-components (a simple div presents the same problem)
• I tried to investigate in performance tab of Chrome dev tools and I noticed that I get React measures even without "?react_perf"
• React Tree Reconciliation seems very long, do not know why

It is not a big problem for me, but it looks weird, so I decided to share it on Twitter, then @gaearon invited me to submit an issue about it.

[gaearon]

Can you turn this into a smaller self-contained example without third party libs?

[gregberge]

I will try to do it but right now I don’t have time, I think doing a setState every 16ms on 20 components in parallel should be sufficient to make it slow.

[sebmarkbage]

Note that if you’re using the StrictMode component, this is expected since it double renders. If you’re not, then it is not expected and probably a bug.

[alshakero]

Here is a fairly simple comparison. Exactly the same code with different react versions:

• 16.3: https://codesandbox.io/s/px9ny4v50 | 201.82ms per frame
• 16.2: https://codesandbox.io/s/m369j7jomp | 54.76ms per frame

[mlazowik]

Profile of the example provided by @alshakero

• 16.2:
• 16.3:

Looks like scheduling is somehow off and leaves idle gaps?

[gaearon]

We're spending quite a bit more time in performUnitOfWork (which is the hottest path).
I think this line might be the culprit.

Want to try commenting that line out in the bundle and see if it helps?

[mlazowik]

@gaearon yep, this is it.

[gaearon]

Good. I wonder why. Is it the allocation, the enumeration over a fiber, or changing hidden class of the target itself.

Can you try to set stashedWorkInProgressProperties to {} instead of null here and then do

Object.assign(stashedWorkInProgressProperties, workInProgress);

instead and see if it’s still as bad?

[mlazowik]

stashedWorkInProgressProperties is undefined there, so this errors out.

EDIT: Oh, I see what you want to check. Gimme a sec.

[mlazowik]

@gaearon

if (true && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
    allocStashed();
    copyWIP();
}

function allocStashed() {
    stashedWorkInProgressProperties = {};
}

function copyWIP() {
    Object.assign(stashedWorkInProgressProperties, workInProgress);
}

[gaearon]

Seems like it's still quite a lot. I bet we need to preserve the hidden class and explicitly write out all assignments.

[mlazowik]

Hm, maybe it's possible to write a generic function to do that based on type annotations? blazingFastClone

[mlazowik]

@gaearon an ad hoc confirmation that hidden class is the issue:

if (true && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
    stashedWorkInProgressProperties = createFiber(
        workInProgress.tag,
        workInProgress.pendingProps,
        workInProgress.key,
        workInProgress.mode
    );
}

[mlazowik]

I forgot to actually copy.

if (true && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
    stashedWorkInProgressProperties = createFiber(
        workInProgress.tag,
        workInProgress.pendingProps,
        workInProgress.key,
        workInProgress.mode
    );
    Object.assign(stashedWorkInProgressProperties, workInProgress);
}

This is still slow, but faster than with stashedWorkInProgressProperties = {}.

[gaearon]

I tried locally and handwritten version seems pretty fast.

[gaearon]

https://github.com/facebook/react/pull/12510

[gregberge]

Tested on the original example and it is much better, thanks!

[gaearon]

Fixed in 16.3.1.