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

src: allocate Buffer memory using ArrayBuffer allocator #26207

Closed
wants to merge 8 commits into from

Conversation

addaleax
Copy link
Member

This PR allows using an ArrayBuffer allocator that was not provided by Node.js itself, and may not be malloc()-compatible.
The target use case are embedders like Electron, which is currently floating patches on top of Node and V8 in order to achieve this (fyi @codebytere).


[The first commit is #26201 in order to avoid conflicts]

deps: V8: cherry-pick d3308d0

Original commit message:

[api] Add Isolate::GetArrayBufferAllocator()

This allows non-monolithic embedders to always allocate memory
for ArrayBuffer instances using the right allocation method.

This is based on a patch that Electron is currently using.

Refs: https://github.com/electron/electron/blob/1898f9162073910c05958295c612deec6121a892/patches/common/v8/array_buffer.patch

Refs: v8/v8@d3308d0

src: make IsolateData store ArrayBufferAllocator

This enables us to identify whether we are using an
allocator that we know more about than what the generic
ArrayBuffer::Allocator API provides, in particular
whether it is malloc()-compatible.

worker: copy transferList ArrayBuffers on unknown allocator

If the ArrayBuffer::Allocator used to create ArrayBuffers
in the current Isolate is not a Node.js ArrayBufferAllocator,
we cannot know that it is malloc()-based, an in particular it might
not be compatible with the ArrayBuffer::Allocator on the receiving
end of the connection.

src: add debugging array allocator

Add a subclass of ArrayBufferAllocator that performs additional
debug checking, which in particular verifies that:

  • All ArrayBuffer backing stores have been allocated with this
    allocator, or have been explicitly marked as coming from a
    compatible source.
  • All memory allocated by the allocator has been freed once it is
    destroyed.
src: add allocation utils to env

Add a RAII utility for managing blocks of memory that have
been allocated with the ArrayBuffer::Allocator for a given
Isolate.

src: allocate Buffer memory using ArrayBuffer allocator

Always use the right allocator for memory that is turned into
an ArrayBuffer at a later point.

This enables embedders to use their own ArrayBuffer::Allocators,
and is inspired by Electron’s electron/node@f61bae3440e. It should
render their downstream patch unnecessary.

Refs: electron/node@f61bae3

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

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 19, 2019
@addaleax addaleax added v8 engine Issues and PRs related to the V8 dependency. embedding Issues and PRs related to embedding Node.js in another project. labels Feb 19, 2019
@addaleax
Copy link
Member Author

addaleax commented Feb 19, 2019

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM with a nit

@@ -470,6 +476,36 @@ enum class DebugCategory {
CATEGORY_COUNT
};

// A unique-pointer-ish object that is compatible with the JS engine's
// ArrayBuffer::Allocator.
struct AllocatedBuffer {
Copy link
Member

Choose a reason for hiding this comment

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

AllocatedLibuvBuffer? Or instead of storing a uv_buf_t as member, store the fields that can be used to wrap/unwrap uv_buf_t?

Copy link
Member Author

Choose a reason for hiding this comment

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

AllocatedLibuvBuffer?

So, I’m not super happy with the name either. I wouldn’t want to include a reference to libuv, because uv_but_t was just chosen here as a nicer variant of std::pair<char*,size_t> that happens to work nicely with some pieces in our code base that already use it the same way.

Ideally, the name would be something that refers to the fact that this memory is usable for ArrayBuffers, but that would make any name even longer … ArrayBufferBuffer sounds weird, ArrayBufferStorage or ArrayBufferMemory is long but maybe okay, …?

Or instead of storing a uv_buf_t as member, store the fields that can be used to wrap/unwrap uv_buf_t?

I don’t really have strong feelings about that… I can make that change after/while rebasing (later today, after #26201 lands).

Copy link
Member

Choose a reason for hiding this comment

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

because uv_but_t was just chosen here as a nicer variant of std::pair<char*,size_t> that happens to work nicely with some pieces in our code base that already use it the same way.

That was what I guessed as well, thanks for the explanation. I don't have strong feelings about using uv_buf_t underneath, but it would be nice to mention why it is chosen in the comments as one may think that AllocatedBuffer depends on libuv for a more sophisticated reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

@joyeecheung Does 1e17828 sound good to you?

(& to be clear, I’m still happy about suggestions for another name for this class if that fits better.)

@joyeecheung
Copy link
Member

BTW: if we are refactoring the allocation methods used across the code base, is it possible to create allocation methods that take an Isolate* to do the optional LowMemoryNotification(), instead of doing Isolate::GetCurrent() for that in UncheckedRealloc?

@addaleax
Copy link
Member Author

BTW: if we are refactoring the allocation methods used across the code base, is it possible to create allocation methods that take an Isolate* to do the optional LowMemoryNotification(), instead of doing Isolate::GetCurrent() for that in UncheckedRealloc?

I think so, but this PR doesn’t change a lot of other code that still uses our Malloc() family or MallocedBuffer, which doesn’t later get converted to ArrayBuffers… I guess we could go on in this direction, though, and do as much with env->AllocateManaged() (side note: also not an ideal name) as we can?

Original commit message:

    [api] Add `Isolate::GetArrayBufferAllocator()`

    This allows non-monolithic embedders to always allocate memory
    for ArrayBuffer instances using the right allocation method.

    This is based on a patch that Electron is currently using.

    Refs: https://github.com/electron/electron/blob/1898f9162073910c05958295c612deec6121a892/patches/common/v8/array_buffer.patch
    Change-Id: I39a614343118a0594aab48699a99cc2aad5b7ba9
    Reviewed-on: https://chromium-review.googlesource.com/c/1462003
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#59697}

Refs: v8/v8@d3308d0
This enables us to identify whether we are using an
allocator that we know more about than what the generic
`ArrayBuffer::Allocator` API provides, in particular
whether it is `malloc()`-compatible.
If the `ArrayBuffer::Allocator` used to create `ArrayBuffer`s
in the current `Isolate` is not a Node.js `ArrayBufferAllocator`,
we cannot know that it is `malloc()`-based, an in particular it might
not be compatible with the `ArrayBuffer::Allocator` on the receiving
end of the connection.
Add a subclass of `ArrayBufferAllocator` that performs additional
debug checking, which in particular verifies that:

- All `ArrayBuffer` backing stores have been allocated with this
  allocator, or have been explicitly marked as coming from a
  compatible source.
- All memory allocated by the allocator has been freed once it is
  destroyed.
Add a RAII utility for managing blocks of memory that have
been allocated with the `ArrayBuffer::Allocator` for a given
`Isolate`.
Always use the right allocator for memory that is turned into
an `ArrayBuffer` at a later point.

This enables embedders to use their own `ArrayBuffer::Allocator`s,
and is inspired by Electron’s electron/node@f61bae3440e. It should
render their downstream patch unnecessary.

Refs: electron/node@f61bae3
@addaleax
Copy link
Member Author

addaleax commented Feb 21, 2019

@addaleax
Copy link
Member Author

Landed in b0869c6...84e02b1, thanks for the reviews!

@addaleax addaleax closed this Feb 25, 2019
@addaleax addaleax deleted the arraybufferallocator branch February 25, 2019 01:04
addaleax added a commit that referenced this pull request Feb 25, 2019
Original commit message:

    [api] Add `Isolate::GetArrayBufferAllocator()`

    This allows non-monolithic embedders to always allocate memory
    for ArrayBuffer instances using the right allocation method.

    This is based on a patch that Electron is currently using.

    Refs: https://github.com/electron/electron/blob/1898f9162073910c05958295c612deec6121a892/patches/common/v8/array_buffer.patch
    Change-Id: I39a614343118a0594aab48699a99cc2aad5b7ba9
    Reviewed-on: https://chromium-review.googlesource.com/c/1462003
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#59697}

Refs: v8/v8@d3308d0

PR-URL: #26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax added a commit that referenced this pull request Feb 25, 2019
This enables us to identify whether we are using an
allocator that we know more about than what the generic
`ArrayBuffer::Allocator` API provides, in particular
whether it is `malloc()`-compatible.

PR-URL: #26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax added a commit that referenced this pull request Feb 25, 2019
If the `ArrayBuffer::Allocator` used to create `ArrayBuffer`s
in the current `Isolate` is not a Node.js `ArrayBufferAllocator`,
we cannot know that it is `malloc()`-based, an in particular it might
not be compatible with the `ArrayBuffer::Allocator` on the receiving
end of the connection.

PR-URL: #26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax added a commit that referenced this pull request Feb 25, 2019
Add a subclass of `ArrayBufferAllocator` that performs additional
debug checking, which in particular verifies that:

- All `ArrayBuffer` backing stores have been allocated with this
  allocator, or have been explicitly marked as coming from a
  compatible source.
- All memory allocated by the allocator has been freed once it is
  destroyed.

PR-URL: #26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax added a commit that referenced this pull request Feb 25, 2019
Add a RAII utility for managing blocks of memory that have
been allocated with the `ArrayBuffer::Allocator` for a given
`Isolate`.

PR-URL: #26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax added a commit that referenced this pull request Feb 25, 2019
Always use the right allocator for memory that is turned into
an `ArrayBuffer` at a later point.

This enables embedders to use their own `ArrayBuffer::Allocator`s,
and is inspired by Electron’s electron/node@f61bae3440e. It should
render their downstream patch unnecessary.

Refs: electron/node@f61bae3

PR-URL: #26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Feb 28, 2019
Original commit message:

    [api] Add `Isolate::GetArrayBufferAllocator()`

    This allows non-monolithic embedders to always allocate memory
    for ArrayBuffer instances using the right allocation method.

    This is based on a patch that Electron is currently using.

    Refs: https://github.com/electron/electron/blob/1898f9162073910c05958295c612deec6121a892/patches/common/v8/array_buffer.patch
    Change-Id: I39a614343118a0594aab48699a99cc2aad5b7ba9
    Reviewed-on: https://chromium-review.googlesource.com/c/1462003
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#59697}

Refs: v8/v8@d3308d0

PR-URL: nodejs#26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Feb 28, 2019
This enables us to identify whether we are using an
allocator that we know more about than what the generic
`ArrayBuffer::Allocator` API provides, in particular
whether it is `malloc()`-compatible.

PR-URL: nodejs#26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Feb 28, 2019
If the `ArrayBuffer::Allocator` used to create `ArrayBuffer`s
in the current `Isolate` is not a Node.js `ArrayBufferAllocator`,
we cannot know that it is `malloc()`-based, an in particular it might
not be compatible with the `ArrayBuffer::Allocator` on the receiving
end of the connection.

PR-URL: nodejs#26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Feb 28, 2019
Add a subclass of `ArrayBufferAllocator` that performs additional
debug checking, which in particular verifies that:

- All `ArrayBuffer` backing stores have been allocated with this
  allocator, or have been explicitly marked as coming from a
  compatible source.
- All memory allocated by the allocator has been freed once it is
  destroyed.

PR-URL: nodejs#26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Feb 28, 2019
Add a RAII utility for managing blocks of memory that have
been allocated with the `ArrayBuffer::Allocator` for a given
`Isolate`.

PR-URL: nodejs#26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Feb 28, 2019
Always use the right allocator for memory that is turned into
an `ArrayBuffer` at a later point.

This enables embedders to use their own `ArrayBuffer::Allocator`s,
and is inspired by Electron’s electron/node@f61bae3440e. It should
render their downstream patch unnecessary.

Refs: electron/node@f61bae3

PR-URL: nodejs#26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax added a commit that referenced this pull request Mar 1, 2019
Original commit message:

    [api] Add `Isolate::GetArrayBufferAllocator()`

    This allows non-monolithic embedders to always allocate memory
    for ArrayBuffer instances using the right allocation method.

    This is based on a patch that Electron is currently using.

    Refs: https://github.com/electron/electron/blob/1898f9162073910c05958295c612deec6121a892/patches/common/v8/array_buffer.patch
    Change-Id: I39a614343118a0594aab48699a99cc2aad5b7ba9
    Reviewed-on: https://chromium-review.googlesource.com/c/1462003
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#59697}

Refs: v8/v8@d3308d0

PR-URL: #26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Backport-PR-URL: #26302
Reviewed-By: Michaël Zasso <targos@protonmail.com>
addaleax added a commit that referenced this pull request Mar 1, 2019
This enables us to identify whether we are using an
allocator that we know more about than what the generic
`ArrayBuffer::Allocator` API provides, in particular
whether it is `malloc()`-compatible.

PR-URL: #26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Backport-PR-URL: #26302
Reviewed-By: Michaël Zasso <targos@protonmail.com>
addaleax added a commit that referenced this pull request Mar 1, 2019
If the `ArrayBuffer::Allocator` used to create `ArrayBuffer`s
in the current `Isolate` is not a Node.js `ArrayBufferAllocator`,
we cannot know that it is `malloc()`-based, an in particular it might
not be compatible with the `ArrayBuffer::Allocator` on the receiving
end of the connection.

PR-URL: #26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Backport-PR-URL: #26302
Reviewed-By: Michaël Zasso <targos@protonmail.com>
addaleax added a commit that referenced this pull request Mar 1, 2019
Add a subclass of `ArrayBufferAllocator` that performs additional
debug checking, which in particular verifies that:

- All `ArrayBuffer` backing stores have been allocated with this
  allocator, or have been explicitly marked as coming from a
  compatible source.
- All memory allocated by the allocator has been freed once it is
  destroyed.

PR-URL: #26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Backport-PR-URL: #26302
Reviewed-By: Michaël Zasso <targos@protonmail.com>
addaleax added a commit that referenced this pull request Mar 1, 2019
Add a RAII utility for managing blocks of memory that have
been allocated with the `ArrayBuffer::Allocator` for a given
`Isolate`.

PR-URL: #26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Backport-PR-URL: #26302
Reviewed-By: Michaël Zasso <targos@protonmail.com>
addaleax added a commit that referenced this pull request Mar 1, 2019
Always use the right allocator for memory that is turned into
an `ArrayBuffer` at a later point.

This enables embedders to use their own `ArrayBuffer::Allocator`s,
and is inspired by Electron’s electron/node@f61bae3440e. It should
render their downstream patch unnecessary.

Refs: electron/node@f61bae3

PR-URL: #26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Backport-PR-URL: #26302
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@BridgeAR BridgeAR mentioned this pull request Mar 4, 2019
addaleax added a commit to addaleax/node that referenced this pull request May 26, 2019
Track received data correctly. Specifically, for the buffer that
is used for receiving data, we previously would try to increment
the current memory usage by its length, and later decrement it
by that, but in the meantime the buffer had been turned over to V8
and its length reset to zero. This gave the impression that more and
more memory was consumed by the HTTP/2 session when it was in fact not.

Fixes: nodejs#27416
Refs: nodejs#26207
addaleax added a commit that referenced this pull request May 29, 2019
Track received data correctly. Specifically, for the buffer that
is used for receiving data, we previously would try to increment
the current memory usage by its length, and later decrement it
by that, but in the meantime the buffer had been turned over to V8
and its length reset to zero. This gave the impression that more and
more memory was consumed by the HTTP/2 session when it was in fact not.

Fixes: #27416
Refs: #26207

PR-URL: #27914
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request May 31, 2019
Track received data correctly. Specifically, for the buffer that
is used for receiving data, we previously would try to increment
the current memory usage by its length, and later decrement it
by that, but in the meantime the buffer had been turned over to V8
and its length reset to zero. This gave the impression that more and
more memory was consumed by the HTTP/2 session when it was in fact not.

Fixes: #27416
Refs: #26207

PR-URL: #27914
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MarshallOfSound pushed a commit to electron/node that referenced this pull request Jun 18, 2019
Track received data correctly. Specifically, for the buffer that
is used for receiving data, we previously would try to increment
the current memory usage by its length, and later decrement it
by that, but in the meantime the buffer had been turned over to V8
and its length reset to zero. This gave the impression that more and
more memory was consumed by the HTTP/2 session when it was in fact not.

Fixes: nodejs/node#27416
Refs: nodejs/node#26207

PR-URL: nodejs/node#27914
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
deepak1556 pushed a commit to electron/node that referenced this pull request Jul 10, 2019
Always use the right allocator for memory that is turned into
an `ArrayBuffer` at a later point.

This enables embedders to use their own `ArrayBuffer::Allocator`s,
and is inspired by Electron’s f61bae3440e. It should
render their downstream patch unnecessary.

Refs: f61bae3

PR-URL: nodejs/node#26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
deepak1556 pushed a commit to electron/node that referenced this pull request Jul 10, 2019
This enables us to identify whether we are using an
allocator that we know more about than what the generic
`ArrayBuffer::Allocator` API provides, in particular
whether it is `malloc()`-compatible.

PR-URL: nodejs/node#26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
deepak1556 pushed a commit to electron/node that referenced this pull request Jul 10, 2019
If the `ArrayBuffer::Allocator` used to create `ArrayBuffer`s
in the current `Isolate` is not a Node.js `ArrayBufferAllocator`,
we cannot know that it is `malloc()`-based, an in particular it might
not be compatible with the `ArrayBuffer::Allocator` on the receiving
end of the connection.

PR-URL: nodejs/node#26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
deepak1556 pushed a commit to electron/node that referenced this pull request Jul 10, 2019
Add a subclass of `ArrayBufferAllocator` that performs additional
debug checking, which in particular verifies that:

- All `ArrayBuffer` backing stores have been allocated with this
  allocator, or have been explicitly marked as coming from a
  compatible source.
- All memory allocated by the allocator has been freed once it is
  destroyed.

PR-URL: nodejs/node#26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
deepak1556 pushed a commit to electron/node that referenced this pull request Jul 10, 2019
Add a RAII utility for managing blocks of memory that have
been allocated with the `ArrayBuffer::Allocator` for a given
`Isolate`.

PR-URL: nodejs/node#26207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
deepak1556 pushed a commit to electron/node that referenced this pull request Jul 12, 2019
Track received data correctly. Specifically, for the buffer that
is used for receiving data, we previously would try to increment
the current memory usage by its length, and later decrement it
by that, but in the meantime the buffer had been turned over to V8
and its length reset to zero. This gave the impression that more and
more memory was consumed by the HTTP/2 session when it was in fact not.

Fixes: nodejs/node#27416
Refs: nodejs/node#26207

PR-URL: nodejs/node#27914
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
embedding Issues and PRs related to embedding Node.js in another project. lib / src Issues and PRs related to general changes in the lib or src directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants