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

Add Support for User Timing API #972

Closed
davertron opened this issue Jul 12, 2018 · 12 comments
Closed

Add Support for User Timing API #972

davertron opened this issue Jul 12, 2018 · 12 comments

Comments

@davertron
Copy link

davertron commented Jul 12, 2018

React recently added support for the User Timing API (https://developer.mozilla.org/en-US/docs/Web/API/User_Timing_API) which is very useful for debugging performance issues. Would it be possible to implement this api inside of react-redux as well?

I was debugging an issue recently and it would have saved me a bunch of time if the react-redux layer (specifically, mapStateToProps in this case) would have shown up on the User Timing graph. You can see the specific issue here, for reference.

I don't have a ton of experience with the API, but I'd be willing to take a shot at a PR if others think this would be useful.

@timdorr
Copy link
Member

timdorr commented Jul 12, 2018

I'm not sure if there's much for us to do here. The code that would be slow in your components is provided to us. If your mSTP is slow, then you can add timing within that function. And it's usually not that the function is slow per se, more that it's being called an excessive number of times. So, it's less about timing and more about counters.

@davertron
Copy link
Author

I don't think it's so much that it's not possible to get timing without this, as obviously you can just add "console.time" statements around code that you want to time. I think this is the same for React; you can wrap any calls in your components with timing calls. But that requires that you know where to add the timing statements; if you just run a profile in the dev tools and have no idea what's slow, the user timing api helps you track down problem areas. React is using that now, but you essentially just end up seeing "componentWillReceiveProps" on your connected component being slow with no other useful information if you're using react-redux. Since I've run into this issue now, I would know in the future to check my mSTP, reselect selectors, etc., but it would be awesome if that stuff showed up explicitly in the profile.

As far as the "usually it's not that function that's slow" part, I have no data on that so I'll take your word for it. In my specific case it was definitely that a selector I had was very slow, but I have no idea how common that is.

TD;DR It's my opinion that it would improve the user experience for people using react-redux if it used the user timing api similar to how React uses it.

@cellog
Copy link
Contributor

cellog commented Aug 16, 2018

I investigated adding this, actually inside the performance benchmark #983 @markerikson is working on. It turns out that performance.measure is quite costly, and it transformed 60fps into 0.1fps when I added it just to measure the time from dispatch to render in a connected component

Even adding it strategically dramatically affected performance such that I couldn't use the app at all (60fps down to 3-4fps). So, it is possible, but I think that it is much more difficult to add this well than it appears. Perhaps this can be revisited when 6.0 pre-releases start coming out?

I wonder if it might be best to provide a system that can be opted in to add them rather than providing them by default? react-redux-profile or react-redux-debug?

@gaearon
Copy link
Contributor

gaearon commented Aug 16, 2018

You might want to look at facebook/react#13234 and other PRs @bvaughn has been sending recently. The plan is to have something generic libs and apps can tap into, that would have a very low overhead, and that could be visualized with React DevTools.

@cellog
Copy link
Contributor

cellog commented Aug 16, 2018

Mark saw that commit and mentioned it, so it's definitely on the radar, and it's a really good idea, much appreciated!

@markerikson
Copy link
Contributor

Yeah, I just had a chance to sit down and talk with @bvaughn . Definitely seems like something we can benefit from (both the new DevTools Profiler UI, and the Interaction Tracking API).

@bhovhannes
Copy link

@davertron you may find redux-profiler useful. Still cannot find time to write a medium post for it 😄

It does not measure time spend in mapStateToProps function, but renders markers for each dispatched redux action thus making clear which action triggered rerender of React component.

@markerikson
Copy link
Contributor

@bhovhannes : looks neat. Unfortunately, the changes in v6 mean the "notify listeners" part isn't going to be as useful, because most of the time there's only going to be one listener - <Provider>.

@OliverJAsh
Copy link
Contributor

Is the idea here to use React's new interaction tracing capability? I guess we would trace each re-render that happened due to a Redux store change. This way it would be clear when a component update was triggered because of a Redux store change.

If that sounds like a good plan, I'm happy to help.

@markerikson
Copy link
Contributor

That was something that crossed my mind, yes.

It would be particularly beneficial for our benchmarking capabilities ( https://github.com/reduxjs/react-redux-benchmarks ) to be able to not just measure raw page FPS when we throw thousands of components into a page with dozens of updates per second, but actually track other metrics like time to mount, time to complete an individual render cycle, breaking that render cycle down into smaller pieces to see where the delays are, etc.

If you can assist with that in any way, it would be great!

@markerikson
Copy link
Contributor

This issue would be particularly relevant to the plans I just laid out in #1177 .

@markerikson
Copy link
Contributor

FWIW, I'd still love to have a good way to time Redux+React behavior from time of dispatch until committed render, but this isn't anything we're going to get around to ourselves. Would love to see a PR, though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants