Optimize OptionData object for search contexts#67073
Optimize OptionData object for search contexts#67073mountiny merged 25 commits intoExpensify:mainfrom
Conversation
Created SearchOptionData type with only 42 essential properties (down from 50+) for search and list contexts, reducing memory usage by 16% per option. Updated createOption function and related search utilities to use optimized interface while maintaining full OptionData for other contexts.
sosek108
changed the title
melvin-bot
bot
requested review from
aldo-expensify
and removed request for
a team
|
@aldo-expensify 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] |
|
@sosek108 do you have an issue to link to this PR? |
aldo-expensify
requested review from
ikevin127
and removed request for
aldo-expensify
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-hybrid.mp4Android: mWeb Chromeandroid-mweb.mp4iOS: HybridAppios-hybrid.mp4iOS: mWeb Safariios-mweb.mp4MacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
(5) Code review completed.
🔄 Moving forward with checklist completion.
| * Sort options by a given comparator and return first sorted options. | ||
| * Function uses a min heap to efficiently get the first sorted options. | ||
| */ | ||
| function optionsOrderBy<T = SearchOptionData>(options: T[], comparator: (option: T) => number | string, limit?: number, filter?: (option: T) => boolean | undefined, reversed = false): T[] { |
There was a problem hiding this comment.
NAB: Missing Error Handling - Here are some thoughts on this function based on comprehensive review, noting that some of the concerns might not be valid:
1. Comparator Function Failures
The comparator function is called multiple times without error handling:
// These calls can throw exceptions if comparator fails
if (comparator(option) > comparator(peekedValue)) {
// ...
}What could go wrong:
• If option.text is undefined and comparator tries to call .toLowerCase() on it
• If option.lastVisibleActionCreated is an invalid date string
• If the comparator accesses a property that doesn't exist on the option object
• If the comparator receives malformed data that causes runtime errors
Example failure scenario:
const recentReportComparator = (option: SearchOptionData) => {
// This could throw if private_isArchived is undefined and causes type coercion issues
// or if lastVisibleActionCreated is not a valid date string
return `${option.private_isArchived ? 0 : 1}_${option.lastVisibleActionCreated ?? ''}`;
};
// If lastVisibleActionCreated is unexpectedly null or contains invalid characters,
// the string concatenation might result in comparison issues2. Filter Function Failures
if (filter && !filter(option)) {
return;
}What could go wrong:
• Filter function throws an exception when processing malformed option data
• Filter function tries to access properties that don't exist
• Filter function performs expensive operations that timeout or fail
3. Heap Operation Failures
heap.push(option);
heap.pop();
const peekedValue = heap.peek();What could go wrong:
• Memory allocation failures during heap operations
• Heap corruption due to invalid comparator results
• Stack overflow in recursive heap operations with very large datasets
4. Input Validation Missing (this depends on the filtering function)
options.forEach((option) => {
// No validation that option is a valid objectWhat could go wrong:
• options array contains null or undefined values
• options array is not actually an array
• Individual options are malformed or missing required properties
Real-World Bug Scenarios These Could Cause
Scenario 1: Crash During Search
// User types in search, SearchOptionData has corrupted lastVisibleActionCreated
const corruptedOption = {
// ... other properties
lastVisibleActionCreated: "invalid-date-string",
private_isArchived: undefined // Should be boolean
};
// This causes recentReportComparator to fail
const recentReportComparator = (option: SearchOptionData) => {
return `${option.private_isArchived ? 0 : 1}_${option.lastVisibleActionCreated ?? ''}`;
// TypeError: Cannot read property of undefined, or string comparison fails
};App Impact: Complete crash of search functionality, white screen, user can't search for chats/people.
Scenario 2: Performance Degradation
// Filter function has an expensive operation that fails
const expensiveFilter = (option: SearchOptionData) => {
// This could throw if text is null/undefined
return option.text.toLowerCase().includes(searchTerm.toLowerCase());
};App Impact: Search becomes extremely slow or hangs, making the app unresponsive.
Scenario 3: Inconsistent Results
// Comparator fails for some items but not others
const inconsistentComparator = (option: SearchOptionData) => {
if (!option.lastVisibleActionCreated) {
throw new Error("Missing timestamp"); // Only some options fail
}
return option.lastVisibleActionCreated;
};App Impact: Search results are inconsistent, some valid options disappear from results, user can't find their conversations.
Scenario 4: Memory Leaks
// Heap operations fail midway, leaving heap in corrupted state
options.forEach((option) => {
heap.push(option); // Fails on 500th iteration due to memory issues
// Heap is now in inconsistent state, subsequent operations fail
});App Impact: App gradually slows down, eventually crashes due to memory issues.
Comprehensive Error Handling Solution
(use as a guide, only for issues which are actually valid / make sense in real-world scenarios)
Code spoiler
function optionsOrderBy<T = SearchOptionData>(
options: T[],
comparator: (option: T) => number | string,
limit?: number,
filter?: (option: T) => boolean | undefined,
reversed = false
): T[] {
// Input validation
if (!Array.isArray(options) || options.length === 0) {
return [];
}
if (typeof comparator !== 'function') {
console.error('[OptionsListUtils] Invalid comparator function provided');
return options.slice(0, limit || options.length);
}
try {
Timing.start(CONST.TIMING.SEARCH_MOST_RECENT_OPTIONS);
const heap = reversed ? new MaxHeap<T>(comparator) : new MinHeap<T>(comparator);
const maxProcessingTime = 5000; // 5 second timeout
const startTime = Date.now();
for (const option of options) {
// Timeout protection
if (Date.now() - startTime > maxProcessingTime) {
console.warn('[OptionsListUtils] Processing timeout, returning partial results');
break;
}
// Validate option
// Note: This is usually done through the filteringFunction, but the one that takes
// report: SearchOption<Report> as argument is missing some checks
if (!option || typeof option !== 'object') {
continue;
}
// Safe filter execution
try {
if (filter && !filter(option)) {
continue;
}
} catch (filterError) {
console.warn('[OptionsListUtils] Filter function failed for option:', filterError);
continue; // Skip this option, don't break entire operation
}
// Safe heap operations
try {
if (limit && heap.size() >= limit) {
const peekedValue = heap.peek();
if (!peekedValue) {
// This shouldn't happen with proper heap implementation
console.warn('[OptionsListUtils] Heap peek returned null when size >= limit');
heap.push(option);
continue;
}
try {
if (comparator(option) > comparator(peekedValue)) {
heap.pop();
heap.push(option);
}
} catch (comparatorError) {
console.warn('[OptionsListUtils] Comparator failed:', comparatorError);
// Continue processing other options
continue;
}
} else {
heap.push(option);
}
} catch (heapError) {
console.error('[OptionsListUtils] Heap operation failed:', heapError);
// Try to continue with remaining options
continue;
}
}
// Extract results safely
const result: T[] = [];
try {
while (!heap.isEmpty() && result.length < (limit || Number.MAX_SAFE_INTEGER)) {
const item = heap.pop();
if (item) {
result.unshift(item); // MinHeap gives smallest first, we want reverse order
}
}
} catch (extractError) {
console.error('[OptionsListUtils] Failed to extract results from heap:', extractError);
// Return what we have so far
}
Timing.end(CONST.TIMING.SEARCH_MOST_RECENT_OPTIONS);
return result;
} catch (error) {
console.error('[OptionsListUtils] Critical error in optionsOrderBy:', error);
Timing.end(CONST.TIMING.SEARCH_MOST_RECENT_OPTIONS);
// Fallback: return simple sorted subset
try {
const filtered = options.filter((option, index) => {
if (index > 1000) return false; // Limit processing to prevent further issues
try {
return !filter || filter(option);
} catch {
return false;
}
});
return filtered.slice(0, limit || filtered.length);
} catch {
// Last resort: return empty array
return [];
}
}
}cc @aldo-expensify for some eyes on this one as well since it's an impactful function for this PR
There was a problem hiding this comment.
Thanks for such deep analysis! I am going to look into that. Also feedback from @aldo-expensify will definitely be handy as well.
There was a problem hiding this comment.
- Comparator Function Failures
Sorry if I am missing something but from the example you brought, I could not see how it could crash as all the values have fallbacks or are primitives that should be coerced properly. Could you give more insight of what you had in mind? Thanks ❤️
There was a problem hiding this comment.
- Filter Function Failures
Exceptions raised in filter function will bubble up and should be handled by parent-level try/catch block:
// Safe filter execution
try {
if (filter && !filter(option)) {
continue;
}
} catch {
continue; // Skip this option, don't break entire operation
}
```There was a problem hiding this comment.
- Heap Operation Failures & 4. Memory Leaks
In theory, yes. How would you guard against that?
I've looked through the usages of both MinHeap and MaxHeap - they are used inside optionsOrderBy helper. Unless the invocation of this function is closed over and its reference is kept alive, those heaps should be garbage-collected by the runtime GC, making it's quite unlikely to crash due to memory overflow.
Do you know any memory allocation control mechanisms that we are using for heavy memory use-cases in the app right now (preferably those who are persisted between callstack states, i.e. global scope).
There was a problem hiding this comment.
Sorry if I am missing something but from the example you brought, I could not see how it could crash as all the values have fallbacks or are primitives that should be coerced properly.
I think we're good to go here since as you said they have solid fallbacks 👍
Thanks for thoughtfully addressing the comments!
src/libs/OptionsListUtils.ts
Outdated
| // if maxElements is passed, filter the recent reports by searchString and return only most recent reports (@see recentReportsComparator) | ||
| const searchTerms = deburr(searchString ?? '') |
There was a problem hiding this comment.
NAB: Performance Concerns (might not be 100% valid, but thought I should mention it)
The new implementation in getValidOptions performs multiple iterations and filtering operations that could be optimized:
// Lines 1863-1900: Multiple iterations over the same data
const searchTerms = deburr(searchString ?? '')
.toLowerCase()
.split(' ')
.filter((term) => term.length > 0);
const filteringFunction = (report: SearchOption<Report>) => {
// ... complex filtering logic that runs for every report
};
filteredReports = optionsOrderBy(options.reports, recentReportComparator, maxElements, filteringFunction);
// Then later, another filtering operation:
const {recentReports, workspaceOptions, selfDMOption} = getValidReports(filteredReports, {
// ... more filtering
});Problem: The code performs search term processing and filtering multiple times, which could be expensive for large datasets.
Suggestion: Use memoization and combine filtering operations:
const processedSearchTerms = useMemo(() => {
return deburr(searchString ?? '')
.toLowerCase()
.split(' ')
.filter((term) => term.length > 0);
}, [searchString]);
// Combine filtering operations to reduce iterations
const combinedFilteringFunction = useCallback((report: SearchOption<Report>) => {
// Combine all filtering logic here
return isValidReport(report, config) && searchTermsMatch(report, processedSearchTerms);
}, [config, processedSearchTerms]);Since useMemo and useCallback are not available outside of react components, one can go with manual memoization, using a caching mechanism. This would reduces redundant calculations by returning cached results for previously computed inputs. Here’s how to approach it:
- Define a Cache Using Map: If we're expecting complex objects as keys, consider using a
WeakMapfor garbage collection support - for simpler keys or when you control input lifecycle, aMapsuffices. - Generate a Unique Key for Caching: Construct a string or hash that represents the unique parameters for each call to
getValidOptions- ensure it captures all relevant input. - Implement Caching Logic: Check the cache before performing computations. If an entry exists, return the cached result; otherwise, compute and cache it.
- Clear Cache When Needed: Consider when you'd want to invalidate or clear the cache, such as significant data changes, to ensure data consistency.
Benefits of Manual Memoization
- Efficiency: Avoids redundant processing by leveraging cached results, reducing computational overhead.
- Scalability: Improves performance especially in large datasets where filters and computations are expensive.
- Flexibility: Allows custom logic for cache invalidation and key construction tailored to our performance needs.
Considerations:
- Memory Usage: Cached data consumes memory. Choose a strategy that balances speed with resource usage, possibly involving LRU (Least Recently Used) cache strategies.
- Complexity: Ensure the key generation logic is efficient and produces unique identifiers for the parameter sets.
- Invalidation Strategy: Understand when and why to invalidate the cache to ensure data integrity and freshness.
By implementing memoization thoughtfully, we can achieve meaningful performance improvements, especially in frequent operations with repetitive inputs.
There was a problem hiding this comment.
Thanks for raising this! Appreciate you calling out the potential performance concern.
That said, I would suggest we can implement it as a next iteration on working of the performance if actually needed, as current PR volume is already quite big.
As you mentioned yourself, the suggestion lacks data needed to ensure such memoization would benefit us instead of undermining overall results of the PR.
If you agree, I would consider it as a next step after rolling out this PR. Let me know what you think, thanks :)
There was a problem hiding this comment.
The performence concers are valid and I agree that we can handle them in anouther round of Proposal/Solution
|
❌ |
|
Here's a test from iPhone Simulator (this is slower than a real device running the production app):
cache-clean.mp4Noting that the data does show up after some delay, which might be API call related, taking a while on iPhone Simulator since after calling On Web it's much faster, barely noticeable (which is what's expected on native production app as well): Screen.Recording.2025-07-26.at.16.19.57.mov |
|
🟢 PR Reviewer Checklist completed. Awaiting for the author / CME to address the code review related comments before Approving. |
|
Hi @ikevin127, thanks for the review 👋 I am filling in for @sosek108 as he is on vacation. I am going to resolve conflicts and address your comments. |
Co-authored-by: Kevin Brian Bader <56457735+ikevin127@users.noreply.github.com>
Addressed ✅ |
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
LGTM: Comments were addressed and changes applied where needed ✅
@aldo-expensify Take it away, let us know your take on some of my previous comments (if you think they require attention).
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
Whoa, this PR is huge! It seems (at a first quick pass) to go well beyond the stated P/S. I'm happy to review it, but:
- There was no discussion of replacing the SuffixUkkonenTree with a MaxHeap
- I expected to see benchmarks (for a PR whose only claim is a performance improvement, it's essential)
- I expected to see screenshots in the PR description on all platforms
|
@roryabraham @ikevin127 All checks are passing now. PR is ready for another review |
|
@roryabraham Kind bump about this PR. Today I've resolved merge conflicts. |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Approving to move this ahead before conflicts come in
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
This PR has caused: selection is not working properly on reports > filters > from/to pages. Particularly coz of passing default reportID
Why it broke the selection functionality ? App/src/components/Search/SearchFiltersParticipantsSelector.tsx Lines 198 to 200 in 4982940 Now since string(0) is a truthy value this check is broken. Solution: Since SearchFiltersParticipantsSelector component only has participants and not reports, this check should have never existed in the first place. App/src/components/Search/SearchFiltersParticipantsSelector.tsx Lines 198 to 200 in 4982940 |
Are you sure? As far I can see, reports are used there for The declaration in |
|
I am pretty positive SearchFiltersParticipantsSelector has no reports included as list item. On the other hand changes you suggested would be a safer option at the moment as the PR is about to hit staging. |
|
@ChavdaSachin I've already posted a Proposal #67123 (comment). I hope we can process this quickly |
|
Yes I was able to confirm, it is safe to remove the check I suggested. |
|
Explanation: App/src/components/Search/SearchFiltersParticipantsSelector.tsx Lines 65 to 66 in b0ebd29 Then Inside App/src/libs/OptionsListUtils/index.ts Line 1793 in b0ebd29 And filteringFunction here used isValidReport function to filter out invalid options.App/src/libs/OptionsListUtils/index.ts Line 1781 in b0ebd29 Notice we are passing the config to isValidReport function which is received from SearchFiltersParticipantsSelector page and that config does not include params such as includeMultipleParticipantReports, includeThreads, includeMoneyRequests .... and hence isValidReport function uses it's default params, and filters out all the reports ultimately.
App/src/libs/OptionsListUtils/index.ts Lines 1473 to 1481 in b0ebd29 cc. @sosek108 |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.1-0 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.1-20 🚀
|
| if (result.private_isArchived) { | ||
| result.lastMessageText = translateLocal('reportArchiveReasons.default'); | ||
| } else { | ||
| result.lastMessageText = report.lastMessageText ?? ''; |
There was a problem hiding this comment.
Coming from #72537 checklist: This change get incorrect lastMessageText cause by previous logic we get lastMesssageText from getLastMessageTextForReport function so we have to restore this logic to resolve #72537
- const lastMessageTextFromReport = getLastMessageTextForReport(report, lastActorDetails, undefined, !!result.private_isArchived);
- let lastMessageText = lastMessageTextFromReport;9 participants
Created SearchOptionData type with only 42 essential properties (down from 50+) for search and list contexts, reducing memory usage by 16% per option. Updated createOption function and related search utilities to use optimized interface while maintaining full OptionData for other contexts.
Explanation of Change
Optimizes memory usage in search and option list contexts by creating a targeted SearchOptionData interface that includes only the properties actually used, reducing memory allocation by 16% per option object.
Fixed Issues
$ #67123
PROPOSAL: #67123
Tests
Search
Create expense
Offline tests
N/A
QA Steps
Search
Create expense
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))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
android.mov
Android: mWeb Chrome
iOS: Native
ios.mov
Nagranie.z.ekranu.2025-07-24.o.16.44.04.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
https://github.com/user-attachments/assets/4c9b89d8-e426-422f-8d15-3d8c2fac5d6e