-
Notifications
You must be signed in to change notification settings - Fork 418
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
Improve DataTable re-rendering performance #2993
Conversation
89589b6
to
a5c98d4
Compare
@@ -16,8 +16,9 @@ import shortid from 'shortid'; | |||
|
|||
import classNames from 'classnames'; | |||
import assign from 'lodash.assign'; | |||
import isEqual from 'lodash.isequal'; | |||
import memoize from 'memoize-one'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with using lodash.memoize
is it only bases its memoization on the first argument supplied to the function by default, and you need to hand-write the cache key calculation otherwise. memoize-one
is free of that shortcoming and is a popular library that's lightweight and performant.
I think there is only one usage of lodash.memoize
that I could remove if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, one memoize library would be great!
} | ||
return result; | ||
}, | ||
isEqual |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because assistiveText
is an object rather than a scalar value, we need to provide a comparison function
@@ -587,6 +687,13 @@ class DataTable extends React.Component { | |||
} | |||
}; | |||
|
|||
// eslint-disable-next-line camelcase | |||
UNSAFE_componentWillUpdate(nextProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fixing an existing issue (not related to performance) where navigating cells via keyboard and pressing spacebar wasn't triggering the interactive element.
I spent limited amount of time fixing this because it's out of scope so I didn't look into ways of avoiding an unsafe lifecycle.
columns.push({ | ||
Cell, | ||
props, | ||
dataTableProps: this.props, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When extracting this code, I removed this property since dataTableProps
were not being used.
if (child && child.type.displayName === DataTableColumn.displayName) { | ||
const { children, ...columnProps } = child.props; | ||
|
||
const props = assign({}, this.props); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This essentially propagates all table props onto columns. In the extracted function I replaced this with individual props that are actually used. Passing more props than necessary can exacerbate the issue with unnecessary re-renders.
// Always use the canonical component name as the React display name. | ||
static displayName = DATA_TABLE_HEAD; | ||
// ### Prop Types | ||
const propTypes = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the changes in this file are to refactor the component to a function so that it can use the context helper hook.
showRowActions: PropTypes.bool, | ||
}; | ||
|
||
const ActionsHeader = (props) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted the ActionsHeader
and SelectHeader
subcomponents since otherwise the hook calls were under a condition which is not allowed. (and also just seemed a good thing to do in general)
*/ | ||
width: PropTypes.string, | ||
}; | ||
const DataTableHeaderCell = (props) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored this to a function as well to use the context helper hook.
a5c98d4
to
f910a09
Compare
f910a09
to
1b99eb0
Compare
@@ -98,6 +98,7 @@ | |||
"lodash.isfunction": "^3.0.9", | |||
"lodash.memoize": "^4.1.2", | |||
"lodash.reject": "^4.6.0", | |||
"memoize-one": "^6.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need some approval process to use a new lib?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure, @interactivellama?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have license and security checking from Snyk in our CI, so it should conform to Snyk's policy https://app.snyk.io/org/salesforce-oss/pr-checks/ab6331d2-5eb8-4973-8bf5-aab69313e91f
props.fixedLayout | ||
); | ||
const { fixedHeader, canSelectRows } = props; | ||
const getContent = (idSuffix, style, ariaHidden) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make any perf difference to make such functions as callback or useMemo? I noticed there are couple similar functions in other files as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this particular one is in the table head so it only renders O(1) per table as opposed to O(rowcount) so I left it as is
I can see a noticeable difference on a table of 250 rows (7 columns) when selecting a row! 🎉 |
@alexzherdev Please merge when ready. |
Additional description
This improves re-rendering performance of the DataTable in scenarios involving many rows (in the hundreds/thousands). Main optimizations introduced:
<DataTable>
: memoizedTableContext
value so that if table gets re-rendered with no real changes (e.g. when its parent re-rendered and didn't pass any new props), it doesn't trigger unnecessary re-renders in the context consumers<DataTable>
: memoized assistive text and columns passed down to rows to reduce their re-renders<DataTableRow>
: any change inTableContext
would mark all rows for re-rendering due touseContext
in the row. Implemented option 3 here to stop unnecessary re-rendering below the row (with the exception of relevant nested context consumers)context-helper
: turned it into a hook to enableuseCallback
. This was necessary so that the functions returned from the helper could be properly used asuseMemo
dependencies elsewhere.Note that to take advantage of these improvements, the consumer application needs to memoize the columns passed into the table:
Traces below are from a consumer application showing reduced re-rendering when clicking a row (these are taken from the re-render caused by the focus change in the table). Overall rendering time (measured via Chrome's profiler) decreased by 40-50%.
Before
After
CONTRIBUTOR checklist (do not remove)
Please complete for every pull request
npm run lint:fix
has been run and linting passes.components/component-docs.json
CI tests pass (npm test
).REVIEWER checklist (do not remove)
components/component-docs.json
tests.Required only if there are markup / UX changes
last-slds-markup-review
inpackage.json
and push.last-accessibility-review
, topackage.json
and push.npm run local-update
within locally cloned site repo to confirm the site will function correctly at the next release.