-
Notifications
You must be signed in to change notification settings - Fork 50k
Measure time between scheduling an async callback and flushing it #11506
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failing some tests on CI + probably need to update prettier.
Typo: https://github.com/acdlite/react/blob/3888bbc15a73d67f0a1a0def041942bc1e248e8b/packages/react-reconciler/src/ReactFiberScheduler.js#L1434 (deadlne) :P
| } | ||
|
|
||
| export function startRequestCallbackTimer(): void { | ||
| if (enableUserTimingAPI && !isWaitingForCallback) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep this nested? I want to keep all top level conditions uniform so it's obvious they all have to include the enableUserTimingAPI flag check.
| exports[`ReactDebugFiberPerf captures all lifecycles 1`] = ` | ||
| "// Mount | ||
| "⚛ (Waiting for async callback...) | ||
| // Mount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments are a bit confusing now. They were meant to be attached to the phases below. Any way we can avoid nesting them?
3888bbc to
5da6b4d
Compare
| export function stopRequestCallbackTimer(didExpire: boolean): void { | ||
| if (enableUserTimingAPI) { | ||
| isWaitingForCallback = false; | ||
| const warning = didExpire ? 'Async work expired' : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not very obvious to me what this means. Is there a way to phrase it in a more user-accessible way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's think of something better. I was trying to keep it concise. What it means is:
"An async update starved because the main thread was blocked"
5da6b4d to
240b858
Compare
Helps detect starvation issues.
1ccf6b7 to
c0a62af
Compare
|
@gaearon What if I changed it to say "Warning: Main thread is blocked"? Is that clearer? |
| `${' '.repeat(activeMeasure.indent + 1)}// ${comment}`, | ||
| ); | ||
| comments.push(comment); | ||
| // activeMeasure.children.push( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accept to unblock but let's make the message clearer and fix CI
Most users won't know what "expires" means
c0a62af to
f475a52
Compare
…cebook#11506) * Measure time between scheduling an async callback and flushing it Helps detect starvation issues. * Debug comments should print directly above the next measure * Better warning message Most users won't know what "expires" means
Helps detect starvation issues.