Cache Intl instances to improve performance #894
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zbigg
changed the title
zbigg
changed the title
Pull Request Test Coverage Report for Build 10990614340Details
💛 - Coveralls |
|
Visit the preview URL for this PR (updated for commit fffcd45): https://cartodb-fb-storybook-react-dev--pr894-zbigg-cache-intl-v5om38k0.web.app (expires Mon, 30 Sep 2024 08:41:07 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 517cc4d31d7e09cf277774e034094b67c301cd4c |
zbigg
force-pushed
the
zbigg/cache-intl-instances
branch
from
2f2d7b3
to
af88c72
Compare
zbigg
commented
Jul 26, 2024
| // this is very naive cache that bases on fact that Intl instance is actually same for most of time | ||
| // so we can reuse those maps across several instances of same components | ||
| // note, useMemo can't do that globally and flattenMessages over _app_ and c4r is quite costly and would | ||
| // be paid for evey c4r component mounted |
There was a problem hiding this comment.
This cache assumes we have 1 active Intl instance in app which is usually the case.
It can be changed to WeakMap<IntlConfig, IntlShape> if we have use case where one app use many languages at once, not sure if we need it.
There was a problem hiding this comment.
Changed to WeakMap, so we're on safe side.
zbigg
force-pushed
the
zbigg/cache-intl-instances
branch
from
a127d5f
to
31ab222
Compare
zbigg
force-pushed
the
zbigg/cache-intl-instances
branch
from
95e68a8
to
898268f
Compare
donmccurdy
approved these changes
Sep 20, 2024
| // so we can reuse those maps across several instances of same components | ||
| // note, useMemo can't cache accross many that globally and flattenMessages over _app_ and c4r messages is quite costly | ||
| // and would be paid for every c4r component mounted. | ||
| let cachedInstance = intlInstanceCache.get(intlConfig); |
There was a problem hiding this comment.
Suggested change
| let cachedInstance = intlInstanceCache.get(intlConfig); | |
| const cachedInstance = intlInstanceCache.get(intlConfig); |
zbigg
force-pushed
the
zbigg/cache-intl-instances
branch
from
898268f
to
fffcd45
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Cache intl instances globally.
flattenMessagesover c4r and app could take up to 2-4 milliseconds (on fast machine). And with current,useMemoapproach it was executed for each component mounted .. and lost when unmounted.Real app can mount even 10+ c4r+intl-based components a time, causing some mounts to miss sane deadlines and cause warnings like:
(it's not that this call is only one slow in big components, but this can be easily avoided as
intlConfiginstance is generally stable)We also don't want to waste memory to keep next 10+ copies of all messages (3.5k keys)
Type of change
(choose one and remove the others)
Acceptance
N/A performance issue
Basic checklist