-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
fix: improve getDisplayNameForParticipant performance #45907
fix: improve getDisplayNameForParticipant performance #45907
Conversation
@marcochavezf Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
This is a left-heavy view from Jason's trace where we can see a cumulated result for all the invocations to this function. The end result will depend on the end cache hit rate cc @kacper-mikolajczak Ideally we'd have before+after for this very account, on paper this looks like can be brought down to be eg. 10x less. |
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.
Great finding, thanks Olimpia! Curious if we could improve that further by employing cache on higher level (cache formatPhoneNumber
entirely)
if (!personalDetails) { | ||
return ''; | ||
} |
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.
It looks like this early return was added in the PR. What is its impact on the function flow?
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.
I added it to avoid unnecessary calculations and function calls if personalDetails
is not available. I think this minor optimization may help in scenarios where the function may be called frequently.
src/libs/ReportUtils.ts
Outdated
let formattedLogin = phoneNumberCache[personalDetails.login ?? '']; | ||
if (!formattedLogin) { | ||
formattedLogin = LocalePhoneNumber.formatPhoneNumber(personalDetails.login ?? ''); | ||
// Store the formatted phone number in the cache | ||
phoneNumberCache[personalDetails.login ?? ''] = formattedLogin; |
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.
What do you think about extracting personalDetails.login ?? ''
into variable?
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.
done
Thanks! @kacper-mikolajczak I think caching on a higher level might be a good idea. I will create a separate PR for this. Or do you mean adding caching to the |
Yeah let's treat it as next improvement in standalone PR. If it would be worth it performance-wise to cache phone number formatter entirely, I think we could also use memoization tool which is going to be merged soon :) |
Reviewer Checklist
Screenshots/VideosNOTE: I tested on the intro page of workspaces as we use Android: NativeScreen.Recording.2024-07-23.at.1.33.48.AM.movAndroid: mWeb ChromeScreen.Recording.2024-07-23.at.1.34.45.AM.moviOS: NativeScreen.Recording.2024-07-23.at.1.37.16.AM.moviOS: mWeb SafariScreen.Recording.2024-07-23.at.1.38.25.AM.movMacOS: Chrome / SafariScreen.Recording.2024-07-23.at.1.27.05.AM.movMacOS: DesktopScreen.Recording.2024-07-23.at.1.44.31.AM.mov |
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.
Changes looking good, @allgandalf is doing a checklist
@@ -1859,6 +1859,9 @@ function getPersonalDetailsForAccountID(accountID: number): Partial<PersonalDeta | |||
} | |||
|
|||
const hiddenTranslation = Localize.translateLocal('common.hidden'); | |||
|
|||
const phoneNumberCache: Record<string, string> = {}; |
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.
Are there any other functions we could use this cache in this Utils file? Lets see if we can use it there in next PR
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.
I think the way forward for this would be to reuse the global cache proposed by Kacper in the other PR so we can swap every usage with this cached one in one go.
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 appears to be the only place in this file where the formatPhoneNumber
is used
@OlimpiaZurek do we need testing steps here for the QA? |
@allgandalf I think it's just about testing the LHN in general - it's the only affected part of the app, ordering & the displayNames should work the same way as before. Other than that, no changes. |
Cool, I am testing accordingly wherever https://github.com/search?q=repo%3AExpensify%2FApp%20getDisplayNameForParticipant&type=code |
Yup that's the perfect way, thanks! |
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.
LGTM, functionality didn't get affected! I was stunned by reducing the time to micro seconds by caching 🫨 , great work here @adhorodyski @OlimpiaZurek and everyone involved
Thanks for the testing |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.11-0 🚀
|
🚀 Cherry-picked to staging by https://github.com/francoisl in version: 9.0.11-2 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.11-5 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.12-0 🚀
|
Details
This PR improves the performance of getDisplayNameForParticipant by adding a caching mechanism for phone number formatting.
Before:
After:
Fixed Issues
$ #45925
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop