From d4b70755159d486c6c760de51ba1787fae90235b Mon Sep 17 00:00:00 2001 From: Tomasz Lesniakiewicz Date: Mon, 26 Feb 2024 15:26:54 +0100 Subject: [PATCH 1/8] feat: memoize SidebarLinksData --- src/pages/home/sidebar/SidebarLinksData.js | 41 ++++++++++++++-------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index c4cc0713c596..0c8125cae79f 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -2,7 +2,7 @@ import {deepEqual} from 'fast-equals'; import lodashGet from 'lodash/get'; import lodashMap from 'lodash/map'; import PropTypes from 'prop-types'; -import React, {useCallback, useEffect, useMemo, useRef} from 'react'; +import React, {memo, useCallback, useEffect, useMemo, useRef} from 'react'; import {View} from 'react-native'; import {withOnyx} from 'react-native-onyx'; import _ from 'underscore'; @@ -136,18 +136,14 @@ function SidebarLinksData({ const reportIDsRef = useRef(null); const isLoading = isLoadingApp; + + const optionItemsMemoized = useMemo( + () => SidebarUtils.getOrderedReportIDs(null, chatReports, betas, policies, priorityMode, allReportActions, transactionViolations, activeWorkspaceID, policyMemberAccountIDs), + [chatReports, betas, policies, priorityMode, allReportActions, transactionViolations, activeWorkspaceID, policyMemberAccountIDs], + ); + const optionListItems = useMemo(() => { - const reportIDs = SidebarUtils.getOrderedReportIDs( - null, - chatReports, - betas, - policies, - priorityMode, - allReportActions, - transactionViolations, - activeWorkspaceID, - policyMemberAccountIDs, - ); + const reportIDs = optionItemsMemoized; if (deepEqual(reportIDsRef.current, reportIDs)) { return reportIDsRef.current; @@ -160,7 +156,7 @@ function SidebarLinksData({ reportIDsRef.current = reportIDs; } return reportIDsRef.current || []; - }, [chatReports, betas, policies, priorityMode, allReportActions, transactionViolations, activeWorkspaceID, policyMemberAccountIDs, isLoading, network.isOffline, prevPriorityMode]); + }, [optionItemsMemoized, priorityMode, isLoading, network.isOffline, prevPriorityMode]); // We need to make sure the current report is in the list of reports, but we do not want // to have to re-generate the list every time the currentReportID changes. To do that @@ -334,4 +330,21 @@ export default compose( initialValue: {}, }, }), -)(SidebarLinksData); +)( + memo( + SidebarLinksData, + (prevProps, nextProps) => + _.isEqual(prevProps.chatReports, nextProps.chatReports) && + _.isEqual(prevProps.allReportActions, nextProps.allReportActions) && + prevProps.isLoadingApp === nextProps.isLoadingApp && + prevProps.priorityMode === nextProps.priorityMode && + _.isEqual(prevProps.betas, nextProps.betas) && + _.isEqual(prevProps.policies, nextProps.policies) && + prevProps.network.isOffline === nextProps.network.isOffline && + _.isEqual(prevProps.insets, nextProps.insets) && + prevProps.onLinkClick === nextProps.onLinkClick && + _.isEqual(prevProps.policyMembers, nextProps.policyMembers) && + _.isEqual(prevProps.transactionViolations, nextProps.transactionViolations) && + _.isEqual(prevProps.currentUserPersonalDetails, nextProps.currentUserPersonalDetails), + ), +); From 88c5fd8e5c29cc7dc1b70d863483abe0fc7db86a Mon Sep 17 00:00:00 2001 From: Tomasz Lesniakiewicz Date: Tue, 27 Feb 2024 11:53:59 +0100 Subject: [PATCH 2/8] add missing props to memo --- src/pages/home/sidebar/SidebarLinksData.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index 0c8125cae79f..59bf74586b4b 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -345,6 +345,7 @@ export default compose( prevProps.onLinkClick === nextProps.onLinkClick && _.isEqual(prevProps.policyMembers, nextProps.policyMembers) && _.isEqual(prevProps.transactionViolations, nextProps.transactionViolations) && - _.isEqual(prevProps.currentUserPersonalDetails, nextProps.currentUserPersonalDetails), + _.isEqual(prevProps.currentUserPersonalDetails, nextProps.currentUserPersonalDetails) && + prevProps.currentReportID === nextProps.currentReportID, ), ); From 3aa217604ce487b9748412e60d1827822c74f1f7 Mon Sep 17 00:00:00 2001 From: Tomasz Lesniakiewicz Date: Tue, 27 Feb 2024 14:13:51 +0100 Subject: [PATCH 3/8] chore: add comments for memoization --- src/pages/home/sidebar/SidebarLinksData.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index 59bf74586b4b..c5749893d7fb 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -331,6 +331,12 @@ export default compose( }, }), )( + /* + While working on audit on the App Start App metric we noticed that by memoizing SidebarLinksData we can avoid 1 additional run of getOrderedReportIDs. + With that we can reduce app start up time by ~2.5s on heavy account. + After finding and fixing core issues with getOrderedReportIDs performance we might remove the memoization + More details - https://github.com/Expensify/App/issues/35234#issuecomment-1926914534 + */ memo( SidebarLinksData, (prevProps, nextProps) => From f726b6b0e192f5b3e8ffdf3e76cf8e648122468a Mon Sep 17 00:00:00 2001 From: Tomasz Lesniakiewicz Date: Mon, 11 Mar 2024 13:24:30 +0100 Subject: [PATCH 4/8] chore: run prettier --- src/pages/home/sidebar/SidebarLinksData.js | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index 613402e04d35..eac46480152d 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -151,8 +151,21 @@ function SidebarLinksData({ const isLoading = isLoadingApp; const optionItemsMemoized = useMemo( - () => SidebarUtils.getOrderedReportIDs(null, chatReports, betas, policies, priorityMode, allReportActions, transactionViolations, activeWorkspaceID, policyMemberAccountIDs), - [chatReports, betas, policies, priorityMode, allReportActions, transactionViolations, activeWorkspaceID, policyMemberAccountIDs], + () => + SidebarUtils.getOrderedReportIDs( + null, + chatReports, + betas, + policies, + priorityMode, + allReportActions, + transactionViolations, + activeWorkspaceID, + policyMemberAccountIDs, + reportIDsWithErrors, + canUseViolations, + ), + [chatReports, betas, policies, priorityMode, allReportActions, transactionViolations, activeWorkspaceID, policyMemberAccountIDs, reportIDsWithErrors, canUseViolations], ); const optionListItems = useMemo(() => { From 0f93e35c4431e734c9358e588e33437215e9399f Mon Sep 17 00:00:00 2001 From: Tomasz Lesniakiewicz Date: Thu, 21 Mar 2024 12:56:37 +0100 Subject: [PATCH 5/8] feat: adjust to recent main --- src/pages/home/sidebar/SidebarLinksData.js | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index 1bb14b7183e0..0b01df2c93ab 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -149,24 +149,12 @@ function SidebarLinksData({ transactionViolations, activeWorkspaceID, policyMemberAccountIDs, - reportIDsWithErrors, - canUseViolations, ), - [chatReports, betas, policies, priorityMode, allReportActions, transactionViolations, activeWorkspaceID, policyMemberAccountIDs, reportIDsWithErrors, canUseViolations], + [chatReports, betas, policies, priorityMode, allReportActions, transactionViolations, activeWorkspaceID, policyMemberAccountIDs], ); const optionListItems = useMemo(() => { - const reportIDs = SidebarUtils.getOrderedReportIDs( - null, - chatReports, - betas, - policies, - priorityMode, - allReportActions, - transactionViolations, - activeWorkspaceID, - policyMemberAccountIDs, - ); + const reportIDs = optionItemsMemoized; if (deepEqual(reportIDsRef.current, reportIDs)) { return reportIDsRef.current; @@ -179,7 +167,7 @@ function SidebarLinksData({ reportIDsRef.current = reportIDs; } return reportIDsRef.current || []; - }, [chatReports, betas, policies, priorityMode, allReportActions, transactionViolations, activeWorkspaceID, policyMemberAccountIDs, isLoading, network.isOffline, prevPriorityMode]); + }, [optionItemsMemoized, isLoading, network.isOffline, prevPriorityMode]); // We need to make sure the current report is in the list of reports, but we do not want // to have to re-generate the list every time the currentReportID changes. To do that From d787f1b55aac5a2b1635870ff8f405130d3492e3 Mon Sep 17 00:00:00 2001 From: Tomasz Lesniakiewicz Date: Thu, 21 Mar 2024 13:25:20 +0100 Subject: [PATCH 6/8] chore: lint fix --- src/pages/home/sidebar/SidebarLinksData.js | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index 0b01df2c93ab..c5749893d7fb 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -138,18 +138,7 @@ function SidebarLinksData({ const isLoading = isLoadingApp; const optionItemsMemoized = useMemo( - () => - SidebarUtils.getOrderedReportIDs( - null, - chatReports, - betas, - policies, - priorityMode, - allReportActions, - transactionViolations, - activeWorkspaceID, - policyMemberAccountIDs, - ), + () => SidebarUtils.getOrderedReportIDs(null, chatReports, betas, policies, priorityMode, allReportActions, transactionViolations, activeWorkspaceID, policyMemberAccountIDs), [chatReports, betas, policies, priorityMode, allReportActions, transactionViolations, activeWorkspaceID, policyMemberAccountIDs], ); @@ -167,7 +156,7 @@ function SidebarLinksData({ reportIDsRef.current = reportIDs; } return reportIDsRef.current || []; - }, [optionItemsMemoized, isLoading, network.isOffline, prevPriorityMode]); + }, [optionItemsMemoized, priorityMode, isLoading, network.isOffline, prevPriorityMode]); // We need to make sure the current report is in the list of reports, but we do not want // to have to re-generate the list every time the currentReportID changes. To do that From 9f2ee85b6aceb581a317109cc962ca6c8f11daba Mon Sep 17 00:00:00 2001 From: Tomasz Lesniakiewicz Date: Thu, 21 Mar 2024 15:31:54 +0100 Subject: [PATCH 7/8] chore: update comment --- src/pages/home/sidebar/SidebarLinksData.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index c5749893d7fb..8395c3d1e716 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -333,8 +333,7 @@ export default compose( )( /* While working on audit on the App Start App metric we noticed that by memoizing SidebarLinksData we can avoid 1 additional run of getOrderedReportIDs. - With that we can reduce app start up time by ~2.5s on heavy account. - After finding and fixing core issues with getOrderedReportIDs performance we might remove the memoization + With that we can reduce app start up time by ~2s on heavy account. More details - https://github.com/Expensify/App/issues/35234#issuecomment-1926914534 */ memo( From 2b62829feab1c5feecabc2def26eea34a8ef2311 Mon Sep 17 00:00:00 2001 From: Tomasz Lesniakiewicz Date: Fri, 22 Mar 2024 13:19:32 +0100 Subject: [PATCH 8/8] chore: update comment --- src/pages/home/sidebar/SidebarLinksData.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index 8395c3d1e716..0c1f8b8c5972 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -332,7 +332,7 @@ export default compose( }), )( /* - While working on audit on the App Start App metric we noticed that by memoizing SidebarLinksData we can avoid 1 additional run of getOrderedReportIDs. + While working on audit on the App Start App metric we noticed that by memoizing SidebarLinksData we can avoid 2 additional run of getOrderedReportIDs. With that we can reduce app start up time by ~2s on heavy account. More details - https://github.com/Expensify/App/issues/35234#issuecomment-1926914534 */