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: make FastBuffer safe to construct #36587

Closed
wants to merge 1 commit into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Dec 20, 2020

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

@aduh95 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
Copy link
Contributor Author

aduh95 commented Dec 20, 2020

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 20, 2020

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

                                                                     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 (***)

@aduh95 aduh95 marked this pull request as ready for review December 20, 2020 17:16
@aduh95 aduh95 removed the needs-benchmark-ci PR that need a benchmark CI run. label Dec 20, 2020
@aduh95
Copy link
Contributor Author

aduh95 commented Dec 20, 2020

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.

@mscdex
Copy link
Contributor

mscdex commented Dec 20, 2020

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.

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 20, 2020

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.

@aduh95 aduh95 changed the title buffer: improve FastBuffer constructor performance buffer: make FastBuffer safe to construct Dec 20, 2020
@mscdex
Copy link
Contributor

mscdex commented Dec 20, 2020

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.

@ExE-Boss
Copy link
Contributor

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

@mscdex
Copy link
Contributor

mscdex commented Dec 20, 2020

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.

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 20, 2020

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.

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 21, 2020
Using an explicit constructor is necessary to avoid relying on
`Array.prototype[Symbol.iterator]` and `%ArrayIteratorPrototype%.next`,
which can be mutated by users.
@aduh95
Copy link
Contributor Author

aduh95 commented Dec 21, 2020

Force pushed to fix the commit message.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 21, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 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 pending requests for changes, and a CI started. labels Dec 25, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 25, 2020
@github-actions
Copy link
Contributor

Landed in bd6f230...0187716

@github-actions github-actions bot closed this Dec 25, 2020
nodejs-github-bot pushed a commit that referenced this pull request Dec 25, 2020
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 aduh95 deleted the faster-buffer branch December 25, 2020 18:45
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
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>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
targos pushed a commit that referenced this pull request May 25, 2021
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>
targos pushed a commit that referenced this pull request Jun 5, 2021
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>
targos pushed a commit that referenced this pull request Jun 11, 2021
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>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants