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: always allocate typed arrays outside heap #2893

Closed
wants to merge 1 commit into from

Conversation

trevnorris
Copy link
Contributor

By default v8 allocates typed arrays <= 64 bytes inside the v8 heap. In
these cases the memory pointer returned by Buffer::Data() can change
while the memory is being operated on. Resolve by passing a flag that
forces all typed arrays outside the v8 heap.

Fixes: 74178a5 "buffer: construct Uint8Array in JS"

R=@indutny
R=@bnoordhuis

By default v8 allocates typed arrays <= 64 bytes inside the v8 heap. In
these cases the memory pointer returned by Buffer::Data() can change
while the memory is being operated on. Resolve by passing a flag that
forces all typed arrays outside the v8 heap.

Fixes: 74178a5 "buffer: construct Uint8Array in JS"
@trevnorris
Copy link
Contributor Author

@targos
Copy link
Member

targos commented Sep 16, 2015

Does it change anything for the users of typed arrays (apart from Buffer) ?

@trevnorris
Copy link
Contributor Author

@targos Yes. No in heap allocated v8 typed arrays are allowed. This is a safer way moving forward as people need to be able to inherit from buffer by creating their own Uint8Array. Example: #2882 (comment)

If any of these were allowed to be v8 heap allocated then the returned pointer from Buffer::Data() could become invalid in the middle of an operation.

@ChALkeR ChALkeR added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Sep 16, 2015
@indutny
Copy link
Member

indutny commented Sep 16, 2015

LGTM

trevnorris added a commit that referenced this pull request Sep 16, 2015
By default v8 allocates typed arrays <= 64 bytes inside the v8 heap. In
these cases the memory pointer returned by Buffer::Data() can change
while the memory is being operated on. Resolve by passing a flag that
forces all typed arrays outside the v8 heap.

Fixes: 74178a5 "buffer: construct Uint8Array in JS"
PR-URL: #2893
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@trevnorris
Copy link
Contributor Author

Thanks. Landed in 16f86d6.

@trevnorris
Copy link
Contributor Author

@Fishrock123 Should be able to cherry-pick the commit this references, and this commit, onto v4.

@bnoordhuis
Copy link
Member

For my understanding, does that mean ArrayBuffer::IsExternal() should return true from now on for all arraybuffers? Might be worth adding a few CHECKs to that effect to src/node_buffer.cc.

trevnorris added a commit that referenced this pull request Sep 16, 2015
By default v8 allocates typed arrays <= 64 bytes inside the v8 heap. In
these cases the memory pointer returned by Buffer::Data() can change
while the memory is being operated on. Resolve by passing a flag that
forces all typed arrays outside the v8 heap.

Fixes: 74178a5 "buffer: construct Uint8Array in JS"
PR-URL: #2893
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@Fishrock123 Fishrock123 mentioned this pull request Sep 16, 2015
7 tasks
@trevnorris trevnorris deleted the no-ab-heap-alloc branch September 16, 2015 17:22
@trevnorris
Copy link
Contributor Author

@bnoordhuis IsExternal() doesn't mean it exists outside the heap. Means that the lifetime of the memory is controlled by the user (e.g. ArrayBuffer is created with kExternalized (the default)). But we always create with kInternalized.

@MylesBorins
Copy link
Contributor

landed in lts-v4.x-staging as 4b4cfa2

addaleax added a commit to addaleax/node that referenced this pull request Feb 25, 2019
This was added in 16f86d6, based on
the assumption that otherwise, the memory behind `ArrayBuffer`
instances could be moved around on the heap while native code
holds references to it.

This does not match what V8 actually does (and also did at the time):

- The option/build variable was about always only about TypedArrays,
  not ArrayBuffers. Calls like `new ArrayBuffer(4)` call into C++
  regardless of the option value, but calls like `new Uint8Array(4)`
  would not call into C++ under V8 defaults.
- When first accessing a heap-allocated TypedArray’s `ArrayBuffer`,
  whether that is through the JS `.buffer` getter or the C++
  `ArrayBufferView::Buffer()` function, a copy of the contents is
  created using the ArrayBuffer allocator and stored as the
  (permanent, unmovable) backing store.

As a consequence, the memory returned by `ArrayBuffer::GetContents()`
is not moved around, because it is fixed once the `ArrayBuffer`
object itself first comes into explicit existence in any way.

Removing this build option significantly speeds up creation of typed
arrays from JS:

    $ ./node benchmark/compare.js --new ./node --old ./node-master --runs 10 --filter buffer-creation.js buffers | Rscript benchmark/compare.R
                                                                         confidence improvement accuracy (*)     (**)    (***)
     buffers/buffer-creation.js n=1024 len=10 type='buffer()'                  ***    593.66 %      ±28.64%  ±41.10%  ±60.36%
     buffers/buffer-creation.js n=1024 len=10 type='fast-alloc-fill'           ***    675.42 %      ±90.67% ±130.24% ±191.54%
     buffers/buffer-creation.js n=1024 len=10 type='fast-alloc'                ***    663.55 %      ±58.41%  ±83.87% ±123.29%
     buffers/buffer-creation.js n=1024 len=10 type='fast-allocUnsafe'                   3.10 %       ±9.63%  ±13.22%  ±18.07%
     buffers/buffer-creation.js n=1024 len=10 type='slow-allocUnsafe'                   4.67 %       ±5.55%   ±7.77%  ±10.97%
     buffers/buffer-creation.js n=1024 len=10 type='slow'                              -2.48 %       ±4.47%   ±6.12%   ±8.34%
     buffers/buffer-creation.js n=1024 len=1024 type='buffer()'                        -1.91 %       ±4.71%   ±6.45%   ±8.79%
     buffers/buffer-creation.js n=1024 len=1024 type='fast-alloc-fill'                 -1.34 %       ±7.53%  ±10.33%  ±14.10%
     buffers/buffer-creation.js n=1024 len=1024 type='fast-alloc'                       0.52 %       ±5.00%   ±6.87%   ±9.40%
     buffers/buffer-creation.js n=1024 len=1024 type='fast-allocUnsafe'                 0.39 %       ±5.65%   ±7.78%  ±10.67%
     buffers/buffer-creation.js n=1024 len=1024 type='slow-allocUnsafe'                -0.13 %       ±5.68%   ±7.83%  ±10.77%
     buffers/buffer-creation.js n=1024 len=1024 type='slow'                            -5.07 %       ±7.15%   ±9.80%  ±13.35%
     buffers/buffer-creation.js n=1024 len=2048 type='buffer()'                         0.57 %       ±2.70%   ±3.74%   ±5.16%
     buffers/buffer-creation.js n=1024 len=2048 type='fast-alloc-fill'                 -1.60 %       ±4.96%   ±6.79%   ±9.25%
     buffers/buffer-creation.js n=1024 len=2048 type='fast-alloc'                       1.29 %       ±3.79%   ±5.20%   ±7.09%
     buffers/buffer-creation.js n=1024 len=2048 type='fast-allocUnsafe'                 2.73 %       ±8.79%  ±12.05%  ±16.41%
     buffers/buffer-creation.js n=1024 len=2048 type='slow-allocUnsafe'                -0.99 %       ±6.27%   ±8.65%  ±11.91%
     buffers/buffer-creation.js n=1024 len=2048 type='slow'                            -5.98 %       ±6.24%   ±8.71%  ±12.20%
     buffers/buffer-creation.js n=1024 len=4096 type='buffer()'                        -1.75 %       ±3.48%   ±4.78%   ±6.56%
     buffers/buffer-creation.js n=1024 len=4096 type='fast-alloc-fill'                 -3.18 %       ±3.97%   ±5.45%   ±7.45%
     buffers/buffer-creation.js n=1024 len=4096 type='fast-alloc'                       2.05 %       ±4.05%   ±5.58%   ±7.65%
     buffers/buffer-creation.js n=1024 len=4096 type='fast-allocUnsafe'                 1.44 %       ±5.51%   ±7.63%  ±10.57%
     buffers/buffer-creation.js n=1024 len=4096 type='slow-allocUnsafe'          *     -4.77 %       ±4.30%   ±5.90%   ±8.06%
     buffers/buffer-creation.js n=1024 len=4096 type='slow'                            -3.31 %       ±6.38%   ±8.86%  ±12.34%
     buffers/buffer-creation.js n=1024 len=8192 type='buffer()'                         0.06 %       ±2.70%   ±3.77%   ±5.31%
     buffers/buffer-creation.js n=1024 len=8192 type='fast-alloc-fill'                 -1.20 %       ±3.30%   ±4.53%   ±6.17%
     buffers/buffer-creation.js n=1024 len=8192 type='fast-alloc'                      -1.46 %       ±2.75%   ±3.84%   ±5.38%
     buffers/buffer-creation.js n=1024 len=8192 type='fast-allocUnsafe'                 1.27 %       ±4.69%   ±6.49%   ±8.98%
     buffers/buffer-creation.js n=1024 len=8192 type='slow-allocUnsafe'                -1.68 %       ±3.30%   ±4.62%   ±6.49%
     buffers/buffer-creation.js n=1024 len=8192 type='slow'                            -2.49 %       ±3.24%   ±4.44%   ±6.07%
     (Re-running the outlier with 30 runs instead of 10:)
     buffers/buffer-creation.js n=1024 len=4096 type='slow-allocUnsafe'                 2.06 %       ±2.39%   ±3.19%   ±4.15%

The performance gains effect are undone once native code accesses
the underlying ArrayBuffer, but then again that a) does not happen for
all TypedArrays, and b) it should also make sense to look into using
`ArrayBufferView::CopyContents()` in some places, which is made
specifically to avoid such a performance impact and allows us to
use the benefits of heap-allocated typed arrays.

Refs: nodejs@16f86d6
Refs: nodejs#2893
Refs: nodejs@74178a5#commitcomment-13250880
Refs: http://logs.libuv.org/node-dev/2015-09-15
addaleax added a commit that referenced this pull request Mar 1, 2019
This was added in 16f86d6, based on
the assumption that otherwise, the memory behind `ArrayBuffer`
instances could be moved around on the heap while native code
holds references to it.

This does not match what V8 actually does (and also did at the time):

- The option/build variable was about always only about TypedArrays,
  not ArrayBuffers. Calls like `new ArrayBuffer(4)` call into C++
  regardless of the option value, but calls like `new Uint8Array(4)`
  would not call into C++ under V8 defaults.
- When first accessing a heap-allocated TypedArray’s `ArrayBuffer`,
  whether that is through the JS `.buffer` getter or the C++
  `ArrayBufferView::Buffer()` function, a copy of the contents is
  created using the ArrayBuffer allocator and stored as the
  (permanent, unmovable) backing store.

As a consequence, the memory returned by `ArrayBuffer::GetContents()`
is not moved around, because it is fixed once the `ArrayBuffer`
object itself first comes into explicit existence in any way.

Removing this build option significantly speeds up creation of typed
arrays from JS:

    $ ./node benchmark/compare.js --new ./node --old ./node-master --runs 10 --filter buffer-creation.js buffers | Rscript benchmark/compare.R
                                                                         confidence improvement accuracy (*)     (**)    (***)
     buffers/buffer-creation.js n=1024 len=10 type='buffer()'                  ***    593.66 %      ±28.64%  ±41.10%  ±60.36%
     buffers/buffer-creation.js n=1024 len=10 type='fast-alloc-fill'           ***    675.42 %      ±90.67% ±130.24% ±191.54%
     buffers/buffer-creation.js n=1024 len=10 type='fast-alloc'                ***    663.55 %      ±58.41%  ±83.87% ±123.29%
     buffers/buffer-creation.js n=1024 len=10 type='fast-allocUnsafe'                   3.10 %       ±9.63%  ±13.22%  ±18.07%
     buffers/buffer-creation.js n=1024 len=10 type='slow-allocUnsafe'                   4.67 %       ±5.55%   ±7.77%  ±10.97%
     buffers/buffer-creation.js n=1024 len=10 type='slow'                              -2.48 %       ±4.47%   ±6.12%   ±8.34%
     buffers/buffer-creation.js n=1024 len=1024 type='buffer()'                        -1.91 %       ±4.71%   ±6.45%   ±8.79%
     buffers/buffer-creation.js n=1024 len=1024 type='fast-alloc-fill'                 -1.34 %       ±7.53%  ±10.33%  ±14.10%
     buffers/buffer-creation.js n=1024 len=1024 type='fast-alloc'                       0.52 %       ±5.00%   ±6.87%   ±9.40%
     buffers/buffer-creation.js n=1024 len=1024 type='fast-allocUnsafe'                 0.39 %       ±5.65%   ±7.78%  ±10.67%
     buffers/buffer-creation.js n=1024 len=1024 type='slow-allocUnsafe'                -0.13 %       ±5.68%   ±7.83%  ±10.77%
     buffers/buffer-creation.js n=1024 len=1024 type='slow'                            -5.07 %       ±7.15%   ±9.80%  ±13.35%
     buffers/buffer-creation.js n=1024 len=2048 type='buffer()'                         0.57 %       ±2.70%   ±3.74%   ±5.16%
     buffers/buffer-creation.js n=1024 len=2048 type='fast-alloc-fill'                 -1.60 %       ±4.96%   ±6.79%   ±9.25%
     buffers/buffer-creation.js n=1024 len=2048 type='fast-alloc'                       1.29 %       ±3.79%   ±5.20%   ±7.09%
     buffers/buffer-creation.js n=1024 len=2048 type='fast-allocUnsafe'                 2.73 %       ±8.79%  ±12.05%  ±16.41%
     buffers/buffer-creation.js n=1024 len=2048 type='slow-allocUnsafe'                -0.99 %       ±6.27%   ±8.65%  ±11.91%
     buffers/buffer-creation.js n=1024 len=2048 type='slow'                            -5.98 %       ±6.24%   ±8.71%  ±12.20%
     buffers/buffer-creation.js n=1024 len=4096 type='buffer()'                        -1.75 %       ±3.48%   ±4.78%   ±6.56%
     buffers/buffer-creation.js n=1024 len=4096 type='fast-alloc-fill'                 -3.18 %       ±3.97%   ±5.45%   ±7.45%
     buffers/buffer-creation.js n=1024 len=4096 type='fast-alloc'                       2.05 %       ±4.05%   ±5.58%   ±7.65%
     buffers/buffer-creation.js n=1024 len=4096 type='fast-allocUnsafe'                 1.44 %       ±5.51%   ±7.63%  ±10.57%
     buffers/buffer-creation.js n=1024 len=4096 type='slow-allocUnsafe'          *     -4.77 %       ±4.30%   ±5.90%   ±8.06%
     buffers/buffer-creation.js n=1024 len=4096 type='slow'                            -3.31 %       ±6.38%   ±8.86%  ±12.34%
     buffers/buffer-creation.js n=1024 len=8192 type='buffer()'                         0.06 %       ±2.70%   ±3.77%   ±5.31%
     buffers/buffer-creation.js n=1024 len=8192 type='fast-alloc-fill'                 -1.20 %       ±3.30%   ±4.53%   ±6.17%
     buffers/buffer-creation.js n=1024 len=8192 type='fast-alloc'                      -1.46 %       ±2.75%   ±3.84%   ±5.38%
     buffers/buffer-creation.js n=1024 len=8192 type='fast-allocUnsafe'                 1.27 %       ±4.69%   ±6.49%   ±8.98%
     buffers/buffer-creation.js n=1024 len=8192 type='slow-allocUnsafe'                -1.68 %       ±3.30%   ±4.62%   ±6.49%
     buffers/buffer-creation.js n=1024 len=8192 type='slow'                            -2.49 %       ±3.24%   ±4.44%   ±6.07%
     (Re-running the outlier with 30 runs instead of 10:)
     buffers/buffer-creation.js n=1024 len=4096 type='slow-allocUnsafe'                 2.06 %       ±2.39%   ±3.19%   ±4.15%

The performance gains effect are undone once native code accesses
the underlying ArrayBuffer, but then again that a) does not happen for
all TypedArrays, and b) it should also make sense to look into using
`ArrayBufferView::CopyContents()` in some places, which is made
specifically to avoid such a performance impact and allows us to
use the benefits of heap-allocated typed arrays.

Refs: 16f86d6
Refs: #2893
Refs: 74178a5#commitcomment-13250880
Refs: http://logs.libuv.org/node-dev/2015-09-15

PR-URL: #26301
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Mar 1, 2019
This was added in 16f86d6, based on
the assumption that otherwise, the memory behind `ArrayBuffer`
instances could be moved around on the heap while native code
holds references to it.

This does not match what V8 actually does (and also did at the time):

- The option/build variable was about always only about TypedArrays,
  not ArrayBuffers. Calls like `new ArrayBuffer(4)` call into C++
  regardless of the option value, but calls like `new Uint8Array(4)`
  would not call into C++ under V8 defaults.
- When first accessing a heap-allocated TypedArray’s `ArrayBuffer`,
  whether that is through the JS `.buffer` getter or the C++
  `ArrayBufferView::Buffer()` function, a copy of the contents is
  created using the ArrayBuffer allocator and stored as the
  (permanent, unmovable) backing store.

As a consequence, the memory returned by `ArrayBuffer::GetContents()`
is not moved around, because it is fixed once the `ArrayBuffer`
object itself first comes into explicit existence in any way.

Removing this build option significantly speeds up creation of typed
arrays from JS:

    $ ./node benchmark/compare.js --new ./node --old ./node-master --runs 10 --filter buffer-creation.js buffers | Rscript benchmark/compare.R
                                                                         confidence improvement accuracy (*)     (**)    (***)
     buffers/buffer-creation.js n=1024 len=10 type='buffer()'                  ***    593.66 %      ±28.64%  ±41.10%  ±60.36%
     buffers/buffer-creation.js n=1024 len=10 type='fast-alloc-fill'           ***    675.42 %      ±90.67% ±130.24% ±191.54%
     buffers/buffer-creation.js n=1024 len=10 type='fast-alloc'                ***    663.55 %      ±58.41%  ±83.87% ±123.29%
     buffers/buffer-creation.js n=1024 len=10 type='fast-allocUnsafe'                   3.10 %       ±9.63%  ±13.22%  ±18.07%
     buffers/buffer-creation.js n=1024 len=10 type='slow-allocUnsafe'                   4.67 %       ±5.55%   ±7.77%  ±10.97%
     buffers/buffer-creation.js n=1024 len=10 type='slow'                              -2.48 %       ±4.47%   ±6.12%   ±8.34%
     buffers/buffer-creation.js n=1024 len=1024 type='buffer()'                        -1.91 %       ±4.71%   ±6.45%   ±8.79%
     buffers/buffer-creation.js n=1024 len=1024 type='fast-alloc-fill'                 -1.34 %       ±7.53%  ±10.33%  ±14.10%
     buffers/buffer-creation.js n=1024 len=1024 type='fast-alloc'                       0.52 %       ±5.00%   ±6.87%   ±9.40%
     buffers/buffer-creation.js n=1024 len=1024 type='fast-allocUnsafe'                 0.39 %       ±5.65%   ±7.78%  ±10.67%
     buffers/buffer-creation.js n=1024 len=1024 type='slow-allocUnsafe'                -0.13 %       ±5.68%   ±7.83%  ±10.77%
     buffers/buffer-creation.js n=1024 len=1024 type='slow'                            -5.07 %       ±7.15%   ±9.80%  ±13.35%
     buffers/buffer-creation.js n=1024 len=2048 type='buffer()'                         0.57 %       ±2.70%   ±3.74%   ±5.16%
     buffers/buffer-creation.js n=1024 len=2048 type='fast-alloc-fill'                 -1.60 %       ±4.96%   ±6.79%   ±9.25%
     buffers/buffer-creation.js n=1024 len=2048 type='fast-alloc'                       1.29 %       ±3.79%   ±5.20%   ±7.09%
     buffers/buffer-creation.js n=1024 len=2048 type='fast-allocUnsafe'                 2.73 %       ±8.79%  ±12.05%  ±16.41%
     buffers/buffer-creation.js n=1024 len=2048 type='slow-allocUnsafe'                -0.99 %       ±6.27%   ±8.65%  ±11.91%
     buffers/buffer-creation.js n=1024 len=2048 type='slow'                            -5.98 %       ±6.24%   ±8.71%  ±12.20%
     buffers/buffer-creation.js n=1024 len=4096 type='buffer()'                        -1.75 %       ±3.48%   ±4.78%   ±6.56%
     buffers/buffer-creation.js n=1024 len=4096 type='fast-alloc-fill'                 -3.18 %       ±3.97%   ±5.45%   ±7.45%
     buffers/buffer-creation.js n=1024 len=4096 type='fast-alloc'                       2.05 %       ±4.05%   ±5.58%   ±7.65%
     buffers/buffer-creation.js n=1024 len=4096 type='fast-allocUnsafe'                 1.44 %       ±5.51%   ±7.63%  ±10.57%
     buffers/buffer-creation.js n=1024 len=4096 type='slow-allocUnsafe'          *     -4.77 %       ±4.30%   ±5.90%   ±8.06%
     buffers/buffer-creation.js n=1024 len=4096 type='slow'                            -3.31 %       ±6.38%   ±8.86%  ±12.34%
     buffers/buffer-creation.js n=1024 len=8192 type='buffer()'                         0.06 %       ±2.70%   ±3.77%   ±5.31%
     buffers/buffer-creation.js n=1024 len=8192 type='fast-alloc-fill'                 -1.20 %       ±3.30%   ±4.53%   ±6.17%
     buffers/buffer-creation.js n=1024 len=8192 type='fast-alloc'                      -1.46 %       ±2.75%   ±3.84%   ±5.38%
     buffers/buffer-creation.js n=1024 len=8192 type='fast-allocUnsafe'                 1.27 %       ±4.69%   ±6.49%   ±8.98%
     buffers/buffer-creation.js n=1024 len=8192 type='slow-allocUnsafe'                -1.68 %       ±3.30%   ±4.62%   ±6.49%
     buffers/buffer-creation.js n=1024 len=8192 type='slow'                            -2.49 %       ±3.24%   ±4.44%   ±6.07%
     (Re-running the outlier with 30 runs instead of 10:)
     buffers/buffer-creation.js n=1024 len=4096 type='slow-allocUnsafe'                 2.06 %       ±2.39%   ±3.19%   ±4.15%

The performance gains effect are undone once native code accesses
the underlying ArrayBuffer, but then again that a) does not happen for
all TypedArrays, and b) it should also make sense to look into using
`ArrayBufferView::CopyContents()` in some places, which is made
specifically to avoid such a performance impact and allows us to
use the benefits of heap-allocated typed arrays.

Refs: 16f86d6
Refs: #2893
Refs: 74178a5#commitcomment-13250880
Refs: http://logs.libuv.org/node-dev/2015-09-15

PR-URL: #26301
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants