-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Add Support for User Timing API #972
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
Comments
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. |
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. |
I investigated adding this, actually inside the performance benchmark #983 @markerikson is working on. It turns out that 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? |
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. |
Mark saw that commit and mentioned it, so it's definitely on the radar, and it's a really good idea, much appreciated! |
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). |
@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 |
@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 - |
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. |
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! |
This issue would be particularly relevant to the plans I just laid out in #1177 . |
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! |
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.
The text was updated successfully, but these errors were encountered: