buffer: improve fill(number) performance #31489

mscdex · 2020-01-24T07:48:49Z

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

nodejs-github-bot · 2020-01-24T07:49:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28603/

bnoordhuis

LGTM % comments.

bnoordhuis · 2020-01-24T09:00:03Z

lib/buffer.js

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

+const TypedArrayPrototype = ObjectGetPrototypeOf(Uint8Array.prototype);


Suggested change

const TypedArrayPrototype = ObjectGetPrototypeOf(Uint8Array.prototype);

const Uint8ArrayPrototype = ObjectGetPrototypeOf(Uint8Array.prototype);

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

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

@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

src/node_buffer.cc

nodejs-github-bot · 2020-01-24T11:12:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28604/

lib/buffer.js

targos · 2020-01-24T15:10:18Z

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

nodejs-github-bot · 2020-01-24T15:19:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28607/

nodejs-github-bot · 2020-01-25T23:03:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28668/

bnoordhuis · 2020-01-26T01:21:22Z

lib/buffer.js

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


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 · 2020-01-26T02:01:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28673/

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>

nodejs-github-bot · 2020-01-27T01:05:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28705/

Trott · 2020-01-27T02:34:39Z

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

Trott · 2020-01-27T02:59:17Z

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. 😍

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>

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 added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 24, 2020

bnoordhuis approved these changes Jan 24, 2020

View reviewed changes

mscdex force-pushed the buffer-improve-fill-number-perf branch from 039c099 to aa45188 Compare January 24, 2020 11:08

addaleax approved these changes Jan 24, 2020

View reviewed changes

BridgeAR approved these changes Jan 24, 2020

View reviewed changes

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

cjihrig approved these changes Jan 24, 2020

View reviewed changes

targos reviewed Jan 24, 2020

View reviewed changes

lib/buffer.js Outdated Show resolved Hide resolved

targos reviewed Jan 24, 2020

View reviewed changes

lib/buffer.js Outdated Show resolved Hide resolved

mscdex force-pushed the buffer-improve-fill-number-perf branch from aa45188 to ac9de4f Compare January 24, 2020 15:18

Trott approved these changes Jan 25, 2020

View reviewed changes

bnoordhuis reviewed Jan 26, 2020

View reviewed changes

mscdex force-pushed the buffer-improve-fill-number-perf branch from ac9de4f to 59cba9a Compare January 27, 2020 00:17

mscdex merged commit 59cba9a into nodejs:master Jan 27, 2020

mscdex deleted the buffer-improve-fill-number-perf branch January 27, 2020 00:21

codebytere mentioned this pull request Feb 17, 2020

v13.9.0 proposal #31837

Merged

codebytere mentioned this pull request Mar 17, 2020

v12.16.2 proposal #32313

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: improve fill(number) performance #31489

buffer: improve fill(number) performance #31489

mscdex commented Jan 24, 2020 •

edited

nodejs-github-bot commented Jan 24, 2020

bnoordhuis left a comment

bnoordhuis Jan 24, 2020

devsnek Jan 24, 2020

mscdex Jan 24, 2020 •

edited

nodejs-github-bot commented Jan 24, 2020

targos commented Jan 24, 2020

nodejs-github-bot commented Jan 24, 2020

nodejs-github-bot commented Jan 25, 2020

bnoordhuis Jan 26, 2020

nodejs-github-bot commented Jan 26, 2020

nodejs-github-bot commented Jan 27, 2020

Trott commented Jan 27, 2020

Trott commented Jan 27, 2020

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

buffer: improve fill(number) performance #31489

buffer: improve fill(number) performance #31489

Conversation

mscdex commented Jan 24, 2020 • edited

Checklist

nodejs-github-bot commented Jan 24, 2020

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Jan 24, 2020

Choose a reason for hiding this comment

devsnek Jan 24, 2020

Choose a reason for hiding this comment

mscdex Jan 24, 2020 • edited

Choose a reason for hiding this comment

nodejs-github-bot commented Jan 24, 2020

targos commented Jan 24, 2020

nodejs-github-bot commented Jan 24, 2020

nodejs-github-bot commented Jan 25, 2020

bnoordhuis Jan 26, 2020

Choose a reason for hiding this comment

nodejs-github-bot commented Jan 26, 2020

nodejs-github-bot commented Jan 27, 2020

Trott commented Jan 27, 2020

Trott commented Jan 27, 2020

mscdex commented Jan 24, 2020 •

edited

mscdex Jan 24, 2020 •

edited