perf(native-filters): Decrease number of unnecessary rerenders in native filters #17115
Conversation
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?
| @@ -73,25 +72,37 @@ const FilterControls: FC<FilterControlsProps> = ({ | |||
| const dashboardHasTabs = useDashboardHasTabs(); | |||
| const showCollapsePanel = dashboardHasTabs && cascadeFilters.length > 0; | |||
|
|
|||
| const cascadePopoverFactory = useCallback( | |||
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
| @@ -274,6 +281,8 @@ const FilterBar: React.FC<FiltersBarProps> = ({ | |||
| ); | |||
| const isInitialized = useInitialization(); | |||
|
|
|||
| const tabPaneStyle = useMemo(() => ({ overflow: 'auto', height }), [height]); | |||
There was a problem hiding this comment.
Isn't this overkill? I remembered this point.
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
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