[Data Grid] Fix row, cell and header memoizations

[lauri865]

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

Think I managed to pin down everything I reported in #15618

Before:

before-memo.mp4

After:

memo-fixed.mp4

[mui-bot]

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

Generated by 🚫 dangerJS against 57d4f0e

[lauri865]

Now also fixed rows having to re-render on focus/tabindex change, as that responsibility already falls to the cells. This removed an extra call to getCellParams on a row level to each cell. And focusedColumnIndex is only passed to invisible focused rows as others don't need to be aware of that.

[lauri865]

I moved Grid Header, Body and Footer to GridRoot and memoized GridRoot. I'm not sure it has any significant performance improvements, however, currently grid state change triggers the re-rendering of the whole tree. Which makes it very difficult to debug why each component re-rendered, as, unless they're memoized, they will automatically re-render because the parent re-render.

But without the change, it also makes use of any selectors in the non-memoized tree (e.g. within VirtualScroller) pure overhead, as they could be reading the root state to begin with.

As a direct result of this change, I could easily debug that each focus change causes the whole VirtualScroller to re-render, even if the RenderContext doesn't change. The cause was in the virtual focus cell implementation, whereby VirtualScroller reacted to each focus change, even though in reality, it only needs to concern about cases where focus is outside of the RenderContext, which is what I fixed now.

[lauri865]

✅ All tests passing – ready for review.

[romgrk]

Could that condition be expressed in terms of pinnedPosition instead? And would that allow us to keep the type of pinnedOffset: number instead of ?: number? And could we move the flip logic inside the if check? That would also simplify rtlFlipSide as it would receive a PinnedColumnPosition instead of a PinnedColumnPosition | undefined.

[lauri865]

I actually did it like that initially, but it felt wrong to me. Why create a value that shouldn't exist – could end up in adding accidental inline styles for non-pinned columns that should never exist. The type of pinnedOffset could be constrained to only exist if pinnedPoisition is left/right though.

Moving the rtlFlipSide could make sense, albeit it currently also narrows the type of pinnedPosition – if pinnedSide is defined, pinnedPosition can only be left or right, so maybe not much of a change:

[romgrk]

If you have time, the proper solution should be to fix the TODO comment here:

mui-x/packages/x-data-grid/src/hooks/features/rowSelection/utils.ts

Lines 49 to 50 in 90c1fc2

	// TODO v8: Use `createSelectorV8`
	export function getCheckboxPropsSelector(groupId: GridRowId, autoSelectParents: boolean) {

We now have support for selectors with arguments, which is what the checkbox selectors were missing.

[lauri865]

I might be misunderstanding the new selectors as there's no documentation and only one implementation so far in the code base, but wouldn't it still spit out a new object and trigger a new render, even if the object values stay the same?

[romgrk]

createSelectorMemoizedV8 would memoize its output based on its dependencies, e.g.

const checkboxSelector = createSelectorMemoizedV8(
  aSelector,
  bSelector,
  cSelector,
  (a, b, c, args) => { return { a, b, c } /* this result object would be memoized */ }
)

Though I think @MBilalShafi might handle it as part of #15200, so you could ignore this.

[lauri865]

This part I understand, but wouldn't it only work if a selector returns a slice of the state. In this case we're dealing with derived state, and creating a new object

In this case, you have two options:

• Use a selector that returns the derived state – return value never holds a stable reference, memoization fails.
• Put the state derivation logic in the last function – last function gets triggered also when unrelated rows get selected/deselected. Last function still creates an object with unstable object reference, everything still re-renders when other rows re-render.

So, while it fixes unrelated state triggering re-renders, it still doesn't enable us to hone in on the specific row's state. Either we need to not return an object if the row is not selected, or still need objectShallowCompare to memoize it. 🤔

[romgrk]

In this case we're dealing with derived state, and creating a new object

Is it clear to you that the derived state (aka the new object) is memoized by createSelectorMemoized? So it stays stable across state updates until one of its dependency (in the example above, aSelector, bSelector and cSelector) changes. That function is similar to React.useMemo in its behavior.

[lauri865]

Ok, I think I understand now after looking at the source code. Args are also passed nested into all of the subselectors, in which case gridSortedRowIdsSelector needs to return the correct slice based on the args.

I was first under the impression that args are only passed to the last function that takes together the selector results, in which case it would be triggered by unrelated gridSortedRowIdsSelector changes, and we would create a new object every time.

But if args are shared naively across any nested selectors, how do you plan to avoid any conflicts? Seems like something that needs utmost caution if used widely.

[romgrk]

Another case of "should be a selector with arguments", but this is low-impact, so you could leave a TODO comment and I'm ok with it.

[romgrk]

Though now that I re-read, those findIndex() calls aren't great. Those are O(n) costs that are called everytime the state updates, I think it would be preferable to memoize that logic somehow (either through createSelectorMemoized or React.useMemo).

[lauri865]

Good catch. Reduced the lookup cost now, but if you have any ideas on how to make it even better, I'm all ears. Would rowIdToRowIndex lookup map make any sense?

[lauri865]

Pushed another alternative with lookup maps.

[romgrk]

I think I don't love the lookup maps, they add an O(n) cost (plus a bunch of memory I/O) when the rows are updated to build the lookup map. For the use-cases where rows are updated frequently, it could have an effect. I would favor going back to memoization. If you removed React.useMemo, I guess selector memoization would be the alternative. Here is an example how I think it could work (some of the dependency selectors are placeholders, can't remember the right names atm):

https://gist.github.com/romgrk/7f7a682578a1c5f2bbcd8b82aef3148e

[lauri865]

Pushed a quick example of what I'd do – lookupMap creation takes 10ms in dev for 100k rows under page load (probably less under normal conditions). Would take anywhere from 60-100% longer if we used rowId as the map key instead of object reference, and consume much more memory with uuid ids.

[lauri865]

Forgot to update the selector itself, which can now be improved thanks to the change to useGridVisibleRows. But you get the gist.

Since currentPage wasn't really a selector before, it didn't work with memoized selectors.

[romgrk]

This selector needs objectShallowCompare still, since it revalidates every time renderContextSelector changes, however, the resulting derived state doesn't always change when renderContextSelector changes, but we can't provide a stable object reference. Would it be different with the v8 selectors?

The trick would be to create intermediate selectors, so that you can compute e.g. isFocusedCellInContext when renderContext changes, but the value of isFocusedCellInContext would stay the same which wouldn't recompute the final focusedVirtualCell selector. I think columnIndex would also need to come from an intermediate selector.

You can keep the intermediate selectors in the new file, no need to expose every selector publicly.

[lauri865]

Would intermediate selectors also have to be memoized or is it enough if the parent selector is? 🤔

[romgrk]

A selector should be memoized if either of these is true:

• The selector returns a non-primitive value (object, array, set, map, ...)
• The selector is computationally expensive (e.g. O(n) operation)

So for example the isFocusedCellInContextSelector would not need to be memoized, because it returns a boolean which is referentially equal across runs.

[lauri865]

Unit tests are passing now, we'll see about browser ones.

This selector needs objectShallowCompare still, since it revalidates every time renderContextSelector changes, however, the resulting derived state doesn't always change when renderContextSelector changes, but we can't provide a stable object reference. Would it be different with the v8 selectors?

const focusedVirtualCell = useGridSelector(
    apiRef,
    gridFocusedVirtualCellSelector,
    objectShallowCompare,
);

@romgrk, let me know if you're happy with the change around useGridVisibleRows / getVisibleRows. If so, we should clean up the current consumers of those methods as well, as they don't need to provide props anymore.

This change forced me to reorder state initialization as well – visible rows are dependent on pagination, which has always been the case, but didn't surface before due to props being passed around.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[romgrk]

let me know if you're happy with the change around useGridVisibleRows / getVisibleRows.

Very happy, there's so much refactoring that I'd like to do and not enough time. Not sure I'd refactor every consumer right now, this PR is already large. You can leave a TODO note and/or open a tracking github issue for that.

[lauri865]

let me know if you're happy with the change around useGridVisibleRows / getVisibleRows.

Very happy, there's so much refactoring that I'd like to do and not enough time. Not sure I'd refactor every consumer right now, this PR is already large. You can leave a TODO note and/or open a tracking github issue for that.

Makes sense. Already left a to-do note on useGridVisibleRows and getVisibleRows.

[lauri865]

No idea why datepickers related test is failing, but otherwise we're looking good.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[lauri865]

@romgrk, just fyi, I'm going on a parental leave beginning of next year, in case you need any additional input from me to get this merged.

The last commits are a bit offtopic, but fixes some issues I hit during rebasing that were introduced by another PR. The issues weren't caught by unit tests due to an extraneous check for virtualization that hid them from unit tests. I only discovered the issues because I needed some props to live within useGridVirtualScroller. A fairly small change though and keeps the renderContext logic centralised + abolishes the need for any manual overrides to renderContext in multiple places. Happy to roll them back if they should slot into another PR though, but that would block this PR until it's merged.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.