Traverse commit phase effects iteratively #20094
Conversation
The current traversal logic is spread between ReactFiberWorkLoop and ReactFiberCommitWork, and it's a bit awkward, especially when refactoring. Idk the ideal module structure, so for now I'd rather keep it all in one file.
We suspect that using the JS stack to traverse through the tree in the commit phase is slower than traversing iteratively. I've kept the recursive implementation behind a flag, both so we have the option to run an experiment comparing the two, and so we can revert it easily later if needed.
facebook-github-bot
added
CLA Signed
React Core Team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 368930d:
|
Details of bundled changes.Comparing: 4e5d7fa...368930d react-dom
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (experimental) |
Details of bundled changes.Comparing: 4e5d7fa...368930d react-dom
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (stable) |
acdlite
force-pushed
the
no-stack-commit-phase
branch
from
4ca681d
to
d20491e
Compare
acdlite
force-pushed
the
no-stack-commit-phase
branch
from
d20491e
to
acb051e
Compare
|
Looks like another build variant the reconciler benchmark might want to compare in the short term, @rickhanlonii. |
There was a problem hiding this comment.
The algos and overall strategy look good to me 👍
Will leave the detailed review to @bvaughn, who has context.
acdlite
force-pushed
the
no-stack-commit-phase
branch
from
acb051e
to
368930d
Compare
|
@bvaughn Ok this is ready for you to look at |
There was a problem hiding this comment.
This change looks good. The new iterative methods that have to account for Profiler (and soon Offscreen) are a lot less readable in the iterative forks though :(
We aren't actually running any tests again the old recursive methods anymore, are we? That seems potentially problematic.
| let child = firstChild; | ||
| while (child !== null) { | ||
| nextEffect = child; | ||
| iterativelyCommitLayoutEffects_begin(child, finishedRoot); |
There was a problem hiding this comment.
This seems like a reasonable approach 👍
| } | ||
| nextEffect = finishedWork; | ||
|
|
||
| if ((finishedWork.flags & LayoutMask) !== NoFlags) { |
There was a problem hiding this comment.
Shouldn't have any practical implications, but we check LayoutMask in the iterative branch and Update | Callback in the recursive branch. Might be worth updating the latter to also check LayoutMask.
That's right, since the new reconciler is only enabled in the www variant build. I did turn it on locally to manually test, which can continue to do until the new reconciler is merged into the old. Then we can use |
| @@ -84,6 +84,8 @@ export const enableDiscreteEventFlushingChange = true; | |||
| // to the correct value. | |||
| export const enableNewReconciler = __VARIANT__; | |||
|
|
|||
| export const enableRecursiveCommitTraversal = false; | |||
There was a problem hiding this comment.
Should this be either true or in the dynamic values?
There was a problem hiding this comment.
See this comment #20094 (comment)
Andrew's plan was to split iterative and recursive between old and new forks (once we merge the new fork into the old). For now, we're turning on iterative only for the new build (to see if that fixes the top line metrics).
Does make me a little uneasy that we don't have automated tests covering the new + recursive combination, but if we're able to merge new -> old soon, hopefully it's ok?
There was a problem hiding this comment.
Yeah ignore me, I'm a clown. This forces iterative mode which is what we want.
There was a problem hiding this comment.
Nah, we're both clowns. I also had the same initial thought, b'c intuitively I think of the feature flag enabling the new thing :)
|
In layman terms, what does this PR mean? Ya'll doing some exciting things! |
|
PR = pull request |
|
@maraisr this PR reworks the algorithms that fire effects and mutations in the commit phase to test a theory for why perf regressed in one of our algorithms that hasn't been released yet. These changes don't have any user-facing impact, and is just part of our research for now. |
|
Still mega interesting though! |
Summary: Base sync before adding Flight files. This sync includes the following changes: - **[454c2211c](facebook/react@454c2211c )**: Refactor SchedulerHostConfigs ([#20025](facebook/react#20025)) //<Ricky>// - **[56e9feead](facebook/react@56e9feead )**: Remove Blocks ([#20138](facebook/react#20138)) //<Sebastian Markbåge>// - **[3fbd47b86](facebook/react@3fbd47b86 )**: Serialize pending server components by reference (lazy component) ([#20137](facebook/react#20137)) //<Sebastian Markbåge>// - **[930ce7c15](facebook/react@930ce7c15 )**: Allow values to be encoded by "reference" to a value rather than the value itself ([#20136](facebook/react#20136)) //<Sebastian Markbåge>// - **[39eb6d176](facebook/react@39eb6d176 )**: Rename ([#20134](facebook/react#20134)) //<Sebastian Markbåge>// - **[ffd842335](facebook/react@ffd842335 )**: [Flight] Add support for Module References in transport protocol ([#20121](facebook/react#20121)) //<Sebastian Markbåge>// - **[343d7a4a7](facebook/react@343d7a4a7 )**: Fast Refresh: Don't block DevTools commit hook ([#20129](facebook/react#20129)) //<Brian Vaughn>// - **[779a472b0](facebook/react@779a472b0 )**: Prevent inlining into recursive commit functions ([#20105](facebook/react#20105)) //<Andrew Clark>// - **[25b18d31c](facebook/react@25b18d31c )**: Traverse commit phase effects iteratively ([#20094](facebook/react#20094)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 4e5d7fa...454c221 Reviewed By: rickhanlonii Differential Revision: D24698701 fbshipit-source-id: dfaf692b1051150355dece1657764a484b7ae603
* Move traversal logic to ReactFiberCommitWork The current traversal logic is spread between ReactFiberWorkLoop and ReactFiberCommitWork, and it's a bit awkward, especially when refactoring. Idk the ideal module structure, so for now I'd rather keep it all in one file. * Traverse commit phase effects iteratively We suspect that using the JS stack to traverse through the tree in the commit phase is slower than traversing iteratively. I've kept the recursive implementation behind a flag, both so we have the option to run an experiment comparing the two, and so we can revert it easily later if needed.
Initial implementation done. It's in a working state, but the diff is large so I want to do a tidying pass and go through it again line-by-line. Maybe remove some duplication.
Would appreciate a thorough review. It's a lot of copy-paste-tweak, so it's likely I missed something.
Description
We suspect that using the JS stack to traverse through the tree in the commit phase is slower than traversing iteratively. This changes the commit phase to use while loops and the fiber
returnpointer, instead of recursion.There are a few places in the commit phase where we'd like to read data from the stack, e.g. to check if we're inside a hidden tree. In the render phase, we maintain a virtual "stack" on the heap. We could do that in the commit phase, too, but since the commit phase is synchronous, and most nodes do not need access to the stack, I've chosen to continue using the JS stack (recursion) whenever we hit a node that "provides" data to children. This is pretty rare, though, so it shouldn't have too much of an impact; we were already using this hybrid recursive-iterative strategy in
hideOrUnhideAllChildren.I've kept the recursive implementation behind a flag, both so we have the option to run an experiment comparing the two, and so we can revert it easily later if needed.
To Do