Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: partially replace .concat with .push #13609

Merged
merged 7 commits into from Aug 14, 2021

Conversation

fedeci
Copy link
Member

@fedeci fedeci commented Jul 28, 2021

Q                       A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

We change this.concat(arr) calls with Array.prototype.push.apply(this, arr).
Array.prototype.push.apply(this, arr) is quicker than .push(...arr) because it does not need to spread the array.

@fedeci fedeci added the PR: Performance 🏃‍♀️ A type of pull request used for our changelog categories label Jul 28, 2021
@babel-bot
Copy link
Collaborator

babel-bot commented Jul 28, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/48166/

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 28, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3e5a731:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@fedeci fedeci marked this pull request as draft July 28, 2021 15:26
@fedeci fedeci force-pushed the refactor/replace-concat branch 2 times, most recently from 79b1306 to a9c7016 Compare July 29, 2021 00:03
@fedeci fedeci marked this pull request as ready for review July 29, 2021 13:52
Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

For more information, Shi Ling has posted an article why .push is faster than .concat: https://dev.to/uilicious/javascript-array-push-is-945x-faster-than-array-concat-1oki

@nicolo-ribaudo
Copy link
Member

From the article linked by @JLHwung:

If you find Array.prototype.push.apply(arr1, arr2) verbose, you can use a simple variant using the rest spread ES6 syntax:

arr1.push(...arr2)

The performance difference between Array.prototype.push.apply(arr1, arr2) and arr1.push(...arr2) is negligable.

Unless we have data showing that the first one is actually better, we can probably use the second one (except for in the helper). I'd expect all engines to optimize the spread operator when arr2 is an array and arr2[Symbol.iterator] is the built-in array iterator.

@fedeci
Copy link
Member Author

fedeci commented Aug 4, 2021

These are the results of a bench with JSbench.me. Getting a 25-30% more perf for free doesn't sound bad to me, what do you think?

// Setup
let arr0 = []
let arr1 = [0, 1]

// .push(...arr)
for (let i = 0; i < 2; i++) {
	arr0.push(...arr1)
}

// .concat(arr)
for (let i = 0; i < 2; i++) {
	arr0 = arr0.concat(arr1)
}

// .push.apply(this, arr)
for (let i = 0; i < 2; i++) {
	Array.prototype.push.apply(arr0, arr1)
}

Results:
.push 4970969 ops/s ± 1.23% | 28.8% slower
.concat 37578 ops/s ± 3.29% | 99.46 slower
.push.apply 6981916 ops/s ± 1.09% | Fastest

@JLHwung
Copy link
Contributor

JLHwung commented Aug 4, 2021

I have setup a benchmark case here: .push(...arr) is as fast asArray.prototype.push.apply in V8 (expected) but the former is 30% slower than the latter in Firefox.

Anyway, Array.prototype.push.apply is almost identical to arr.push.apply().

@fedeci
Copy link
Member Author

fedeci commented Aug 4, 2021

Probably jsbench.me uses the browser engine to run the tests, so it used WebKit for mine.

@lightmare
Copy link
Contributor

Getting a 25-30% more perf for free

That really depends on the engine. For me on FF90, the difference is only about 1%. And for the tiny arrays you used, a custom function is much faster:

// Setup
let arr0 = []
let arr1 = Array.from({length:10}, (_, i) => i);
let N = 10;

function extend(dest, src) {
	const end = src.length;
	for (let di = dest.length, si = 0; si < end; ) {
		dest[di++] = src[si++];
	}
}

// .push(...arr)
for (let i = 0; i < N; i++) {
	arr0.push(...arr1)
}

// .push.apply(this, arr)
for (let i = 0; i < N; i++) {
	Array.prototype.push.apply(arr0, arr1)
}

// extend(dest, src)
for (let i = 0; i < N; i++) {
	extend(arr0, arr1);
}

// .push(...arr) — 215666.98 ops/s ± 1.34%
43.66 % slower

// .push.apply(this, arr) — 215674.72 ops/s ± 1.11%
43.66 % slower

// extend(dest, src) — 382775.7 ops/s ± 5.87%
Fastest

When I increase the source array length to 20, the custom function still wins by ~20%. It starts losing somewhere around length 50. For longer arrays it is slower, but it doesn't have a length limit like .push, e.g. in node:

Array.prototype.push.apply([], {length:125000});
Uncaught RangeError: Maximum call stack size exceeded

@nicolo-ribaudo
Copy link
Member

This code is going to run in V8 (Node.js) 99.9% of the times. Running @JLHwung's jsbench I get these results:

  • .push(...arr) 14,582,342.47 ops/s ± 2.01% (fastest)
  • Array.prototype.push.apply 13,373,289.89 ops/s ± 1.86% (8.29% slower)
  • arr.push.apply 13,429,936.82 ops/s ± 2.14% (7.9% slower)
  • arr.concat 85,879.34 ops/s ± 0.84% (99.41% slower)

The push versions are are similarly fast, so we can just use what's more readable (.push(...arr)).

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@@ -17,7 +17,7 @@ function getQualifiedName(node: t.GenericTypeAnnotation["id"]) {
* Dedupe type annotations.
*/
export default function removeTypeDuplicates(
nodes: ReadonlyArray<t.FlowType | false | null | undefined>,
nodes: (t.FlowType | false | null | undefined)[],
Copy link
Member

Choose a reason for hiding this comment

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

This is exposed as part of the public API, so we cannot change this.

We could clone nodes at the beginning of the function so that we can then .push() into it, but we would need a few benchmarks to check if it's worth it.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case benchmarks are not that useful because we should evaluate a number of different cases.

@@ -41,7 +41,7 @@ export default function removeTypeDuplicates(

if (isTSUnionType(node)) {
if (typeGroups.indexOf(node.types) < 0) {
nodes = nodes.concat(node.types);
nodes.push(...node.types);
Copy link
Member

Choose a reason for hiding this comment

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

Also here.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the TS version nodes is not marked as readonly, should we align it to flow or vice versa?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we can mark the TS version as readonly. In general, public APIs should treat their parameters as readonly unless the function name clearly indicates that it's mutating.

We can do it in a separate PR to have the different changelog entry, since it affects a different kind of users.

packages/babel-traverse/src/path/family.ts Outdated Show resolved Hide resolved
Copy link

@DavidBergeron-eng DavidBergeron-eng left a comment

Choose a reason for hiding this comment

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

(spam)

@nicolo-ribaudo nicolo-ribaudo merged commit 10640b2 into babel:main Aug 14, 2021
@fedeci fedeci deleted the refactor/replace-concat branch August 16, 2021 09:02
Copy link

@DavidBergeron-eng DavidBergeron-eng left a comment

Choose a reason for hiding this comment

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

What does the page run 😒

@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Dec 22, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Performance 🏃‍♀️ A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants