[WIP] Optimize context by using an object for stateNode #12544
Conversation
|
The benchmark should use something more evenly distributed. It is designed to be fast at traversing through non-context fibers. It is also designed to stop and restart. Since this is adding a bunch of allocation we should probably measure the memory usage too. We should also consider whether adding another fiber field would be beneficial. It’s worth noting that this is also a V8 specific optimization. It might be worse in every other engine. So we should probably test those and think about if it is worth the tradeoff. |
| // Store the observedBits on the fiber's stateNode for quick access. | ||
| workInProgress.stateNode = observedBits; | ||
| if (oldProps === null) { |
There was a problem hiding this comment.
Props nullability have had other meanings in the past and doesn't have a defined meaning now but might get one.
What is this meant to test? If it is to test that this is first mount, it should check for current === null, but since this is really just testing whether stateNode is null you might as well just make that explicit (Flow will want you to anyway once we properly type stateNode).
|
I think let's hold off until we have some product code that we can benchmark? Don't want to build something just for this if we'll use it for Relay anyway. |
| if (oldProps === null) { | ||
| workInProgress.stateNode = {bits: observedBits}; | ||
| } else { | ||
| workInProgress.stateNode.bits = observedBits; |
There was a problem hiding this comment.
This will mutate both the current and the workInProgress since they'd share the same object. Generally, that's a big nono because many things can go wrong. We'd have to prove that this can't have those issues and won't get them later somehow.
There was a problem hiding this comment.
Been too long since I worked on Fiber. Thanks for catching.
gaearon
changed the title
|
#12745 is adding another case like this. Perhaps we should use a separate field specifically for V8? 😳 |
I tested a tree view with this, and there appears to be a significant improvement in DEV.
My hypothesis is that
stateNodebeing an object prevents some kind of deopt in the propagation loop wherefibertype switches from one with a numberstateNodeto one with an objectstateNode.Details below.
Self Time
Before
After
Detailed Breakdown
Before
After
Summary
Before
After
Firefox
Before
After
What’s odd is, I haven’t seen a noticeable difference in production builds.
Maybe there’s something GCC does that accidentally mitigates this.
My testing procedure is to make a tree view with 1,000 consumers, and then update the root context 10 times. I tried swapping the bundles here and back a few times, and the difference in DEV consistently reproduces.