-
Notifications
You must be signed in to change notification settings - Fork 46.2k
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
RFC: Add scheduling profiler to DevTools #19892
RFC: Add scheduling profiler to DevTools #19892
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit bc1a8e6:
|
@@ -32,7 +32,7 @@ | |||
|
|||
"devtools_page": "main.html", | |||
|
|||
"content_security_policy": "script-src 'self' 'unsafe-eval'; object-src 'self'", | |||
"content_security_policy": "script-src 'self' 'unsafe-eval' blob:; object-src 'self'", |
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 may be a little unsafe, but it's necessary if we want to load our worker as a blob. https://bugzilla.mozilla.org/show_bug.cgi?id=1294996
As this PR added the url-loader package to 2 other packages, Yarn hoisted it to the root node_modules folder. However, because file-loader was only a dependency of the scheduling profiler package, it was located in the scheduling profiler's node_modules folder. Thus, during the build process, url-loader could not find file-loader.
Hey that's neat. I didn't realize you were interested in working on this 😄 Happy to see it though! I don't think I'm comfortable merging this new tab into the DevTools extension until the integration is further along. But I'd be happy to review and discuss things as we go. Want to make a new fork– or just pick a branch other than |
Apologies for the surprise, the idea of using the Chrome DevTools Protocol was just roasting in my head and I had to get it out 😆
This makes sense! I'm good with either. We may want to better define what we want to achieve though, so that I can work towards that and maybe hand it off to you at some point. Here's what I had in mind:
I'm also happy to drop this if this isn't something we want to work towards. |
Those goals sound good at a high level. I'll want to play with the UI a little probably, but that can be done at any point. Detecting profiling supportWe will probably want to add a new boolean prop that the reconciler injects into DevTools that lets it know if this advanced profiling mode is supported. There's some precedent here for regular profiling. The DevTools backend detects if Profiling is supported by inspecting a Fiber:
And then it notifies the DevTools frontend any time a new root is mounted: react/packages/react-devtools-shared/src/backend/renderer.js Lines 1138 to 1141 in a99bf5c
The frontend then enables or disables the Profiler record button based on whether any of the current roots can even support profiling. Even if profiling is not supported though, the "import profile" functionality still works. I think we'd want to do the same for the new profiler– but we wouldn't be able to determine Profiling support by inspecting a fiber. We'd need to explicitly pass a parameter to DevTools telling it whether the feature flag existed and was enabled: react/packages/react-reconciler/src/ReactFiberReconciler.new.js Lines 712 to 741 in a99bf5c
Unified profilesLonger term, I'd like a single profiling UI with a button that starts Profiling in both React and the browser (if supported) and then aligns the data so the two voices can cross link. That seems like a pretty big lift though, so for now it's okay if we keep them separate. |
Thanks for the pointers! I'll take a closer look at them. In the long term, would we also want to look into recording scheduling profiler data through the existing Profiler's bridge? When I last checked, Safari's exported performance profiles don't contain user timing marks, so the existing approach won't work with Safari. By the way, I have exams coming up next week so I'll likely only be able to get back to this in 1-2 weeks. |
Not sure what you mean by "through the bridge". I have been imagining these two forms of Profiling will eventually be started/stopped with the same button (although the mechanisms for start/stopping them both will be different).
Safari isn't a supported browser for a few reasons. That's fine.
No hurry. |
👍
Ah sorry. I meant using the same architecture as the Profiler, where the extension's frontend uses a bridge to the backend to transfer profiling data. Correct me if I'm wrong, but from what I can tell, the extension injects a DevTools backend script into the running app, and the backend gathers profiling data before sending it to the extension frontend when profiling stops. If that's accurate, I was wondering if it would make sense to have the backend gather scheduling profiler data along with the existing Profiler's data, so that:
But since we don't intend to support Safari, this suggestion is probably irrelevant. I'd imagine that the second goal can be achieved in an easier way (maybe by having the backend enable advanced profiling data collection at runtime when needed).
This is what I have in mind as well 👍 |
You're right that we could log this data somewhere else– either through some new React-specific API or the DevTools backend could e.g. override the native And maybe there are reasons to consider doing this in terms of aligning scheduler profiling data with the pre-existing React Profiler data, but I think supporting Safari is a non-goal. The added context of the call stack flame graph and the non-React user timing marks make the new scheduling profiler much more useful. If anything, I'd like to see us add more such context to the scheduling profiler UI (which would have to come from the browser's profiling data). |
👍 Would we want to prioritize displaying React Profiler data in the scheduling profiler, over browser profiling data? I'd imagine that React context will be more useful as that information probably can't be obtained with existing tools, e.g. with a React component flame chart like these old User Timing measures (if possible), you'll be able to tell which components are actually rendering in a particular render measure in the scheduling profiler UI. |
No, I don't think so. Although it might be helpful to also add the React component stack. 😄
You can tell that now, with the browser's call stack. It's just a lot more buried (especially if you want to know the component stack of a given component.) |
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
This PR and the repo here are really great btw. Thanks again for taking the time to put them together. I'm going to pick this work up now and try to get the new profiler integrated. Sorry it's taken so long 🙇🏼 |
Closing this in favor of ##21897. Thanks again for getting the ball rolling here though! |
Summary
This PR adds the scheduling profiler as a new tab/panel to DevTools. I'm not sure if there are better approaches though; feedback welcome!
The main goal of integrating is to allow us to add a record button to record a performance profile in a future PR. I've figured out a way to do it, and I've made a POC here: https://github.com/taneliang/react-scheduling-profiler-devtools-integration-poc.
The scheduling profiler is hidden behind a new
enableIntegratedSchedulingProfiler
DevTools feature flag. Apart from a bundle size increase (main.js
grows from 1.22 MiB to 1.77 MiB), this PR shouldn't affect any functionality in the existing DevTools if the feature flag is off.This PR is nearly complete, but I'm not sure if adding a tab is what you have in mind @bvaughn. I'm also aware that React 17 will be released soon, and I'm not sure if you'll want to wait until after that release. If this approach looks good and we want to proceed, I'll finish the remaining tasks :)
Notes
main.js
bundle size increases from 1.22 MiB to 1.77 MiB. We can trim this to 1.67 MiB if we remove theprofilerBrowser.png
asset.TODO
Future work
Test Plan
enableIntegratedSchedulingProfiler
to false and ensured scheduling profiler tab/panel does not appear.