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

v8::ArrayBuffer::New() without a BackingStore is deprecated in V8 8.0 #30529

Closed
targos opened this issue Nov 18, 2019 · 6 comments
Closed

v8::ArrayBuffer::New() without a BackingStore is deprecated in V8 8.0 #30529

targos opened this issue Nov 18, 2019 · 6 comments
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency.

Comments

@targos
Copy link
Member

targos commented Nov 18, 2019

The new API is present in V8 7.9, so we can already migrate to it:

node/deps/v8/include/v8.h

Lines 5007 to 5043 in b551c65

/**
* Create a new ArrayBuffer with an existing backing store.
* The created array keeps a reference to the backing store until the array
* is garbage collected. Note that the IsExternal bit does not affect this
* reference from the array to the backing store.
*
* In future IsExternal bit will be removed. Until then the bit is set as
* follows. If the backing store does not own the underlying buffer, then
* the array is created in externalized state. Otherwise, the array is created
* in internalized state. In the latter case the array can be transitioned
* to the externalized state using Externalize(backing_store).
*/
static Local<ArrayBuffer> New(Isolate* isolate,
std::shared_ptr<BackingStore> backing_store);
/**
* Returns a new standalone BackingStore that is allocated using the array
* buffer allocator of the isolate. The result can be later passed to
* ArrayBuffer::New.
*
* If the allocator returns nullptr, then the function may cause GCs in the
* given isolate and re-try the allocation. If GCs do not help, then the
* function will crash with an out-of-memory error.
*/
static std::unique_ptr<BackingStore> NewBackingStore(Isolate* isolate,
size_t byte_length);
/**
* Returns a new standalone BackingStore that takes over the ownership of
* the given buffer. The destructor of the BackingStore invokes the given
* deleter callback.
*
* The result can be later passed to ArrayBuffer::New. The raw pointer
* to the buffer must not be passed again to any V8 API function.
*/
static std::unique_ptr<BackingStore> NewBackingStore(
void* data, size_t byte_length, BackingStoreDeleterCallback deleter,
void* deleter_data);

There is an upstream issue to gather feedback: v8:9908.

@targos targos added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. v8 engine Issues and PRs related to the V8 dependency. labels Nov 18, 2019
@addaleax addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. mentor-available buffer Issues and PRs related to the buffer subsystem. labels Nov 18, 2019
@thangktran
Copy link
Contributor

I would like to work on this issue.

@addaleax
Copy link
Member

@thangktran Cool! This is quite C++-heavy, so it helps if you’re familiar with that. I would recommend starting by looking at ArrayBuffer::New() calls in our C++ code in src/, picking one that uses ArrayBufferCreationMode::kInternalized (that’s the easier case), and then trying to convert it to the new ArrayBuffer::NewBackingStore()-based API.

If you have any questions or need help getting started, feel free to comment here, or ask the #node-dev channel on Freenode IRC (or to ping me there).

@thangktran
Copy link
Contributor

@addaleax Thank you for your suggestion.

@thangktran
Copy link
Contributor

thangktran commented Nov 25, 2019

EDIT: I figured it out. I'll test it and keep you update.

@thangktran
Copy link
Contributor

There appears to be a bug in v8.
I've reported to them https://bugs.chromium.org/p/v8/issues/detail?id=9908

@addaleax
Copy link
Member

@thangktran Yeah, thanks for reporting that 👍

@addaleax addaleax removed help wanted Issues that need assistance from volunteers or PRs that need help to proceed. mentor-available labels Dec 4, 2019
thangktran added a commit to thangktran/node that referenced this issue Dec 9, 2019
ArrayBuffer without BackingStore will soon be deprecated.

Fixes:nodejs#30529
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++. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

3 participants