Fix Overlapping "message" Bug in Performance Track #31528
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 4686872...5ce48b6 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
|
sebmarkbage
force-pushed
the
fixeventtime
branch
from
c6c96e9
to
50656d5
Compare
sebmarkbage
force-pushed
the
fixeventtime
branch
from
50656d5
to
6cf85c7
Compare
eps1lon
approved these changes
Nov 14, 2024
That way we can clamp a new update within the same event which otherwise would have en event time before the old render.
This is a special case which ends with no on-going renders but also doesn't commit. So we need to finalize the render so that we know that we can't. This should probably be handled more like a suspended yield instead.
… the event We do this by applying the clamp lazily and not resetting the event info between renders.
That way we can ignore this event when we log where an update happened.
sebmarkbage
force-pushed
the
fixeventtime
branch
from
30a13cf
to
d1d95a5
Compare
github-actions bot
pushed a commit
that referenced
this pull request
Nov 14, 2024
When you schedule a microtask from render or effect and then call setState (or ping) from there, the "event" is the event that React scheduled (which will be a postMessage). The event time of this new render will be before the last render finished. We usually clamp these but in this scenario the update doesn't happen while a render is happening. Causing overlapping events. Before: <img width="1229" alt="Screenshot 2024-11-12 at 11 01 30 PM" src="https://github.com/user-attachments/assets/9652cf3b-b358-453c-b295-1239cbb15952"> Therefore when we finalize a render we need to store the end of the last render so when we a new update comes in later with an event time earlier than that, we know to clamp it. There's also a special case here where when we enter the `RootDidNotComplete` or `RootSuspendedWithDelay` case we neither leave the root as in progress nor commit it. Those needs to finalize too. Really this should be modeled as a suspended track that we haven't added yet. That's the gap between "Blocked" and "message" below. After: <img width="1471" alt="Screenshot 2024-11-13 at 12 31 34 AM" src="https://github.com/user-attachments/assets/b24f994e-9055-4b10-ad29-ad9b36302ffc"> I also fixed an issue where we may log the same event name multiple times if we're rendering more than once in the same event. In this case I just leave a blank trace between the last commit and the next update. I also adding ignoring of the "message" event at all in these cases when the event is from React's scheduling itself. DiffTrain build for [c13986d](c13986d)
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.
When you schedule a microtask from render or effect and then call setState (or ping) from there, the "event" is the event that React scheduled (which will be a postMessage). The event time of this new render will be before the last render finished.
We usually clamp these but in this scenario the update doesn't happen while a render is happening. Causing overlapping events.
Before:
Therefore when we finalize a render we need to store the end of the last render so when we a new update comes in later with an event time earlier than that, we know to clamp it.
There's also a special case here where when we enter the
RootDidNotCompleteorRootSuspendedWithDelaycase we neither leave the root as in progress nor commit it. Those needs to finalize too. Really this should be modeled as a suspended track that we haven't added yet. That's the gap between "Blocked" and "message" below.After:
I also fixed an issue where we may log the same event name multiple times if we're rendering more than once in the same event. In this case I just leave a blank trace between the last commit and the next update.
I also adding ignoring of the "message" event at all in these cases when the event is from React's scheduling itself.