[data grid] Avoid subscribing to renderContext state in grid root for better scroll performance

[lauri865]

• I have followed (at least) the PR section of the contributing guide.

Probably fixes #15168

@MBilalShafi, started thinking about it, and I can see now why this selector hook is problematic:

mui-x/packages/x-data-grid/src/hooks/features/rows/useGridRowSpanning.ts

Line 228 in 26b1463

const renderContext = useGridSelector(apiRef, gridRenderContextSelector);

We're forcing the root of the grid to re-render every time renderContext changes (which can happen quite a lot when scrolling fast), which adds extra overhead, since all the root hooks need to re-evaluate as well. Regardless of whether rowSpanning is enabled or not. Saw that the same issue is also present in useGridDataSourceLazyLoader. We should rely on events instead of creating new state there.

Removed the top-level renderContext selector hooks for both, and slightly improved rowSpanning feature isolation.

[mui-bot]

Deploy preview: https://deploy-preview-15986--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 401f0fa

[lauri865]

Seed for thought for future refactoring:
I wonder if some of these features/plugins would be better to be loaded as separate components instead for even better isolation 🤔. E.g. core hooks should be loaded inline as it is, and features that need to be manually toggled should be loaded as empty components that are optionally mounted in order to load the feature hooks.

It seems very easy to introduce additional performance overhead with unused features currently.

Could potentially also pave ways for a nicer API for future plugin ecosystem within the community.

[lauri865]

In the latest commits I'm also proposing a slightly easier to follow logic around resetting rowSpanning state. By default every renderContext change uses existing cache, and then specific events will explicitly reset that (row updates, filtering, column changes, etc.). Currently, I was having troubles following what exactly triggers resets and how it works. Removes the need to derive logic based on refs as it's currently done – both difficult to read, but more crucially, difficult to extend (e.g. the case of adding resets based on colDef changes below).

It also fixes a few issues:

• Currently updating columns breaks rowSpanning – state updates are reactive only to renderContext/range, but not colDef changes
• backwardsHiddenCells only scans one row up, which can also lead to odd spanning

[MBilalShafi]

Thank you for the PR, subscribing to renderedRowsIntervalChange rather than binding to the store does seem a better approach.

For my tests, it improved the safari performance ~30-40%, cherry-on-top I can observe a slight (~10%) performance improvement in Chrome too.

As far as #15168 is concerned, the change certainly improved the Safari experience, we could do another benchmark on top of it if there are more areas we could improve in. However, that shouldn't be a blocker for merging this PR.

[lauri865]

Thank you for the PR, subscribing to renderedRowsIntervalChange rather than binding to the store does seem a better approach.

For my tests, it improved the safari performance ~30-40%, cherry-on-top I can observe a slight (~10%) performance improvement in Chrome too.

As far as #15168 is concerned, the change certainly improved the Safari experience, we could do another benchmark on top of it if there are more areas we could improve in. However, that shouldn't be a blocker for merging this PR.

Great, thanks for testing!

Unrelated to the original issue – there are some other root-level hooks that can be optimised as well in the useDataGridPremiumComponent tree by removing instances of useGridSelector. useGridSelector on root level should be only used in exceptional cases to provide better isolation between components. Not every use can be avoided of course, but I quickly saw a number of instances where useGridSelector is used only to provide a dependency to useCallback. That's an easy refactor – just move the selector to useCallback without the hook.

Simple example:

mui-x/packages/x-data-grid-pro/src/hooks/features/columnPinning/useGridColumnPinning.tsx

Line 61 in 758d8b8

const pinnedColumns = useGridSelector(apiRef, gridPinnedColumnsSelector);

But I'd probably move these to another PR.

[MBilalShafi]

I quickly saw a number of instances where useGridSelector is used only to provide a dependency to useCallback. That's an easy refactor – just move the selector to useCallback without the hook.

Agreed 👍, however I remember a few use-cases where directly using selector caused a state out-of-sync issue, we gotta be careful of any such case when refactoring. In general, we should aim to keep things in the context they are required in, especially for the selectors that may regress the performance and opt-in features like row spanning which are not active at all the times.

[github-actions]

Cherry-pick PRs will be created targeting branches: v7.x

[romgrk]

This kind of change is dangerous in my opinion. If we mix state from useGridSelector with state from the apiRef directly, we are mixing state from different render/update cycles. We should not use this pattern to fix performance issues.

If we need to fix re-render issues, we should specialize selectors. For example, the selector could be targetted for renderContext.lastRowIndex specifically instead of the whole renderContext object, that would prevent re-renders while avoiding bugs.

[lauri865]

There's another PR I opened that takes care of the other useGridSelectors as well, which to me is the correct solution here. For reasons I described in the other PR. #16001

Plus using useGridSelectors in non-rendering hooks makes you write state syncing logic declaratively, which leads to useEffects calling useCallbacks whenever useCallback deps change and other spaghetti that's difficult to debug/trace.

[romgrk]

which to me is the correct solution here

I'm open to removing code if we can prove that it won't introduce bugs, but mixing state from different update cycles is pretty important to avoid. For example, if part of the state comes from before a filter has been applied, and another part of the state comes from the latest apiRef.current.state after a filter has been applied, we'd be using state slices that aren't coherent with each other.

Using apiRef directly consistently would avoid the issue, though I'd be curious to see how reactive bindings are working.

I'm still catching up with notifications I missed during the holidays but I'll review #16001 in the next days.

[lauri865]

but mixing state from different update cycles is pretty important to avoid

Completely agree, but you're more likely to do that with by accident with using useGridSelector on root level than not (=mixing stale state from rendering phase with up-to-date state from events/apiRef methods that themselves can hide a state selector in their implementation). Direct state selectors are more commonplace in the code base as it stands today – either directly or indirectly through apiRef methods. Using useGridSelector-s on root-level seems to be a more recent trend in the code base.

Compare the before and after of useGridRowSpanning hook. In which implementation can you answer the question more clearly – under what conditions is the row spanning state reset?

I'm not trying to throw shade to anyone's code in particular. This is not an isolated example, and I could have easily written the same thing myself if I just started out from using useGridSelector instead hooking into the existing events. It's inevitable if you mix two paradigms, you end up writing duct tape to keep them in sync. That duct tape in React is useEffect, which becomes an implicit event that is run under certain conditions. Under which exactly can become quite difficult to understand the more direct and indirect dependencies we end up having, and the less isolated the code is to a particular component.

If you take a step back – no hook in the useDataGridComponent tree is directly responsible for rendering anything – they are not an explicit part of a rendering cycle of any particular component. However, the use of useGridSelector forces those into a declarative paradigm from the outset, while actually serving an imperative function in the context of the data grid.

Just to be clear, even if the motivation around this PR was to take care of a performance regression, the rationale around the other PR (#16001) is not performance per se, it's not me pushing my idea(s/ologies), it's not removing code. But it is to suggest a convention that enables writing more isolated features and easier to follow logic within those features – a foundation that would have prevented this regression in the first place. And it's a pretty simple one.

useGridSelector

• Perfect to use in component rendering life-cycle (e.g GridRow, GridCell, GridBody, etc.)
• Should be avoided in root-level feature hooks / when hooking into the global state machine. The only true reason to use useGridSelector on root level is as a direct dependency of a useEffect, however that's seldom done and necessary, and usually a sign of a missing explicit event as useEffect itself will become that event implicitly, but may be invoked much more frequently than necessary.

FWIW, it doesn't seem to be a new convention either from my understanding of the code base. The older versions of the code base either explicitly or implicitly followed this very convention, at least to a much higher degree. Breaking seems to be a more recent trend.

Open to be proven wrong as always – show me an example where useGridSelector in the affected code is provably necessary. I will do my best to showcase why that may actually not be the case, and come up with an alternative on my end.

[romgrk]

Your points make a lot of sense. The reactivity architecture via useGridSelector() has mostly been my effort, following the point #1 of this discussion, and I initially established the guideline of "always use useGridSelector" as a way to provide a simple & clear rule for the rest of the team to follow. This in particular allows to easily refactor code between components & root-level hooks, which I've been doing a lot for the somewhat recent performance improvements.

It's inevitable if you mix two paradigms, you end up writing duct tape to keep them in sync. That duct tape in React is useEffect, which becomes an implicit event that is run under certain conditions. Under which exactly can become quite difficult to understand the more direct and indirect dependencies we end up having, and the less isolated the code is to a particular component.

Yes that's a major source of bugs and it's been on my radar for some time. Our state management goes through various cycles of updates/renders before settling on a coherent state because we use the async useEffect to sync up parts of the state, whereas we should be using something that can synchronously settle on a valid state. For example, if the rows prop is updated, there are various rounds of (async) updates before the filtered, sorted, and grouped rows slices of state are updated to match the new rows prop.

I'm not sure yet what's the best way to solve the issue. Going all in into selectors could be a solution, because selectors can lazily sync with the latest state value, though they keep their state in a memoized memory cell that doesn't live on the state object so that's less debuggable. Alternatively, we could create some event-based system that updates the state synchronously when another part of the state changes, this would not be lazy but it would be more debuggable.

useGridSelector: Perfect to use in component rendering life-cycle / Should be avoided in root-level feature hooks

I'm ok with that convention but it places a burden on the maintainers to be strict about observing those invariants, whereas creating specialized selectors would provide correctness without having to think about the context before using state data. Let me ping the team to see what's the consensus.

[MBilalShafi]

Really nice discussion points. A couple of cents from my side.

If we mix state from useGridSelector with state from the apiRef directly, we are mixing state from different render/update cycles.

Yes, this should be avoided as much as possible, I supported the change in #15986 specific to the renderContext as this particular state update is a bit unusual, I suspected the performance lag in Safari is due to wrapping of the state update in ReactDOM.flushSync, extracting the value in the event listener seemed like a proper reactive handle for this case. Also supported by the fact that yielded a better scroll performance. The testing I did didn't exhibit any regressions so I moved forward to merge, we could revert if we find one.

An alternate approach for this event (handleScrolling) could be to either extract the other state (dimensions) in the function too or use the argument (params.renderContext) passed to the event handler. 1 would avoid mixing of useGridSelector and apiRef slices, while 2 would remove state out-of-sync without extracting the selector slice. (But only for this instance, we'd need to do similar things for other (useGridRowSpanning)).

useGridSelector should be avoided in root-level feature hooks

If we manage to make that happen cleanly, it'll definitely make it harder for inactive features to impact the performance or unnecessarily re-render/re-compute some parts of the application, improving the overall performance, however, I'd be careful to avoid breaking an expected reactivity with this change.

All our root-level hooks are grouped, so it wouldn't be too hard to do:

I assume there are some use cases where it's inevitable to avoid useGridSelector in root hooks, like when they are used as useEffect dependencies. We'd need to provide an escape hatch to make that happen.

[lauri865]

So in the feature hooks, we use plain state selectors in event listeners, and if there's no event, do we use useGridSelector + useEffect?

I think the first question to ask if the event is missing is whether a new event should be added. If a state change triggers another piece of state to change as a side-effect, useEffect/useLayoutEffect are always worse options to write that logic in React than an event, as things will now happen in different render passes.

useEffect is an escape hatch from declarative rendering paradigm in react. In theory, the only appropriate use for useEffect in feature hooks (in relation to the internal state management) should be to sync props with the internal state. Of course there are other appropriate cases like subscribing to DOM events, etc.

I didn't see any obvious examples where I thought useGridSelector would be necessary in feature hooks, outside of useGridDimensions and useGridRowsMeta (although that could be fixed with more work as well, but since it's a core hook rather than feature hook, it's less of a concern to me; readability is much worse though with useGridSelector).

I'm curious to see if you have any examples though that would prove the inverse. The only theoretical case for useGridSelector I can think of – an event that mutates internal state needs to access the rendered value of the state (=old state). The only other case I can think of is that a feature hook needs to send the grid to re-render when no component depends on that state – but that smells of side-effects, and likely has a much nicer solution. If such cases would exist, they would likely be broken by react compiler in the future as well. But I haven't found any such cases yet, and this change would have highlighted them as well:
#15666 (comment)

[lauri865]

Ah, almost forgot. An alternative escape hatch to useGridSelector/useEffect could be useGridSelectorChange. Instead of creating/updating state, it would call a callback if the resulting selector value changes.

But I haven't seen a place where I would use it yet, except for useGridDimensions / useGridRowsMeta.

[cherniavskii]

The only theoretical case for useGridSelector I can think of – an event that mutates internal state needs to access the rendered value of the state (=old state)

In this case, the old state = the current state (before the new one is applied), so a regular selector should work here. Does this make sense?

[lauri865]

The only theoretical case for useGridSelector I can think of – an event that mutates internal state needs to access the rendered value of the state (=old state)

In this case, the old state = the current state (before the new one is applied), so a regular selector should work here. Does this make sense?

I meant a case, e.g.:

• onKeyDown DOM event fires
• focusCell state is changed (but updated state is not rendered until the next keyframe)
• onCellKeyDown internal event is fired
• For whatever reason, the onCellKeyDown consumer in a feature hook (=different hook than the one that manages focus state) needs to know the currently rendered focusCell

It's all purely theoretical though – I was just stretching my imagination to come up with a case where useGridSelector would be a necessary choice over straight selectors in callbacks (as I proposed), and hence tried to poke holes in my other PR. For avoidance of doubt, I haven't seen any practical example in the code where this is an actual use case.