Skip to content

perf: create report -> transactions map to optimize performance #55297

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.

Sign up for GitHub

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

Merged
merged 15 commits into from
Jan 22, 2025
Merged

perf: create report -> transactions map to optimize performance #55297

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
cc54548
create report-transactions map to improve performance
TMisiukiewicz Jan 15, 2025
cf9b7e4
chore: refactor to rely on existing map of report transactions from r…
adhorodyski Jan 17, 2025
a861593
chore: wrap hasOnlyPendingTransactions in a useMemo
adhorodyski Jan 17, 2025
3097d47
fix: money report header errors
adhorodyski Jan 17, 2025
cc5e0f0
Merge remote-tracking branch 'upstream/main' into perf/create-map-tra…
TMisiukiewicz Jan 20, 2025
54077fe
fix lint-changed
TMisiukiewicz Jan 20, 2025
2c82b01
fix typecheck
TMisiukiewicz Jan 20, 2025
784e0e9
fix tests
TMisiukiewicz Jan 20, 2025
9effd36
fix lint changed
TMisiukiewicz Jan 20, 2025
9f7ca1d
Merge remote-tracking branch 'upstream/main' into perf/create-map-tra…
TMisiukiewicz Jan 21, 2025
ae77d7e
use every instead of some
TMisiukiewicz Jan 21, 2025
8fc0d31
fix linter
TMisiukiewicz Jan 21, 2025
54dab76
merge main
TMisiukiewicz Jan 22, 2025
1c30c60
code review updates
TMisiukiewicz Jan 22, 2025
596e731
check transactions length
TMisiukiewicz Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 29 additions & 4 deletions src/libs/TransactionUtils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ type BuildOptimisticTransactionParams = {
};

let allTransactions: OnyxCollection<Transaction> = {};
let transactionsByReport: Record<string, Set<string>> = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.TRANSACTION,
waitForCollectionCallback: true,
Expand All @@ -76,6 +77,18 @@ Onyx.connect({
return;
}
allTransactions = Object.fromEntries(Object.entries(value).filter(([, transaction]) => !!transaction));

transactionsByReport = {};
Object.entries(allTransactions).forEach(([transactionID, transaction]) => {
if (!transaction?.reportID) {
return;
}

if (!transactionsByReport[transaction.reportID]) {
transactionsByReport[transaction.reportID] = new Set();
}
transactionsByReport[transaction.reportID].add(transactionID);
});
},
});

Expand Down Expand Up @@ -818,10 +831,22 @@ function hasRoute(transaction: OnyxEntry<Transaction>, isDistanceRequestType?: b
}

function getAllReportTransactions(reportID?: string, transactions?: OnyxCollection<Transaction>): Transaction[] {
const reportTransactions: Transaction[] = Object.values(transactions ?? allTransactions ?? {}).filter(
(transaction): transaction is Transaction => !!transaction && transaction.reportID === reportID,
);
return reportTransactions;
if (!reportID) {
return [];
}

if (transactions) {
return Object.values(transactions).filter((transaction): transaction is Transaction => !!transaction && transaction.reportID === reportID);
}
Copy link
Contributor

@rayane-d rayane-d Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we optimize this for components when transactions are provided also? can we consider using useOnyx with a selector in components to fetch only the relevant transactions for a specific reportID?

App/src/components/ReportActionItem/ReportPreview.tsx

Line 146 in 5d25d69

const [transactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION);

App/src/components/ReportActionItem/ReportPreview.tsx

Line 160 in 5d25d69

const allTransactions = useMemo(() => getAllReportTransactions(iouReportID, transactions), [iouReportID, transactions]);

App/src/components/MoneyReportHeader.tsx

Line 118 in 5d25d69

const [transactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION);

App/src/pages/ReportDetailsPage.tsx

Line 111 in 5d25d69

const [transactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION);

This will reduce computations when transactions change in Onyx.

Pseudocode:

function transactionsByReportIDSelector(transactions: OnyxCollection<Transaction>, reportID: string):  Transaction[] {
    return Object.values(transactions).filter((transaction): transaction is Transaction => !!transaction && transaction.reportID === reportID);
}

const [reportTransactions, reportTransactionsMetadata] = useOnyx({
    key: ONYXKEYS.COLLECTION.TRANSACTION,
    selector: (transactions) => transactionsByReportIDSelector(transactions, reportID),
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rayane-djouah for bringing this, I think we can do even better 😀 Looks like all this work is redundant because in each scenario we pass in the whole initial list of transactions. This means we're not filtering any subset (2nd parameter), but essentially re-do the same work multiple time.

I've also stumbled upon this PR which introduced the same improvement (but based on Array, not Set). I think we can unify all usages of getAllReportTransactions to just getReportTransactions that is there in ReportUtils (instead of TransactionUtils). Implementation with the 2nd parameter is not being used anywhere correctly.

You're also right about moving to useOnyx wherever possible! I'll update the PR today and test this on my end.


const transactionIDs = transactionsByReport[reportID];
if (!transactionIDs) {
return [];
}

return Array.from(transactionIDs)
.map((transactionID) => allTransactions?.[transactionID])
.filter((transaction): transaction is Transaction => !!transaction);
}

function waypointHasValidAddress(waypoint: RecentWaypoint | Waypoint): boolean {
Expand Down
Loading