perf(native-filters): Decrease number of unnecessary rerenders in native filters#17115
Conversation
villebro
left a comment
villebro
left a comment
There was a problem hiding this comment.
AMAZING work! So much unnecessary rerendering cleaned up here, wow! Code LGTM and works very well! 👍
There was a problem hiding this comment.
Left some non-blocking comments. Unfortunately, we still don't have enough automated tests to make us feel safe merging these types of changes. Can we get @jinghua-qa approval before merging?
There was a problem hiding this comment.
Shouldn't be useMemo here? Why are you preserving the instance of the function instead of its result?
There was a problem hiding this comment.
It's a function, because the result depends on index which we get from portalNodes.map. Here the result will be memoized for each index value
There was a problem hiding this comment.
If we didn't useMemo here we'd get a different object on each render, meaning the prop through which we pass that object would change and trigger rerender
|
The difference in the number of renders is remarkable! Really cool work! 🚀 |
|
/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true FEATURE_DASHBOARD_CROSS_FILTERS=true FEATURE_DASHBOARD_NATIVE_FILTERS_SET=true FEATURE_DASHBOARD_FILTERS_EXPERIMENTAL=true |
|
@suddjian Ephemeral environment spinning up at http://52.25.11.193:8080. Credentials are |
|
Thanks so much for the work! let's merge it into master and I will test it with our benchmark dashboards to see the improvement. |
kgabryje
force-pushed
the
perf/native-filters
branch
from
00ca6c4 to
cf328e6
Compare
Codecov Report
@@ Coverage Diff @@
## master #17115 +/- ##
=======================================
Coverage 76.89% 76.90%
=======================================
Files 1038 1038
Lines 55515 55540 +25
Branches 7564 7564
=======================================
+ Hits 42690 42712 +22
- Misses 12575 12578 +3
Partials 250 250
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Ephemeral environment shutdown and build artifacts deleted. |
|
@graceguo-supercat Please let me know the results once you've tested 🙂 |
…ive filters (apache#17115) * perf(native-filters): decrease number of redundant rerenders * More perf improvements * lint fix
mistercrunch
added
🏷️ bot
…ive filters (apache#17115) * perf(native-filters): decrease number of redundant rerenders * More perf improvements * lint fix
6 participants
SUMMARY
The goal of this PR is to remove redundant rerenders of key components that build
FilterBarin order to enhance the performance of native filters.Components refactored:
FilterBar,FilterControls,FilterControl,FilterValue,CascadePopover.The method was similar to the one used in PRs #16421, #16444, #16525, #16545.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
The number of redundant rerenders of key components the changes (measured by counting logs produced by
whyDidYouRenderlibrary) - tested onVideo Game Salesdashboard with 5 native filters added:BEFORE
After
Loading time on large test dashboard (~80 charts, 15 native filters) - measured as time from opening the dashboard to all charts and native filters fully loaded:
BEFORE
~60 seconds
before.mov
AFTER
~45 seconda
after.mov
TESTING INSTRUCTIONS
Every feature related to native filters should work the same as before
ADDITIONAL INFORMATION
CC @junlincc @rusackas