From 8fe04262af698fc0fd16a97617db8b5dbb90d792 Mon Sep 17 00:00:00 2001 From: Marco polo Date: Tue, 11 Jun 2024 03:25:05 -1000 Subject: [PATCH] Stop `reduce` anti-pattern contagion (#22454) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary & Motivation When using reduce you're supposed to accumulate the value, currently these call sites are recreating the accumulator on every iteration which leads to N^2 perf. ## How I Tested These Changes Shaves 80ms off rendering the Run Timeline Screenshot 2024-06-11 at 6 27 46 AM A little over 200ms if you include the subsequent frames --- .../backfill/BackfillTerminationDialog.tsx | 10 +++++-- .../src/nav/useCodeLocationsStatus.tsx | 9 ++++-- .../ui-core/src/runs/LogsFilterInput.tsx | 7 +++-- .../ui-core/src/runs/RunActionsMenu.tsx | 29 ++++++++++++++----- .../packages/ui-core/src/runs/RunTimeline.tsx | 5 ++-- .../src/runs/useQueryPersistedLogFilter.ts | 8 ++++- .../src/workspace/CodeLocationMenu.tsx | 10 +++++-- 7 files changed, 56 insertions(+), 22 deletions(-) diff --git a/js_modules/dagster-ui/packages/ui-core/src/instance/backfill/BackfillTerminationDialog.tsx b/js_modules/dagster-ui/packages/ui-core/src/instance/backfill/BackfillTerminationDialog.tsx index afffc8f7e2d6a..c3d44acf9b783 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/instance/backfill/BackfillTerminationDialog.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/instance/backfill/BackfillTerminationDialog.tsx @@ -45,9 +45,13 @@ export const BackfillTerminationDialog = ({backfill, onClose, onComplete}: Props ); return ( unfinishedPartitions?.reduce( - (accum, partition) => - partition && partition.runId ? {...accum, [partition.runId]: true} : accum, - {}, + (accum, partition) => { + if (partition && partition.runId) { + accum[partition.runId] = true; + } + return accum; + }, + {} as Record, ) || {} ); }, [backfill, data]); diff --git a/js_modules/dagster-ui/packages/ui-core/src/nav/useCodeLocationsStatus.tsx b/js_modules/dagster-ui/packages/ui-core/src/nav/useCodeLocationsStatus.tsx index 23f8245d909f0..5de068c3e9496 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/nav/useCodeLocationsStatus.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/nav/useCodeLocationsStatus.tsx @@ -138,9 +138,12 @@ export const useCodeLocationsStatus = (): StatusAndMessage | null => { // to have finished loading so quickly, but go ahead and indicate that the location has // been added, then reload the workspace. if (currentEntries.length > previousEntries.length && !currentlyLoading.length) { - const previousMap: {[id: string]: true} = previousEntries.reduce( - (accum, {id}) => ({...accum, [id]: true}), - {}, + const previousMap = previousEntries.reduce( + (accum, {id}) => { + accum[id] = true; + return accum; + }, + {} as Record, ); // Count the number of new code locations. diff --git a/js_modules/dagster-ui/packages/ui-core/src/runs/LogsFilterInput.tsx b/js_modules/dagster-ui/packages/ui-core/src/runs/LogsFilterInput.tsx index 9866047772a13..52869c42c121b 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/runs/LogsFilterInput.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/runs/LogsFilterInput.tsx @@ -67,9 +67,10 @@ export const LogsFilterInput = (props: Props) => { const perProvider = suggestionProviders.reduce( (accum, provider) => { const values = provider.values(); - return provider.token - ? {...accum, [provider.token]: {fuse: new Fuse(values, fuseOptions), all: values}} - : accum; + if (provider.token) { + accum[provider.token] = {fuse: new Fuse(values, fuseOptions), all: values}; + } + return accum; }, {} as {[token: string]: {fuse: Fuse; all: string[]}}, ); diff --git a/js_modules/dagster-ui/packages/ui-core/src/runs/RunActionsMenu.tsx b/js_modules/dagster-ui/packages/ui-core/src/runs/RunActionsMenu.tsx index e0b9dc33cf29e..b4cb9c28e0e0a 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/runs/RunActionsMenu.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/runs/RunActionsMenu.tsx @@ -340,27 +340,42 @@ export const RunBulkActionsMenu = React.memo((props: RunBulkActionsMenuProps) => ); const terminateableIDs = terminatableRuns.map((r) => r.id); const terminateableMap = terminatableRuns.reduce( - (accum, run) => ({...accum, [run.id]: run.canTerminate}), - {}, + (accum, run) => { + accum[run.id] = run.canTerminate; + return accum; + }, + {} as Record, ); const deleteableIDs = selected.map((run) => run.id); - const deletionMap = selected.reduce((accum, run) => ({...accum, [run.id]: run.canTerminate}), {}); + const deletionMap = selected.reduce( + (accum, run) => { + accum[run.id] = run.canTerminate; + return accum; + }, + {} as Record, + ); const reexecuteFromFailureRuns = selected.filter( (r) => failedStatuses.has(r?.status) && r.hasReExecutePermission, ); const reexecuteFromFailureMap = reexecuteFromFailureRuns.reduce( - (accum, run) => ({...accum, [run.id]: run.id}), - {}, + (accum, run) => { + accum[run.id] = run.id; + return accum; + }, + {} as Record, ); const reexecutableRuns = selected.filter( (r) => doneStatuses.has(r?.status) && r.hasReExecutePermission, ); const reexecutableMap = reexecutableRuns.reduce( - (accum, run) => ({...accum, [run.id]: run.id}), - {}, + (accum, run) => { + accum[run.id] = run.id; + return accum; + }, + {} as Record, ); const closeDialogs = () => { diff --git a/js_modules/dagster-ui/packages/ui-core/src/runs/RunTimeline.tsx b/js_modules/dagster-ui/packages/ui-core/src/runs/RunTimeline.tsx index becc39d98413f..c0dad3ef886bc 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/runs/RunTimeline.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/runs/RunTimeline.tsx @@ -99,8 +99,9 @@ export const RunTimeline = (props: Props) => { (accum, job) => { const {repoAddress} = job; const repoKey = repoAddressAsURLString(repoAddress); - const jobsForRepo = accum[repoKey] || []; - return {...accum, [repoKey]: [...jobsForRepo, job]}; + accum[repoKey] = accum[repoKey] || []; + accum[repoKey]!.push(job); + return accum; }, {} as Record, ), diff --git a/js_modules/dagster-ui/packages/ui-core/src/runs/useQueryPersistedLogFilter.ts b/js_modules/dagster-ui/packages/ui-core/src/runs/useQueryPersistedLogFilter.ts index ce8e049f4beb5..ed56e3394e8fe 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/runs/useQueryPersistedLogFilter.ts +++ b/js_modules/dagster-ui/packages/ui-core/src/runs/useQueryPersistedLogFilter.ts @@ -63,7 +63,13 @@ export const decodeRunPageFilters = (qs: {[key: string]: string}) => { levels: levelsValues .map((level) => level.toUpperCase()) .filter((level) => LogLevel.hasOwnProperty(level)) - .reduce((accum, level) => ({...accum, [level]: true}), {}), + .reduce( + (accum, level) => { + accum[level] = true; + return accum; + }, + {} as Record, + ), } as LogFilter; }; diff --git a/js_modules/dagster-ui/packages/ui-core/src/workspace/CodeLocationMenu.tsx b/js_modules/dagster-ui/packages/ui-core/src/workspace/CodeLocationMenu.tsx index 2af8260f40d05..a98435b3f27a1 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/workspace/CodeLocationMenu.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/workspace/CodeLocationMenu.tsx @@ -138,9 +138,13 @@ const CodeLocationConfig = ({ displayMetadata: WorkspaceRepositoryLocationNode['displayMetadata']; }) => { const yamlString = useMemo(() => { - const kvPairs = displayMetadata.reduce((accum, item) => { - return {...accum, [item.key]: item.value}; - }, {}); + const kvPairs = displayMetadata.reduce( + (accum, item) => { + accum[item.key] = item.value; + return accum; + }, + {} as Record, + ); return yaml.stringify(kvPairs); }, [displayMetadata]);