Skip to content

Commit

Permalink
Cleanup spread operators in reduce calls (elastic#157471)
Browse files Browse the repository at this point in the history
## Summary

The spread operator is costly and put pressure on GC. It should be
avoided when possible, especially in loops.

This PR adapts a lot of `reduce` calls in the codebase to remove the
usages of the diabolic spread operator, when possible.

Note: the PR is not fully exhaustive. I focused on the server-side, as
we're more directly impacted than on browser-side code regarding
performances.

## Removing `...` usages in `kittens.reduce()`

For `reduce` loops, the spread operator can usually easily be replaced:

#### - setting a value on the accum object and returning it

#### BAD
```ts
  return this.toArray().reduce(
      (acc, renderer) => ({
        ...acc,
        [renderer.name]: renderer,
      }),
      {} as Record<string, ExpressionRenderer>
    );
```

#### GOOD
```ts
  return this.toArray().reduce((acc, renderer) => {
      acc[renderer.name] = renderer;
      return acc;
    }, {} as Record<string, ExpressionRenderer>);
```


#### - assigning values to the accum object and returning it 

#### BAD
```ts
  const allAggs: Record<string, any> = fieldAggRequests.reduce(
      (aggs: Record<string, any>, fieldAggRequest: unknown | null) => {
        return fieldAggRequest ? { ...aggs, ...(fieldAggRequest as Record<string, any>) } : aggs;
      },
      {}
    );
```

#### GOOD
```ts
  const allAggs = fieldAggRequests.reduce<Record<string, any>>(
      (aggs: Record<string, any>, fieldAggRequest: unknown | null) => {
        if (fieldAggRequest) {
          Object.assign(aggs, fieldAggRequest);
        }
        return aggs;
      },
      {}
    );
```

#### - pushing items to the accum list and returning it 

#### BAD
```ts
  const charsFound = charToArray.reduce(
    (acc, char) => (value.includes(char) ? [...acc, char] : acc),
    [] as string[]
  );
```

#### GOOD
```ts
  const charsFound = charToArray.reduce((acc, char) => {
    if (value.includes(char)) {
      acc.push(char);
    }
    return acc;
  }, [] as string[]);
```

## Questions

#### Are you sure all the changes in this are strictly better for
runtime performances?

Yes, yes I am.

#### How much better?

Likely not much.

#### Are you planning on analyzing the perf gain?

Nope.

#### So why did you do it?

I got tired of seeing badly used spread operators in my team's owned
code, and I had some extra time during on-week, so I spent a few hours
adapting the usages in all our runtime/production codebase.

#### Was it fun?

Take your best guess.

---------

Co-authored-by: Kibana Machine <[email protected]>
  • Loading branch information
2 people authored and delanni committed May 25, 2023
1 parent 1fa9a8e commit 0a6a2b1
Show file tree
Hide file tree
Showing 132 changed files with 690 additions and 773 deletions.
10 changes: 5 additions & 5 deletions packages/core/http/core-http-server-internal/src/http_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,12 +262,12 @@ export class HttpConfig implements IHttpConfig {
this.securityResponseHeaders = securityResponseHeaders;
this.customResponseHeaders = Object.entries(rawHttpConfig.customResponseHeaders ?? {}).reduce(
(headers, [key, value]) => {
return {
...headers,
[key]: Array.isArray(value) ? value.map((e) => convertHeader(e)) : convertHeader(value),
};
headers[key] = Array.isArray(value)
? value.map((e) => convertHeader(e))
: convertHeader(value);
return headers;
},
{}
{} as Record<string, string | string[]>
);
this.maxPayload = rawHttpConfig.maxPayload;
this.name = rawHttpConfig.name;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ export class NodeService {

// We assume the combination of node roles has been validated and avoid doing additional checks here.
this.roles = NODE_ALL_ROLES.reduce((acc, curr) => {
return { ...acc, [camelCase(curr)]: (roles as string[]).includes(curr) };
acc[camelCase(curr) as keyof NodeRoles] = (roles as string[]).includes(curr);
return acc;
}, {} as NodeRoles);

return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,11 +194,9 @@ const recursiveRewrite = (
newValue = recursiveRewrite(value, nestedContext, [...parents, key]);
}

return {
...memo,
[newKey]: newValue,
};
}, {});
memo[newKey] = newValue;
return memo;
}, {} as Record<string, unknown>);
};

const childContext = (context: ValidationContext, path: string): ValidationContext => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ const getMatchPhrasePrefixFields = ({
fields = types.reduce((typeFields, type) => {
const defaultSearchField = registry.getType(type)?.management?.defaultSearchField;
if (defaultSearchField) {
return [...typeFields, `${type}.${defaultSearchField}`];
typeFields.push(`${type}.${defaultSearchField}`);
}
return typeFields;
}, [] as string[]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,7 @@ export class SavedObjectsImporter implements ISavedObjectsImporter {
this.#importSizeLimit = importSizeLimit;
this.#importHooks = typeRegistry.getAllTypes().reduce((hooks, type) => {
if (type.management?.onImport) {
return {
...hooks,
[type.name]: [type.management.onImport],
};
hooks[type.name] = [type.management.onImport];
}
return hooks;
}, {} as Record<string, SavedObjectsImportHook[]>);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,12 @@ import type { SavedObjectsTypeMappingDefinitions } from '@kbn/core-saved-objects
export const buildTypesMappings = (
types: SavedObjectsType[]
): SavedObjectsTypeMappingDefinitions => {
return types.reduce((acc, { name: type, mappings }) => {
return types.reduce<SavedObjectsTypeMappingDefinitions>((acc, { name: type, mappings }) => {
const duplicate = acc.hasOwnProperty(type);
if (duplicate) {
throw new Error(`Type ${type} is already defined.`);
}
return {
...acc,
[type]: mappings,
};
acc[type] = mappings;
return acc;
}, {});
};
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,11 @@ export function buildActiveMigrations({
referenceTransforms,
});

if (!typeTransforms.transforms.length) {
return migrations;
if (typeTransforms.transforms.length) {
migrations[type.name] = typeTransforms;
}

return {
...migrations,
[type.name]: typeTransforms,
};
return migrations;
}, {} as ActiveMigrations);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,16 @@ export class DocumentMigrator implements VersionedTransformer {
throw new Error('Migrations are not ready. Make sure prepareMigrations is called first.');
}

return Object.entries(this.migrations).reduce(
return Object.entries(this.migrations).reduce<SavedObjectsMigrationVersion>(
(acc, [type, { latestVersion, immediateVersion }]) => {
const version = includeDeferred ? latestVersion : immediateVersion;
const latestMigrationVersion =
migrationType === 'core' ? version.core : maxVersion(version.migrate, version.convert);

return latestMigrationVersion ? { ...acc, [type]: latestMigrationVersion } : acc;
if (latestMigrationVersion) {
acc[type] = latestMigrationVersion;
}
return acc;
},
{}
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,10 @@ type SortPredicatePieDonutFn = (visParams: PartitionVisParams) => SortFn;
type SortPredicatePureFn = () => SortFn;

export const extractUniqTermsMap = (dataTable: Datatable, columnId: string) =>
[...new Set(dataTable.rows.map((item) => item[columnId]))].reduce(
(acc, item, index) => ({
...acc,
[item]: index,
}),
{}
);
[...new Set(dataTable.rows.map((item) => item[columnId]))].reduce((acc, item, index) => {
acc[item] = index;
return acc;
}, {});

export const sortPredicateSaveSourceOrder: SortPredicatePureFn =
() =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,17 +153,18 @@ export const getLayerFormats = (
}),
{}
),
splitSeriesAccessors: splitColumnAccessors?.reduce((formatters, splitAccessor) => {
const formatObj = getAccessorWithFieldFormat(splitAccessor, table.columns);
const accessor = Object.keys(formatObj)[0];
return {
...formatters,
[accessor]: {
splitSeriesAccessors: splitColumnAccessors?.reduce<SplitAccessorsFieldFormats>(
(formatters, splitAccessor) => {
const formatObj = getAccessorWithFieldFormat(splitAccessor, table.columns);
const accessor = Object.keys(formatObj)[0];
formatters[accessor] = {
format: formatObj[accessor],
formatter: formatFactory(formatObj[accessor]),
},
};
}, {}),
};
return formatters;
},
{}
),
splitColumnAccessors: getAccessorWithFieldFormat(splitColumnAccessor, table.columns),
splitRowAccessors: getAccessorWithFieldFormat(splitRowAccessor, table.columns),
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,17 +128,15 @@ export class SpecDefinitionsService {
if (overrideFile) {
merge(loadedSpec, JSON.parse(readFileSync(overrideFile, 'utf8')));
}
const spec: Record<string, EndpointDescription> = {};
Object.entries(loadedSpec).forEach(([key, value]) => {
if (acc[key]) {
// add time to remove key collision
spec[`${key}${Date.now()}`] = value;
acc[`${key}${Date.now()}`] = value;
} else {
spec[key] = value;
acc[key] = value;
}
});

return { ...acc, ...spec };
return acc;
}, {} as Record<string, EndpointDescription>);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ const cheapSuggestionAggSubtypes: { [key: string]: OptionsListSuggestionAggregat
parse: (rawEsResult) => ({
suggestions: get(rawEsResult, 'aggregations.suggestions.buckets')?.reduce(
(acc: OptionsListSuggestions, suggestion: EsBucket) => {
return [...acc, { value: suggestion.key, docCount: suggestion.doc_count }];
acc.push({ value: suggestion.key, docCount: suggestion.doc_count });
return acc;
},
[]
),
Expand All @@ -76,7 +77,8 @@ const cheapSuggestionAggSubtypes: { [key: string]: OptionsListSuggestionAggregat
parse: (rawEsResult) => ({
suggestions: get(rawEsResult, 'aggregations.suggestions.buckets')?.reduce(
(acc: OptionsListSuggestions, suggestion: EsBucket & { key_as_string: string }) => {
return [...acc, { value: suggestion.key_as_string, docCount: suggestion.doc_count }];
acc.push({ value: suggestion.key_as_string, docCount: suggestion.doc_count });
return acc;
},
[]
),
Expand Down Expand Up @@ -151,7 +153,8 @@ const cheapSuggestionAggSubtypes: { [key: string]: OptionsListSuggestionAggregat
suggestions: sortedSuggestions
.slice(0, 10) // only return top 10 results
.reduce((acc: OptionsListSuggestions, suggestion: EsBucket) => {
return [...acc, { value: suggestion.key, docCount: suggestion.doc_count }];
acc.push({ value: suggestion.key, docCount: suggestion.doc_count });
return acc;
}, []),
};
},
Expand Down Expand Up @@ -189,7 +192,8 @@ const cheapSuggestionAggSubtypes: { [key: string]: OptionsListSuggestionAggregat
parse: (rawEsResult) => ({
suggestions: get(rawEsResult, 'aggregations.nestedSuggestions.suggestions.buckets')?.reduce(
(acc: OptionsListSuggestions, suggestion: EsBucket) => {
return [...acc, { value: suggestion.key, docCount: suggestion.doc_count }];
acc.push({ value: suggestion.key, docCount: suggestion.doc_count });
return acc;
},
[]
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ const expensiveSuggestionAggSubtypes: { [key: string]: OptionsListSuggestionAggr

const suggestions = get(rawEsResult, `${basePath}.suggestions.buckets`)?.reduce(
(acc: OptionsListSuggestions, suggestion: EsBucket) => {
return [...acc, { value: suggestion.key, docCount: suggestion.doc_count }];
acc.push({ value: suggestion.key, docCount: suggestion.doc_count });
return acc;
},
[]
);
Expand All @@ -120,7 +121,8 @@ const expensiveSuggestionAggSubtypes: { [key: string]: OptionsListSuggestionAggr
parse: (rawEsResult) => {
const suggestions = get(rawEsResult, 'aggregations.suggestions.buckets')?.reduce(
(acc: OptionsListSuggestions, suggestion: EsBucket & { key_as_string: string }) => {
return [...acc, { value: suggestion.key_as_string, docCount: suggestion.doc_count }];
acc.push({ value: suggestion.key_as_string, docCount: suggestion.doc_count });
return acc;
},
[]
);
Expand Down Expand Up @@ -200,7 +202,8 @@ const expensiveSuggestionAggSubtypes: { [key: string]: OptionsListSuggestionAggr
const suggestions = sortedSuggestions
.slice(0, request.size)
.reduce((acc: OptionsListSuggestions, suggestion: EsBucket) => {
return [...acc, { value: suggestion.key, docCount: suggestion.doc_count }];
acc.push({ value: suggestion.key, docCount: suggestion.doc_count });
return acc;
}, []);
const totalCardinality =
(get(rawEsResult, `aggregations.suggestions.buckets.ipv4.unique_terms.value`) ?? 0) +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@ function flatten(obj: any, keyPrefix = '') {
nestedRows.push(
...obj[key]
.map((nestedRow: any) => flatten(nestedRow, prefix + key))
.reduce((acc: any, object: any) => [...acc, ...object], [])
.reduce((acc: unknown[], object: unknown[]) => {
acc.push(...object);
return acc;
}, [])
);
} else if (typeof obj[key] === 'object' && obj[key] !== null) {
const subRows = flatten(obj[key], prefix + key);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ export function fetchProvider(getIndexForType: (type: string) => Promise<string>
const { transientCount = 0, persistedCount = 0 } = buckets.reduce(
(usage: Partial<ReportedUsage>, bucket: SessionPersistedTermsBucket) => {
const key = bucket.key_as_string === 'false' ? 'transientCount' : 'persistedCount';
return { ...usage, [key]: bucket.doc_count };
usage[key] = bucket.doc_count;
return usage;
},
{}
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,13 +110,10 @@ export function readFieldCapsResponse(
searchable: isSearchable,
aggregatable: isAggregatable,
readFromDocValues: false,
conflictDescriptions: types.reduce(
(acc, esType) => ({
...acc,
[esType]: capsByType[esType].indices,
}),
{}
),
conflictDescriptions: types.reduce((acc, esType) => {
acc[esType] = capsByType[esType].indices;
return acc;
}, {} as Record<string, estypes.Indices | undefined>),
metadata_field: capsByType[types[0]].metadata_field,
};
// This is intentionally using a "hash" and a "push" to be highly optimized with very large indexes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,10 @@ export const useFormIsModified = ({
return;
}

return fieldPathsToDiscard.reduce(
(acc, path) => ({ ...acc, [path]: true }),
{} as { [key: string]: {} }
);
return fieldPathsToDiscard.reduce((acc, path) => {
acc[path] = true;
return acc;
}, {} as { [key: string]: {} });

// discardArrayToString === discard, we don't want to add it to the dependencies so
// the consumer does not need to memoize the "discard" array they provide.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,11 @@ export const stripOutUndefinedValues = <R>(obj: GenericObject): R => {
.filter(({ 1: value }) => value !== undefined)
.reduce((acc, [key, value]) => {
if (typeof value === 'object' && value !== null && !Array.isArray(value)) {
return { ...acc, [key]: stripOutUndefinedValues(value) };
acc[key as keyof R] = stripOutUndefinedValues(value);
} else {
acc[key as keyof R] = value;
}

return { ...acc, [key]: value };
return acc;
}, {} as R);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@
export const containsChars = (chars: string | string[]) => (value: string) => {
const charToArray = Array.isArray(chars) ? (chars as string[]) : ([chars] as string[]);

const charsFound = charToArray.reduce(
(acc, char) => (value.includes(char) ? [...acc, char] : acc),
[] as string[]
);
const charsFound = charToArray.reduce((acc, char) => {
if (value.includes(char)) {
acc.push(char);
}
return acc;
}, [] as string[]);

return {
charsFound,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,10 +179,10 @@ export const postprocessAnnotations = (

let extraFields: Record<string, string | number | string[] | number[]> = {};
if (annotationConfig?.extraFields?.length) {
extraFields = annotationConfig.extraFields.reduce(
(acc, field) => ({ ...acc, [`field:${field}`]: row[fieldsColIdMap[field]] }),
{}
);
extraFields = annotationConfig.extraFields.reduce((acc, field) => {
acc[`field:${field}`] = row[fieldsColIdMap[field]];
return acc;
}, {} as typeof extraFields);
}
if (annotationConfig?.textField) {
extraFields[`field:${annotationConfig.textField}`] =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,10 @@ export class ExpressionRendererRegistry implements IRegistry<ExpressionRenderer>
}

public toJS(): Record<string, ExpressionRenderer> {
return this.toArray().reduce(
(acc, renderer) => ({
...acc,
[renderer.name]: renderer,
}),
{} as Record<string, ExpressionRenderer>
);
return this.toArray().reduce((acc, renderer) => {
acc[renderer.name] = renderer;
return acc;
}, {} as Record<string, ExpressionRenderer>);
}

public toArray(): ExpressionRenderer[] {
Expand Down
Loading

0 comments on commit 0a6a2b1

Please sign in to comment.