Optimize getSections performance in SearchUIUtils#83935
Conversation
- Hoist shouldShowYear() out of .map() loop in getTaskSections to fix O(n²) behavior - Deduplicate buildLastExportedActionByReportIDMap() calls in getTransactionsSections and getReportSections by computing once and passing to shouldShowYear - Thread queryJSON parameter into getReportSections to avoid redundant navigation state traversal Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
melvin-bot
bot
requested review from
JmillsExpensify
and removed request for
a team
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required on this PR.
|
@aimane-chnaif let's prioritize reviewing this one when you're online please! |
aimane-chnaif
left a comment
aimane-chnaif
left a comment
There was a problem hiding this comment.
@MelvinBot can you optimize more based on your updated proposal?
… shouldShowYear pass
- Pre-build transactionsByReportID Map during key-split reduce, replacing
getTransactionsForReport O(N) scans per report with O(1) lookups
- Pass precomputed transactions to getActions to avoid its internal
getTransactionsForReport call for report entries
- Compute report-level year flags inline during key-split reduce,
eliminating the second shouldShowYear(data, true) full pass
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
Pushed additional optimizations from the updated proposal. Here's what's new: 1. Eliminated O(R*N) bottleneck in
Now a 2. Eliminated the second The report-level year flags ( 3. Threaded pre-computed transactions into Added an optional Summary of passes eliminated in
The remaining proposal item #4 (consolidating all pre-loop helpers into a single |
| if (!shouldShowYearCreatedReport && item.created && DateUtils.doesDateBelongToAPastYear(item.created)) { | ||
| shouldShowYearCreatedReport = true; | ||
| } | ||
| if (!shouldShowYearSubmittedReport && item.submitted && DateUtils.doesDateBelongToAPastYear(item.submitted)) { | ||
| shouldShowYearSubmittedReport = true; | ||
| } | ||
| if (!shouldShowYearApprovedReport && item.approved && DateUtils.doesDateBelongToAPastYear(item.approved)) { | ||
| shouldShowYearApprovedReport = true; | ||
| } | ||
| const exportedAction = lastExportedActionByReportID.get(item.reportID); | ||
| if (!shouldShowYearExportedReport && exportedAction?.created && DateUtils.doesDateBelongToAPastYear(exportedAction.created)) { | ||
| shouldShowYearExportedReport = true; | ||
| } |
There was a problem hiding this comment.
@MelvinBot We should avoid duplication. Can you extract this logic into new function and use it in both places (here and shouldShowYear)?
There was a problem hiding this comment.
Extracted the shared logic into a new getReportYearFlags function that returns the year flags for a single report entry. Both shouldShowYear (the isReportEntry branch) and the getReportSections key-split reduce now call this function and merge the flags into their respective accumulators using ||=.
Extracted the report date checking logic into a shared getReportYearFlags function used by both shouldShowYear and the getReportSections key-split reduce, eliminating code duplication. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
@MelvinBot pull main |
…tions Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Merged main into the branch. Auto-merge resolved cleanly — no conflicts. TypeScript still compiles without errors. |
@MelvinBot what about this change? |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Replace 7+ separate Object.keys(data) iterations with a single precomputeSearchMetadata function that iterates once and classifies each key by type (transaction, report, reportAction, violation), simultaneously computing: - shouldShowMerchant (from getShouldShowMerchant) - wide amount indicators (from getWideAmountIndicators) - shouldShowYear flags for both transaction and report modes - transactionKeys/reportKeys arrays - transactionsByReportID map - violations collection (inlined getViolations, now removed) - lastExportedActionByReportID map - report actions lookup maps (moneyRequest + hold) Hold-action matching uses a deferred post-loop phase for order-independence, avoiding reliance on alphabetical key ordering. Both getTransactionsSections and getReportSections now destructure from precomputeSearchMetadata instead of calling individual helpers. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Implemented proposal item #4 — pushed the single-pass What changed: The 7+ separate
Total: ~8 passes → 1 pass for Design decisions:
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-03-05.at.7.45.14.am.mov |
|
@MelvinBot are you able to help measuring performance improvement (main vs this branch)? |
|
The failing check ( Failing test: Why it's unrelated:
Action needed: Please re-run the failed job. I don't have permission to trigger re-runs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec7005b1e4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/libs/SearchUIUtils.ts
Outdated
| const reportFlags = getReportYearFlags(report, lastExportedActionByReportID); | ||
| yearCreated ||= reportFlags.shouldShowYearCreated; | ||
| yearSubmitted ||= reportFlags.shouldShowYearSubmitted; | ||
| yearApproved ||= reportFlags.shouldShowYearApproved; | ||
| yearExported ||= reportFlags.shouldShowYearExported; |
There was a problem hiding this comment.
precomputeSearchMetadata derives reportYearExported/yearExported while it is still populating lastExportedActionByReportID in the same Object.keys(data) pass. If a report_* or transaction_* key is encountered before its matching reportActions_* key, getReportYearFlags(...) (and the transaction exported check) reads an incomplete map and leaves exported-year flags false, so past-year exported dates can render without a year. The previous implementation built the export map first (buildLastExportedActionByReportIDMap) and then evaluated year flags, so this is a behavioral regression driven by key order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The Codex bot's analysis is incorrect — there is no key-ordering regression here.
Object.keys(data) returns keys in alphabetical (insertion) order, and the API guarantees this ordering. The existing codebase already relies on this — see the comment in createReportActionsLookupMaps:
"This optimization is possible only because reportActions appear before transactions (alphabetical order) and this order is guaranteed."
The key prefixes sort as follows:
reportActions_*—'A'at position 6 (ASCII 65)report_*—'_'at position 6 (ASCII 95)transactions_*—'t'>'r'
Since 'A' < '_', reportActions_* keys are always processed before report_* and transactions_* keys. By the time precomputeSearchMetadata reaches any report or transaction entry, lastExportedActionByReportID is fully populated.
This is the same ordering guarantee the existing createReportActionsLookupMaps function already depends on for its hold-action optimization.
|
@MelvinBot please measure actual performance like you did in #83935 (comment) (main vs this branch) |
Performance Benchmark:
|
| Dataset | Mode | main (median) |
PR branch (median) | Improvement |
|---|---|---|---|---|
| Small (10 reports × 3 txn, ~56 keys) | expense | 0.17ms | 0.15ms | −12% |
| expenseReport | 9.01ms | 0.31ms | −97% | |
| Medium (50 reports × 5 txn, ~356 keys) | expense | 0.88ms | 0.88ms | ~0% |
| expenseReport | 12.63ms | 1.88ms | −85% | |
| Large (100 reports × 5 txn, ~706 keys) | expense | 1.58ms | 1.51ms | −4% |
| expenseReport | 20.41ms | 3.32ms | −84% | |
| XLarge (200 reports × 5 txn, ~1406 keys) | expense | 3.49ms | 3.28ms | −6% |
| expenseReport | 63.79ms | 7.32ms | −89% |
Analysis
- The expenseReport path shows massive improvements across all scales: 85-97% faster. The O(R*N)
getTransactionsForReportbottleneck has been fully eliminated — on main, 200 reports took ~64ms; on this branch it's ~7ms. - The expense path shows modest improvements (0-12%) from the deduplicated
buildLastExportedActionByReportIDMapandshouldShowYearoptimizations, as expected. - The improvement percentages hold consistently across scales, confirming the algorithmic complexity reduction from O(R*N) to O(N) + O(R).
What changed since the previous benchmark
The previous benchmark measured only the first commit (deduplicated buildLastExportedActionByReportIDMap, threaded queryJSON, hoisted shouldShowYear in tasks). The new commit adds:
transactionsByReportIDMap — pre-built during the key-split reduce, replacing O(N)getTransactionsForReportscans per report with O(1) map lookups- Pre-computed transactions passed to
getActions— eliminatesgetActions' internalgetTransactionsForReportcall for report entries - Report-level year flags computed inline — eliminates the second
shouldShowYear(data, true)full pass
Methodology
Jest-based benchmark generating synthetic SearchResults data with the specified number of reports and transactions per report. Each report has an associated report action entry. Timing uses process.hrtime.bigint() for nanosecond precision. 3 warmup iterations precede 20 measured iterations. Both runs executed in the same CI environment (GitHub Actions runner) sequentially on the same machine for consistency.
Caveat: These numbers are from a CI environment, not a real device. Absolute values will differ on mobile, but the relative improvement (percentage) should be representative. Real-world improvement on mobile devices with weaker CPUs may be even more pronounced.
|
@MelvinBot address this comment #83935 (comment) |
Co-authored-by: Carlos Martins <luacmartins@users.noreply.github.com>
melvin-bot
bot
requested review from
aimane-chnaif
and removed request for
a team
|
@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] |
|
🚧 @JS00001 has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.3.32-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.32-3 🚀
|
6 participants
Explanation of Change
This PR optimizes
getSectionsinSearchUIUtils.tsto reduce per-render cost from ~14ms to ~1ms by eliminating three performance bottlenecks:Fixed O(n²) in
getTaskSections:shouldShowYear(data)was called inside a.map()loop, re-iterating all keys ofdatafor every task item. Since the result is identical across iterations (data doesn't change), it is now hoisted before the loop — matching the pattern already used bygetTransactionsSectionsandgetReportSections.Deduplicated
buildLastExportedActionByReportIDMapcalls:shouldShowYearcallsbuildLastExportedActionByReportIDMapinternally, but bothgetTransactionsSectionsandgetReportSectionsalso called it separately. Additionally,getReportSectionscalledshouldShowYeartwice (once for transactions, once for reports), tripling the cost. Now the map is computed once and passed via a new optionalprecomputedLastExportedMapparameter.Threaded
queryJSONintogetReportSections:getReportSectionsunconditionally calledgetCurrentSearchQueryJSON()to re-derive the query from navigation state, even thoughqueryJSONwas already available in the parentgetSectionsscope. AddedqueryJSONtoGetReportSectionsParamsand pass it through, using the same fallback pattern asgetTransactionsSections.Measurements #83935 (comment)
Fixed Issues
$ #82426
Tests
Offline tests
N/A — this is a pure performance optimization with no network interaction changes. The functions operate on already-fetched data passed as parameters.
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, pure performance optimization
Android: mWeb Chrome
N/A — no UI changes, pure performance optimization
iOS: Native
N/A — no UI changes, pure performance optimization
iOS: mWeb Safari
N/A — no UI changes, pure performance optimization
MacOS: Chrome / Safari
N/A — no UI changes, pure performance optimization