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

Combine Profiler data sources #22529

Open
2 of 12 tasks
bvaughn opened this issue Oct 8, 2021 · 2 comments
Open
2 of 12 tasks

Combine Profiler data sources #22529

bvaughn opened this issue Oct 8, 2021 · 2 comments

Comments

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Oct 8, 2021

Time sensitive API changes (ideally these should land before 18.0 release):

  • Add DevTools API/hooks for collecting Timeline profiling data #23102
  • React (DEV and profiling builds) should call the new Timeline hooks when present instead of logging User Timing data #23102

(DevTools will decide whether or not to store the data or log User Timing marks, since it knows when it's profiling.)

Additional, non-blocking changes:

  • DevTools hook changes
    • Generate a unique session ID when profiling is started
    • Log sync marks (with session ID) periodically (e.g. during commit) when profiling is active
    • Collect component stacks for (maybe lazily? maybe cache per Fiber in a WeakMap?)
    • Mark internal module ranges (see here) only if/when profiling is started.
  • DevTools Timeline changes
    • Update pre-processing script to make use of the new React <-> DevTools data format
    • Support optional tracing data
      • Verify logic for aligning data shared between React <-> DevTools with the optional performance mark data
      • Create UI for separate, optional import (and update the preprocessor to handle this data)

Motivation

There are currently two React profilers: the "legacy" profiler (which reads data from Fibers during the commit phase) and the "scheduling" profiler (which reads data in the form of User Timing marks in a Chrome performance profile). This separation is confusing as both profilers live in the same extension/app but import/export different types of data.

Let's take a step back and revisit how the scheduling profiler works...

Recording a profile currently requires a user to do the following:

  1. Click "record" in Chrome
  2. Use the app
  3. Click "stop" in Chrome
  4. Export the profile JSON
  5. Import it into the React DevTools

It would be nice if DevTools could start/stop recording and import the data itself, but the only way to do this is using the Chrome DevTools Protocol which would require the use of an extremely powerful permission that I don't think we would want to ask users for.

This approach also has the downside of not working with React Native (or non-Chrome browsers).

So why do we do it? We do it because the profile gives us a lot of nice extra data: CPU samples of the JavaScript stack, Network requests, screenshots (if enabled), user events (e.g. "click").

But we don't need this data. The scheduling profiler could still be a useful tool even if included only the React specific marks.

My proposal then is that we consider doing this:

  • Replace the user timing API with a direct React-to-DevTools API (where these marks are logged to DevTools directly).
  • When profiling starts in React DevTools, generate a unique ID string
  • Log this unique ID to the User Profiling API along with each commit (e.g. "react-sync-marker-<uid>-<index>-<timestamp>")
  • Allow users to export/import additional native profiling information (which can be aligned using the sync marks) but is not required.

Doing this has a few benefits:

  • We could start both profilers and join the commit and scheduling profiler data streams (huge benefit).
  • It would make the profiler easier to use in the simple case (if you didn't want/need native profiler info) since you could start/stop it from within React DevTools.
  • It would let us add component and call stacks without the extra serialization overhead.

It would have the following downside though:

  • Recording profiles with native information would be a little more complicated, since you'd need to start React DevTools and then start the browser profiler. (We could show instructions for doing this in the DevTools profiler though.)

A note about component and call stacks

The scheduling profiler currently shows component names for things like state-updates (e.g. "state update scheduled by Foo") but it does not include component or call stacks. These would both be useful because:

  • Component stack would help identify which instance of foo was e.g. scheduling the update
  • Call stack would show e.g. which piece of state was being updated (and by what)

The reason we don't include this information is because we won't want to pay the cost of string serializing it all and logging it as a User Timing mark.

@tagalong420
Copy link

@tagalong420 tagalong420 commented Oct 8, 2021

@bvaughn bvaughn changed the title DevTools: Support native and component call stacks in Scheduling Profiler DevTools: Combine profiler data streams Oct 11, 2021
@bvaughn bvaughn changed the title DevTools: Combine profiler data streams Combine Profiler data sources Oct 12, 2021
@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Oct 12, 2021

Note that this issue is related to #22090.

bvaughn added a commit that referenced this issue Oct 21, 2021
…nt) (#22578)

DevTools (and its profilers) should not require users to be familiar with React internals. Although the scheduling profiler includes a CPU sample flame graph, it's there for advanced use cases and shouldn't be required to identify common performance issues.

This PR proposes adding new marks around component effects. This will enable users to identify components with slow effect create/destroy functions without requiring them to dig through the call stack. (Once #22529 lands, these new marks will also include component stacks, making them more useful still.)

For example, here's a profile with a long-running effect. Without this change, it's not clear why the effects phase takes so long. After this change, it's more clear why that the phase is longer because of a specific component.

We may consider adding similar marks around render phase hooks like useState, useReducer, useMemo. I avoided doing that in this PR because it would be a pretty pervasive change to the ReactFiberHooks file.

Note that this change should have no effect on production bundles since everything is guarded behind a profiling feature flag.

Going to tag more people than I normally would for this pR, since it touches both reconciler and DevTools packages. Feel free to ignore though if you don't have strong feelings.

Note that although this PR adds new marks to the scheduling profiler, it's done in a way that's backwards compatible for older profiles.
KamranAsif added a commit to KamranAsif/react that referenced this issue Nov 4, 2021
…nt) (facebook#22578)

DevTools (and its profilers) should not require users to be familiar with React internals. Although the scheduling profiler includes a CPU sample flame graph, it's there for advanced use cases and shouldn't be required to identify common performance issues.

This PR proposes adding new marks around component effects. This will enable users to identify components with slow effect create/destroy functions without requiring them to dig through the call stack. (Once facebook#22529 lands, these new marks will also include component stacks, making them more useful still.)

For example, here's a profile with a long-running effect. Without this change, it's not clear why the effects phase takes so long. After this change, it's more clear why that the phase is longer because of a specific component.

We may consider adding similar marks around render phase hooks like useState, useReducer, useMemo. I avoided doing that in this PR because it would be a pretty pervasive change to the ReactFiberHooks file.

Note that this change should have no effect on production bundles since everything is guarded behind a profiling feature flag.

Going to tag more people than I normally would for this pR, since it touches both reconciler and DevTools packages. Feel free to ignore though if you don't have strong feelings.

Note that although this PR adds new marks to the scheduling profiler, it's done in a way that's backwards compatible for older profiles.
@bvaughn bvaughn self-assigned this Jan 11, 2022
bvaughn added a commit that referenced this issue Jan 13, 2022
Until now, DEV and PROFILING builds of React recorded Timeline profiling data using the User Timing API. This commit changes things so that React records this data by calling methods on the DevTools hook. (For now, DevTools still records that data using the User Timing API, to match previous behavior.)

This commit is large but most of it is just moving things around:

* New methods have been added to the DevTools hook (in "backend/profilingHooks") for recording the Timeline performance events.
* Reconciler's "ReactFiberDevToolsHook" has been updated to call these new methods (when they're present).
* User Timing method calls in "SchedulingProfiler" have been moved to DevTools "backend/profilingHooks" (to match previous behavior, for now).
* The old reconciler tests, "SchedulingProfiler-test" and "SchedulingProfilerLabels-test", have been moved into DevTools "TimelineProfiler-test" to ensure behavior didn't change unexpectedly.
* Two new methods have been added to the injected renderer interface: injectProfilingHooks() and getLaneLabelMap().

Relates to #22529.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants