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.

Sign up for GitHub

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

Jump to bottom

[v11.x] src: allocate Buffer memory using ArrayBuffer allocator #26302

Closed
wants to merge 6 commits into from
Closed

[v11.x] src: allocate Buffer memory using ArrayBuffer allocator #26302

wants to merge 6 commits into from

Conversation

addaleax
Copy link
Member

Backport of #26207. Conflicts were only in the last commit in node_buffer.cc, the rest applied cleanly.

@nodejs-github-bot
Copy link
Collaborator

@addaleax sadly an error occured when I tried to trigger a build :(

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v11.x labels Feb 25, 2019
@addaleax
deps: V8: cherry-pick d3308d0
ce5da3f 
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
src: make IsolateData store ArrayBufferAllocator
cf75949 
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
worker: copy transferList ArrayBuffers on unknown allocator
29ac9ef 
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
src: add debugging array allocator
86fdd46 
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
src: add allocation utils to env
9627be8 
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
src: allocate Buffer memory using ArrayBuffer allocator
3a08a7a 
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
Copy link
Member Author

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

addaleax added a commit that referenced this pull request Mar 1, 2019
@addaleax
deps: V8: cherry-pick d3308d0
7f08e02 
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
@addaleax
src: make IsolateData store ArrayBufferAllocator
437bb25 
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
@addaleax
worker: copy transferList ArrayBuffers on unknown allocator
20dc172 
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
@addaleax
src: add debugging array allocator
238fa57 
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
@addaleax
src: add allocation utils to env
2826076 
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
@addaleax
src: allocate Buffer memory using ArrayBuffer allocator
3c11b4e 
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>
@addaleax
Copy link
Member Author

addaleax commented Mar 1, 2019

Landed in 68accb5...3c11b4e

@addaleax addaleax closed this Mar 1, 2019
@addaleax addaleax deleted the backport-26207 branch March 1, 2019 09:25
@targos targos added this to Closed PRs in v11.x Mar 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
No open projects
v11.x
  
Closed PRs
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@addaleax @nodejs-github-bot @targos