Merged
Conversation
kacper-mikolajczak
approved these changes
Mar 8, 2024
adhorodyski
approved these changes
Mar 8, 2024
roryabraham
approved these changes
Mar 8, 2024
grgia
approved these changes
Mar 11, 2024
Contributor
grgia
left a comment
There was a problem hiding this comment.
Interesting to read the description on this one, nice work!
50 tasks
This file contains hidden or 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
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.
Details
This PR is part of implementation phase of the ongoing audit by Callstack. Here we are trying to reduce the extra milliseconds that are adding up due to adding each key individually in
Onyx->getAllKeysmethod when keys are accessed from the storage and then added to the cache in a loop.This operation takes around ~36 ms, see hermes trace below:
Since, we are getting all the keys from storage at this point, we can avoid using
addKeywithin a loop and instead set all of the keys at once. We can do this by exposing a method fromOnyxCacheto achieve this.After these changes the execution time is now around ~22 ms, see hermes trace below:
We get ~15 ms of reduction in the execution time 🚀
Related Issues
Dedicated Issue: Expensify/App#37995
App Startup Audit Issue: Expensify/App#35234 (comment)
Automated Tests
Manual Tests
Author Checklist
### Related Issuessection aboveTestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)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.mov
Android: mWeb Chrome
android-web.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov