Skip to content
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

[Audit][Implementation] getOrderedReportIDs - cache removal #36565

Merged
merged 4 commits into from
Feb 20, 2024
Merged

[Audit][Implementation] getOrderedReportIDs - cache removal #36565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
76c7490
remove caching
kacper-mikolajczak Feb 12, 2024
7aad33c
Merge branch 'main' into audit/implementation/get-ordered-raports-ids…
kacper-mikolajczak Feb 15, 2024
c103561
fix merge issues
kacper-mikolajczak Feb 15, 2024
035d65a
Merge branch 'main' into audit/implementation/get-ordered-raports-ids…
kacper-mikolajczak Feb 16, 2024
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
37 changes: 0 additions & 37 deletions src/libs/SidebarUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,22 +58,6 @@ function compareStringDates(a: string, b: string): 0 | 1 | -1 {
return 0;
}

// Define a cache object to store the memoized results
const reportIDsCache = new Map<string, string[]>();

// Function to set a key-value pair while maintaining the maximum key limit
function setWithLimit<TKey, TValue>(map: Map<TKey, TValue>, key: TKey, value: TValue) {
if (map.size >= 5) {
// If the map has reached its limit, remove the first (oldest) key-value pair
const firstKey = map.keys().next().value;
map.delete(firstKey);
}
map.set(key, value);
}

// Variable to verify if ONYX actions are loaded
let hasInitialReportActions = false;

/**
* @returns An array of reportIDs sorted in the proper order
*/
Expand All @@ -88,26 +72,6 @@ function getOrderedReportIDs(
currentPolicyID = '',
policyMemberAccountIDs: number[] = [],
): string[] {
const currentReportActions = allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${currentReportId}`];
let reportActionCount = currentReportActions?.length ?? 0;
reportActionCount = Math.max(reportActionCount, 1);

// Generate a unique cache key based on the function arguments
const cachedReportsKey = JSON.stringify(
[currentReportId, allReports, betas, policies, priorityMode, reportActionCount, currentPolicyID, policyMemberAccountIDs],
// Exclude some properties not to overwhelm a cached key value with huge data, which we don't need to store in a cacheKey
(key, value: unknown) => (['participantAccountIDs', 'participants', 'lastMessageText', 'visibleChatMemberAccountIDs'].includes(key) ? undefined : value),
);

// Check if the result is already in the cache
const cachedIDs = reportIDsCache.get(cachedReportsKey);
if (cachedIDs && hasInitialReportActions) {
return cachedIDs;
}

// This is needed to prevent caching when Onyx is empty for a second render
hasInitialReportActions = Object.values(lastReportActions).length > 0;

const isInGSDMode = priorityMode === CONST.PRIORITY_MODE.GSD;
const isInDefaultMode = !isInGSDMode;
const allReportsDictValues = Object.values(allReports);
Expand Down Expand Up @@ -196,7 +160,6 @@ function getOrderedReportIDs(
// Now that we have all the reports grouped and sorted, they must be flattened into an array and only return the reportID.
// The order the arrays are concatenated in matters and will determine the order that the groups are displayed in the sidebar.
const LHNReports = [...pinnedAndGBRReports, ...draftReports, ...nonArchivedReports, ...archivedReports].map((report) => report.reportID);
setWithLimit(reportIDsCache, cachedReportsKey, LHNReports);
return LHNReports;
}

Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/sidebar/SidebarLinksData.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ const propTypes = {

const defaultProps = {
chatReports: {},
allReportActions: {},
isLoadingApp: true,
priorityMode: CONST.PRIORITY_MODE.DEFAULT,
betas: [],
Expand All @@ -111,6 +110,7 @@ const defaultProps = {
accountID: '',
},
transactionViolations: {},
allReportActions: {},
Copy link
Contributor

Choose a reason for hiding this comment

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

afaik this is still being passed down to the component, right? Is it still necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

the original comment also stripped down the prop being passed as a dependency, is this still a thing?

Copy link
Contributor

Choose a reason for hiding this comment

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

@kacper-mikolajczak Would you be able to check this one out please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't remove this prop. I need this in #35907

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok in that case we can go ahead with merge, right?

Copy link
Contributor

@adhorodyski adhorodyski Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

cc @mountiny @shubham1206agra I'm a little worried that the above mentioned PR can introduce a performance regression in this scope - didn't have time to measure it yet, but since it eg. maps, reduces all of the keys or checks against regexp on this big data set it can become a bottleneck rather quickly (if not already). This is not related to this PR, but I wanted to let you know that we should test it first with this in mind as it directly impacts all of our core metrics.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets keep an eye out for that one, also I have asked Adam to try to add more complex test scenario for the reassure test so we can have better integrated protection from regressing this method

Copy link
Contributor

Choose a reason for hiding this comment

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

@adhorodyski Thanks for the heads up. I, too have the same doubt honestly. I will try to do something about this.

};

function SidebarLinksData({
Expand Down
Loading