-
Notifications
You must be signed in to change notification settings - Fork 97
fix(theming): getColor memoization peformance
#1960
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
Conversation
ze-flo
left a comment
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.
Incredible speed improvement!! 🚄 🚀 🔥🔥
| const BASELINE_NOCACHE = 2780; | ||
| const BASELINE_CACHE = 2000; |
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.
While not critical for this update, we could discuss ways to improve the story without having to rely on a hardcoded BASELINE.
| testGetColor(); | ||
| updateColor(initialColor); | ||
|
|
||
| const endTime = performance.now(); |
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 captured after React has completed all state updates. getColor, on its own, is probably much faster than that! 🚀
Description
As advertised by the supporting stories, this PR swaps
getColorandgetColorV8JSON.stringifyargument memoization with a custom key generation backed byWeakMapcached object comparisons. The before/after results are shown below...Before (
JSON.stringify)After (custom
toKey)Checklist
design updates will be Garden Designer approved (add the designer as a reviewer)npm start)renders as expected with reversed (RTL) directionrenders as expected with Bedrock CSS (?bedrock)tested for WCAG 2.1 AA accessibility compliancetested in Chrome, Firefox, Safari, and Edge