Optimize getFeedOptions internal computation#83516
Conversation
Reduce per-render cost of getFeedOptions by eliminating repeated allocations and redundant computation in its call chain: - Hoist feedNamesMapping and its keys array to module scope in getBankName so they are allocated once at import time - Move CSV early-exit check before the linear scan in getBankName - Hoist isCustomFeed array to module scope constant - Replace rest spread in buildFeedKeysWithAssignedCards with a lightweight key check to avoid per-entry object allocation - Hoist getCardFeedsForDisplay call outside the reduce loop in SearchQueryUtils so it runs once instead of per filter value Co-authored-by: Carlos Martins <luacmartins@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot I tried measuring performance again, and it has decreased from 0.76 ms to 0.36 ms on my local environment. Is there any other room for improvement? |
|
Great work on the ~52% improvement (0.76ms → 0.36ms). There are a few more optimization opportunities I identified — though at 0.36ms you're already in a good spot, so these would be diminishing returns: Higher Impact
Lower Impact
Items 1 and 2 are the ones most likely to yield measurable gains. Items 3-5 are micro-optimizations that are worth doing if you're already in the area but probably won't show up in benchmarks individually. That said, 0.36ms is already fast for this kind of utility — unless profiling shows this function is called very frequently per render cycle (e.g., dozens of times due to the |
|
The only failing check is This check will resolve automatically once a reviewer posts their reviewer checklist on the PR. No code fix is needed. |
Moves CUSTOM_FEED_PREFIXES, feedNamesMapping, and feedNamesMappingKeys to the top of CardUtils.ts alongside other module-level constants. Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
|
The only failing check is All other checks are either passing or still pending (tests, typecheck, ESLint, builds, etc.). No code fix is needed. |
|
The only failing check is PR Reviewer Checklist ( All code-related checks (ESLint, typecheck, Prettier, builds, verifySignedCommits, spellcheck) are passing. Tests and perf-tests are still pending. |
|
@luacmartins, if you have an account that makes getFeedOptions take ~12ms per-render. Can you try to measure it again on this PR? |
|
I do, but I've been trying to get a trace for the past 30min and the profiler keeps crashing on that account 😂 |
|
I wasn't able to get a profile in an account, but I wrote perf tests to verify these changes. Here are the results: I think this one is specially significant: |
|
@hoangzinh I think we can go ahead with this PR given the measurements above |
|
@hoangzinh 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] |
|
@luacmartins 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] |
|
The only failing check is PR Reviewer Checklist — it's failing because no reviewer has posted the reviewer checklist comment yet. This is not a code issue and requires no code fix. All code-related checks are passing: ESLint, typecheck, Prettier, all 8 test jobs, builds (iOS/Android/desktop/web), perf-tests, spellcheck, verifySignedCommits, codecov, and CLA. |
sure, I will try to complete checklist today |
|
@MelvinBot can you resolve conflicts? |
Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
|
@luacmartins 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] |
|
Resolved the merge conflict in
The resolution keeps both: the module-scope hoisting from this PR and the PEX/type updates from main. |
|
The failing checks on this PR are not related to the code changes and don't require a code fix:
All code-related checks are passing: tests (all 8 jobs), typecheck, ESLint, Prettier, spellcheck, builds on non-macOS runners, and Storybook tests. |
|
The failing checks are unrelated to this PR's code changes. Failing checks:
Root cause: The Mobile-Expensify submodule's Evidence this is unrelated:
This will likely resolve when the Mobile-Expensify submodule's engine constraint is updated to allow |
|
@MelvinBot can you try to merge the latest main? It was probably fixed |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-02-27.at.15.46.14.movAndroid: mWeb ChromeScreen.Recording.2026-02-27.at.15.42.33.moviOS: HybridAppScreen.Recording.2026-02-27.at.15.53.51.moviOS: mWeb SafariScreen.Recording.2026-02-27.at.15.55.05.movMacOS: Chrome / SafariScreen.Recording.2026-02-27.at.15.39.47.mov |
…dOptions Co-authored-by: Vinh Hoang <hoangzinh@users.noreply.github.com>
|
Done — merged the latest |
|
@luacmartins all yours |
Explanation of Change
Optimizes the internal computation of
getFeedOptionsby eliminating repeated object allocations and redundant work in its call chain. The ~12ms per-render cost came from several compounding inefficiencies:getBankNameallocated a 17-entryfeedNamesMappingobject and its keys array on every call, then ran a linear.find()scan. The CSV early-exit check also ran after the scan instead of before it. Now the mapping and keys are hoisted to module scope (allocated once at import), and CSV feeds exit before the scan.isCustomFeedallocated a new 3-element array on every call. Now hoisted to a module-scopeCUSTOM_FEED_PREFIXESconstant.buildFeedKeysWithAssignedCardsused rest spread destructuring (const {cardList, ...assignedCards} = cards) per workspace card entry, creating a new object just to check if it had any keys. Replaced with a lightweightObject.keys(cards).some(k => k !== 'cardList')check that avoids the allocation.getCardFeedsForDisplaywas called inside a.reduce()loop inSearchQueryUtils.ts— once per feed filter value. Hoisted outside the loop so it runs once.Fixed Issues
$ #82422
PROPOSAL: #82422 (comment)
Tests
Offline tests
These changes are to pure utility functions that compute feed display options from in-memory Onyx data. No network calls are involved, so offline behavior is unchanged.
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso 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
N/A - No UI changes, internal computation optimization only
Android: mWeb Chrome
N/A - No UI changes, internal computation optimization only
iOS: Native
N/A - No UI changes, internal computation optimization only
iOS: mWeb Safari
N/A - No UI changes, internal computation optimization only
MacOS: Chrome / Safari
N/A - No UI changes, internal computation optimization only