Skip to content

Commit

Permalink
Stop reduce anti-pattern contagion (#22454)
Browse files Browse the repository at this point in the history
## 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

<img width="398" alt="Screenshot 2024-06-11 at 6 27 46 AM"
src="https://github.com/dagster-io/dagster/assets/2286579/a9c51a5b-1371-40bd-87a6-9b35885d9fef">

A little over 200ms if you include the subsequent frames
  • Loading branch information
salazarm authored Jun 11, 2024
1 parent 7f29d97 commit 8fe0426
Show file tree
Hide file tree
Showing 7 changed files with 56 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, boolean>,
) || {}
);
}, [backfill, data]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, true>,
);

// Count the number of new code locations.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>; all: string[]}},
);
Expand Down
29 changes: 22 additions & 7 deletions js_modules/dagster-ui/packages/ui-core/src/runs/RunActionsMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, boolean>,
);

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<string, boolean>,
);

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<string, string>,
);

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<string, string>,
);

const closeDialogs = () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, TimelineJob[]>,
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, boolean>,
),
} as LogFilter;
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, string>,
);
return yaml.stringify(kvPairs);
}, [displayMetadata]);

Expand Down

1 comment on commit 8fe0426

@github-actions
Copy link

Choose a reason for hiding this comment

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

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-3rjkjsjhz-elementl.vercel.app

Built with commit 8fe0426.
This pull request is being automatically deployed with vercel-action

Please sign in to comment.