Skip to content

buffer: improve fill(number) performance - #31489

Merged
mscdex merged 1 commit into
nodejs:masterfrom
mscdex:buffer-improve-fill-number-perf
Jan 27, 2020
Merged

buffer: improve fill(number) performance#31489
mscdex merged 1 commit into
nodejs:masterfrom
mscdex:buffer-improve-fill-number-perf

Conversation

@mscdex

@mscdexmscdex commented Jan 24, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

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

@mscdexmscdex 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-botnodejs-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

nodejs-github-bot commented Jan 24, 2020

Copy link
Copy Markdown
Collaborator

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

bnoordhuis
bnoordhuis approved these changes Jan 24, 2020
View reviewed changes

@bnoordhuisbnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM % comments.

Comment threadlib/buffer.js Outdated

@bnoordhuisbnoordhuisJan 24, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
constTypedArrayPrototype=ObjectGetPrototypeOf(Uint8Array.prototype);
constUint8ArrayPrototype=ObjectGetPrototypeOf(Uint8Array.prototype);

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

@devsnekdevsnekJan 24, 2020

Copy link
Copy Markdown
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.

@mscdexmscdexJan 24, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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.

Comment threadlib/buffer.js Outdated
Comment threadsrc/node_buffer.cc Outdated
@mscdex
mscdexforce-pushed the buffer-improve-fill-number-perf branch from 039c099 to aa45188CompareJanuary 24, 2020 11:08
@nodejs-github-bot

nodejs-github-bot commented Jan 24, 2020

Copy link
Copy Markdown
Collaborator

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

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 24, 2020
targos
targos reviewed Jan 24, 2020
View reviewed changes
Comment threadlib/buffer.js Outdated
targos
targos reviewed Jan 24, 2020
View reviewed changes
Comment threadlib/buffer.js Outdated
@targos

targos commented Jan 24, 2020

Copy link
Copy Markdown
Member

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

@mscdex
mscdexforce-pushed the buffer-improve-fill-number-perf branch from aa45188 to ac9de4fCompareJanuary 24, 2020 15:18
@nodejs-github-bot

nodejs-github-bot commented Jan 24, 2020

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Jan 25, 2020

Copy link
Copy Markdown
Collaborator

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

bnoordhuis
bnoordhuis reviewed Jan 26, 2020
View reviewed changes
Comment threadlib/buffer.js Outdated

@bnoordhuisbnoordhuisJan 26, 2020

Copy link
Copy Markdown
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

nodejs-github-bot commented Jan 26, 2020

Copy link
Copy Markdown
Collaborator

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

@mscdex
buffer: improve fill(number) performance
59cba9a
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
mscdexforce-pushed the buffer-improve-fill-number-perf branch from ac9de4f to 59cba9aCompareJanuary 27, 2020 00:17
@mscdex
mscdex merged commit 59cba9a into nodejs:masterJan 27, 2020
@mscdex
mscdex deleted the buffer-improve-fill-number-perf branch January 27, 2020 00:21
@nodejs-github-bot

nodejs-github-bot commented Jan 27, 2020

Copy link
Copy Markdown
Collaborator

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

@Trott

Trott commented Jan 27, 2020

Copy link
Copy Markdown
Member

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

@Trott

Trott commented Jan 27, 2020

Copy link
Copy Markdown
Member

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
@mscdex@codebytere
buffer: improve fill(number) performance
9d45393
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>
@codebyterecodebytere mentioned this pull request Feb 17, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 15, 2020
@mscdex@codebytere
buffer: improve fill(number) performance
cfbaffd
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
@mscdex@codebytere
buffer: improve fill(number) performance
1849c2c
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>
@codebyterecodebytere mentioned this pull request Mar 17, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis approved these changes
@targostargostargos left review comments
@TrottTrottTrott approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@cjihrigcjihrigcjihrig approved these changes
@BridgeARBridgeARBridgeAR approved these changes
+1 more reviewer
@devsnekdevsnekdevsnek left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.bufferIssues and PRs related to the buffer subsystem.c++Issues and PRs that require attention from people who are familiar with C++.performanceIssues and PRs related to the performance of Node.js.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@mscdex@nodejs-github-bot@targos@Trott@bnoordhuis@addaleax@cjihrig@devsnek@BridgeAR