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

buffer: improve fill(number) performance #31489

Merged
merged 1 commit into from
Jan 27, 2020

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Jan 24, 2020

Uses the native typedArray.fill() when filling with a number instead of calling into the binding fill function.

Benchmark results:

                                                                            confidence improvement accuracy (*)   (**)  (***)
 buffers/buffer-fill.js n=20000 size=65536 type='fill("")'                        ***      4.72 %       ±0.90% ±1.20% ±1.57%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("t", "utf8")'               ***      3.76 %       ±1.00% ±1.34% ±1.74%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("t", 0, "utf8")'            ***      4.30 %       ±0.74% ±0.99% ±1.29%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("t", 0)'                    ***      5.56 %       ±0.90% ±1.20% ±1.56%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("t")'                       ***      4.51 %       ±1.01% ±1.35% ±1.77%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("test")'                             0.18 %       ±0.50% ±0.67% ±0.87%
 buffers/buffer-fill.js n=20000 size=65536 type='fill(0)'                         ***      4.10 %       ±0.84% ±1.12% ±1.46%
 buffers/buffer-fill.js n=20000 size=65536 type='fill(100)'                       ***      4.75 %       ±0.97% ±1.30% ±1.69%
 buffers/buffer-fill.js n=20000 size=65536 type='fill(400)'                       ***      5.17 %       ±1.12% ±1.50% ±1.95%
 buffers/buffer-fill.js n=20000 size=65536 type='fill(Buffer.alloc(1), 0)'                -0.50 %       ±0.66% ±0.88% ±1.15%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("")'                         ***     17.23 %       ±1.65% ±2.20% ±2.86%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("t", "utf8")'                ***     12.32 %       ±1.62% ±2.18% ±2.87%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("t", 0, "utf8")'             ***     13.12 %       ±2.05% ±2.73% ±3.56%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("t", 0)'                     ***     18.16 %       ±1.00% ±1.33% ±1.74%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("t")'                        ***     12.18 %       ±1.64% ±2.18% ±2.86%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("test")'                             -0.34 %       ±1.20% ±1.59% ±2.08%
 buffers/buffer-fill.js n=20000 size=8192 type='fill(0)'                          ***     26.16 %       ±2.99% ±3.99% ±5.21%
 buffers/buffer-fill.js n=20000 size=8192 type='fill(100)'                        ***     28.79 %       ±1.14% ±1.52% ±1.98%
 buffers/buffer-fill.js n=20000 size=8192 type='fill(400)'                        ***     28.91 %       ±1.20% ±1.60% ±2.08%
 buffers/buffer-fill.js n=20000 size=8192 type='fill(Buffer.alloc(1), 0)'                 -0.40 %       ±1.31% ±1.75% ±2.28%
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@mscdex mscdex added buffer Issues and PRs related to the buffer subsystem. performance Issues and PRs related to the performance of Node.js. labels Jan 24, 2020
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 24, 2020
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM % comments.

lib/buffer.js Outdated
@@ -101,6 +104,13 @@ const {
addBufferPrototypeMethods
} = require('internal/buffer');

const TypedArrayPrototype = ObjectGetPrototypeOf(Uint8Array.prototype);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const TypedArrayPrototype = ObjectGetPrototypeOf(Uint8Array.prototype);
const Uint8ArrayPrototype = ObjectGetPrototypeOf(Uint8Array.prototype);

Uint8Array.prototype.constructor.name is Uint8Array, not TypedArray.

Copy link
Member

Choose a reason for hiding this comment

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

This line is correct as-is. The prototype of Uint8Array.prototype is the %TypedArray%.prototype.

Copy link
Contributor Author

@mscdex mscdex Jan 24, 2020

Choose a reason for hiding this comment

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

@devsnek is correct, this is the prototype of the prototype, which is TypedArray. The same thing is done in internal/util/types.js.

lib/buffer.js Outdated Show resolved Hide resolved
src/node_buffer.cc Outdated Show resolved Hide resolved
@mscdex mscdex force-pushed the buffer-improve-fill-number-perf branch from 039c099 to aa45188 Compare January 24, 2020 11:08
@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 24, 2020
lib/buffer.js Outdated Show resolved Hide resolved
lib/buffer.js Outdated Show resolved Hide resolved
@targos
Copy link
Member

targos commented Jan 24, 2020

I think we could also directly add TypedArray and its prototype to the primordials. I'll do that after this PR.

@mscdex mscdex force-pushed the buffer-improve-fill-number-perf branch from aa45188 to ac9de4f Compare January 24, 2020 15:18
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

if (res < 0) {
if (res === -1)
throw new ERR_INVALID_ARG_VALUE('value', value);
throw new ERR_BUFFER_OUT_OF_BOUNDS();
Copy link
Member

Choose a reason for hiding this comment

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

Future enhancement: res < 0 is sometimes a coercion-to-nan, sometimes not because bindingFill() returns either undefined or a number.

Changing Fill() in node_buffer.cc to always return a number is probably beneficial for performance.

@nodejs-github-bot
Copy link
Collaborator

PR-URL: nodejs#31489
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@mscdex mscdex force-pushed the buffer-improve-fill-number-perf branch from ac9de4f to 59cba9a Compare January 27, 2020 00:17
@mscdex mscdex merged commit 59cba9a into nodejs:master Jan 27, 2020
@mscdex mscdex deleted the buffer-improve-fill-number-perf branch January 27, 2020 00:21
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Jan 27, 2020

This seems to have landed without first getting a clean CI run.

@Trott
Copy link
Member

Trott commented Jan 27, 2020

This seems to have landed without first getting a clean CI run.

Although if there was an impressive-benchmarks-mean-you-can-land-without-a-clean-CI rule, this would definitely qualify. 😍

codebytere pushed a commit that referenced this pull request Feb 17, 2020
PR-URL: #31489
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebytere codebytere mentioned this pull request Feb 17, 2020
codebytere pushed a commit that referenced this pull request Mar 15, 2020
PR-URL: #31489
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
PR-URL: #31489
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebytere codebytere mentioned this pull request Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants