-
Notifications
You must be signed in to change notification settings - Fork 569
Profiler RFC #51
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
Profiler RFC #51
Changes from 1 commit
317e02a
dfe022f
58afaa1
b529613
1475772
7cb55e6
3e9edf1
6062c52
b1834f9
c263445
a5c38f4
009a0b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,9 +6,11 @@ | |
|
|
||
| New React profiling component that collects timing information in order to measure the "cost" of rendering. | ||
|
|
||
| Note that an experimental release of this component is available in version 16.4 as `React.unstable_Profiler`. | ||
| 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. | ||
|
|
||
| # Basic example | ||
| 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. | ||
|
|
||
| # Usage example | ||
|
|
||
| `Profiler` can be declared anywhere within a React tree to measure the cost of rendering that portion of the tree. | ||
|
|
||
|
|
@@ -74,7 +76,8 @@ function onRenderCallback( | |
| actualDuration: number, | ||
| baseDuration: number, | ||
| startTime: number, | ||
| commitTime: number | ||
| commitTime: number, | ||
| interactions: Array<{ name: string, timestamp: number }>, | ||
| ): void { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if you use
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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)There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Unstable APIs can change. We haven't decided one way or another. This is kind of an open thread for discussion.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... | ||
| } | ||
|
|
@@ -85,15 +88,17 @@ function onRenderCallback( | |
| The `id` value of the `Profiler` tag that was measured. This value can change between renders if e.g. it is derived from `state` or `props`. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| #### `phase: "mount" | "update"` | ||
| Either the string "mount" or "update" (depending on whether this root was newly mounted or has just been updated). | ||
| 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. | ||
|
|
||
| Ideally, this time should decrease significantly after the initial mount. Althoguh 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.) | ||
| 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. This 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 no `shouldComponentUpdate` 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. | ||
|
|
@@ -103,6 +108,11 @@ Start time isn't just the commit time less the "actual" time, because in async r | |
| #### `commitTime: number` | ||
| 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). | ||
|
|
||
| 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. | ||
|
|
||
| # Drawbacks | ||
|
|
||
| Overuse of this component might negatively impact application performance. | ||
|
|
@@ -121,8 +131,7 @@ A reactjs.org blog post would be a good initial start. | |
|
|
||
| Perhaps we could provide some sort of discoverability within React DevTools. | ||
|
|
||
| # Unresolved questions | ||
| # Related proposals | ||
|
|
||
| * How will people use this? The current proposal is kind of low level and would probably benefit from some reusable abstractions being built on top of it that e.g. aggregate/batch render timings. | ||
| * Are the proposed timing metrics sufficiently useful? | ||
| * How will this feature integrate with React DevTools? I have some ideas but nothing concrete yet to share. | ||
| * [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 | ||
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.
Shape changed slightly and matches in 16.8.4 (guess this originates in the
scheduler?)Will the implementation change or is the proposal outdated?
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.
Ah, yeah. This RFC is a bit outdated.