-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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(dashboard): decrease number of rerenders of FiltersBadge #16545
Conversation
Codecov Report
@@ Coverage Diff @@
## master #16545 +/- ##
==========================================
+ Coverage 76.71% 76.74% +0.03%
==========================================
Files 1002 1002
Lines 53780 53846 +66
Branches 6859 6893 +34
==========================================
+ Hits 41257 41325 +68
+ Misses 12286 12285 -1
+ Partials 237 236 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Thanks for the improvement! The code looks good to me. |
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.
Did my best to go through all code changes with a magnifying glass, and everything looks really clean. Also did my best to break this in testing, but wasn't able to find any problems; LGTM 👍
…#16545) * perf(dashboard): decrease rerenders in FiltersBadge * implement caching of dashboard filter indicators * Implement caching for native filter indicators
…#16545) * perf(dashboard): decrease rerenders in FiltersBadge * implement caching of dashboard filter indicators * Implement caching for native filter indicators
SUMMARY
Due to incorrect passing of props and setting state of FiltersBadge, we were rerendering the component multiple times. It's very expensive, as FiltersBadge calls
selectIndicatorsForChart
andselectNativeIndicatorsForChart
functions, which traverse dashboard layout tree. Rerendering filters badge for every chart resulted in thousands of unnecessary traverses of dashboard layout tree, causing the performance to take a hit. This PR introduces caching and memoization for FiltersBadge, which reduces the amount of redundant rerenders.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
I measured the number of rerenders of FiltersBadge with whyDidYouRender library (https://github.com/welldone-software/why-did-you-render). It reports every rerender of a component that wasn't necessary and could've been avoided (e.g. by memoizing props or using PureComponent).
Before:
https://user-images.githubusercontent.com/15073128/131681210-7fd4ff10-fd9d-49c3-b467-2448eb99f7c8.mov
After:
https://user-images.githubusercontent.com/15073128/131681806-7f1da2f8-5ae5-416b-a3c3-6879ce4e4f7f.mov
TESTING INSTRUCTIONS
Everything should work as before
ADDITIONAL INFORMATION
CC @junlincc