Optimize `reduce` loops - late 2023 edition #168812

pgayvallet · 2023-10-13T08:14:46Z

Summary

Follow-up of #157471, see original PR for more context.

Don't expect the same perf gain though, I don't think this PR impacted any bottlenecks, as the last one did.

Still, faster is always better, and you become the GC's BFF

…ms-again

kibana-ci · 2023-10-16T09:11:11Z

💔 Build Failed

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #44 / lens app - TSVB Open in Lens Metric "before all" hook for "should show the "Edit Visualization in Lens" menu item"

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`aiops`	587.2KB	587.2KB	+14.0B
`expressionXY`	131.2KB	131.2KB	+8.0B
`fleet`	1.2MB	1.2MB	-20.0B
`indexManagement`	561.2KB	561.2KB	-20.0B
`lists`	147.5KB	147.5KB	+4.0B
`logExplorer`	195.0KB	195.0KB	+13.0B
`observabilityOnboarding`	240.4KB	240.4KB	-4.0B
`securitySolution`	13.0MB	13.0MB	-226.0B
`ux`	166.0KB	166.0KB	+13.0B
total			-218.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`controls`	37.9KB	37.9KB	-7.0B
`data`	407.4KB	407.4KB	+7.0B
`eventAnnotation`	19.7KB	19.7KB	+4.0B
`expressions`	98.1KB	98.1KB	-5.0B
`expressionXY`	38.7KB	38.6KB	-80.0B
`kbnUiSharedDeps-srcJs`	2.2MB	2.2MB	+1.0B
`observability`	101.3KB	101.2KB	-12.0B
total			-92.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`lists`	17	19	+2

Total ESLint disabled count

id	before	after	diff
`lists`	22	24	+2

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

sphilipse

ent-search changes LGTM

dej611 · 2023-10-16T10:49:40Z

src/plugins/vis_types/timeseries/server/lib/vis_data/get_series_data.ts

@@ -117,7 +117,10 @@ export async function getSeriesData(
      [panel.id]: {
        annotations,
        id: panel.id,
-        series: series.reduce((acc, s) => acc.concat(s), []),
+        series: series.reduce((acc, s) => {


I did profile this already and concat is way faster than push + spread.
In terms of GCC pressure both approach will create a new Array and clean up but the concat approach avoids to iterate on the second one.

Anyway I've created a benchmark test case to verify my previous findings and... the thing is "nuanced": https://jsbenchmark.com/#eyJjYXNlcyI6W3siaWQiOiI3d0loS0FkVWpWTGc4YVNCRUd3R1giLCJjb2RlIjoiY29uc3QgYyA9IERBVEEuc2VyaWVzLnJlZHVjZSggKG1lbW8sIGFycikgPT4ge1xuXHRyZXR1cm4gWy4uLm1lbW8sIC4uLmFycl07XG59LCBEQVRBLmEpOyIsIm5hbWUiOiJSZWR1Y2Ugd2l0aCBzcHJlYWQifSx7ImlkIjoiQW10S1F0TVNleDB3bDFnVXJLSmRFIiwiY29kZSI6ImNvbnN0IGMgPSBEQVRBLnNlcmllcy5yZWR1Y2UoIChtZW1vLCBhcnIpID0+IG1lbW8uY29uY2F0KGFyciksIERBVEEuYSk7IiwibmFtZSI6IlJlZHVjZSB3aXRoIGNvbmNhdCJ9LHsiaWQiOiI4cENpY1pTdTYwaTYxLURRTXdUV1kiLCJjb2RlIjoiY29uc3QgYyA9IERBVEEuc2VyaWVzLnJlZHVjZSggKG1lbW8sIGFycikgPT4ge1xuXHRtZW1vLnB1c2goLi4uYXJyKTtcblx0cmV0dXJuIG1lbW87XG59LCBEQVRBLmEpOyIsIm5hbWUiOiJSZWR1Y2Ugd2l0aCBwdXNoICsgc3ByZWFkIn0seyJpZCI6IlVhZFk1U0NDZl9RR05KZTEwSWpDTSIsImNvZGUiOiJjb25zdCBjID0gREFUQS5hLmNvbmNhdChEQVRBLnNlcmllcy5mbGF0KCkpIiwiZGVwZW5kZW5jaWVzIjpbXSwibmFtZSI6IkNvbmNhdCArIGZsYXQifV0sImNvbmZpZyI6eyJuYW1lIjoiU2ltcGxlIGV4YW1wbGUgdGVzdCIsInBhcmFsbGVsIjp0cnVlLCJnbG9iYWxUZXN0Q29uZmlnIjp7ImRlcGVuZGVuY2llcyI6W119LCJkYXRhQ29kZSI6InJldHVybiB7XG4gIGE6IEFycmF5LmZyb20oe2xlbmd0aDogMTAwMH0pLm1hcCgoXywgaSk9PmAke2l9YCksXG4gIHNlcmllczogQXJyYXkuZnJvbSh7bGVuZ3RoOiAxMH0pLmZpbGwoQXJyYXkuZnJvbSh7bGVuZ3RoOiAyMDAwfSkubWFwKChfLCBpKT0+YCR7aX1gKSlcbn07In19

Surprisingly Chrome fails to execute on the push + spread on my machine. But concat in Chrome is super fast.
Safari seems to be faster with push + spread, with concat second.
Firefox is way faster with concat.

They are still micro-benchmarks, so probably it is not worth push more here, but my point is that concat is "fast enough" already and there's no need to make the code more complex in this case.

Have you tried on a real server-side environment? The results from my personal benchmarks are surprisingly different between chrome/firefox and nodejs.

Now if you think some changes are unnecessary, I'm fine with it. would that be the only file you want me to revert?

Can you report some results from the nodejs benchmarks?

As I said, if the perf toll is not dramatically higher with concat I would rather keep this version that the other. Otherwise I think it is ok to change it.

So I've taken the benchmarking code below and made it more generic to test various scenarios:

const Benchmark = require('benchmark'); function createDataset(series, length){ return Array.from({length: series}).fill(Array.from({length}).map((_, i)=>`${i}`)).map((v, i) => [i, v]).sort(([a], [b]) => a - b); } for( const dataset of [createDataset(3, 3), createDataset(3, 100), createDataset(100, 3), createDataset(100, 100), createDataset(100, 1000)]){ console.log(`${Array(10).fill('-').join('')} Series: ${dataset.length} - Length: ${dataset[0][1].length} ${Array(10).fill('-').join('')}`) const suite = new Benchmark.Suite(`Series: ${dataset.length} - Length: ${dataset[0].length}`); suite .add('flatMap', function () { const out1 = dataset.flatMap(([, result]) => result); }) .add('reduce with concat', function () { const out1 = dataset.reduce((acc, [, result]) => acc.concat(result), []); }) .add('reduce with push', function () { const out1 = dataset.reduce((acc, [, result]) => { acc.push(...result); return acc; }, []); }) .add('map with flat', function () { const out1 = dataset.map(([, result]) => result).flat(); }) .add('concat with map', function () { const out1 = [].concat(...dataset.map(([, result]) => result)); }); suite // add listeners .on('cycle', function (event) { console.log(String(event.target)); }) .on('complete', function () { console.log('Fastest is ' + this.filter('fastest').map('name')); }); suite.run(); }

So I've tested different dimension scenarios and added some more reduce/concat tricks.
The result were interesting:

---------- Series: 3 - Length: 3 ---------- flatMap x 1,229,080 ops/sec ±0.11% (101 runs sampled) reduce with concat x 5,801,331 ops/sec ±0.09% (99 runs sampled) reduce with push x 16,493,742 ops/sec ±0.29% (99 runs sampled) map with flat x 1,166,655 ops/sec ±0.08% (100 runs sampled) concat with map x 8,050,295 ops/sec ±0.32% (99 runs sampled) Fastest is reduce with push ---------- Series: 3 - Length: 100 ---------- flatMap x 49,539 ops/sec ±0.08% (98 runs sampled) reduce with concat x 2,806,867 ops/sec ±0.24% (97 runs sampled) reduce with push x 1,464,221 ops/sec ±0.17% (98 runs sampled) map with flat x 49,311 ops/sec ±0.07% (97 runs sampled) concat with map x 4,517,977 ops/sec ±0.15% (97 runs sampled) Fastest is concat with map ---------- Series: 100 - Length: 3 ---------- flatMap x 41,307 ops/sec ±0.12% (98 runs sampled) reduce with concat x 79,167 ops/sec ±0.12% (99 runs sampled) reduce with push x 450,069 ops/sec ±0.14% (96 runs sampled) map with flat x 40,161 ops/sec ±0.07% (101 runs sampled) concat with map x 443,343 ops/sec ±0.14% (100 runs sampled) Fastest is reduce with push ---------- Series: 100 - Length: 100 ---------- flatMap x 1,490 ops/sec ±0.07% (99 runs sampled) reduce with concat x 4,846 ops/sec ±0.44% (100 runs sampled) reduce with push x 33,905 ops/sec ±0.16% (101 runs sampled) map with flat x 1,488 ops/sec ±0.06% (99 runs sampled) concat with map x 173,222 ops/sec ±0.17% (97 runs sampled) Fastest is concat with map ---------- Series: 100 - Length: 1000 ---------- flatMap x 144 ops/sec ±0.09% (83 runs sampled) reduce with concat x 163 ops/sec ±0.79% (85 runs sampled) reduce with push x 1,903 ops/sec ±0.64% (86 runs sampled) map with flat x 144 ops/sec ±0.19% (83 runs sampled) concat with map x 9,167 ops/sec ±0.69% (92 runs sampled) Fastest is concat with map

So I've learned few things:

concat is not that fast within a reduce as I have imagined in node

push is faster then the given array spread is short

arguments spread is way faster than I thought initially for longer arrays (tens of items)

So ok with the push version for now, but perhaps I might refactor later critical parts using the [].concat trick 😄

afharo

LGTM

afharo · 2023-10-16T11:05:42Z

packages/kbn-std/src/iteration/map.ts

+    .reduce((acc: T2[], [, result]) => {
+      acc.push(...result);
+      return acc;
+    }, []);


nit: I wonder if flatMap would do the same...

hmmm, nevermind:

Hold on... that was on Chrome...

Safari

And Firefox

Are consistently faster 🤷

Running a benchmark on NodeJS shows reduce with push is best

flatMap x 1,226,284 ops/sec ±0.40% (97 runs sampled) reduce with concat x 6,033,030 ops/sec ±0.19% (101 runs sampled) reduce with push x 16,624,071 ops/sec ±0.36% (93 runs sampled) Fastest is reduce with push

This is the piece of code I ran:

const Benchmark = require('benchmark'); const suite = new Benchmark.Suite; const results = [ [0, ["a", "b", "c"]], [2, ["g", "h", "i"]], [1, ["d", "e", "f"]], ].sort(([a], [b]) => a - b); suite .add('flatMap', function () { const out1 = results.flatMap(([, result]) => result); }) .add('reduce with concat', function () { const out1 = results.reduce((acc, [, result]) => acc.concat(result), []); }) .add('reduce with push', function () { const out1 = results.reduce((acc, [, result]) => { acc.push(...result); return acc; }, []); }); suite // add listeners .on('cycle', function (event) { console.log(String(event.target)); }) .on('complete', function () { console.log('Fastest is ' + this.filter('fastest').map('name')); }); suite.run();

efegurkan

Enterprise Search looks good, and we thank you <3

cnasikas

ResponseOps changes LGTM

walterra

ML team related changes LGTM. 👍

Ikuni17

OPs changes lgtm

dgieselaar · 2023-10-16T11:58:43Z

x-pack/plugins/apm/server/routes/transactions/breakdown/index.ts

-      return {
-        ...p,
-        [name]: p[name].concat({
+    const updatedSeries = kpiNames.reduce(


chapeau for being able to read this code 😄 (it's mine so I can say that)

criamico

Fleet changes LGTM

nchaulet

Fleet changes LGTM!

tomsonpl

Defend Workflows changes LGTM 👍

cauemarcondes

Profiling changes LGTM

thomheymann · 2023-10-17T08:46:45Z

x-pack/plugins/security/server/saved_objects/ensure_authorized.ts

-    (acc, [, privilegeSet]) => new Set([...acc, ...privilegeSet]),
+    (acc, [, privilegeSet]) => {
+      privilegeSet.forEach((entry) => acc.add(entry));
+      return acc;
+    },


I'm not sure these are functionally identical. The Set was used to enforce uniqueness from my understanding.

delanni · 2023-10-17T09:16:18Z

packages/kbn-json-ast/src/snip.ts

@@ -25,7 +25,8 @@ export function snip(source: string, snips: Snip[]) {
    .reduce((acc: Snip[], s) => {
      const prev = acc.at(-1);
      if (!prev || prev[1] < s[0]) {
-        return [...acc, s];
+        acc.push(s);
+        return acc;


There's a similar spread a few lines below. In the light of total elimination, should that be replaced by something like:

const merged: Snip = [Math.min(prev[0], s[0]), Math.max(prev[1], s[1])]; acc[acc.length-1] = merged; return acc;

delanni

Operations 👍

vitaliidm · 2023-10-17T08:49:16Z

packages/kbn-rule-data-utils/src/alerts_as_data_rbac.ts

-  }, {} as EsQueryConfig);
-  return paramKeysWithValues;
+    return acc;
+  }, {} as Record<string, unknown>);


castings like this in reduce can be avoided by passing interface in reduce generic

const paramKeysWithValues = Object.keys(params).reduce<Record<string, unknown>>((acc, key) => {

vitaliidm · 2023-10-17T08:56:03Z

x-pack/plugins/security_solution/common/utils/sourcerer.ts

@@ -18,6 +22,9 @@ export const sortWithExcludesAtEnd = (indices: string[]) => {
 export const ensurePatternFormat = (patternList: string[]): string[] =>
  sortWithExcludesAtEnd([
    ...new Set(
-      patternList.reduce((acc: string[], pattern: string) => [...pattern.split(','), ...acc], [])
+      patternList.reduce((acc: string[], pattern: string) => {
+        acc.unshift(...pattern.split(','));


Interestingly, is unshift significant better here to use?
unshift adds element to beginning of array, thus having O(n).
But readability is a bit better in original code with spread

vitaliidm · 2023-10-17T09:29:21Z

...ins/security_solution/server/lib/detection_engine/migrations/get_signal_versions_by_index.ts

-      ...agg,
-      [_index]: signalsVersions,
-    };
+    (agg as Record<string, unknown>)[_index] = signalsVersions;


I think casting here also can be resolved by using generic

vitaliidm · 2023-10-17T09:29:37Z

x-pack/plugins/security_solution/server/lib/detection_engine/routes/utils.ts

@@ -193,6 +194,7 @@ export const convertToSnakeCase = <T extends Record<string, unknown>>(
  }
  return Object.keys(obj).reduce((acc, item) => {
    const newKey = snakeCase(item);
-    return { ...acc, [newKey]: obj[item] };
+    (acc as Record<string, unknown>)[newKey] = obj[item];


I think casting here also can be resolved by using generic

vitaliidm · 2023-10-17T09:33:15Z

.../plugins/lists/server/services/exception_lists/utils/import/find_all_exception_list_types.ts

+  return foundListsResponse.data.reduce((acc, list) => {
+    // eslint-disable-next-line no-param-reassign
+    acc[list.list_id] = list;
+    return acc;
+  }, {} as Record<string, ExceptionListSchema>);


Suggested change

return foundListsResponse.data.reduce((acc, list) => {

// eslint-disable-next-line no-param-reassign

acc[list.list_id] = list;

return acc;

}, {} as Record<string, ExceptionListSchema>);

return foundListsResponse.data.reduce<Record<string, ExceptionListSchema>>((acc, list) => {

// eslint-disable-next-line no-param-reassign

acc[list.list_id] = list;

return acc;

}, {});

vitaliidm · 2023-10-17T09:33:38Z

...ins/lists/server/services/exception_lists/utils/import/find_all_exception_list_item_types.ts

+  return transformedResponse.data.reduce((acc, item) => {
+    // eslint-disable-next-line no-param-reassign
+    acc[item.item_id] = item;
+    return acc;
+  }, {} as Record<string, ExceptionListItemSchema>);


Suggested change

return transformedResponse.data.reduce((acc, item) => {

// eslint-disable-next-line no-param-reassign

acc[item.item_id] = item;

return acc;

}, {} as Record<string, ExceptionListItemSchema>);

return transformedResponse.data.reduce<Record<string, ExceptionListItemSchema>>((acc, item) => {

// eslint-disable-next-line no-param-reassign

acc[item.item_id] = item;

return acc;

}, {});

pgayvallet · 2023-10-17T10:19:47Z

Closing as won't fix

## Summary Cherry picked from #168812 the changes from `core` packages and the `security` and `spaces` plugins --------- Co-authored-by: Kibana Machine <[email protected]>

pgayvallet added 11 commits October 13, 2023 10:01

optimize reduce loops

bf5dcf6

Some other optims

52d553e

moar

ab7ac12

moar

8f7d129

moar

4b2c4fe

moar

91f706d

moar

30bb31a

moar

35aeea4

Merge remote-tracking branch 'upstream/main' into kbn-xxx-reduce-opti…

7ebde8e

…ms-again

moar

264c2db

moar

605a66b

pgayvallet added 4 commits October 15, 2023 10:05

moar

4b1067d

moar

c71383a

moar

7cc32bd

fix types

b2108b1

pgayvallet changed the title ~~Optimize reduce loops - 2023 edition~~ Optimize reduce loops - late 2023 edition Oct 15, 2023

pgayvallet added 2 commits October 16, 2023 08:37

revert array/set conversion

b30e195

fix things

18600e4

pgayvallet marked this pull request as ready for review October 16, 2023 10:04

pgayvallet requested review from a team as code owners October 16, 2023 10:04

shahzad31 approved these changes Oct 16, 2023

View reviewed changes

sphilipse approved these changes Oct 16, 2023

View reviewed changes

dej611 reviewed Oct 16, 2023

View reviewed changes

afharo approved these changes Oct 16, 2023

View reviewed changes

efegurkan approved these changes Oct 16, 2023

View reviewed changes

cnasikas approved these changes Oct 16, 2023

View reviewed changes

walterra approved these changes Oct 16, 2023

View reviewed changes

Ikuni17 approved these changes Oct 16, 2023

View reviewed changes

dgieselaar reviewed Oct 16, 2023

View reviewed changes

dgieselaar approved these changes Oct 16, 2023

View reviewed changes

paul-tavares approved these changes Oct 16, 2023

View reviewed changes

criamico approved these changes Oct 16, 2023

View reviewed changes

pjhampton approved these changes Oct 16, 2023

View reviewed changes

nchaulet approved these changes Oct 16, 2023

View reviewed changes

tomsonpl approved these changes Oct 16, 2023

View reviewed changes

ThomThomson approved these changes Oct 16, 2023

View reviewed changes

cauemarcondes approved these changes Oct 16, 2023

View reviewed changes

kpollich approved these changes Oct 16, 2023

View reviewed changes

thomheymann reviewed Oct 17, 2023

View reviewed changes

delanni mentioned this pull request Oct 17, 2023

Cleanup spread operators in reduce calls #157471

Merged

pheyos approved these changes Oct 17, 2023

View reviewed changes

delanni reviewed Oct 17, 2023

View reviewed changes

delanni approved these changes Oct 17, 2023

View reviewed changes

vitaliidm reviewed Oct 17, 2023

View reviewed changes

pgayvallet closed this Oct 17, 2023

afharo mentioned this pull request Oct 27, 2023

[logging] Escape control chars #169773

Merged

3 tasks

pgayvallet mentioned this pull request Nov 9, 2023

Minor reduce perf optimizations #170922

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize `reduce` loops - late 2023 edition #168812

Optimize `reduce` loops - late 2023 edition #168812

pgayvallet commented Oct 13, 2023 •

edited

Loading

kibana-ci commented Oct 16, 2023 •

edited

Loading

ESLint disabled line counts

Total ESLint disabled count

sphilipse left a comment

dej611 Oct 16, 2023

pgayvallet Oct 16, 2023

dej611 Oct 16, 2023

dej611 Oct 17, 2023

afharo left a comment

afharo Oct 16, 2023

afharo Oct 16, 2023

afharo Oct 16, 2023 •

edited

Loading

efegurkan left a comment

cnasikas left a comment

walterra left a comment

Ikuni17 left a comment

dgieselaar Oct 16, 2023

criamico left a comment

nchaulet left a comment

tomsonpl left a comment

cauemarcondes left a comment

thomheymann Oct 17, 2023

delanni Oct 17, 2023

delanni left a comment

vitaliidm Oct 17, 2023

vitaliidm Oct 17, 2023

vitaliidm Oct 17, 2023

vitaliidm Oct 17, 2023

vitaliidm Oct 17, 2023

vitaliidm Oct 17, 2023

pgayvallet commented Oct 17, 2023

Optimize reduce loops - late 2023 edition #168812

Optimize reduce loops - late 2023 edition #168812

Conversation

pgayvallet commented Oct 13, 2023 • edited Loading

Summary

kibana-ci commented Oct 16, 2023 • edited Loading

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

sphilipse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afharo Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

efegurkan left a comment

Choose a reason for hiding this comment

cnasikas left a comment

Choose a reason for hiding this comment

walterra left a comment

Choose a reason for hiding this comment

Ikuni17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

criamico left a comment

Choose a reason for hiding this comment

nchaulet left a comment

Choose a reason for hiding this comment

tomsonpl left a comment

Choose a reason for hiding this comment

cauemarcondes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delanni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet commented Oct 17, 2023

Optimize `reduce` loops - late 2023 edition #168812

Optimize `reduce` loops - late 2023 edition #168812

pgayvallet commented Oct 13, 2023 •

edited

Loading

kibana-ci commented Oct 16, 2023 •

edited

Loading

afharo Oct 16, 2023 •

edited

Loading