Track which fibers scheduled the current render work #15658
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
ReactDOM: size: 0.0%, gzip: 🔺+0.1% Details of bundled changes.Comparing: 50b50c2...91b3678 react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
Generated by 🚫 dangerJS |
bvaughn
force-pushed
the
scheduler-tracking
branch
from
523f00d to
fa0d5b1
Compare
bvaughn
changed the title
bvaughn
force-pushed
the
scheduler-tracking
branch
from
813348d to
6145677
Compare
bvaughn
force-pushed
the
scheduler-tracking
branch
2 times, most recently
from
efc6f2e to
f0562c7
Compare
|
I think there's a cycle here in some cases where flushing pending updates can cause the updater to get lost. I can reliably reproduce it in a small test app (below). If I type a couple of characters, I'll get a commit with no updaters. function Input() {
const [text, setText] = useState('');
const handleChange = event => setText(event.currentTarget.value);
// To reproduce the bug, type (more than once) into the input below.
return <input type="text" value={text} onChange={handleChange} />;
}
// The bug doesn't occur in a sync root:
// render(<Input />, document.getElementById('root'));
const root = createRoot(document.getElementById('root'));
root.render(<Input />);Haven't been able to catch it in a test yet, because the flushing behavior is different. I think I have a fix for this but I would feel better about it if I could capture the broken behavior in a test first. |
|
Okay. I think I captured a variant of the bug in a new test (failing before) and fixed. |
bvaughn
force-pushed
the
scheduler-tracking
branch
from
0602449 to
91b3678
Compare
|
Rebased after #15664 landed. |
|
I'm going to defer to @sebmarkbage to review this since I think you discussed the overall approach with him already |
|
Lucky @sebmarkbage 🎉 |
|
Ping @sebmarkbage 😄 |
|
I think this PR is still worth at least considering. Leaving open for now until @sebmarkbage indicates level of interest. (If interested, I'll rebase.) |
|
This would have been a useful feature 😄 It's unfortunate that it never got reviewed. |
|
I can review it! I don't think I've seen it before. I doubt Seb sees GH mentions. :) |
🤣 At least I'm not the only one who does this! Thanks for the offer Dan! I'll clean it up sometime this week and ping you. |
|
It’s been... a while. |
|
Well fortunately, only....everything...has changed since I wrote this. Should not be a problem. |
bvaughn
changed the title
|
I'm returning this to draft status while I work on rebasing and updating the expiration times logic to lanes logic. |
bvaughn
force-pushed
the
scheduler-tracking
branch
from
91b3678 to
373acb6
Compare
bvaughn
commented
Apr 2, 2021
Comment on lines
750
to
800
| export function addFiberToLanesMap( | ||
| root: FiberRoot, | ||
| fiber: Fiber, | ||
| lanes: Lanes | Lane, | ||
| ) { | ||
| if (enableUpdaterTracking) { | ||
| if (isDevToolsPresent) { | ||
| const pendingUpdatersLaneMap = root.pendingUpdatersLaneMap; | ||
| while (lanes > 0) { | ||
| const index = laneToIndex(lanes); | ||
| const lane = 1 << index; | ||
|
|
||
| const updaters = pendingUpdatersLaneMap[index]; | ||
| updaters.add(fiber); | ||
|
|
||
| lanes &= ~lane; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| export function movePendingFibersToMemoized(root: FiberRoot, lanes: Lanes) { | ||
| if (enableUpdaterTracking) { | ||
| if (isDevToolsPresent) { | ||
| const pendingUpdatersLaneMap = root.pendingUpdatersLaneMap; | ||
| const memoizedUpdaters = root.memoizedUpdaters; | ||
| while (lanes > 0) { | ||
| const index = laneToIndex(lanes); | ||
| const lane = 1 << index; | ||
|
|
||
| const updaters = pendingUpdatersLaneMap[index]; | ||
| if (updaters.size > 0) { | ||
| updaters.forEach(fiber => { | ||
| const alternate = fiber.alternate; | ||
| if (alternate === null || !memoizedUpdaters.has(alternate)) { | ||
| memoizedUpdaters.add(fiber); | ||
| } | ||
| }); | ||
| updaters.clear(); | ||
| } | ||
|
|
||
| lanes &= ~lane; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
It seemed best to put these functions in ReactFiberLane so they had access to laneToIndex to make lanes->indices mapping easier.
bvaughn
force-pushed
the
scheduler-tracking
branch
from
373acb6 to
f892d9b
Compare
|
Comparing: 6ea7491...ff40a42 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
|
Didn't see pings on GH. I think I must have forgotten a lot of the context here. But as I read it, this is really mostly because we want to track it through retries of Suspense and Offscreen. Is that right? Because otherwise we can infer this info in the devtools when we loop over the tree after a commit. If it's just for that commit. This is similar to how the Cache has to attach itself to a Suspense boundary. I wonder if this would be more scoped if we did it that way. We could store the parents that participated in an update on an internal context and then snapshot that on the Suspense/Offscreen/Hydrating boundary. Then devtools would pick up this info in the traversal of the commit either from the updated hook or the suspense boundary that was retries. There's a subtle difference also if you create this list when something is called with setState on it vs if it participated in the render. Because things that called setState but that thing was deleted or in a hidden subtree didn't contribute to the rendering (just the cloning) of that commit. Not sure which one is better. |
sebmarkbage
reviewed
Apr 2, 2021
Yes.
Some initial concern at the idea of maintaining that over time, across React versions. Seems like the internal logic of Suspense (and Offscreen) has already changed a lot and will likely be changing more (e.g. with effects semantics) – something explicit feels more future proof. |
bvaughn
force-pushed
the
scheduler-tracking
branch
from
f892d9b to
6aeb775
Compare
bvaughn
changed the title
bvaughn
force-pushed
the
scheduler-tracking
branch
from
6aeb775 to
0c6681e
Compare
|
CI failure is unrelated to code changes:
|
bvaughn
force-pushed
the
scheduler-tracking
branch
2 times, most recently
from
cbcc3f5 to
8ef9fe9
Compare
|
Can I get a review of this please? :) |
bvaughn
force-pushed
the
scheduler-tracking
branch
from
8ef9fe9 to
8c058c7
Compare
This change is gated behind a new profiling-mode-only feature flag, "enableUpdaterTracking".
bvaughn
force-pushed
the
scheduler-tracking
branch
from
8c058c7 to
cbcbfbe
Compare
sebmarkbage
approved these changes
Apr 9, 2021
|
Much appreciated! |
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Apr 20, 2021
Summary: This sync includes the following changes: - **[f7cdc8936](facebook/react@f7cdc8936 )**: Also turn off enableSyncDefaultUpdates in RN test renderer ([#21293](facebook/react#21293)) //<Ricky>// - **[4c9eb2af1](facebook/react@4c9eb2af1 )**: Add dynamic flags to React Native ([#21291](facebook/react#21291)) //<Ricky>// - **[9eddfbf5a](facebook/react@9eddfbf5a )**: [Fizz] Two More Fixes ([#21288](facebook/react#21288)) //<Sebastian Markbåge>// - **[11b07597e](facebook/react@11b07597e )**: Fix classes ([#21283](facebook/react#21283)) //<Sebastian Markbåge>// - **[96d00b9bb](facebook/react@96d00b9bb )**: [Fizz] Random Fixes ([#21277](facebook/react#21277)) //<Sebastian Markbåge>// - **[81ef53953](facebook/react@81ef53953 )**: Always insert a dummy node with an ID into fallbacks ([#21272](facebook/react#21272)) //<Sebastian Markbåge>// - **[a4a940d7a](facebook/react@a4a940d7a )**: [Fizz] Add unsupported Portal/Scope components ([#21261](facebook/react#21261)) //<Sebastian Markbåge>// - **[f4d7a0f1e](facebook/react@f4d7a0f1e )**: Implement useOpaqueIdentifier ([#21260](facebook/react#21260)) //<Sebastian Markbåge>// - **[dde875dfb](facebook/react@dde875dfb )**: [Fizz] Implement Hooks ([#21257](facebook/react#21257)) //<Sebastian Markbåge>// - **[a597c2f5d](facebook/react@a597c2f5d )**: [Fizz] Fix reentrancy bug ([#21270](facebook/react#21270)) //<Sebastian Markbåge>// - **[15e779d92](facebook/react@15e779d92 )**: Reconciler should inject its own version into DevTools hook ([#21268](facebook/react#21268)) //<Brian Vaughn>// - **[4f76a28c9](facebook/react@4f76a28c9 )**: [Fizz] Implement New Context ([#21255](facebook/react#21255)) //<Sebastian Markbåge>// - **[82ef450e0](facebook/react@82ef450e0 )**: remove obsolete SharedArrayBuffer ESLint config ([#21259](facebook/react#21259)) //<Henry Q. Dineen>// - **[dbadfa2c3](facebook/react@dbadfa2c3 )**: [Fizz] Classes Follow Up ([#21253](facebook/react#21253)) //<Sebastian Markbåge>// - **[686b635b7](facebook/react@686b635b7 )**: Prevent reading canonical property of null ([#21242](facebook/react#21242)) //<Joshua Gross>// - **[bb88ce95a](facebook/react@bb88ce95a )**: Bugfix: Don't rely on `finishedLanes` for passive effects ([#21233](facebook/react#21233)) //<Andrew Clark>// - **[343710c92](facebook/react@343710c92 )**: [Fizz] Fragments and Iterable support ([#21228](facebook/react#21228)) //<Sebastian Markbåge>// - **[933880b45](facebook/react@933880b45 )**: Make time-slicing opt-in ([#21072](facebook/react#21072)) //<Ricky>// - **[b0407b55f](facebook/react@b0407b55f )**: Support more empty types ([#21225](facebook/react#21225)) //<Sebastian Markbåge>// - **[39713716a](facebook/react@39713716a )**: Merge isObject branches ([#21226](facebook/react#21226)) //<Sebastian Markbåge>// - **[8a4a59c72](facebook/react@8a4a59c72 )**: Remove textarea special case from child fiber ([#21222](facebook/react#21222)) //<Sebastian Markbåge>// - **[dc108b0f5](facebook/react@dc108b0f5 )**: Track which fibers scheduled the current render work ([#15658](facebook/react#15658)) //<Brian Vaughn>// - **[6ea749170](facebook/react@6ea749170 )**: Fix typo in comment ([#21214](facebook/react#21214)) //<inokawa>// - **[b38ac13f9](facebook/react@b38ac13f9 )**: DevTools: Add post-commit hook ([#21183](facebook/react#21183)) //<Brian Vaughn>// - **[b943aeba8](facebook/react@b943aeba8 )**: Fix: Passive effect updates are never sync ([#21215](facebook/react#21215)) //<Andrew Clark>// - **[d389c54d1](facebook/react@d389c54d1 )**: Offscreen: Use JS stack to track hidden/unhidden subtree state ([#21211](facebook/react#21211)) //<Brian Vaughn>// - **[c486dc1a4](facebook/react@c486dc1a4 )**: Remove unnecessary processUpdateQueue ([#21199](facebook/react#21199)) //<Sebastian Markbåge>// - **[cf45a623a](facebook/react@cf45a623a )**: [Fizz] Implement Classes ([#21200](facebook/react#21200)) //<Sebastian Markbåge>// - **[75c616554](facebook/react@75c616554 )**: Include actual type of `Profiler#id` on type mismatch ([#20306](facebook/react#20306)) //<Sebastian Silbermann>// - **[1214b302e](facebook/react@1214b302e )**: test: Fix "couldn't locate all inline snapshots" ([#21205](facebook/react#21205)) //<Sebastian Silbermann>// - **[1a02d2792](facebook/react@1a02d2792 )**: style: delete unused isHost check ([#21203](facebook/react#21203)) //<wangao>// - **[782f689ca](facebook/react@782f689ca )**: Don't double invoke getDerivedStateFromProps for module pattern ([#21193](facebook/react#21193)) //<Sebastian Markbåge>// - **[e90c76a65](facebook/react@e90c76a65 )**: Revert "Offscreen: Use JS stack to track hidden/unhidden subtree state" ([#21194](facebook/react#21194)) //<Brian Vaughn>// - **[1f8583de8](facebook/react@1f8583de8 )**: Offscreen: Use JS stack to track hidden/unhidden subtree state ([#21192](facebook/react#21192)) //<Brian Vaughn>// - **[ad6e6ec7b](facebook/react@ad6e6ec7b )**: [Fizz] Prepare Recursive Loop for More Types ([#21186](facebook/react#21186)) //<Sebastian Markbåge>// - **[172e89b4b](facebook/react@172e89b4b )**: Reland Remove redundant initial of isArray ([#21188](facebook/react#21188)) //<Sebastian Markbåge>// - **[7c1ba2b57](facebook/react@7c1ba2b57 )**: Proposed new Suspense layout effect semantics ([#21079](facebook/react#21079)) //<Brian Vaughn>// - **[316aa3686](facebook/react@316aa3686 )**: [Scheduler] Fix de-opt caused by out-of-bounds access ([#21147](facebook/react#21147)) //<Andrey Marchenko>// - **[b4f119cdf](facebook/react@b4f119cdf )**: Revert "Remove redundant initial of isArray ([#21163](facebook/react#21163))" //<Sebastian Markbage>// - **[c03197063](facebook/react@c03197063 )**: Revert "apply prettier ([#21165](facebook/react#21165))" //<Sebastian Markbage>// - **[94fd1214d](facebook/react@94fd1214d )**: apply prettier ([#21165](facebook/react#21165)) //<Behnam Mohammadi>// - **[b130a0f5c](facebook/react@b130a0f5c )**: Remove redundant initial of isArray ([#21163](facebook/react#21163)) //<Behnam Mohammadi>// - **[2c9fef32d](facebook/react@2c9fef32d )**: Remove redundant initial of hasOwnProperty ([#21134](facebook/react#21134)) //<Behnam Mohammadi>// - **[1cf9978d8](facebook/react@1cf9978d8 )**: Don't pass internals to callbacks ([#21161](facebook/react#21161)) //<Sebastian Markbåge>// - **[b9e4c10e9](facebook/react@b9e4c10e9 )**: [Fizz] Implement all the DOM attributes and special cases ([#21153](facebook/react#21153)) //<Sebastian Markbåge>// - **[f8ef4ff57](facebook/react@f8ef4ff57 )**: Flush discrete passive effects before paint ([#21150](facebook/react#21150)) //<Andrew Clark>// - **[b48b38af6](facebook/react@b48b38af6 )**: Support nesting of startTransition and flushSync (alt) ([#21149](facebook/react#21149)) //<Sebastian Markbåge>// Changelog: [General][Changed] - React Native sync for revisions c9aab1c...f7cdc89 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D27740113 fbshipit-source-id: 6e27204d78e3e16ed205170006cb97c0d6bfa957
koto
pushed a commit
to koto/react
that referenced
this pull request
Jun 15, 2021
Tracked Fibers are called "updaters" and are exposed to DevTools via a 'memoizedUpdaters' property on the ReactFiberRoot. The implementation of this feature follows a vaguely similar approach as interaction tracing, but does not require reference counting since there is no subscriptions API. This change is in support of a new DevTools Profiler feature that shows which Fiber(s) scheduled the selected commit in the Profiler. All changes have been gated behind a new feature flag, 'enableUpdaterTracking', which is enabled for Profiling builds by default. We also only track updaters when DevTools has been detected, to avoid doing unnecessary work.
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces the concept of tracking which Fiber(s) schedule work with React.
Tracked Fibers are called "updaters" and are exposed to DevTools via a
memoizedUpdatersproperty on theReactFiberRoot. The implementation of this feature follows a vaguely similar approach as interaction tracing, but does not require reference counting since there is no subscriptions API.This change is in support of a new DevTools Profiler feature that shows which Fiber(s) scheduled the selected commit in the Profiler.
All changes have been gated behind a new feature flag,
enableUpdaterTracking, which is enabled for Profiling builds by default. We also only track updaters when DevTools has been detected, to avoid doing unnecessary work.