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

Open
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
@bvaughn
Collaborator

bvaughn commented May 22, 2018

View formatted RFC


Feedback is welcome!

  • Would you use these timings?
  • If not (or if so) is there something else we could add to make it more useful to you?
  • Do you have ideas for abstractions (or visualizations) we could build on top of this to make it more useful and/or beginner friendly?

Related PRs

@fbartho

This comment has been minimized.

fbartho commented May 22, 2018

I certainly would use these timings / this component in my team's app. Identifying which "screen" suffers the most would be very valuable.

Is there a model in this for profiling "time until interactive" - networkTime -- This way we can see the fully loaded data, and optimize the React Render tree directly?

Also valuable: if it could automatically bubble up which sub-component is most at fault, or the number of re-renders of subcomponents. This way we can identify data misuse that leads to repeat renders.

@tizmagik

This comment has been minimized.

tizmagik commented May 22, 2018

Would it be possible to detect/report on “useless” or unnecessary updates?

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented May 23, 2018

Thank you both for your feedback 😄

Is there a model in this for profiling "time until interactive" - networkTime -- This way we can see the fully loaded data, and optimize the React Render tree directly?

I'm sorry, but I'm not sure I understand what you're suggesting. Could you elaborate?

Also valuable: if it could automatically bubble up which sub-component is most at fault, or the number of re-renders of subcomponents. This way we can identify data misuse that leads to repeat renders.

I think this might be better handled by DevTools. (I have an idea that I hope to prototype for how the new Profiler might integrate with DevTools to power some local visualizations/graphs of an application. More to come soon...)

Something like the number of re-renders seems easier to detect and handle locally, because it's easier to reproduce / more predictable (regardless of system specs). On the other hand, timing is often impacted by hardware capabilities and browser/versions, and may be difficult for devs to get an accurate idea about from their (generally high-end) machines.

Also, timing is probably the most important "cost" of rendering. If a component re-renders unnecessarily, but it's super fast- is it actually that important? On the other hand, if it's slow each time the app renders- regardless of whether the render was "useless" or not, it should probably be looked into.

That being said, I haven't thought a lot about non timing aspects of this. Maybe I'm overlooking something. Let's see what others think about it. 😄

Would it be possible to detect/report on “useless” or unnecessary updates?

Something like maicki/why-did-you-update?

To be honest, I haven't considered this much. Initially, I think it might be tricky, because of e.g. inline functions for event handlers. (These might interact with the DOM even though it was unnecessary. Seems hard for React to detect without adding overhead of something like a per-renderer property whitelist, etc.)

Definitely seems useful. Although similar to the number of renders/re-renders, it seems more predictable / easier to reproduce locally than timing- so maybe this is something that belongs in DEV mode only, rather than a profiling build?

@gaearon

This comment has been minimized.

Member

gaearon commented May 23, 2018

I think the notion of “useless” never made a lot of sense. It was a handy heuristic in some cases but it both failed to identify some “less useful” cases and incorrectly tagged “useful” updates as wasted.

Now, I know a lot of people want to see this heuristic come back in some form. I think what we really want there is a scale rather than a yes/no flag. If a big tree was reconciled and it took a lot of time, but the result was just a couple of DOM changes, it’s still worth looking into, even though it’s not completely “wasted”. Similarly, a fast re-render of a few components that didn’t lead to DOM updates isn’t necessarily useful to act upon if it’s fast.

So I think what we want is a scale of how much the update cost (in time) vs how much it affected the output (host effects, lifecycles). And we’d like to see the updates (as setState calls?) that have the worst ratio on that scale.

I don’t know if it needs to be a part of this RFC. But that’s how I see “wasted” heuristic in the future if we add it again.

Does that make sense?

@paularmstrong

This comment has been minimized.

paularmstrong commented May 23, 2018

This looks really great!

Would you use these timings?

Absolutely yes. We have a timing HOC at Twitter and it's not going to work once React hits v17.0.0 and deprecates some things. (it's rudimentary, but it gets us enough information to alert on critical regressions)

As for this particular RFC:

id attribute: I'm a bit unclear on this, as the RFC isn't specific...

Does this have to be unique? If I were to use the same id on many instances, e.g. in a timeline of Tweets, <Profiler id="Tweet" onRender={this._handleProfilerRender}><Tweet /></Profiler>, would _handleProfilerRender be called for each, or once per render cycle for all of those with the same id?

The way it looks with the definition for baseTime is that I will _handleProfilerRender will be called only once with the most recent time it was called.

@fbartho

This comment has been minimized.

fbartho commented May 23, 2018

@bvaughn in reply to your response: yes I totally agree that some of my requests might live better in the DevTools. -- I'm also considering this from the concept of a React-Native platform as my first target, I'm not sure that redirects your comments, but maybe that gives you more context?

In regards to your question re my comment about "time-till-interactive" I'm trying to optimize for performance when many of the components are embedded in GraphQL Query components from Apollo. It's want to have more insight into how long it takes for my UI to "settle" which is a time-diff between componentDidMount and the last re-render after data comes back. I admit I'm a bit confused as to how to best profile & track which parts of my component tree need love.

@alvaropinot

This comment has been minimized.

alvaropinot commented May 23, 2018

Thanks for sharing, I will definitely use it.

I can think about the following ideas:

  • Add a generic implementation for the onRender method that might provide a mechanism for beginners to see perf results with 0 setup. Maybe a separate package can do this. This could be the seed of a profile reporter that could be integrated within react dev tools.
  • Consider toggling the component behaviour on/off dependending on a prop. This might allow disabling it in prod or disabling it when devtools are not open, or within any scenario where the performance impact of profiling might be negative.
  • It can be specially interesting for wrapping components in a performance/TDD approach where min metrics should be fullfilled to obtain a passing perf test.

Again thanks for sharing, hope those 👆help 😁

@miraage

This comment has been minimized.

miraage commented May 23, 2018

Opinion: it would be amazing to have an option to toggle <Profiler> via React DevTools or a browser extension.

I think it might be useful to avoid using "hacks" (e.g. global variables, global functions) in production.

E.g. you push potential bottlenecks wrapped into a disabled <Profiler> (so your site's users see no difference), while you can do your measurements by your own.

// EDIT

Oh, basically I described 2nd idea from @alvaropinot.
Next time I'll read comments first. :)

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented May 23, 2018

@paularmstrong Thanks!

id attribute: I'm a bit unclear on this, as the RFC isn't specific...
Does this have to be unique? If I were to use the same id on many instances...would _handleProfilerRender be called for each, or once per render cycle for all of those with the same id?

Callbacks will always be called once per Profiler (assuming that Profiler has at least one child that rendered).

The ID is for your benefit only. If you are using a single, centralized logging function- the ID can help identify which Profiler (which tree) it is referring to. It does not have to be unique. React doesn't use it or check it in any way, just passes it through.

The way it looks with the definition for baseTime is that I will _handleProfilerRender will be called only once with the most recent time it was called.

I'm not sure what you're saying. Clarify?


@fbartho

yes I totally agree that some of my requests might live better in the DevTools. -- I'm also considering this from the concept of a React-Native platform as my first target, I'm not sure that redirects your comments, but maybe that gives you more context?

React Native support is a central focus for this new component, and will also be a central focus for the DevTools work I'll be doing soon. We realize that RN is currently a bit harder to profile than React DOM.

In regards to your question re my comment about "time-till-interactive" I'm trying to optimize for performance when many of the components are embedded in GraphQL Query components from Apollo. It's want to have more insight into how long it takes for my UI to "settle" which is a time-diff between componentDidMount and the last re-render after data comes back.

Seems like you should be able to infer this based on when your callback is called? The first time it is called for a tree, the phase will be "mount" (so that's your starting point) and then subsequent calls (while it "settles"m as you put it) will have a phase of "update". You can use the commitTime of these calls to compute your own delta.


@alvaropinot

Add a generic implementation for the onRender method that might provide a mechanism for beginners to see perf results with 0 setup.

This is where React DevTools fits in! Can't wait to show you what Sebastian and I have been brainstorming here. 😄

Consider toggling the component behaviour on/off dependending on a prop. This might allow disabling it in prod or disabling it when devtools are not open, or within any scenario where the performance impact of profiling might be negative.

You could do this via a HOC (if you wanted). The way we intend for this to work by default though will be: ON for development and profiling bundles, OFF for production. (It doesn't really matter if it's always on for dev mode, since the perf impact is minimal. You can switch which production bundle of React is used- regular or profiling- to turn it off for production mode.)


@miraage

Opinion: it would be amazing to have an option to toggle via React DevTools or a browser extension.

Yes! This is planned! 😁 Will share something soon.

@jamesreggio

This comment has been minimized.

jamesreggio commented May 23, 2018

This is super exciting — especially that React Native support will be included from the start.

I have a couple questions/suggestions:

  1. Just to confirm, an update to any descendent of a Profiler component (not just its immediate descendants) will trigger the onRender callback, correct?

  2. It would be nice to be able to correlate onRender callbacks on separate Profiler components that fire during the same rendering transaction. For example, if a Redux dispatch causes an update in two disjoint subtrees, each with a separate Profiler, I'd like to be able to correlate the two onRender callbacks to the dispatch. Perhaps you could include an auto-incrementing transaction ID in the callback data?

  3. How does this behave if a subtree that contains a Profiler gets jettisoned during the dispatching of an exception? Suspense is going to make this pattern more common, and I may still want to measure the amount of time spent on an update phase that ultimately results in the unmounting of a subtree. The example I'm thinking of looks something like this:

<Timeout>
 {didTimeout => didTimeout ? <Spinner /> : (
   <Lots of other components on this screen>
     <Profiler>
       <AsyncResource />
     </Profiler>
   </Others>
 )}
</Timeout>

In this case, a cache miss on AsyncResource is going to cause everything under Timeout to unmount, including the Profiler. It'd still be interesting to be able to profile the cost of the mount/update phase that led to the immediate unmounting.

(I'm not an expert on Suspense, so please forgive me if this use case is unclear. I'd be happy to clarify.)

baseTime: number,
startTime: number,
commitTime: number
): void {

This comment has been minimized.

@j-f1

j-f1 May 23, 2018

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

This comment has been minimized.

@bvaughn

bvaughn May 23, 2018

Collaborator

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.

This comment has been minimized.

@jamesreggio

jamesreggio May 23, 2018

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.

This comment has been minimized.

@gaearon

gaearon May 23, 2018

Member

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.

This comment has been minimized.

@bvaughn

bvaughn May 23, 2018

Collaborator

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.

This comment has been minimized.

@bvaughn

bvaughn May 23, 2018

Collaborator

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.

This comment has been minimized.

@alvaropinot

alvaropinot Jun 11, 2018

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)

This comment has been minimized.

@jamesreggio

jamesreggio Jun 15, 2018

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

This comment has been minimized.

@bvaughn

bvaughn Jun 15, 2018

Collaborator

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.

This comment has been minimized.

@bvaughn

bvaughn Jun 20, 2018

Collaborator

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.

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented May 23, 2018

@jamesreggio

Just to confirm, an update to any descendent of a Profiler component (not just its immediate descendants) will trigger the onRender callback, correct?

Correct.

It would be nice to be able to correlate onRender callbacks on separate Profiler components that fire during the same rendering transaction. For example, if a Redux dispatch causes an update in two disjoint subtrees, each with a separate Profiler, I'd like to be able to correlate the two onRender callbacks to the dispatch.

The commitTime parameter can be used for this.

How does this behave if a subtree that contains a Profiler gets jettisoned during the dispatching of an exception?

In the scenario you describe, since the components underneath of the Timeout component would not be committed, the Profiler inside of it would not be called. You could measure the "cost" of this by wrapping a Profiler around the Timeout component though, I believe.

bors bot added a commit to mythmon/corsica-tree-status that referenced this pull request May 24, 2018

Merge #20
20: Update react monorepo to v16.4.0 r=renovate[bot] a=renovate[bot]

This Pull Request renovates the package group "react monorepo".


-   [react-dom](https://github.com/facebook/react) (`dependencies`): from `16.3.2` to `16.4.0`
-   [react](https://github.com/facebook/react) (`dependencies`): from `16.3.2` to `16.4.0`

# Release Notes
<details>
<summary>facebook/react</summary>

### [`v16.4.0`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1640-May-23-2018)
[Compare Source](facebook/react@8e5f12c...v16.4.0)
##### React

* Add a new [experimental](`reactjs/rfcs#51) `React.unstable_Profiler` component for measuring performance. ([@&#8203;bvaughn] in [#&#8203;12745](`facebook/react#12745))
##### React DOM

* Add support for the Pointer Events specification. ([@&#8203;philipp-spiess] in [#&#8203;12507](`facebook/react#12507))
* Properly call `getDerivedStateFromProps()` regardless of the reason for re-rendering. ([@&#8203;acdlite] in [#&#8203;12600](`facebook/react#12600) and [#&#8203;12802](`facebook/react#12802))
* Fix a bug that prevented context propagation in some cases. ([@&#8203;gaearon] in [#&#8203;12708](`facebook/react#12708))
* Fix re-rendering of components using `forwardRef()` on a deeper `setState()`. ([@&#8203;gaearon] in [#&#8203;12690](`facebook/react#12690))
* Fix some attributes incorrectly getting removed from custom element nodes. ([@&#8203;airamrguez] in [#&#8203;12702](`facebook/react#12702))
* Fix context providers to not bail out on children if there's a legacy context provider above. ([@&#8203;gaearon] in [#&#8203;12586](`facebook/react#12586))
* Add the ability to specify `propTypes` on a context provider component. ([@&#8203;nicolevy] in [#&#8203;12658](`facebook/react#12658))
* Fix a false positive warning when using `react-lifecycles-compat` in `<StrictMode>`. ([@&#8203;bvaughn] in [#&#8203;12644](`facebook/react#12644))
* Warn when the `forwardRef()` render function has `propTypes` or `defaultProps`. ([@&#8203;bvaughn] in [#&#8203;12644](`facebook/react#12644))
* Improve how `forwardRef()` and context consumers are displayed in the component stack. ([@&#8203;sophiebits] in [#&#8203;12777](`facebook/react#12777))
* Change internal event names. This can break third-party packages that rely on React internals in unsupported ways. ([@&#8203;philipp-spiess] in [#&#8203;12629](`facebook/react#12629))
##### React Test Renderer

* Fix the `getDerivedStateFromProps()` support to match the new React DOM behavior. ([@&#8203;koba04] in [#&#8203;12676](`facebook/react#12676))
* Fix a `testInstance.parent` crash when the parent is a fragment or another special node. ([@&#8203;gaearon] in [#&#8203;12813](`facebook/react#12813))
* `forwardRef()` components are now discoverable by the test renderer traversal methods. ([@&#8203;gaearon] in [#&#8203;12725](`facebook/react#12725))
* Shallow renderer now ignores `setState()` updaters that return `null` or `undefined`. ([@&#8203;koba04] in [#&#8203;12756](`facebook/react#12756))
##### React ART

* Fix reading context provided from the tree managed by React DOM. ([@&#8203;acdlite] in [#&#8203;12779](`facebook/react#12779))
##### React Call Return (Experimental)

* This experiment was deleted because it was affecting the bundle size and the API wasn't good enough. It's likely to come back in the future in some other form. ([@&#8203;gaearon] in [#&#8203;12820](`facebook/react#12820))
##### React Reconciler (Experimental)

* The [new host config shape](https://github.com/facebook/react/blob/c601f7a64640290af85c9f0e33c78480656b46bc/packages/react-noop-renderer/src/createReactNoop.js#L82-L285) is flat and doesn't use nested objects. ([@&#8203;gaearon] in [#&#8203;12792](`facebook/react#12792))

---


</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>

bors bot added a commit to mozilla/delivery-console that referenced this pull request May 24, 2018

Merge #164
164: Update react monorepo to v16.4.0 r=rehandalal a=renovate[bot]

This Pull Request renovates the package group "react monorepo".


-   [react-dom](https://github.com/facebook/react) (`dependencies`): from `16.3.2` to `16.4.0`
-   [react](https://github.com/facebook/react) (`dependencies`): from `16.3.2` to `16.4.0`

# Release Notes
<details>
<summary>facebook/react</summary>

### [`v16.4.0`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1640-May-23-2018)
[Compare Source](facebook/react@8e5f12c...v16.4.0)
##### React

* Add a new [experimental](`reactjs/rfcs#51) `React.unstable_Profiler` component for measuring performance. ([@&#8203;bvaughn] in [#&#8203;12745](`facebook/react#12745))
##### React DOM

* Add support for the Pointer Events specification. ([@&#8203;philipp-spiess] in [#&#8203;12507](`facebook/react#12507))
* Properly call `getDerivedStateFromProps()` regardless of the reason for re-rendering. ([@&#8203;acdlite] in [#&#8203;12600](`facebook/react#12600) and [#&#8203;12802](`facebook/react#12802))
* Fix a bug that prevented context propagation in some cases. ([@&#8203;gaearon] in [#&#8203;12708](`facebook/react#12708))
* Fix re-rendering of components using `forwardRef()` on a deeper `setState()`. ([@&#8203;gaearon] in [#&#8203;12690](`facebook/react#12690))
* Fix some attributes incorrectly getting removed from custom element nodes. ([@&#8203;airamrguez] in [#&#8203;12702](`facebook/react#12702))
* Fix context providers to not bail out on children if there's a legacy context provider above. ([@&#8203;gaearon] in [#&#8203;12586](`facebook/react#12586))
* Add the ability to specify `propTypes` on a context provider component. ([@&#8203;nicolevy] in [#&#8203;12658](`facebook/react#12658))
* Fix a false positive warning when using `react-lifecycles-compat` in `<StrictMode>`. ([@&#8203;bvaughn] in [#&#8203;12644](`facebook/react#12644))
* Warn when the `forwardRef()` render function has `propTypes` or `defaultProps`. ([@&#8203;bvaughn] in [#&#8203;12644](`facebook/react#12644))
* Improve how `forwardRef()` and context consumers are displayed in the component stack. ([@&#8203;sophiebits] in [#&#8203;12777](`facebook/react#12777))
* Change internal event names. This can break third-party packages that rely on React internals in unsupported ways. ([@&#8203;philipp-spiess] in [#&#8203;12629](`facebook/react#12629))
##### React Test Renderer

* Fix the `getDerivedStateFromProps()` support to match the new React DOM behavior. ([@&#8203;koba04] in [#&#8203;12676](`facebook/react#12676))
* Fix a `testInstance.parent` crash when the parent is a fragment or another special node. ([@&#8203;gaearon] in [#&#8203;12813](`facebook/react#12813))
* `forwardRef()` components are now discoverable by the test renderer traversal methods. ([@&#8203;gaearon] in [#&#8203;12725](`facebook/react#12725))
* Shallow renderer now ignores `setState()` updaters that return `null` or `undefined`. ([@&#8203;koba04] in [#&#8203;12756](`facebook/react#12756))
##### React ART

* Fix reading context provided from the tree managed by React DOM. ([@&#8203;acdlite] in [#&#8203;12779](`facebook/react#12779))
##### React Call Return (Experimental)

* This experiment was deleted because it was affecting the bundle size and the API wasn't good enough. It's likely to come back in the future in some other form. ([@&#8203;gaearon] in [#&#8203;12820](`facebook/react#12820))
##### React Reconciler (Experimental)

* The [new host config shape](https://github.com/facebook/react/blob/c601f7a64640290af85c9f0e33c78480656b46bc/packages/react-noop-renderer/src/createReactNoop.js#L82-L285) is flat and doesn't use nested objects. ([@&#8203;gaearon] in [#&#8203;12792](`facebook/react#12792))

---


</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>

@kobayang kobayang referenced this pull request May 24, 2018

Closed

frontend night #4 #4

@jpnelson

This comment has been minimized.

jpnelson commented Jul 4, 2018

@bvaughn Nice work! I came up with a pretty similar proposal for a slightly different use case: https://github.com/jpnelson/react-component-timing/blob/master/README.md

The primary differences between what I would like and what is in this RFC are:

  1. I would like to care a little bit about the definition of a "meaningful" state for the app, by examining props and determining the "loading" state (similar to #51 (comment))
  2. I would like to have this data in runtime in production, to be a metric used for tools like new relic, google analytics and the like

So, my questions are

  1. Do you imagine this as a low level component so that what I would want could be built on top of this? I would think this is the best approach, I don't think the Profiler should worry as much about the semantics of meaningfulness, only low level rendering times that user space libraries couldn't.

  2. Would it be bad (or even possible?) to use this in a production environment, to report to third party tools? I am worried about the performance overhead. This could be fixed somewhat by sampling rates, but it's still not great. You mention that the User timing API has some overhead, I'm yet to test it but I'd be concerned if it slowed things down considerably for real users. If it does, I'd have to investigate cheaper ways of measuring timing of things.

I'm yet to try out unstable_Profiler so maybe some of my questions will be answered by doing that, but just a heads up about what I'm thinking.

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Jul 5, 2018

  1. Do you imagine this as a low level component so that what I would want could be built on top of this? I would think this is the best approach, I don't think the Profiler should worry as much about the semantics of meaningfulness, only low level rendering times that user space libraries couldn't.

I'm not entirely sure what you have in mind, but yes you could build abstractions on top of the Profiler API.

Maybe you could create a proof of concept to see if the proposed API is adequate?

  1. Would it be bad (or even possible?) to use this in a production environment, to report to third party tools? I am worried about the performance overhead. This could be fixed somewhat by sampling rates, but it's still not great. You mention that the User timing API has some overhead, I'm yet to test it but I'd be concerned if it slowed things down considerably for real users. If it does, I'd have to investigate cheaper ways of measuring timing of things.

The description mentions some related PRs, including facebook/react/pull/12886 which adds a production + profiling bundle for ReactDOM and ReactNative renderers. This build even shipped with react-dom@16.4.1 and react-native@0.56.0 if you'd like to play around with it. (You could use e.g. Webpack alias to remap "react-dom" imports to "react-dom/cjs/react-dom.profiling.min.js".)

The overhead added by calling performance.now() and accumulating times should be minimal, but it is still added overhead.

I also think you're right that you would want to use some kind of sampling or aggregation when saving this data for real users. You wouldn't want to try saving all timings. 😄

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Aug 3, 2018

Hello everyone! 👋 It's been a few weeks since I've given an update on this RFC, but I just pushed a few changes that tie in a new proposed interaction-tracking package and how I envision it integrating with the Profiler API.

Related PRs for anyone interested in digging in:

I'd like to also share that I've made progress on the DevTools profiler plug-in which you can see an early version of here. In case anyone would like to dig in deeper with that, here's the PR:

@jrnail23

This comment has been minimized.

jrnail23 commented Sep 5, 2018

I just wanted to chime in and say that one of my key points for React performance tools is that some of our key performance optimization work targets somewhat older browser versions, as some of our most important customers have IT-controlled workstations that tend to lag behind quite a bit.

In particular, we have performance issues specific to Firefox v52 (yep, mandated by one of those big companies), but since FF52 doesn't support PerformanceObserver, profiling has proven to be a real pain.

So I guess I'm generally advocating for fallback support or customizable measurement mechanisms instead of relying exclusively on newer (non-universal) browser APIs. This concern may apply more directly to some of your other performance/profiling features, but this looked like as good a place as any to raise it. 😄

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Sep 5, 2018

So I guess I'm generally advocating for fallback support or customizable measurement mechanisms instead of relying exclusively on newer (non-universal) browser APIs.

I don't understand what you mean with regard to this proposal. The profiler API does not use the PerformanceObserver API. It uses performance.now() if available and falls back to Date.now() otherwise– just as React's scheduler does.

@jrnail23

This comment has been minimized.

jrnail23 commented Sep 5, 2018

@bvaughn, if that's the case, then great! I may be getting my tools mixed up here -- sorry for any misunderstanding.

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Sep 5, 2018

No worries 😄

@lixiong-intelex

This comment has been minimized.

lixiong-intelex commented Sep 7, 2018

Hi @bvaughn , a dumb question, will I be able to use the new Profile feature after I upgrade React to 16.5.0 and React Dev Tools to 3.3.2?

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Sep 7, 2018

Yes, the profiler plugin will work with 16.5– assuming you're either running in dev mode or using the profiling build (e.g. import ReactDOM from 'react-dom/profiling')

@jrnail23

This comment has been minimized.

jrnail23 commented Sep 7, 2018

@lixiong-intelex, I also had to update react-dom to v16.5.0 to get the profiling tab to show.

@lixiong-intelex

This comment has been minimized.

lixiong-intelex commented Sep 7, 2018

@jrnail23 that's what I found out as well. Thanks!

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Sep 7, 2018

Ah, I see the confusion. react and react-dom should always use the same versions. When you asked about upgrading "react" I assumed you meant both. :)

@sw-yx

This comment has been minimized.

sw-yx commented Oct 6, 2018

is this rfc settled?

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Oct 6, 2018

Probably. We'll likely close it in the next couple of weeks, when we remove the "unstable_" prefix.

Why do you ask?

@sw-yx

This comment has been minimized.

sw-yx commented Oct 6, 2018

was just browsing rfc's and saw this was still open thats all. thought it was stale. congrats on shipping!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment