[DataGrid] Remove try/catch from GridCell due to performance issues

[lauri865]

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

Fixes the performance issue described in #15615

We had about 150ms of scripting overhead when updating rows fast on typing via external filters (e.g. typing 5 letters, so 5 updates = 150ms in ~1s of user interaction). The larger the viewport, the larger the overhead. After this small change, all of it is gone, and it's mostly layout updates now.

[mui-bot]

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

Generated by 🚫 dangerJS against 24149ee

[cherniavskii]

LGTM, thanks!

[github-actions]

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

[romgrk]

Did you have a chance to benchmark the perf impact outside of updating rows? This PR introduces an additional .getRow() call, for a large dataset that's indexing into a large hashmap (rowsLookup[id]), it can be expensive.

Initially I kept the try/catch because I couldn't change the getCellParams API to return an empty value because it would be a breaking change, but now that we're preparing v8, we could get rid of the throw.

[lauri865]

getRowParams calls getRow anyways – I doubt calling it twice in a row has any impact in any modern javascript engine. Pretty sure that try/catch would have a higher overhead than that even in a passing case.

[lauri865]

Just did a test in all major browsers – sequential reads are virtually for free. Even when accessing random keys on the same object, but certainly the same key. There's a tiny bit of overhead when .getRow() is called from different functions, but even that is encountered once per row at most (and probably less).

Even if a viewport would fit 100 rows, it probably wouldn't register, as only the first row during first access of the object would be the slowest.

Theoretically, of course better to return a null. But there are probably lower hanging fruits wrt performance, like fixing memoization, state propagation, and potentially virtualization.

While I have you here – have you guys benchmarked your approach to virtualization? Is it a conscious choice performance-wise to transform the whole RenderZone at once vs. individual rows? In theory, the browser may need to do more work painting when individual rows change – larger paintable area + potential containment issues. But haven't benchmarked myself, just a hunch. Most other virtualization implementations I've seen and used have settled on per-row transforms for whatever reason (incl. Ag-grid).

[romgrk]

Pretty sure that try/catch would have a higher overhead than that even in a passing case.

IIRC, the relevant engines have removed try/catch deoptimizations, so it should be as efficient as normal code as long as it doesn't throw.

While I have you here – have you guys benchmarked your approach to virtualization?

I did extensive work on virtualization last year, when we reworked the layout to use CSS sticky headers. Back then I benchmarked heavily a few different approaches (and profiled ag-grid extensively to understand how they perform), including transform: translate() rows with contain: strict. In theory it could prevent some painting, in practice I couldn't find a substantial difference, though the exploration reached its time limit so maybe there's something to do, but I don't think it would be a huge improvement. Letting the browser layout rows also allows CSS open/close animations on the detail panels, which wouldn't work with transform positioning.

But there are probably lower hanging fruits wrt performance, like fixing memoization, state propagation, and potentially virtualization.

Yes, there seems to have been perf regressions, back last year the memoization was working correctly.

[lauri865]

You're right about try/catch in case it doesn't throw, my knowledge there was outdated.

Appreciate the context about virtualization. It does make some features like dynamic rowHeight also easier to implement I suppose.

After thinking a bit more about the getCellParams API, I'm not sure making it return null is necessarily good for DX either. We use it in a few different places in our app code (8 instances), and in every use, it's very explicit that the cell/row does exist. Say, we're focusing in on a cell or hovering it or triggering a context menu on it or copying a value from it, and need to access the cell's props. Why wouldn't it suddenly exist? Then we need to write extra code only to please typescript. We don't have any try catch logic either, and haven't had issues so far at least 🤔. One way or the other, it would be better if the cell value existed until the row has unmounted.

[cherniavskii]

Should I merge the same changes into v7? #15621

[lauri865]

From our side, yes please 🙏