-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add user timing marks for scheduling profiler tool #11
Conversation
Combines those defined in both DebugTracing.js and [this branch](facebook/react@master...bvaughn:root-event-marks#diff-9c522844edfceec1e53144f429f7103fR123).
* LICENSE file in the root directory of this source tree. | ||
* | ||
* @emails react-core | ||
* @jest-environment node |
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.
@jevakallio These are the steps to repro the bug we discussed on Discord:
- In
packages/shared/ReactFeatureFlags.js
, flipenableSchedulingProfiling
to true - Run
yarn test --watch SchedulingProfiling
. All tests should pass. .skip
the test on line 55 (if you don't do this, after the next step it'll pass and thus fail the@gate
check -- probably easier to debug this on another failing test without gate in the way)- Delete this
@jest-environment node
line. - The first test should fail as the
marks
array is empty (instead of being filled with the expected marks). If you.skip
it, the next first test will fail.
It may be easier to step through this with a debugger, but the cause is that during the first test that runs, supportsUserTiming
defined in packages/react-reconciler/src/SchedulingProfiling.js is false. I'm not sure why this is happening, and I'm not sure if changing the Jest env is the best solution to this.
Our beforeEach
code sets a mock global.performance
in this file. It appears that it's not getting set before that line is executed in the default Jest env (jsdom
?), but it is getting set in the node
environment. Also, it looks like SchedulingProfiling.js
is getting require
d after every test in order to re-trigger that check.
This env change was also done in this other test file that also sets global.performance
. Maybe this is the correct solution? It'll be cool if we could figure out why 😆
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.
I'm able to reproduce the issue. Will look into debugging it tomorrow, but don't hold your breath :)
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.
Took a look at this, did not cover the root cause so far.
Things I investigated and ruled out:
-
Babel transpilation hoists require calls to the top of module -
require.cache
is primed before first test by some other module
Things I haven't looked at yet:
- How jsdom and node module resolution semantics differ. Does jsdom preprocess the file before loading it?
- Ghosts
// TODO: Confirm that this makes sense | ||
!includesSomeLane(workInProgressRootRenderLanes, lane) | ||
// Original criterion: expirationTime > renderExpirationTime | ||
// Location: https://github.com/bvaughn/react/blob/root-event-marks/packages/react-reconciler/src/ReactFiberWorkLoop.js#L2846 |
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.
@jevakallio Would also appreciate if you could see if this new condition makes sense.
This code marks when a render is cancelled, and the condition replaces an expiration date-based one. I'm not too confident of my understanding of both lanes and expiration times, so this is the best I could come up with.
The marks look like they're in the correct position at least, and I don't see them in unexpected places either.
Update: the screenshots below are outdated: our bitmask is now transmitted in decimal (e.g. --render-start-512
)
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.
This will detect an interruption, but it will also have false positives.
I believe a way to make it have fewer false positives would be to check that there are actually WIP lanes, e.g.
if (
workInProgressRoot !== null &&
workInProgressRootRenderLanes !== NoLanes &&
!includesSomeLane(workInProgressRootRenderLanes, lane)
) {
// ...
}
But I think this will also still have false positives during hydration. I'm actually not sure of what to do here. Maybe we should just leave this as a "TODO" entirely and we can discuss it with Andrew on the final PR.
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.
Oh good catch. Checking for WIP lanes makes sense.
I know this doesn't really affect our profiler tool, but I'd love to understand the lanes architecture better: why would there be false positives during hydration?
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.
To be clear, I think we should remove this entirely for now. Even with the wip lanes check, I think this would report renders are being "abandoned" when they weren't. Like I said, I'm actually not sure of what to do here 😄 let's talk more about it on the final PR.
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 sounds good. I've removed markRenderAbandoned and this call in 4101fd7, and we can revert it if we figure out a way to do this. Thank you!
Added callsites for the remaining user timing marks |
…onsistency, add blank lines
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.
Everything looks good to me! Checkout the nit in my comment below.
Resolves PR review comment facebook#19223 (comment)
Resolves PR review comment facebook#19223 (comment)
Resolves PR review comment facebook#19223 (comment)
Resolves PR review comment facebook#19223 (comment)
Resolves PR review comments: 1. facebook#19223 (comment) 1. facebook#19223 (comment)
Co-authored-by: Brian Vaughn <brian.david.vaughn@gmail.com>
Resolves PR review comment facebook#19223 (comment)
High level breakdown of this commit: * Add a enableSchedulingProfiling feature flag. * Add functions that call User Timing APIs to a new SchedulingProfiler file. The file follows DebugTracing's structure. * Add user timing marks to places where DebugTracing logs. * Add user timing marks to most other places where @bvaughn's original draft DebugTracing branch marks. * Tests added * 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. Co-authored-by: Kartik Choudhary <kartik.c918@gmail.com> Co-authored-by: Kartik Choudhary <kartikc.918@gmail.com> Co-authored-by: Brian Vaughn <brian.david.vaughn@gmail.com>
Upstream PR is landed! 🎉 |
Summary
This PR adds User Timing marks at key points in the reconciler's execution. The marks will be used by a new concurrent mode profiler tool that we're building.
High level breakdown:
Resolves #5.
Test Plan
yarn test
yarn test-prod
yarn lint
yarn flow
Roadmap to completion
mark(Commit|(Layout|Passive)Effects)(Started|Stopped)
markRenderYielded
andmarkPassiveEffects*
@jest-environment node
is required forsupportsUserTiming
to be true when the first test is run (Add user timing marks for scheduling profiler tool #11 (comment))Figure out why there are 2 consecutive commit stops in cascading class component updates and cascading layout updatesMy bad, there are also 2 commit starts, so this makes sense.Internal code reviewSkippedDebugTracing
is relanded on master