buffer: make FastBuffer safe to construct by aduh95 · Pull Request #36587 · nodejs/node · GitHub

Default constructor iterates over the argument when calling the super constructor, we can get better result by passing an exact number of arguments on Apple Silicon processors.

Benchmark I run locally

                                                                     confidence improvement accuracy (*)   (**)  (***)
 buffers/buffer-creation.js n=600000 len=10 type='fast-alloc-fill'           ***      3.12 %       ±1.34% ±1.79% ±2.34%
 buffers/buffer-creation.js n=600000 len=10 type='fast-alloc'                         1.34 %       ±1.37% ±1.83% ±2.41%
 buffers/buffer-creation.js n=600000 len=10 type='fast-allocUnsafe'          ***      2.81 %       ±0.22% ±0.29% ±0.38%
 buffers/buffer-creation.js n=600000 len=10 type='slow-allocUnsafe'          ***      1.06 %       ±0.47% ±0.63% ±0.82%
 buffers/buffer-creation.js n=600000 len=1024 type='fast-alloc-fill'         ***      2.07 %       ±0.66% ±0.88% ±1.15%
 buffers/buffer-creation.js n=600000 len=1024 type='fast-alloc'                       1.10 %       ±1.42% ±1.89% ±2.47%
 buffers/buffer-creation.js n=600000 len=1024 type='fast-allocUnsafe'        ***      2.04 %       ±0.78% ±1.04% ±1.35%
 buffers/buffer-creation.js n=600000 len=1024 type='slow-allocUnsafe'                 1.22 %       ±3.00% ±4.00% ±5.21%
 buffers/buffer-creation.js n=600000 len=4096 type='fast-alloc-fill'                 -0.57 %       ±0.69% ±0.92% ±1.19%
 buffers/buffer-creation.js n=600000 len=4096 type='fast-alloc'                      -0.55 %       ±0.65% ±0.86% ±1.12%
 buffers/buffer-creation.js n=600000 len=4096 type='fast-allocUnsafe'        ***      6.19 %       ±1.30% ±1.72% ±2.24%
 buffers/buffer-creation.js n=600000 len=4096 type='slow-allocUnsafe'        ***      5.92 %       ±1.09% ±1.46% ±1.90%
 buffers/buffer-creation.js n=600000 len=8192 type='fast-alloc-fill'                  0.88 %       ±0.98% ±1.30% ±1.70%
 buffers/buffer-creation.js n=600000 len=8192 type='fast-alloc'                      -0.22 %       ±1.16% ±1.54% ±2.00%
 buffers/buffer-creation.js n=600000 len=8192 type='fast-allocUnsafe'                 0.81 %       ±1.50% ±2.00% ±2.62%
 buffers/buffer-creation.js n=600000 len=8192 type='slow-allocUnsafe'                 1.11 %       ±1.17% ±1.56% ±2.03%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 16 comparisons, you can thus
expect the following amount of false-positive results:
  0.80 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.16 false positives, when considering a   1% risk acceptance (**, ***),
  0.02 false positives, when considering a 0.1% risk acceptance (***)

Related Issues

Refs: #36428
Refs: #36532

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/774/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/775/

Benchmark results are not as good as the one I got locally, that might be related to using a different OS / architecture, not sure 🤔

@mscdex FYI

Details

                                                                     confidence improvement accuracy (*)    (**)   (***)
buffers/buffer-creation.js n=600000 len=1024 type='fast-alloc'                      -0.65 %       ±5.67%  ±7.55%  ±9.84%
buffers/buffer-creation.js n=600000 len=1024 type='fast-alloc-fill'                  3.19 %       ±6.33%  ±8.42% ±10.96%
buffers/buffer-creation.js n=600000 len=1024 type='fast-allocUnsafe'                 1.59 %       ±4.97%  ±6.61%  ±8.61%
buffers/buffer-creation.js n=600000 len=1024 type='slow-allocUnsafe'                -3.15 %       ±4.80%  ±6.39%  ±8.32%
buffers/buffer-creation.js n=600000 len=10 type='fast-alloc'                         4.20 %       ±8.95% ±11.91% ±15.50%
buffers/buffer-creation.js n=600000 len=10 type='fast-alloc-fill'                    0.17 %      ±10.80% ±14.38% ±18.75%
buffers/buffer-creation.js n=600000 len=10 type='fast-allocUnsafe'                  -3.57 %       ±7.19%  ±9.58% ±12.49%
buffers/buffer-creation.js n=600000 len=10 type='slow-allocUnsafe'                   0.30 %       ±6.21%  ±8.27% ±10.79%
buffers/buffer-creation.js n=600000 len=4096 type='fast-alloc'                       2.64 %       ±5.10%  ±6.79%  ±8.85%
buffers/buffer-creation.js n=600000 len=4096 type='fast-alloc-fill'                 -1.68 %       ±4.13%  ±5.49%  ±7.15%
buffers/buffer-creation.js n=600000 len=4096 type='fast-allocUnsafe'          *     -5.78 %       ±4.56%  ±6.06%  ±7.89%
buffers/buffer-creation.js n=600000 len=4096 type='slow-allocUnsafe'         **     -5.83 %       ±3.91%  ±5.20%  ±6.77%
buffers/buffer-creation.js n=600000 len=8192 type='fast-alloc'                      -1.86 %       ±4.64%  ±6.18%  ±8.05%
buffers/buffer-creation.js n=600000 len=8192 type='fast-alloc-fill'           *      4.20 %       ±3.59%  ±4.78%  ±6.23%
buffers/buffer-creation.js n=600000 len=8192 type='fast-allocUnsafe'                -4.04 %       ±4.99%  ±6.65%  ±8.67%
buffers/buffer-creation.js n=600000 len=8192 type='slow-allocUnsafe'                 2.05 %       ±6.44%  ±8.58% ±11.19%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 16 comparisons, you can thus
expect the following amount of false-positive results:
 0.80 false positives, when considering a   5% risk acceptance (*, **, ***),
 0.16 false positives, when considering a   1% risk acceptance (**, ***),
 0.02 false positives, when considering a 0.1% risk acceptance (***)

                                                                     confidence improvement accuracy (*)    (**)   (***)
buffers/buffer-creation.js n=600000 len=1024 type='fast-alloc'                      -1.56 %       ±4.32%  ±5.75%  ±7.49%
buffers/buffer-creation.js n=600000 len=1024 type='fast-alloc-fill'                 -3.01 %       ±4.59%  ±6.10%  ±7.94%
buffers/buffer-creation.js n=600000 len=1024 type='fast-allocUnsafe'                -0.74 %       ±4.29%  ±5.71%  ±7.43%
buffers/buffer-creation.js n=600000 len=1024 type='slow-allocUnsafe'                -0.80 %       ±4.49%  ±5.98%  ±7.81%
buffers/buffer-creation.js n=600000 len=10 type='fast-alloc'                         1.45 %       ±7.79% ±10.37% ±13.50%
buffers/buffer-creation.js n=600000 len=10 type='fast-alloc-fill'                    3.03 %       ±6.52%  ±8.68% ±11.31%
buffers/buffer-creation.js n=600000 len=10 type='fast-allocUnsafe'                   4.72 %       ±8.13% ±10.84% ±14.14%
buffers/buffer-creation.js n=600000 len=10 type='slow-allocUnsafe'                   0.52 %       ±8.99% ±11.96% ±15.56%
buffers/buffer-creation.js n=600000 len=4096 type='fast-alloc'                       0.42 %       ±3.72%  ±4.95%  ±6.44%
buffers/buffer-creation.js n=600000 len=4096 type='fast-alloc-fill'                 -4.15 %       ±4.26%  ±5.67%  ±7.39%
buffers/buffer-creation.js n=600000 len=4096 type='fast-allocUnsafe'                -3.64 %       ±4.19%  ±5.58%  ±7.27%
buffers/buffer-creation.js n=600000 len=4096 type='slow-allocUnsafe'                -1.67 %       ±4.44%  ±5.92%  ±7.70%
buffers/buffer-creation.js n=600000 len=8192 type='fast-alloc'                       2.27 %       ±3.68%  ±4.90%  ±6.38%
buffers/buffer-creation.js n=600000 len=8192 type='fast-alloc-fill'                  0.21 %       ±4.48%  ±5.96%  ±7.76%
buffers/buffer-creation.js n=600000 len=8192 type='fast-allocUnsafe'                -2.39 %       ±3.93%  ±5.24%  ±6.84%
buffers/buffer-creation.js n=600000 len=8192 type='slow-allocUnsafe'                 1.14 %       ±4.32%  ±5.74%  ±7.48%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 16 comparisons, you can thus
expect the following amount of false-positive results:
 0.80 false positives, when considering a   5% risk acceptance (*, **, ***),
 0.16 false positives, when considering a   1% risk acceptance (**, ***),
 0.02 false positives, when considering a 0.1% risk acceptance (***)

From what I can see based on the architecture I have at my disposal, this PR is neutral for performance on x86 macOS & Linux, and improve the perf on arm64 macOS.

I'm inclined to believe Jenkins on this one as this is literally the same behavior as before. If anything, I'd expect a minor perf hit because V8 may be able to optimize calling default constructors of built-in types.

this is literally the same behavior as before

Except it doesn't use Array.prototype[Symbol.iterator]. Given the benchmark results, I should probably change the commit message to not give false hope on what this is actually doing.

Except it doesn't use Array.prototype[Symbol.iterator]

I don't follow. What I'm referring to is the change in this PR:

From an implicit, inherited constructor:

class FastBuffer extends Uint8Array {}

To an explicit constructor that passes the exact same parameters:

class FastBuffer extends Uint8Array {
  // eslint-disable-next-line no-useless-constructor
  constructor(bufferOrLength, byteOffset, length) {
    super(bufferOrLength, byteOffset, length);
  }
}

Neither of these should invoke anything Array-related.

@mscdex Except that a missing constructor in a derived class is equivalent to:

constructor(...args) { super(...args) }

And the spread operator invokes the [Symbol.iterator] method, and if you were to do:

delete Array.prototype[Symbol.iterator];
new (class extends Object {})();

You’ll get:

Uncaught TypeError: Found non-callable @@iterator

Until tc39/ecma262#2216 is implemented.

I think if this is going to be merged, we should have at least a code comment describing why this is being done this way.

I'm +0 on this since it doesn't really seem to change much, at least with the Buffer creation benchmarks.

Relevant spec section: https://tc39.es/ecma262/#sec-runtime-semantics-classdefinitionevaluation

If constructor is empty, then

If ClassHeritageopt is present, then

Let constructorText be the source text

constructor(...args) { super(...args); }

Note that there's also a difference because the super constructor always receive 3 arguments. According to the spec, this would cause a slight difference when new FastBuffer is invoqued with no arguments (there's a shortcut for that case); since the benchmark doesn't show any perf regression, that should be fine.

Using an explicit constructor is necessary to avoid relying on `Array.prototype[Symbol.iterator]` and `%ArrayIteratorPrototype%.next`, which can be mutated by users.

Force pushed to fix the commit message.

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

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

Landed in bd6f230...0187716

Using an explicit constructor is necessary to avoid relying on `Array.prototype[Symbol.iterator]` and `%ArrayIteratorPrototype%.next`, which can be mutated by users. PR-URL: #36587 Refs: #36428 Refs: #36532 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

aduh95 added buffer Issues and PRs related to the buffer subsystem. needs-benchmark-ci PR that need a benchmark CI run. labels Dec 20, 2020

aduh95 marked this pull request as ready for review December 20, 2020 17:16

aduh95 removed the needs-benchmark-ci PR that need a benchmark CI run. label Dec 20, 2020

aduh95 changed the title buffer: improve FastBuffer constructor performance buffer: make FastBuffer safe to construct Dec 20, 2020

ExE-Boss approved these changes Dec 20, 2020

View reviewed changes

aduh95 commented Dec 20, 2020

View reviewed changes

Comment thread lib/internal/buffer.js Outdated

This was referenced Dec 20, 2020

lib: make safe primordials safe to construct #36428

Closed

lib: add primordials.SafeArrayIterator #36532

Merged

Trott approved these changes Dec 21, 2020

View reviewed changes

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 21, 2020

buffer: make FastBuffer safe to construct

657c772

Using an explicit constructor is necessary to avoid relying on `Array.prototype[Symbol.iterator]` and `%ArrayIteratorPrototype%.next`, which can be mutated by users.

aduh95 force-pushed the faster-buffer branch from 9e816db to 657c772 Compare December 21, 2020 13:24

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 21, 2020

lpinca approved these changes Dec 24, 2020

View reviewed changes

aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Dec 25, 2020

github-actions Bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 25, 2020

github-actions Bot closed this Dec 25, 2020

aduh95 deleted the faster-buffer branch December 25, 2020 18:45

danielleadams mentioned this pull request Jan 12, 2021

v15.6.0 proposal #36889

Merged

targos mentioned this pull request Jun 6, 2021

v14.17.1 release proposal #38948

Merged

buffer: make FastBuffer safe to construct by aduh95 · Pull Request #36587 · nodejs/node · GitHub

Uh oh!

Uh oh!

buffer: make FastBuffer safe to construct - #36587

buffer: make FastBuffer safe to construct#36587aduh95 wants to merge 1 commit intonodejs:masternodejs/node:masterfrom aduh95:faster-bufferaduh95/node:faster-bufferCopy head branch name to clipboard

Conversation

aduh95 commented Dec 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issues

Checklist

Uh oh!

aduh95 commented Dec 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Dec 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Dec 20, 2020

Uh oh!

mscdex commented Dec 20, 2020

Uh oh!

aduh95 commented Dec 20, 2020

Uh oh!

mscdex commented Dec 20, 2020

Uh oh!

ExE-Boss commented Dec 20, 2020

Uh oh!

mscdex commented Dec 20, 2020

Uh oh!

aduh95 commented Dec 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

aduh95 commented Dec 21, 2020

Uh oh!

nodejs-github-bot commented Dec 21, 2020

Uh oh!

nodejs-github-bot commented Dec 24, 2020

Uh oh!

github-actions Bot commented Dec 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

buffer: make FastBuffer safe to construct#36587
aduh95 wants to merge 1 commit into
nodejs:masterfrom
aduh95:faster-buffer

aduh95 commented Dec 20, 2020 •
edited
Loading

aduh95 commented Dec 20, 2020 •
edited
Loading

aduh95 commented Dec 20, 2020 •
edited
Loading

aduh95 commented Dec 20, 2020 •
edited
Loading