Cleanup spread operators in reduce calls
#157471
Conversation
pgayvallet
changed the title
reduce operationsreduce calls
pgayvallet
added
chore
performance
release_note:skip
|
@pgayvallet it's a nice idea to improve performance by these changes 👍 I'm curious if there are plans to have some protection against spread operator usage in the code as it can return while the codebase will continue evolving. On top if that it's interesting if this type of changes can be automated with GhatGPT or any other tool. |
I spent some time searching for the proper es-lint rule, but I couldn't find something suiting exactly what I was looking for (meaning, avoid spread only when unnecessary). AFAIK it's either all or nothing, which is far from being good enough for the kind of precise banning / validation I was looking for.
ChatGPT? Last time I asked him development questions, I'm afraid he failed my interview 😅 . Any other tool? Maybe. I did not spent time looking for one to be honest. |
| [] as string[] | ||
| ); | ||
| return indexMaps.reduce((indexNames, indexMap) => { | ||
| indexNames.push(...Object.keys(indexMap)); |
There was a problem hiding this comment.
Out of curiosity, why not replace the spread here with concat?
Is the creation of the new array potentially more expensive than the spread of the currentValue?
There was a problem hiding this comment.
You mean
return indexNames.concat(Object.keys(indexMap));
instead of
indexNames.push(...Object.keys(indexMap));
return indexNames;
right?
Now, yes, I think that the allocation/creation of the new array via concat should be more expensive than pushing the spread of keys (even if said spread creates a new array too, its length / size is smaller than the one returned by concat).
But I have absolutely no numbers or benchmarks to support my assumptions here, so I may also just be wrong. All I know for sure is that both are better than what the code was previously 😄
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
## 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]>
Yet: 🤨 How should we trust anything anymore? |
|
@delanni right, I apologize. I will close the other PR |
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
reducecalls 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 inkittens.reduce()For
reduceloops, the spread operator can usually easily be replaced:- setting a value on the accum object and returning it
BAD
GOOD
- assigning values to the accum object and returning it
BAD
GOOD
- pushing items to the accum list and returning it
BAD
GOOD
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.