Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Mar 8, 2017

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

screen shot 2017-03-08 at 1 51 30 pm

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.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Mar 8, 2017
@gaearon
Copy link
Collaborator Author

gaearon commented Mar 21, 2017

cc @bvaughn — this is for me to pick up after you do the next sync

@bvaughn
Copy link
Contributor

bvaughn commented Mar 23, 2017

@gaearon FYI landed the 16.0.0-alpha.4 sync yesterday

@hramos hramos changed the title [Not for Merge] Disable ReactPerf and Update Systrace React Integration [WIP] [Not for Merge] Disable ReactPerf and Update Systrace React Integration Aug 4, 2017
@facebook-github-bot
Copy link
Contributor

@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.

@stale
Copy link

stale bot commented Oct 13, 2017

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.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Oct 13, 2017
@stale stale bot closed this Oct 20, 2017
@sdeleon28
Copy link

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 😞

@bvaughn
Copy link
Contributor

bvaughn commented Sep 7, 2018

@sdeleon28 For what it's worth, the new React DevTools profiler plug-in will support the next React Native release.

facebook/react-devtools#1069

@sdeleon28
Copy link

@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 console.time and console.profile a try. Less than ideal, but this is my last resort - Suffocation / No breathing / Don't give a BLEEP if I cut my arm, bleeding

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. JavaScript Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants