Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd user timing marks for scheduling profiler tool #19223
Conversation
Combines those defined in both DebugTracing.js and [this branch](master...bvaughn:root-event-marksdiff-9c522844edfceec1e53144f429f7103fR123).
sizebot
commented
Jul 1, 2020
•
Details of bundled changes.Comparing: b85b476...b766b17 react-art
react-dom
Size changes (stable) |
sizebot
commented
Jul 1, 2020
•
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) || ''; |
This comment has been minimized.
This comment has been minimized.
gaearon
Jul 1, 2020
Member
Any concern that this is a bit expensive? I guess we only do that for suspended components.
This comment has been minimized.
This comment has been minimized.
|
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. |
...ages/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. |
This comment has been minimized.
This comment has been minimized.
bvaughn
Jul 1, 2020
Contributor
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.
This comment has been minimized.
This comment has been minimized.
gaearon
Jul 1, 2020
Member
Btw we have something called fiber._debugID. Maybe we can repurpose this?
This comment has been minimized.
This comment has been minimized.
bvaughn
Jul 1, 2020
•
Contributor
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 comment #19223 (comment)
Resolves PR review comment #19223 (comment)
Resolves PR review comment #19223 (comment)
Resolves PR review comment #19223 (comment)
Co-authored-by: Brian Vaughn <brian.david.vaughn@gmail.com>
Resolves PR review comment #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); | |||
This comment has been minimized.
This comment has been minimized.
bvaughn
Jul 8, 2020
Contributor
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 |
bvaughn
merged commit 40cddfe
into
facebook:master
|
@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! |
taneliang commentedJul 1, 2020
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.