Reduce the cell dom layer

[KevinYaoooooo]

Expected Behavior

scroll behavior could be more faster

Current Behavior

scroll behavior have been more laggy if there are more columns and rows, I have used those two options:

pureRendering
allowCellsRecycling

but it still laggy.
I guess, since there are too many layers on cell component like below screenshot:

Possible Solution

reduce the layer that is unnecessary for the layout and behavior

Steps to Reproduce (for bugs)

produce lots of rows(20+), lots of columns(20+)

Context

it's table to show project list and the attributes of each project on each column.

Your Environment

• Version used: latest version: 0.8.19
• Browser Name and version: Chrome: 72.0.3626.96 (Official Build) (64-bit)
• Operating System and version (desktop or mobile): MacOS Mojave 10.14.2
• Link to your project: internal project

[wcjordan]

These come from FixedDataTable.Cell. There's no requirement to use the default cell so you should feel free to pass in your own React component in it's place.

Another tip, make sure your cells are all PureComponents and don't re-render while scrolling. By and large most performance issues with the grid are a result of using cells which don't check their props to prevent extra rendering.

[KevinYaoooooo]

Thanks @wcjordan , another question: if use my own cell component, can I still use

allowCellsRecycling

option to allow my cell component to be recycled and get the performance benefits?

[wcjordan]

Yep. That's implemented in the row rendering.

[KevinYaoooooo]

Thanks @wcjordan ， I've tried this solution, it works, reduces a lot dom layers, and the cell recycling is work just fine, It feels a little bit better, but i'm not sure, cuz scrolling is still a little bit delay, I wander is there any delay time you've added in when scroll events occured?

is that because I use trackpad when scrolling?

[wcjordan]

Nope

[KevinYaoooooo]

Hey @wcjordan , we are still experiencing some laggy scrolling, we avoid the unnecessary dom layers, we avoid duplicated cell rendering , but still feels scrolling is slower, couldn't catch up with the hand moving speed, compare to react-virtualized MultiGrid, and I've checked the source code, I guess the reason the scrolling is still laggy is because you have to control scrolling stuff from outside and make the row scrolling by spend a react render lifecycle, I'm not sure, Do you think so?

[KevinYaoooooo]

I have some thoughts to continue reducing the dom layer to improve the performance.

1. accept cellClassName or cellWrapperClassName to customize cell style, thus we could still customize the cell style and save a cell wrapper dom layer and use the internal cell wrapper instead:
   

2. reduce the dom layer if there is only one cell inside the cell group and cell group wrapper.
   

3. remove resizer dom(dragKnob) if resizer function is not enabled.
   

4. reduce cellGroupWrapper, use cellGroup directly instead:
   

[wcjordan]

Feel free to fork the repo and try out those ideas.

I'm suspicious that it will help. When I've seen issues like what you describe it has nearly always been the case that the cells are re-rendering causing performance issues. I'd recommend ensuring your cells are pure components and don't re-render on scroll before pursuing anything else.

[KevinYaoooooo]

I've 1, 3, 4, on my forked version, it works well, it did help to improve the performance.

Because when you have large table that has many narrow(small columnWidth) columns and display on a large wide screen , it will produce lots of dom nodes, when you scroll to update the dom position, lots of dom nodes will be handled by outside scrolling event, that means the more layer you have ,the more calculation and update you need to update the position.

so I think reduce the dom nodes could really help to improve the performance. I've tried my version, I guess it helps, but I haven't got the stats yet.

You could checkout the commit that I pushed to my forked version, give me some suggestions or tips would be a very good help, thanks

[wcjordan]

That's great. @pradeepnschrodinger could you check out the fork and see if there's anything we could bring in to the v1 branch?

Kevin, one note FDT isn't well designed for handling lots of columns since it doesn't virtualize them. Once you get to 100s of columns you'll see performance issues. There's a proof-of-concept to virtualize columns at #153 but it has some bugs / unfinished work.

[KevinYaoooooo]

And... the 2nd idea is DONE by just display the single solo directly without cellGroup layer wrapping.

works just fine, here I am to reduce another DOM nodes layer, Yay!.

@wcjordan I see your point, but I don't think there is a need to virtualize the column , because column is just a concept DOM, or should I say, it's just a HOC or virtualize DOM, there is no real column DOM in the dom tree, all you render is cell, and since the cell is already virtualized, I guess column virtualization is not needed anymore.

[pradeepnschrodinger]

@wcjordan Yea, will take a look at the fork soon.

[pradeepnschrodinger]

Hey, sorry for the extremely late response.

@KevinYaoooooo I like your suggestions and I think it would be a nice addition to the v1.0-beta branch!

Although I haven't really tested around your fork and checked the performance, but have taken a good look through the codebase. I also saw a few other changes (something around noRowRenderer and reRenderCellsWhenRowsCountChanged) but I'm assuming these are not related to this issue?

It would be extremely helpful if you can put a PR for each of the suggestions outlined above. It would be easy since you already have these on your fork. You could also put the parts around noRowRenderer if you want.

[pradeepnschrodinger]

And yeah, suggestion 2 seems to be easy and yet another nice addition.
I want to make sure these changes doesn't break products already using FDT due to all the DOM layers missing (I'm thinking of CSS breaking...?)

@KevinYaoooooo @wcjordan So I would suggest doing most of these for the v1.0-beta branch. Thoughts?

[pradeepnschrodinger]

@wcjordan I see your point, but I don't think there is a need to virtualize the column , because column is just a concept DOM, or should I say, it's just a HOC or virtualize DOM, there is no real column DOM in the dom tree, all you render is cell, and since the cell is already virtualized, I guess column virtualization is not needed anymore.

@KevinYaoooooo, sorry, don't think I really understood, but currently, only the rows are virtualized.
We also need to virtualize the columns because without this, ALL the cells in a row will be rendered. This becomes apparent with a heavy drop in performance when lots of columns exist in the table.

[wcjordan]

@pradeepnschrodinger I think you had a branch to work on this. Should we re-open this ticket or open a clean one to track that work?

[pradeepnschrodinger]

@pradeepnschrodinger I think you had a branch to work on this. Should we re-open this ticket or open a clean one to track that work?

@wcjordan, I don't have a branch on this.
For tracking, we'll reopen this one since @KevinYaoooooo has nicely explained the issue.
Maybe it'll be better off to create separate ones to track each of the issue explained here later, depending on the PRs?

@KevinYaoooooo I like your suggestions and I think it would be a nice addition to the v1.0-beta branch!

@KevinYaoooooo, can you open a PR for this? Also, If it's not too much trouble, could you separate out PRs for each of the issue? The target can be master/v1.0-beta. Thanks.