-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[WIP] [Not for Merge] Disable ReactPerf and Update Systrace React Integration #12797
Conversation
cc @bvaughn — this is for me to pick up after you do the next sync |
@gaearon FYI landed the 16.0.0-alpha.4 sync yesterday |
@gaearon I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions. |
What's the status of this? Is there currently any way to profile JS Thread events? I'm really struggling to pinpoint performance bottlenecks with current tooling 😞 |
@sdeleon28 For what it's worth, the new React DevTools profiler plug-in will support the next React Native release. |
@bvaughn that's useful info. Thanks! Still doesn't change the fact that I have zero tools to identify the bottleneck I'm currently facing, tho :( I'm going to give |
This is a work in progress, not for merge yet.
We will likely disable ReactPerf in Fiber because it's not clear how to make it work correctly, and whether the heuristics it used even make sense in Fiber.
Instead, we are now hooking into User Timing API API if one is available in the environment, to report the React measurements. In this PR, I applied an RN sync manually (which is necessary to test this change until we land Fiber support), and implement the necessary User Timing API subset on top of Systrace in the second commit. The corresponding React PR is at facebook/react#9071.
First commit: React sync + enabling Fiber (irrelevant to this PR) 666a004
Second commit: Replace React debugging code with a User Timing polyfill for Fiber 4fb86f7
The result is more or less equivalent to the Systrace React integration we had before, although it also displays some Fiber-specific features (such as a separate commit phase). It also doesn't rely on private React APIs and is consistent with what we use for the browser, so we shouldn't be breaking this integration again in the future.
One unfortunate problem here is that there is no way to re-label Systrace measurements after they happened. In the browser, we are able to rename measurements to include warning messages (since by the time the measurement ends, we know if user did some bad pattern). However,
Systrace
can only include the label when starting the measurements, so the warnings are not exposed.