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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions deps/v8/include/v8.h
Original file line number Diff line number Diff line change
Expand Up @@ -7642,6 +7642,9 @@ class V8_EXPORT Isolate {
*/
void SetIdle(bool is_idle);

/** Returns the ArrayBuffer::Allocator used in this isolate. */
ArrayBuffer::Allocator* GetArrayBufferAllocator();

/** Returns true if this isolate has a current context. */
bool InContext();

Expand Down
5 changes: 5 additions & 0 deletions deps/v8/src/api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8007,6 +8007,11 @@ void Isolate::SetIdle(bool is_idle) {
isolate->SetIdle(is_idle);
}

ArrayBuffer::Allocator* Isolate::GetArrayBufferAllocator() {
i::Isolate* isolate = reinterpret_cast<i::Isolate*>(this);
return isolate->array_buffer_allocator();
}

bool Isolate::InContext() {
i::Isolate* isolate = reinterpret_cast<i::Isolate*>(this);
return isolate->context() != nullptr;
Expand Down
1 change: 1 addition & 0 deletions deps/v8/test/cctest/test-api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20881,6 +20881,7 @@ TEST(IsolateNewDispose) {
CHECK_NOT_NULL(isolate);
CHECK(current_isolate != isolate);
CHECK(current_isolate == CcTest::isolate());
CHECK(isolate->GetArrayBufferAllocator() == CcTest::array_buffer_allocator());

isolate->SetFatalErrorHandler(StoringErrorCallback);
last_location = last_message = nullptr;
Expand Down
82 changes: 76 additions & 6 deletions src/api/environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,82 @@ void* ArrayBufferAllocator::Allocate(size_t size) {
return UncheckedMalloc(size);
}

DebuggingArrayBufferAllocator::~DebuggingArrayBufferAllocator() {
CHECK(allocations_.empty());
}

void* DebuggingArrayBufferAllocator::Allocate(size_t size) {
Mutex::ScopedLock lock(mutex_);
void* data = ArrayBufferAllocator::Allocate(size);
RegisterPointerInternal(data, size);
return data;
}

void* DebuggingArrayBufferAllocator::AllocateUninitialized(size_t size) {
Mutex::ScopedLock lock(mutex_);
void* data = ArrayBufferAllocator::AllocateUninitialized(size);
RegisterPointerInternal(data, size);
return data;
}

void DebuggingArrayBufferAllocator::Free(void* data, size_t size) {
Mutex::ScopedLock lock(mutex_);
UnregisterPointerInternal(data, size);
ArrayBufferAllocator::Free(data, size);
}

void* DebuggingArrayBufferAllocator::Reallocate(void* data,
size_t old_size,
size_t size) {
Mutex::ScopedLock lock(mutex_);
void* ret = ArrayBufferAllocator::Reallocate(data, old_size, size);
if (ret == nullptr) {
if (size == 0) // i.e. equivalent to free().
UnregisterPointerInternal(data, old_size);
return nullptr;
}

if (data != nullptr) {
auto it = allocations_.find(data);
CHECK_NE(it, allocations_.end());
allocations_.erase(it);
}

RegisterPointerInternal(ret, size);
return ret;
}

void DebuggingArrayBufferAllocator::RegisterPointer(void* data, size_t size) {
Mutex::ScopedLock lock(mutex_);
RegisterPointerInternal(data, size);
}

void DebuggingArrayBufferAllocator::UnregisterPointer(void* data, size_t size) {
Mutex::ScopedLock lock(mutex_);
UnregisterPointerInternal(data, size);
}

void DebuggingArrayBufferAllocator::UnregisterPointerInternal(void* data,
size_t size) {
if (data == nullptr) return;
auto it = allocations_.find(data);
CHECK_NE(it, allocations_.end());
CHECK_EQ(it->second, size);
allocations_.erase(it);
}

void DebuggingArrayBufferAllocator::RegisterPointerInternal(void* data,
size_t size) {
if (data == nullptr) return;
CHECK_EQ(allocations_.count(data), 0);
allocations_[data] = size;
}

ArrayBufferAllocator* CreateArrayBufferAllocator() {
return new ArrayBufferAllocator();
if (per_process::cli_options->debug_arraybuffer_allocations)
return new DebuggingArrayBufferAllocator();
else
return new ArrayBufferAllocator();
}

void FreeArrayBufferAllocator(ArrayBufferAllocator* allocator) {
Expand Down Expand Up @@ -124,11 +198,7 @@ IsolateData* CreateIsolateData(Isolate* isolate,
uv_loop_t* loop,
MultiIsolatePlatform* platform,
ArrayBufferAllocator* allocator) {
return new IsolateData(
isolate,
loop,
platform,
allocator != nullptr ? allocator->zero_fill_field() : nullptr);
return new IsolateData(isolate, loop, platform, allocator);
}

void FreeIsolateData(IsolateData* isolate_data) {
Expand Down
110 changes: 108 additions & 2 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,16 @@ inline uv_loop_t* IsolateData::event_loop() const {
return event_loop_;
}

inline uint32_t* IsolateData::zero_fill_field() const {
return zero_fill_field_;
inline bool IsolateData::uses_node_allocator() const {
return uses_node_allocator_;
}

inline v8::ArrayBuffer::Allocator* IsolateData::allocator() const {
return allocator_;
}

inline ArrayBufferAllocator* IsolateData::node_allocator() const {
return node_allocator_;
}

inline MultiIsolatePlatform* IsolateData::platform() const {
Expand Down Expand Up @@ -707,6 +715,104 @@ inline IsolateData* Environment::isolate_data() const {
return isolate_data_;
}

inline char* Environment::AllocateUnchecked(size_t size) {
return static_cast<char*>(
isolate_data()->allocator()->AllocateUninitialized(size));
}

inline char* Environment::Allocate(size_t size) {
char* ret = AllocateUnchecked(size);
CHECK_NE(ret, nullptr);
return ret;
}

inline void Environment::Free(char* data, size_t size) {
if (data != nullptr)
isolate_data()->allocator()->Free(data, size);
}

inline AllocatedBuffer Environment::AllocateManaged(size_t size, bool checked) {
char* data = checked ? Allocate(size) : AllocateUnchecked(size);
if (data == nullptr) size = 0;
return AllocatedBuffer(this, uv_buf_init(data, size));
}

inline AllocatedBuffer::AllocatedBuffer(Environment* env, uv_buf_t buf)
: env_(env), buffer_(buf) {}

inline void AllocatedBuffer::Resize(size_t len) {
char* new_data = env_->Reallocate(buffer_.base, buffer_.len, len);
CHECK_IMPLIES(len > 0, new_data != nullptr);
buffer_ = uv_buf_init(new_data, len);
}

inline uv_buf_t AllocatedBuffer::release() {
uv_buf_t ret = buffer_;
buffer_ = uv_buf_init(nullptr, 0);
return ret;
}

inline char* AllocatedBuffer::data() {
return buffer_.base;
}

inline const char* AllocatedBuffer::data() const {
return buffer_.base;
}

inline size_t AllocatedBuffer::size() const {
return buffer_.len;
}

inline AllocatedBuffer::AllocatedBuffer(Environment* env)
: env_(env), buffer_(uv_buf_init(nullptr, 0)) {}

inline AllocatedBuffer::AllocatedBuffer(AllocatedBuffer&& other)
: AllocatedBuffer() {
*this = std::move(other);
}

inline AllocatedBuffer& AllocatedBuffer::operator=(AllocatedBuffer&& other) {
clear();
env_ = other.env_;
buffer_ = other.release();
return *this;
}

inline AllocatedBuffer::~AllocatedBuffer() {
clear();
}

inline void AllocatedBuffer::clear() {
uv_buf_t buf = release();
env_->Free(buf.base, buf.len);
}

// It's a bit awkward to define this Buffer::New() overload here, but it
// avoids a circular dependency with node_internals.h.
namespace Buffer {
v8::MaybeLocal<v8::Object> New(Environment* env,
char* data,
size_t length,
bool uses_malloc);
}

inline v8::MaybeLocal<v8::Object> AllocatedBuffer::ToBuffer() {
CHECK_NOT_NULL(env_);
v8::MaybeLocal<v8::Object> obj = Buffer::New(env_, data(), size(), false);
if (!obj.IsEmpty()) release();
return obj;
}

inline v8::Local<v8::ArrayBuffer> AllocatedBuffer::ToArrayBuffer() {
CHECK_NOT_NULL(env_);
uv_buf_t buf = release();
return v8::ArrayBuffer::New(env_->isolate(),
buf.base,
buf.len,
v8::ArrayBufferCreationMode::kInternalized);
}

inline void Environment::ThrowError(const char* errmsg) {
ThrowError(v8::Exception::Error, errmsg);
}
Expand Down
31 changes: 26 additions & 5 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
namespace node {

using errors::TryCatchScope;
using v8::ArrayBuffer;
using v8::Boolean;
using v8::Context;
using v8::EmbedderGraph;
Expand Down Expand Up @@ -74,11 +75,14 @@ void* const Environment::kNodeContextTagPtr = const_cast<void*>(
IsolateData::IsolateData(Isolate* isolate,
uv_loop_t* event_loop,
MultiIsolatePlatform* platform,
uint32_t* zero_fill_field) :
isolate_(isolate),
event_loop_(event_loop),
zero_fill_field_(zero_fill_field),
platform_(platform) {
ArrayBufferAllocator* node_allocator)
: isolate_(isolate),
event_loop_(event_loop),
allocator_(isolate->GetArrayBufferAllocator()),
node_allocator_(node_allocator),
uses_node_allocator_(allocator_ == node_allocator_),
platform_(platform) {
CHECK_NOT_NULL(allocator_);
if (platform_ != nullptr)
platform_->RegisterIsolate(isolate_, event_loop);

Expand Down Expand Up @@ -902,6 +906,23 @@ void Environment::BuildEmbedderGraph(Isolate* isolate,
});
}

char* Environment::Reallocate(char* data, size_t old_size, size_t size) {
// If we know that the allocator is our ArrayBufferAllocator, we can let
// if reallocate directly.
if (isolate_data()->uses_node_allocator()) {
return static_cast<char*>(
isolate_data()->node_allocator()->Reallocate(data, old_size, size));
}
// Generic allocators do not provide a reallocation method; we need to
// allocate a new chunk of memory and copy the data over.
char* new_data = AllocateUnchecked(size);
if (new_data == nullptr) return nullptr;
memcpy(new_data, data, std::min(size, old_size));
if (size > old_size)
memset(new_data + old_size, 0, size - old_size);
Free(data, old_size);
return new_data;
}

// Not really any better place than env.cc at this moment.
void BaseObject::DeleteMe(void* data) {
Expand Down
55 changes: 51 additions & 4 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -394,16 +394,20 @@ class Environment;

class IsolateData {
public:
IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop,
IsolateData(v8::Isolate* isolate,
uv_loop_t* event_loop,
MultiIsolatePlatform* platform = nullptr,
uint32_t* zero_fill_field = nullptr);
ArrayBufferAllocator* node_allocator = nullptr);
~IsolateData();
inline uv_loop_t* event_loop() const;
inline uint32_t* zero_fill_field() const;
inline MultiIsolatePlatform* platform() const;
inline std::shared_ptr<PerIsolateOptions> options();
inline void set_options(std::shared_ptr<PerIsolateOptions> options);

inline bool uses_node_allocator() const;
inline v8::ArrayBuffer::Allocator* allocator() const;
inline ArrayBufferAllocator* node_allocator() const;

#define VP(PropertyName, StringValue) V(v8::Private, PropertyName)
#define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName)
#define VS(PropertyName, StringValue) V(v8::String, PropertyName)
Expand Down Expand Up @@ -436,7 +440,9 @@ class IsolateData {

v8::Isolate* const isolate_;
uv_loop_t* const event_loop_;
uint32_t* const zero_fill_field_;
v8::ArrayBuffer::Allocator* const allocator_;
ArrayBufferAllocator* const node_allocator_;
const bool uses_node_allocator_;
MultiIsolatePlatform* platform_;
std::shared_ptr<PerIsolateOptions> options_;

Expand Down Expand Up @@ -470,6 +476,38 @@ 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.)

public:
explicit inline AllocatedBuffer(Environment* env = nullptr);
inline AllocatedBuffer(Environment* env, uv_buf_t buf);
inline ~AllocatedBuffer();
inline void Resize(size_t len);

inline uv_buf_t release();
inline char* data();
inline const char* data() const;
inline size_t size() const;
inline void clear();

inline v8::MaybeLocal<v8::Object> ToBuffer();
inline v8::Local<v8::ArrayBuffer> ToArrayBuffer();

inline AllocatedBuffer(AllocatedBuffer&& other);
inline AllocatedBuffer& operator=(AllocatedBuffer&& other);
AllocatedBuffer(const AllocatedBuffer& other) = delete;
AllocatedBuffer& operator=(const AllocatedBuffer& other) = delete;

private:
Environment* env_;
// We do not pass this to libuv directly, but uv_buf_t is a convenient way
// to represent a chunk of memory, and plays nicely with other parts of core.
uv_buf_t buffer_;

friend class Environment;
};

class Environment {
public:
class AsyncHooks {
Expand Down Expand Up @@ -691,6 +729,15 @@ class Environment {

inline IsolateData* isolate_data() const;

// Utilites that allocate memory using the Isolate's ArrayBuffer::Allocator.
// In particular, using AllocateManaged() will provide a RAII-style object
// with easy conversion to `Buffer` and `ArrayBuffer` objects.
inline AllocatedBuffer AllocateManaged(size_t size, bool checked = true);
inline char* Allocate(size_t size);
inline char* AllocateUnchecked(size_t size);
char* Reallocate(char* data, size_t old_size, size_t size);
inline void Free(char* data, size_t size);

inline bool printed_error() const;
inline void set_printed_error(bool value);

Expand Down
Loading