[pull] main from facebook:main #59
Merged
Conversation
This file contains hidden or 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
We currently have a failing test for React DevTools against React 17. This started failing in #30899, where we changed logic for error tracking and started relying on `onPostCommitFiberRoot` hook. Looking at #21183, `onPostCommitFiberRoot` was shipped in 18, which means that any console errors / warnings emitted in passive effects won't be recorded by React DevTools for React < 18.
Update our various compiler rollup plugins. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31919). * #31927 * #31918 * #31917 * #31916 * __->__ #31919
This flag first moves the `shouldYield()` logic into React itself. We need this for `postTask` compatibility anyway since this logic is no longer a concern of the scheduler. This means that there can also be no global `requestPaint()` that asks for painting earlier. So this is best rolled out with `enableAlwaysYieldScheduler` (and ideally `enableYieldingBeforePassive`) instead of `enableRequestPaint`. Once in React we can change the yield timing heuristics. This uses the previous 5ms for Idle work to keep everything responsive while doing background work. However, for Transitions and Retries we have seen that same thread animations (like loading states animating, or constant animations like cool Three.js stuff) can take CPU time away from the Transition that causes moving into new content to slow down. Therefore we only yield every 25ms. The purpose of this yield is not to avoid the overhead of yielding, which is very low, but rather to intentionally block any frequently occurring other main thread work like animations from starving our work. If we could we could just tell everyone else to throttle their stuff for ideal scheduling but that's not quite realistic. In other words, the purpose of this is to reduce the frame rate of animations to 30 fps and we achieve this by not yielding. We still do yield to allow the animations to not just stall. This seems like a good balance. The 5ms of Idle is because we don't really need to yield less often since the overhead is low. We keep it low to allow 120 fps animations to run if necessary and our work may not be the only work within a frame so we need to yield early enough to leave enough time left. Similarly we choose 25ms rather than say 35ms to ensure that we push long enough to guarantee to half the frame rate but low enough that there's plenty of time left for a rAF to power each animation every other frame. It's also low enough that if something else interrupts the work like a new interaction, we can still be responsive to that within 50ms or so. We also need to yield in case there's I/O work that needs to get bounced through the main thread. This flag is currently off everywhere since we have so many other scheduling flags but that means there's some urgency to roll those out fully so we can test this one. There's also some tests to update since this doesn't go through the Mock scheduler anymore for yields.
A common source of performance problems is due to cascading renders from calling `setState` in `useLayoutEffect` or `useEffect`. This marks the entry from the update to when we start the render as red and `"Cascade"` to highlight this. <img width="964" alt="Screenshot 2024-12-19 at 10 54 59 PM" src="https://github.com/user-attachments/assets/2bfa91e6-1dc1-4b7f-a659-50aaf2a97e83" /> In addition to this case, there's another case where you call `setState` multiple times in the same event causing multiple renders. This might be due to multiple `flushSync`, or spawned a microtasks from a `useLayoutEffect`. In theory it could also be from a microtask scheduled after the first `setState`. This one we can only detect if it's from an event that has a `window.event` since otherwise it's hard to know if we're still in the same event. <img width="1210" alt="Screenshot 2024-12-19 at 11 38 44 PM" src="https://github.com/user-attachments/assets/ee188bc4-8ebb-4e95-b5a5-4d724856c27d" /> I decided against making a ping in a microtask considered a cascade. Because that should ideally be using the Suspense Optimization and so wouldn't be considered multi-pass. <img width="1284" alt="Screenshot 2024-12-19 at 11 07 30 PM" src="https://github.com/user-attachments/assets/2d173750-a475-41a0-b6cf-679d15c4ca97" /> We might consider making the whole render phase and maybe commit phase red but that should maybe reserved for actual errors. The "Blocked" phase really represents the `setState` and so will have the stack trace of the first update.
…wn (#31876) This tracks commit phase errors and marks the component that errored as red. These also get the errors attached to the entry. <img width="1505" alt="Screenshot 2024-12-20 at 2 40 14 PM" src="https://github.com/user-attachments/assets/cac3ead7-a024-4e33-ab27-2e95293c4299" /> In the render phase I just mark the Error Boundary that caught the error. We don't have access to the actual error since it's locked behind closures in the update queue. We could probably expose that someway. <img width="949" alt="Screenshot 2024-12-20 at 1 49 05 PM" src="https://github.com/user-attachments/assets/3032455d-d9f2-462b-9c07-7be23663ecd3" /> Follow ups: Since the Error Boundary doesn't commit its attempted render, we don't log those. If we did then maybe we should just mark the errored component like I do for the commit phase. We could potentially walk the list of errors and log the captured fibers and just log their entries as children. We could also potentially walk the uncommitted Fiber tree by stashing it somewhere or even getting it from the alternate. This could be done on Suspense boundaries too to track failed hydrations. --------- Co-authored-by: Ricky <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.1)
Can you help keep this open source service alive? 💖 Please sponsor : )