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

perf(DataTable): speed up select-all with large tables #13831

Conversation

mrspeaker
Copy link
Contributor

@mrspeaker mrspeaker commented May 19, 2023

Rewrite setAllSelectedState to improve performance with large tables. In my test case on my machine with 10000 rows, selecting all rows previously took 14000ms, this fix reduces it to 8ms. It uses one instance of (local) variable mutation.

Closes #6146
Closes #4338

The setAllSelectedState function is called when you click the select-all (or deselect all), or select one row then hit the cancel button on the bulk-action toolbar (in handleOnCancel). It appears to be "accidentally quadratic" and starts to become an issue with more than a few thousand rows. To avoid it, I do a local mutation of the accumulator rather than re-splatting out the entire object each iteration.

For 10000 rows, this reduces the original 14 second time (this is for Vite transpiled code, 8 seconds for native), down to ~250ms. Most of that time is in the filteredRowIds.includes check. So I added an extra flag (isFiltered) to avoid calling when the filter is not in use. When there are no filtered rows, the function runs in ~8ms.

Changelog

Testing / Reviewing

The fix does not break any existing tests, and should not introduce any changes to the existing behavior of the DataTable: Selecting-all/deselecting all should work as expected. When using the bulk-action toolbar, clicking cancel should deselect the row. If rows are filtered, clicking select-all should select all filtered rows.

To test the performance, you need to make a table with a lot of (at least 10000) rows. With a large dataset like this, the performance improvements are obvious, but if you want to measure it then wrap calls to setAllSelectedState with console.time messages. E.g, inside the DataTable's handleOnCancel function:

...
console.log("Cancel button was hit - call setAllSelectedState");
console.time("setAllSelectedState");
const output = this.setAllSelectedState(state, false, this.getFilteredRowIds());
console.timeEnd("setAllSelectedState");
return {
  shouldShowBatchActions: false,
   ...output
};

@mrspeaker mrspeaker requested a review from a team as a code owner May 19, 2023 04:36
@github-actions
Copy link
Contributor

github-actions bot commented May 19, 2023

DCO Assistant Lite bot All contributors have signed the DCO.

@netlify
Copy link

netlify bot commented May 19, 2023

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 9dbe3fe
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/646e320460eb7c00083fd0c6
😎 Deploy Preview https://deploy-preview-13831--carbon-components-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented May 19, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 9dbe3fe
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/646e32041ea059000873300c
😎 Deploy Preview https://deploy-preview-13831--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@mrspeaker mrspeaker force-pushed the 6146-data-table-select-all-performance branch from d77e1c1 to a5b3d71 Compare May 19, 2023 05:57
@mrspeaker
Copy link
Contributor Author

I have read the DCO document and I hereby sign the DCO.

Rewrite `setAllSelectedState` to improve peformance with
large tables. In my test case with 10000 rows, selecting
all rows previously took 14000ms. This fix reduces it to
8ms. It uses one instance of (local) variable mutation.
Fixes carbon-design-system#6146.

.
@mrspeaker mrspeaker force-pushed the 6146-data-table-select-all-performance branch from a5b3d71 to 8a6f374 Compare May 19, 2023 06:04
Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrspeaker Thank you, this is great! 🎉 This will also close #4338, I've already updated the issue body to reflect this.

Do you think we could speed it up even more by not relying on reduce and just using a for loop instead to build up the rowsByID?

@mrspeaker
Copy link
Contributor Author

mrspeaker commented May 24, 2023

Do you think we could speed it up even more by not relying on reduce and just using a for loop instead to build up the rowsByID?

@tay1orjones - strangely, I thought the same - but when I tested it the for loop was ever-so-slightly slower! I'm not sure why (and I bet it's browser specific), but in either case the time for the loop itself is negligible compared to the loop body. I decided to keep the reduce there to more closely match the coding style of the code base.

@tay1orjones
Copy link
Member

@mrspeaker Very interesting, thanks for testing it out!

@kodiakhq kodiakhq bot merged commit fbb7fce into carbon-design-system:main May 24, 2023
@mrspeaker mrspeaker deleted the 6146-data-table-select-all-performance branch May 29, 2023 04:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants