[HOLD #42200] Fix Intl NumberFormat calls #40341
Conversation
This comment has been minimized.
This comment has been minimized.
|
Hi @quinthar! This is a PR which contains POC we've been discussing here. It tries to improve behaviour of If you could test flow below (which is based on your actions here, so if you feel like it does not match exactly I would be grateful for correction!) on your Android device and post the results in the thread 🙏
|
kacper-mikolajczak
force-pushed
the
fix/intl-number-format
branch
from
3135b80
to
e994e69
Compare
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
Running a new build here https://github.com/Expensify/App/actions/runs/9059268231 |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
|
Heyo @mountiny, shall we put the PR review on hold while we are still discussing how to proceed with the implementation? |
|
lemme know when this is ready for review! |
|
@dangrous I think it is ready |
mountiny
changed the title
|
Put it on hold for the library addition issue started a new build #40341 |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
|
@mananjadhav please go ahead with the review |
| import type {ValueOf} from 'type-fest'; | ||
| import type CONST from '@src/CONST'; | ||
|
|
||
| const numberFormatter = moize(Intl.NumberFormat, { | ||
| isDeepEqual: true, | ||
| maxSize: 10, |
There was a problem hiding this comment.
question, how do we determine the maxSize to be 10?
There was a problem hiding this comment.
It's arbitrary number based on the testing, where we did not tend to go beyond such cache size. We can adjust that if there is better reasoning 👍
There was a problem hiding this comment.
NAB - just to confirm how this works - with maxSize 10, is that only ten sets of in this case locale, options, and number? It seems like we'd need a lot more than 10... but is the performance issue right now that this gets called multiple times in related requests with the same inputs?
| maxSize: 10, | ||
| profileName: 'Intl.NumberFormat', | ||
| }); | ||
|
|
||
| function format(locale: ValueOf<typeof CONST.LOCALES>, number: number, options?: Intl.NumberFormatOptions): string { |
There was a problem hiding this comment.
What's the difference between memoizing the format vs Intl.format calls?
There was a problem hiding this comment.
Sorry, I am not sure if I get the question right - could you please tell me more details? Thanks!
We are caching just Intl.NumberFormat. Both format and formatToParts are the consumers of the cached class.
There was a problem hiding this comment.
Yeah I got that. I meant, instead of caching Intl.NumberFormat, what if we cache format and formatToParts. Does that give us any benefit?
const formatLocal = moize(format, {
isDeepEqual: true,
maxSize: 10,
profileName: 'format',
});
const formatToPartsLocal = moize(formatToParts, {
isDeepEqual: true,
maxSize: 10,
profileName: 'formatToParts',
});
export { formatLocal as format, formatToPartsLocal as formatToParts }
There was a problem hiding this comment.
I'd say it won't help as they rely on same API (the Intl.NumberFormat) so caching the root is more straightforward and thus probably better performance-wise (although I did not measure it)
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-intl-memoize.mp4Android: mWeb Chromemweb-chore-intl-memoize.mp4iOS: Nativeios-intl-memoize.mp4iOS: mWeb Safarimweb-safari-intl-memoize.MP4MacOS: Chrome / Safariweb-intl-memoize_7QoyqAbz.mp4MacOS: Desktopdesktop-int-memoize.mov |
| @@ -68,6 +68,7 @@ | |||
| "lodash": "4.17.21", | |||
| "lottie-react-native": "6.5.1", | |||
| "mapbox-gl": "^2.15.0", | |||
| "moize": "^6.1.6", | |||
There was a problem hiding this comment.
do we need to commit this package-lock change because it's a new module? Or should we not push this file. Not sure how it works
There was a problem hiding this comment.
@dangrous I am not sure what is the practice in Expensify. Generally it's been recommended to commit .lock file as it is required on CI (with npm ci) and it specifies all the exact version of the dependencies (locking them, thus the name). However, repo is relying on overrides so it may differ in such case.
There was a problem hiding this comment.
I'd say if the .lock file is already included in the version control, then changes to it should also be reflected there.
Take it with a grain of salt as I am not aware of all the practices in the repo. If you could ask someone you know possesses the knowledge, thanks!
There was a problem hiding this comment.
Ah I found in our docs, this should be good! I don't know why I thought otherwise.
There was a problem hiding this comment.
Thanks for checking that!
| import type {ValueOf} from 'type-fest'; | ||
| import type CONST from '@src/CONST'; | ||
|
|
||
| const numberFormatter = moize(Intl.NumberFormat, { | ||
| isDeepEqual: true, | ||
| maxSize: 10, |
There was a problem hiding this comment.
NAB - just to confirm how this works - with maxSize 10, is that only ten sets of in this case locale, options, and number? It seems like we'd need a lot more than 10... but is the performance issue right now that this gets called multiple times in related requests with the same inputs?
@dangrous Sorry I have not replied before - it must've slipped between my fingers! Moize uses LRU cache which with If tuple of parameters [ On the tested flows, the cache efficiency is quite high (at least 85%). Let's see how the general discussion around Moize resolves and adjust the settings based on that! |
@kacper-mikolajczak Would you mind pasting the screenshot of the results here. External contributors won't have access to this slack thread. |
|
Sure thing @mananjadhav! Here is an exact message:
|
|
And results of mentioned synthetic tests:
|
Oh nice, so it's not per number, just locale and options. That makes more sense! |
|
@kacper-mikolajczak Now that we have another PR, can we close this one out? |
|
@mananjadhav Definitely, thanks! The PR is continued in #43868 |
|
We discussed paying out this issue in Slack. I will get the payment summary posted shortly. |
Payment Summary:Contributor: @mananjadhav paid $250 via NewDot |
|
$250 approved for @mananjadhav |
Details
PR contains a POC of a possible improvement for
Intl.NumberFormatperformance bottlenecks.Fixed Issues
$ #42386
PROPOSAL: https://expensify.slack.com/archives/C05LX9D6E07/p1713358946087599
Tests
Offline tests
QA Steps
Profilebutton in the bottom right corner (it should turn red)reply in a threadon a messageProfilebutton once again (it should turn gray and pop-up should appear)PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios_web.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4