Add user timing marks for scheduling profiler tool #19223
Conversation
Combines those defined in both DebugTracing.js and [this branch](facebook/react@master...bvaughn:root-event-marks#diff-9c522844edfceec1e53144f429f7103fR123).
…onsistency, add blank lines
Details of bundled changes.Comparing: b85b476...b766b17 react-art
react-dom
Size changes (stable) |
Details of bundled changes.Comparing: b85b476...b766b17 react-art
react-dom
Size changes (experimental) |
| if (enableSchedulingProfiling) { | ||
| if (supportsUserTiming) { | ||
| const id = getWakeableID(wakeable); | ||
| const componentStack = getStackByFiberInDevAndProd(fiber) || ''; |
There was a problem hiding this comment.
Any concern that this is a bit expensive? I guess we only do that for suspended components.
There was a problem hiding this comment.
I'm wondering if we should cache stacks for Fibers we've already seen.
There was a problem hiding this comment.
Great work! 😍
I've left some thoughts and suggestions. I'm also going to tag a couple of others who may want to weigh in on this PR. 😄
Open question for @sebmarkbage: Do we still want to keep these marks (and the debug tracing feature) in the old reconciler fork only? I'm not sure if your initial concern still applies. We'll need to be careful not to wipe both features out when we later merged new->old if so.
packages/react-reconciler/src/__tests__/SchedulingProfiling-test.internal.js
Outdated
Show resolved
Hide resolved
| if (supportsUserTiming) { | ||
| const id = getWakeableID(wakeable); | ||
| const componentStack = getStackByFiberInDevAndProd(fiber) || ''; | ||
| // TODO (brian) Generate and store temporary ID so DevTools can match up a component stack later. |
There was a problem hiding this comment.
I like Dan's suggestion of caching component stacks (using a weak map) for fibers we've seen.
Let's move these TODO comments above the calls to getStackByFiberInDevAndProd since it relates to those calls.
The idea behind the "TODO" was that- once this functionality is more tightly integrated into DevTools- try to store a (weak) map of id-to-Fiber here, and then let DevTools compute the component stack asynchronously later, using the Fiber. I'm not sure if that is feasible though, especially with future plans to replace the alternate model.
There was a problem hiding this comment.
Btw we have something called fiber._debugID. Maybe we can repurpose this?
There was a problem hiding this comment.
That sounds like it could work.
I'm not sure what the implications of the alternate -> previous change would be for this idea. It was kind of a half thought out idea anyway. Just something I wanted to put some thought into later because of the cost of getting the component stack.
Component stack calculation has gotten even more expensive recently with the "native" component stacks so... I don't know how I feel about this anymore. It's definitely useful info to have, but maybe it's no longer worth the cost?
@sebmarkbage may have an opinion.
Resolves PR review comment facebook#19223 (comment)
Resolves PR review comment facebook#19223 (comment)
Resolves PR review comment facebook#19223 (comment)
|
@bvaughn all done! |
| @@ -265,6 +267,10 @@ export function updateContainer( | |||
| const suspenseConfig = requestCurrentSuspenseConfig(); | |||
| const lane = requestUpdateLane(current, suspenseConfig); | |||
|
|
|||
| if (enableSchedulingProfiler) { | |||
| markRenderScheduled(current, lane); | |||
There was a problem hiding this comment.
Sorry I missed this earlier, but doing a final review before merging and noticed the current param is still being passed (but not used). Can we remove it?
|
Looks like we also need to rebase on |
|
Not sure why off the top of my head, but the "merge master" commit seems to be causing tests to fail. Maybe you could undo that commit, and do a rebase instead? That would make it easier to read since d2c090e has a lot of random changes in it. It's weird though b'c I can't reproduce the error locally. |
|
Let me kick CI to re-run just in case. |
|
This is really strange, because I can repro this locally with |
|
Ah, good to note. Sounds like a feature flag problem them. Let me look again. |
|
Try changing all of your |
|
Nice! Great work. |
|
Thanks, that did it. Interesting though, why was CI passing in the previous commits, since the CI scripts before the last merge should have also run the tests in non-experimental release channels 🤔 |
|
@taneliang Unfortunately we've been having some flaky CI troubles the past few days. It's impacted more than just this PR |
Didn't we fix it? #19265 The failures were silent before that fix which is why you didn't see them until you updated this branch. |
|
Nice! I didn't know Dominic had already fixed that. Thanks for the pointer. |
|
Ah, that explains it, looks like it was pretty subtle. Thanks for the explanation! |
Summary
This PR adds User Timing marks at key points in the reconciler's execution.
The marks are used by this new concurrent mode profiler tool that @kartik918 and I are building under @bvaughn and @jevakallio's guidance.
High level breakdown of this PR:
enableSchedulingProfilingfeature flag.DebugTracing's structure.DebugTracinglogs.DebugTracingbranch marks.More context (and discussions with @bvaughn) available at our internal PR MLH-Fellowship#11 and issue MLH-Fellowship#5.
Similar to
DebugTracing, we've only added scheduling profiling calls to the old reconciler fork.Test Plan
yarn testyarn test-prodyarn lintyarn flow domenableSchedulingProfilingandenableDebugTracingset to true. When profiled with Firefox Profiler, the marks are visible in the output.