Stats: use treeSelect for stats-list item's selectors #21507
Conversation
|
Clear win, but we should also track memory usage too and make sure nothing crazy happens with it. |
|
yep, this one in particular is worst case actually. many of the selectors are grabbing primitives |
| const serializedQuery = getSerializedStatsQuery( query ); | ||
| return get( state.stats.lists.items, [ siteId, statType, serializedQuery ], null ); | ||
| } | ||
|
|
||
| function withSerializedQuery( selector, argNum ) { | ||
| return ( ...args ) => { | ||
| const newArgs = [ ...args ]; |
There was a problem hiding this comment.
You don't need to clone the args array here. You can directly mutate it.
By the way, what if the selectors required an already serialized query passed as an argument? That would avoid transforming the argument with this somewhat tricky function. And in the case of PostTrends, its mapStateToProps would do the serialization only once, not three times.
There was a problem hiding this comment.
what if the selectors required an already serialized query
Definitely what i was thinking too! This PR is not in anything close to a mergeable state -- its full of hacks and lacks any polish. It was just a quick test to see if I could address the rerender issue by modifying the selectors + using treeSelect.
| streakData: getSiteStatsForQuery( state, siteId, 'statsStreak', query ), | ||
| } ), | ||
| ( state, siteId, query ) => { | ||
| const serializedQuery = getSerializedStatsQuery( query ); |
There was a problem hiding this comment.
Isn't the query already serialized here?
| } ), | ||
| ( state, siteId, query ) => { | ||
| const serializedQuery = getSerializedStatsQuery( query ); | ||
| return [ siteId, 'statsStreak', serializedQuery ].join(); |
There was a problem hiding this comment.
Why the extra 'statsStreak' string here? And does createCachedSelector even have a third getKey arg?
e7c726b to
14a76f1
Compare
|
This PR is ready for review now. It depends on the Read the commit messages to learn more about the changes. How to test:
Before this patch: The stats chart and the stats module boxes on After this patch: These rerenders disappeared. |
| }; | ||
|
|
||
| export default connect( mapStateToProps, null, null, { | ||
| areStatePropsEqual: compareProps( { deep: [ 'query' ] } ), |
There was a problem hiding this comment.
very cool -- it keeps the ugliness out of the component
| export const getSiteStatsMaxPostsByDay = treeSelect( | ||
| ( state, siteId, query ) => [ getSiteStatsPostStreakData( state, siteId, query ) ], | ||
| ( [ postStreakData ] ) => | ||
| reduce( postStreakData, ( max, count ) => ( count > max ? count : max ), 0 ) || null, |
| return [ siteId, 'statsStreakMax', serializedQuery ].join(); | ||
| export const getSiteStatsTotalPostsForStreakQuery = treeSelect( | ||
| ( state, siteId, query ) => [ getSiteStatsPostStreakData( state, siteId, query ) ], | ||
| ( [ postStreakData ] ) => reduce( postStreakData, ( sum, posts ) => sum + posts, 0 ), |
There was a problem hiding this comment.
FFTI: we could even use the sum function here
There was a problem hiding this comment.
I'm going to remove the "total posts" and "max posts" selectors in a follow-up PR, as they are not really needed. I just didn't want to make this PR more complicated. And I already finished their treeSelect implementation when I realized they can be removed :(
| getSiteStatsMaxPostsByDay.memoizedSelector.cache.clear(); | ||
| getSiteStatsTotalPostsForStreakQuery.memoizedSelector.cache.clear(); | ||
| getSiteStatsNormalizedData.memoizedSelector.cache.clear(); | ||
| getSiteStatsPostStreakData.clearCache(); |
There was a problem hiding this comment.
I think you could use jest.resetModules() here instead of clearCache.
There was a problem hiding this comment.
This is an awesome idea, but I'm afraid it requires nontrivial effort to make it work. Instead of importing all the tested selectors once at the top of the test file, we'd have to require them in each beforeEach call. Otherwise, the jest.resetModules() call doesn't have the desired effect.
It's definitely worth exploring in a standalone PR -- there is a lot of clearCache-style calls in our tests and they could be all avoided.
The previous implementations rarely memoized anything, because they busted the `createSelector` cache each time they were called with different arguments. `treeSelect` can memoize much better in situations like these.
… components These components have `query` props that are objects always created from scratch, and therefore not identical, but they are usually deep-equal. Use `compareProps` to implement proper comparison and avoid wasted rerenders.
14a76f1 to
464038e
Compare
It's only used in `PostTrends.shouldComponentUpdate` as an optimization attempt to reduce rerenders. The changes in #21507 achieve that goal better, though: by selector memoization and deep comparison of the `query` prop. The `shouldComponentUpdate` and the associated selector is no longer needed. This patch even solves a bug where the component wouldn't get rerendered when the `state.canScrollLeft` and `state.canScrollRight` flags changed, and the scrolling arrows on the "Posting Activity" chart wouldn't get the `is-active` CSS class.
It's only used in `PostTrends.shouldComponentUpdate` as an optimization attempt to reduce rerenders. The changes in #21507 achieve that goal better, though: by selector memoization and deep comparison of the `query` prop. The `shouldComponentUpdate` and the associated selector is no longer needed. This patch even solves a bug where the component wouldn't get rerendered when the `state.canScrollLeft` and `state.canScrollRight` flags changed, and the scrolling arrows on the "Posting Activity" chart wouldn't get the `is-active` CSS class.
exploring to see if treeSelect can bring a win to the stats modules.
before
after
