-
Notifications
You must be signed in to change notification settings - Fork 47.4k
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
feat[DevTools]: Use Chrome DevTools Performance extension API #29231
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Thanks for upstreaming this!
Looks good to me, the test failures are expected, because some of the test cases are based on the relative order of the markers and when .clearMarks()
is called.
I am currently blocked with working on something else, but will plan some time for fixing and merging this.
if (!startMarkName) { | ||
console.error( | ||
'endMarkAndClear was unexpectedly called without a corresponding start mark', | ||
); | ||
return; | ||
} |
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.
Note to self: check if these can be safely stored in a stack, especially with concurrent root case
Hey, sorry for the delay, I have started working on merging this. cc @and-oli |
…ed in tests (#29929) ## Summary This is the pre-requisite for #29231. Current implementation of profiling hooks is only using `performance.mark` and then makes `performance.clearMarks` call right after it to free the memory. We've been relying on this assumption in the tests that every mark is cleared by the time we check something. #29231 adds `performance.measure` calls and the `start` mark is not cleared until the corresponding `stop` one is registered, and then they are cleared together. ## How did you test this change? To test against React from source: ``` yarn test --build --project=devtools -r=experimental --ci ``` To test against React 18: ``` ./scripts/circleci/download_devtools_regression_build.js 18.0 --replaceBuild node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion 18.0 --ci ```
@and-oli Applying changes from hoxyq@bc840a5 and rebasing commits on Could you please try this? |
1f5239a
to
d07835a
Compare
@hoxyq I've updated the PR to use the latest API status and applied the changes you suggested to fix the failing tests. Tests seem to be passing now, please lmk if this looks good to you. :) |
This change is a proof of concept of how the new Chrome DevTools Performance extension API (https://bit.ly/rpp-e11y) can be used to surface React runtime data directly in the Chrome DevTools Performance panel. To do this, the hooks in profilingHooks.js that mark beginning and end of React measurements using [Performance marks](https://developer.mozilla.org/en-US/docs/Web/API/PerformanceMeasure) are modified to also use [Performance measure](https://developer.mozilla.org/en-US/docs/Web/API/PerformanceMeasure). with the `detail` field format specification of the Performance extension API. Because the marks in these hooks marks are used by React Profiler, they are kept untouched and the calls to performance.measure are added on top to surface them to the Chrome DevTools Performance panel, along with the browser's native runtime data. Because this is a proof of concept, not all the tasks and marks taken by the React Profiler are added to the Chrome DevTools Performance panel (f.e. update scheduling marks), but this could be done as a follow up of this commit. Note: to enable the user timings to be collected in the first place, the React DevTools extension needs to be installed.
This will be superseded but a new take on the timeline profiler that'll be integrated with performance.measure and replace the React DevTools one. Starting with the flag added in. #30960 |
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. |
Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you! |
Summary
Hi there React team! ⚛️ 👋
This change is a proof of concept of how the new Chrome DevTools Performance extension API (https://bit.ly/rpp-e11y) can be used to surface React runtime data directly in the Chrome DevTools Performance panel.
To do this, the hooks in profilingHooks.js that mark beginning and end of React measurements using Performance marks are modified to also use Performance measure with the
detail
field format specification of the Performance extension API.Because these marks are used by React Profiler, they are kept untouched and the calls to performance.measure are added on top to surface them to the Chrome DevTools Performance panel, along with the browser's native runtime data.
Because this is a proof of concept, not all the tasks and marks taken by the React Profiler are added to the Chrome DevTools Performance panel (f.e. update scheduling marks), but this could be done as a follow up of this commit.
Note: to enable the user timings to be collected in the first place, the React DevTools extension needs to be installed (so that the hooks are loaded to the website). In an alternative approach, the calls to the api could be added directly to the framework so that no extension needs to be installed, but this would require a more careful implementation.
Motivation
We (the Chrome Page Quality team) think allowing developers to extend the Chrome Peformance Panel can significantly offer a better experience for developers looking to improve performance to the current solutions available.
Right now, the React Profiler has its own implementation of a timeline that parses a Chromium's trace data and merges it with its own instrumentation data. This is problematic because
With the proposed API this problems could be significantly alleviated and it could potentially yield to an overall better experience for web developers.
For reference, here's at an example of a timeline recorded by React's profiler being surfaced to the Chrome DevTools Performance timeline:
React Profiler:
Chrome DevTools Performance panel extended with React runtime data:
We aim to collect feeback about the API and explore further potential use cases where the API (or an expansion of it) would offer value in the web ecosystem. The React team is a key player in this space and we would greatly appreciate it if you collaborated in this regard, so please let us know your thoughts! 🙏