Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DataTable: seems to re-render all the rows on select all, even with the pagination. #2979

Open
tmmschmit opened this issue Jun 9, 2022 · 21 comments · May be fixed by #7420 or #7425
Open

DataTable: seems to re-render all the rows on select all, even with the pagination. #2979

tmmschmit opened this issue Jun 9, 2022 · 21 comments · May be fixed by #7420 or #7425
Assignees
Labels
Type: Performance Issue is performance or optimization related
Milestone

Comments

@tmmschmit
Copy link

Describe the bug

Hi, so I'm trying to implement a bulk edit on a DataTable with a large amount of entries. But the more entries the table contains, the more slower it is to display the select rows.
I did some tests with a lazy loading and it's much quicker without taking care of the entries number. But this way I lose the sorting and ordering feature of DataTable.

So is there a way to only re-render the visible rows of a DataTable with pagination?

Thanks!

Reproducer

https://codesandbox.io/s/primereact-test-forked-vqhyjv?file=/src/index.js

PrimeReact version

8.1.1

React version

18.x

Language

ALL

Build / Runtime

Create React App (CRA)

Browser(s)

FireFox 101, Chromium 102

Steps to reproduce the behavior

  1. Open the link
  2. Click on the checkbox in the header
  3. change the ENTRY_MULTIPLIER value to see the render speed change.

Expected behavior

Render only the visible rows of the DataTable when the checkbox in the header is clicked.

@tmmschmit tmmschmit added Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible Type: Bug Issue contains a defect related to a specific component. labels Jun 9, 2022
@tmmschmit tmmschmit changed the title The DataTable seems to re-render all the rows on select all, even with the pagination. DataTable: seems to re-render all the rows on select all, even with the pagination. Jun 9, 2022
@devken-net
Copy link

I got the same performance issue. It seems DataTable re-render all rows and cells every time non-value props (size, showGridlines, resizableColumns, scrollable, etc) were updated.

@mertsincan mertsincan removed the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Aug 25, 2022
@melloware melloware added Type: Performance Issue is performance or optimization related and removed Type: Bug Issue contains a defect related to a specific component. labels Dec 23, 2022
@veladzic
Copy link

veladzic commented Oct 6, 2023

Same issue here. Tried it already with the v10.0.0. Here is the reproducer:

https://codesandbox.io/s/primereact-test-forked-hw8rwr?file=/src/index.js
the issue has already been described here in the discussion: https://github.com/orgs/primefaces/discussions/114

@melloware melloware added this to the Future milestone Oct 6, 2023
@torshid
Copy link

torshid commented Oct 27, 2023

I got the same performance issue. It seems DataTable re-render all rows and cells every time non-value props (size, showGridlines, resizableColumns, scrollable, etc) were updated.

You are right. I have 50 rows in my datatable, even a single selection causes the datatable to re-render everything, as it is also the case when changing a field of a single row (when value mutates). A bit disappointed on this one. :(

image

@mdimitrov94
Copy link

Any news on this? I see a lot of bugs being fixed but not this one. :(

@melloware
Copy link
Member

@mdimitrov94 if you need immediate support I suggest looking into PrimeReact PRO support.

@sja-cslab
Copy link
Contributor

Additional Info:
Same in PrimeReact 10.6.6 but more than that - whenever the parent re-renders, even if you wrap datatable and columns to make them memoized, the table will re-render

@minhnguyenduy99
Copy link

I got the same problem. Is anyone working on to fix this bug ?

@acc-cassio
Copy link

I'm having the same issue here, just tested with the version 10.8.4. Has anyone found a workaround until the issue is fixed?

@djg3577
Copy link

djg3577 commented Nov 12, 2024

Dealing with the same issue on 10.8.4 as well

@acc-cassio
Copy link

acc-cassio commented Nov 13, 2024

I have been able to solve the issue by providing meaningful changes to be checked in the BodyCell props before re-rendering it. See the changes below:

import isEqual from 'lodash/isEqual';

export const BodyCell = React.memo((props) => {
    ...
    // BodyCell component code
    ...
}, (prevProps, nextProps) => {
    const hasRowDataChanged = !isEqual(prevProps.rowData, nextProps.rowData);
    const hasEditingStateChanged = prevProps.editing !== nextProps.editing;
    const hasSelectionChanged = prevProps.selected !== nextProps.selected;
    const hasRowOrCellIndexChanged = prevProps.rowIndex !== nextProps.rowIndex || prevProps.index !== nextProps.index;
    const hasExpandedChanged = prevProps.expanded !== nextProps.expanded;
    const hasAllowSelectionChanged = prevProps.allowCellSelection !== nextProps.allowCellSelection || prevProps.allowRowSelection !== nextProps.allowRowSelection;
    const hasEditModeChanged = prevProps.editMode !== nextProps.editMode;
    const hasEditingMetaChanged = !isEqual(prevProps.editingMeta, nextProps.editingMeta);

    return !(
        hasRowDataChanged ||
        hasEditingStateChanged ||
        hasSelectionChanged ||
        hasRowOrCellIndexChanged ||
        hasExpandedChanged ||
        hasAllowSelectionChanged ||
        hasEditModeChanged ||
        hasEditingMetaChanged
    );
});

There might be other meaningful changes in the props which I have missed. But with the implementation, all basic changes to the datatable trigger the necessary re-render and the unnecessary ones, as in the reported issue, are avoided.

For a full implementation, you can refer to my forked primereact repo: https://github.com/accioly-cassio/custom-primereact

@melloware, could the adjustments be checked and included in one of the next versions of primereact?

@acc-cassio
Copy link

One more insight and idea: particularly for this case and maybe more generally for other components in the library, it might make sense to implement a fastMemo function, as done in other libraries, for a more meaningful check of the changes in the props. See the below implementation:

import * as React from 'react';

export function fastMemo<T>(component: T): T {
  return React.memo(component as any, fastObjectShallowCompare) as unknown as T;
}

const is = Object.is;

function fastObjectShallowCompare<T extends Record<string, any> | null>(a: T, b: T) {
  if (a === b) {
    return true;
  }
  if (!(a instanceof Object) || !(b instanceof Object)) {
    return false;
  }

  let aLength = 0;
  let bLength = 0;

  for (const key in a) {
    aLength += 1;

    if (!is(a[key], b[key])) {
      return false;
    }
    if (!(key in b)) {
      return false;
    }
  }

  for (const _ in b) {
    bLength += 1;
  }
  return aLength === bLength;
}

https://github.com/mui/mui-x/tree/master/packages/x-internals/src

fastMemo Function: it wraps a component in React.memo, but instead of using React.memo's default shallow comparison for props, it uses fastObjectShallowCompare for a more tailored comparison.

fastObjectShallowCompare Function:

Checks if two objects (a and b, which represent props) are strictly equal. If they are, it returns true, meaning no re-render is needed.
If they aren’t strictly equal, it checks each property in a and b to confirm if:
The values are the same, using Object.is.
Both objects have the same keys and number of keys.
If the objects have different keys or any key's value is not identical between a and b, it returns false, signaling a need for re-rendering.

@sja-cslab
Copy link
Contributor

sja-cslab commented Nov 13, 2024

@accioly-cassio I appreciate your work however it would help even more if you make a PR out of your fork.

The fastCompare could be simplified i guess:

function fastObjectShallowCompare<T extends Record<string, any> | null>(a: T, b: T) {
  if (a === b) return true;
  if (!a || !b || typeof a !== 'object' || typeof b !== 'object') return false;

  const keysA = Object.keys(a);
  const keysB = Object.keys(b);

  if (keysA.length !== keysB.length) return false;

  return keysA.every((key) => Object.is(a[key], b[key]));
}

@acc-cassio
Copy link

@sja-cslab, thank you for the simplification, looks better. However, I've noticed that my implementation presents bugs and that this issue is much trickier to solve than expected.

The main issue is that, data from the entire table are parsed to each cell component through the pops and then, inside the component, the state of the cell is calculated based on these value, e.g., whether the cell should be selected or unselected. So, you can't solve the issue by implementing react.memo based on some selected props, since you might always be missing something.

These state calculations need to be moved out of the bodycell component and only the results should be parsed to it as props. It is quite a challenge, since the bodycell component is quite complex and I guess you'll need to do the same for the bodyrow component. I'll keep working on it a bit longer anyway and see what I can achieve.

@sja-cslab
Copy link
Contributor

sja-cslab commented Nov 14, 2024

Additional Info: Same in PrimeReact 10.6.6 but more than that - whenever the parent re-renders, even if you wrap datatable and columns to make them memoized, the table will re-render

@accioly-cassio I know what you're talking about. I've tried the same back then. Good luck and thank you for your efforts

@acc-cassio
Copy link

acc-cassio commented Nov 14, 2024

After some work, I have managed to solve the issue. I took some parameters and functions out of the BodyCells and placed them in the BodyRow component. Also, I have improved the set of BodyCell props to be checked when deciding for a re-render in React.memo. This avoids now a lot of unnecessary re-render, including the issue from this topic: when a row is selected, only the cells of this row are now re-rendered. Please refer to the fork: LINK, branch "dtable-performance".

DISCLAIMER: There are still a bug related to the MetaKey which I haven't been able to solve yet. I will create a pull request when it is fixed.

@melloware
Copy link
Member

@accioly-cassio looking forward to seeing your PR

@acc-cassio
Copy link

acc-cassio commented Nov 15, 2024

MetaKey bug fixed. I did some general testing and haven't noticed anything else abnormal. Pull request created.

Note that I also created a function called "selectiveCompare" in the BodyCell component to make the selection of which props should be checked at the React.memo. It has the same purpose of the fastObjectShallowCompare mentioned earlier but with some added features to make it more useful for the primereact library. Maybe it makes sense to move it to the Utils, since it potentially can be used for optimizing the datatable further and more generally other library components. This selective comparison of the props is important since React.memo will not work as expected mainly if there are objects being parsed in the props, as mentioned here in the thread.

I'm looking forward to see the results of the testing!

@benrhere
Copy link

@acc-cassio thanks for your effort on these two PRs! I (and I imagine others) are looking forward to the performance boost.

@sja-cslab
Copy link
Contributor

too bad that did not make it into 10.8.5

@acc-cassio
Copy link

@benrhere @sja-cslab you can clone my repo and already use it in your project while you wait for the pull request to be merged :)

@benrhere
Copy link

@benrhere @sja-cslab you can clone my repo and already use it in your project while you wait for the pull request to be merged :)

Super! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment