-
Notifications
You must be signed in to change notification settings - Fork 209
fix: Use Map for O(1) column lookups in table width calculations #4142
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
base: main
Are you sure you want to change the base?
fix: Use Map for O(1) column lookups in table width calculations #4142
Conversation
1b1e88e to
2f3337c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4142 +/- ##
=======================================
Coverage 97.14% 97.14%
=======================================
Files 873 873
Lines 25642 25643 +1
Branches 9284 9284
=======================================
+ Hits 24910 24911 +1
Misses 726 726
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| const [columnWidths, setColumnWidths] = useState<null | Map<PropertyKey, number>>(null); | ||
|
|
||
| // Pre-build a Map for column lookups | ||
| const columnById = useMemo(() => new Map(visibleColumns.map(column => [column.id, column])), [visibleColumns]); |
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 visibleColumns is created on every render, so there is no reason to memoize this computation - it will still recompute every time.
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.
btw, it is not O(1) as specified in the PR description but O(n) - which is the cost of dictionary creation
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.
Good catch! I've updated this PR to memoize visibleColumns 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.
But why all this memoization is needed? It adds a lot of clumsiness to the code but the performance impact of transforming columns is negligible even if there are 100 columns, which is rare.
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.
So, the memoization is actually a much bigger win than the Map change: Every time this useEffect gets triggered, it calls updateColumnWidths, which is potentially very expensive because it makes DOM measurements with getBoundingClientRect(). Doing that on table renders that don't affect the column widths is a recipe for making interactions feel slow. I'm sure you've experienced times where there's noticeable lag when typing in a Cloudscape table's search box; the updateColumnWidths call is likely a big contributor to that lag.
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.
I've updated this PR with a "does not measure column widths when re-rendering with unchanged columns" test, which I think is useful for validating the performance improvement here and preventing future regressions.
2f3337c to
6e27500
Compare
6e27500 to
fab25b7
Compare
Description
Currently the table width calculations use repeated
findcalls across columns ingetColumnStyles, which is called for each column, so worst-case performance isO(n^2). This PR switches to creating aMapfor that lookup, making performance consistentlyO(n).How has this been tested?
This functionality is well-covered by existing tests.
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.