Skip to content
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Udpated RFC
  • Loading branch information
Brian Vaughn committed Jul 22, 2019
commit 009a0b4d0963bc748393d17f434b07f4b0d2f769
22 changes: 13 additions & 9 deletions text/0000-profiler.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ New React profiling component that collects timing information in order to measu

This component will also integrate with the [experimental `interaction-tracking` API](https://github.com/facebook/react/pull/13234) so that tracked interactions can be correlated with the render(s) they cause. This enables the calculation of "wall time" (elapsed real time) from when e.g. a user clicks a form button until when the DOM is updated in response. It also enables long-running renders to be more easily attributed and reproduced.

Note that an experimental release of the profiler component is available in version 16.4 as `React.unstable_Profiler`. It does not yet support interactions as that package has not been released.
Note that an experimental release of the `Profiler` component was first included with version 16.4 as `React.unstable_Profiler`. It did not yet support interaction tracing as that package had not been released.

# Usage example

Expand Down Expand Up @@ -58,11 +58,11 @@ render(
);
```

Although `Profiler` is a light-weight component, it should be used only when necessary; each use adds CPU and memory overhead to an application.
Although `Profiler` is a light-weight component, it should be used only when necessary; each use adds some CPU and memory overhead to an application.

# Motivation

It is important that render timing metrics work properly with React's experimental async rendering mode. When asynchronously rendering, React may yield periodically so that an app remains responsive even on low-powered devices. This yielded time (when React is not running) should not be included when considering the "cost" of a render. This distinction is not possible to implement in user space.
It is important for render timing metrics to work properly with React's experimental concurrent rendering mode. In concurrent mode, React may yield to the browser periodically so that an app remains responsive even on low-powered devices. This yielded time (when React is not running) should not be included when considering the "cost" of a render. This distinction is not possible to implement in user space.

Timing measurements should also be significantly lighter weight than the current User Timing API so that they can be gathered in production without negatively impacting user experience. (The User Timing API is currently disabled for production because it is slow.) In addition to a faster implementation, we can further limit the impact on existing apps by creating a new production + profiling bundle. This way, apps that don't make use of the `Profiler` component (or wish to disable it globally) will not incur any additional overhead. (The `Profiler` component will render its children in production mode but its `onRender` callback will not be called.)

Expand All @@ -77,7 +77,7 @@ function onRenderCallback(
baseDuration: number,
startTime: number,
commitTime: number,
interactions: Array<{ name: string, timestamp: number }>,
interactions: Set<{ name: string, timestamp: number }>,
): void {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not pass all of this as a single object so people can pick out the keys they need?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reasonable question!

I'd say the reasons are that I'm following precedent (we don't pass named parameters anywhere else that I can think of off the top of my head) and avoiding allocating a wrapper Object during commit.

I'd be interested to hear what others think about this aspect.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd vote for an object, despite the fact that it breaks with existing React API precedent.

The order of these timing arguments is going to be tough to memorize, and I can imagine only being interested in a subset of them. Using an object also enables you to add additional timing data down the road.

I'd view it as somewhat analogous to an event object, which has a variety of keys, only some of which are of interest for any given listener.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you need to memorize it? I imagine you'd only use Profiler in a few places in the app, and each time could consult the docs.

The need to avoid allocations is pretty important because adding GC pressure can skew the profiling results.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if you use Profiler in more than one place, the callback you pass it is likely shared- (this is why the id parameter exists)- so you would only need to write these params (in the correct order) in a single place.

Copy link
Collaborator Author

@bvaughn bvaughn May 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, this concern makes sense. And I agree that we wouldn't be allocating too many new objects for this, because it would only be one per Profiler per commit. I was just sharing rationale for why it is currently the way it is.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just my personal opinion here, but an object looks good from my point of view as long functions with more that 2/3 args are always hard to remember, you tend to start adding null for the values that you might not want to use, I'm looking at you JSON.stringify(foo, null, 2) 😅 , you also need to remember the order and it's harder to refactor as you impact anyone already using that order.

Plus with the actual syntax for destructuring the function signature looks pretty much the same but with curly braces 😁, the best of two worlds!

onRenderCallback({ id, phase, actualTime, baseTime, startTime, commitTime })

vs

onRenderCallback(id, phase, actualTime, baseTime, startTime, commitTime)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the Profiling API is out in the 16.4.1 release, I assume you decided to take no action on this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unstable_Profiler component was introduced in 16.4.0. The only thing that's new in 16.4.1 is a production+profiling build.

Unstable APIs can change. We haven't decided one way or another. This is kind of an open thread for discussion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just circling back on this particular thread. Sebastian and I chatted about this yesterday, and we've decided to avoid named parameters because the overhead of the wrapper objects (however small each individual one is) will add up in larger applications.

// Aggregate or log render timings...
}
Expand All @@ -91,14 +91,14 @@ The `id` value of the `Profiler` tag that was measured. This value can change be
Identifies whether this component has just been mounted or re-rendered due to a change in `state` or `props`.

#### `actualDuration: number`
Time spent rendering the `Profiler` and its descendants for the current (most recent recent) update. This time tells us how well the subtree makes use of `shouldComponentUpdate` for memoization.
Time spent rendering the `Profiler` and its descendants for the current (most recent recent) update. This time tells us how well the subtree makes use of memoization (e.g. `React.memo`, `useMemo`, `shouldComponentUpdate`).

Ideally, this time should decrease significantly after the initial mount as many of the descendants will only need to re-render if their specific `props` change.

Note that in async mode, under certain conditions, React might render the same component more than once as part of a single commit. (In this event, the "actual" time for an update might be larger than the initial time.)

#### `baseDuration: number`
Duration of the most recent `render` time for each individual component within the `Profiler` tree. In other words, this value will only change when a component is re-rendered. It reflects a worst-case cost of rendering (e.g. the initial mount or no `shouldComponentUpdate` memoization).
Duration of the most recent `render` time for each individual component within the `Profiler` tree. In other words, this value will only change when a component is re-rendered. It reflects a worst-case cost of rendering (e.g. the initial mount or a tree with no memoization).

#### `startTime: number`
Start time identifies when a particular commit started rendering. Although insufficient to determine the cause of the render, it can at least be used to rule out certain interactions (e.g. mouse click, Flux action). This may be helpful if you are also collecting other types of interactions and trying to correlate them with renders.
Expand All @@ -109,9 +109,9 @@ Start time isn't just the commit time less the "actual" time, because in async r
Commit time could be roughly determined using e.g. `performance.now()` within the `onRender` callback, but multiple `Profiler` components would end up with slightly different times for a single commit. Instead, an explicit time is provided (shared between all `Profiler`s in the commit) enabling them to be grouped if desirable.

#### `interactions: Array<{ name: string, timestamp: number }>`
An array of interactions that were being tracked (via the `interaction-tracking` package) when this commit was initially scheduled (e.g. when `render` or `setState` were called).
Set of interactions that were being tracked (via the `interaction-tracking` package) when this commit was initially scheduled (e.g. when `render` or `setState` were called).

In the event of a cascading render (e.g. an update scheduled from `componentDidMount` or `componentDidUpdate`) React will forward these interactions along to the subsequent `onRender` calls.
In the event of a cascading render (e.g. an update scheduled from an effect or `componentDidMount`/`componentDidUpdate`) React will forward these interactions along to the subsequent `onRender` calls.

# Drawbacks

Expand All @@ -134,4 +134,8 @@ Perhaps we could provide some sort of discoverability within React DevTools.
# Related proposals

* [facebook/react/pull/13253](https://github.com/facebook/react/pull/13253): Integration with the proposed `interaction-tracking` package
* [facebook/react-devtools/pull/1069](https://github.com/facebook/react-devtools/pull/1069): Integration with React DevTools
* [facebook/react-devtools/pull/1069](https://github.com/facebook/react-devtools/pull/1069): Integration with React DevTools

# Related documentation
* [fb.me/react-profiling](http://fb.me/react-profiling): Instructions for profiling in production
* [fb.me/react-interaction-tracing](http://fb.me/react-interaction-tracing): Instructions for using interaction tracing with React