From c971497883dc4cc87f19aa0eee6cc5d77a78aa5f Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 14 May 2020 12:03:59 -0700 Subject: [PATCH] src: turn AllocatedBuffer into thin wrapper around v8::BackingStore Alternative to https://github.com/nodejs/node/pull/33381 that reimplements that change on top of moving AllocatedBuffer out of env.h --- src/allocated_buffer-inl.h | 128 ++++++++++++++++++------------------- src/allocated_buffer.h | 48 ++++++++------ src/env-inl.h | 26 +------- src/env.cc | 19 ------ src/env.h | 13 ++-- src/node_buffer.cc | 66 +++---------------- src/node_crypto.cc | 2 +- src/node_internals.h | 5 +- src/node_serdes.cc | 3 +- 9 files changed, 116 insertions(+), 194 deletions(-) diff --git a/src/allocated_buffer-inl.h b/src/allocated_buffer-inl.h index d611ac72f15a34..8d715163b247cb 100644 --- a/src/allocated_buffer-inl.h +++ b/src/allocated_buffer-inl.h @@ -11,97 +11,97 @@ namespace node { +// 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 New(Environment* env, + v8::Local ab, + size_t byte_offset, + size_t length); +} + +NoArrayBufferZeroFillScope::NoArrayBufferZeroFillScope( + IsolateData* isolate_data) + : node_allocator_(isolate_data->node_allocator()) { + if (node_allocator_ != nullptr) node_allocator_->zero_fill_field()[0] = 0; +} + +NoArrayBufferZeroFillScope::~NoArrayBufferZeroFillScope() { + if (node_allocator_ != nullptr) node_allocator_->zero_fill_field()[0] = 1; +} + AllocatedBuffer AllocatedBuffer::AllocateManaged( Environment* env, - size_t size, - int flags) { - char* data = flags & ALLOCATE_MANAGED_UNCHECKED ? - env->AllocateUnchecked(size) : - env->Allocate(size); - if (data == nullptr) size = 0; - return AllocatedBuffer(env, uv_buf_init(data, size)); + size_t size) { + NoArrayBufferZeroFillScope no_zero_fill_scope(env->isolate_data()); + std::unique_ptr bs = + v8::ArrayBuffer::NewBackingStore(env->isolate(), size); + return AllocatedBuffer(env, std::move(bs)); } -inline AllocatedBuffer::AllocatedBuffer(Environment* env, uv_buf_t buf) - : env_(env), buffer_(buf) {} +AllocatedBuffer::AllocatedBuffer( + Environment* env, std::unique_ptr bs) + : env_(env), backing_store_(std::move(bs)) {} + +AllocatedBuffer::AllocatedBuffer( + Environment* env, uv_buf_t buffer) + : env_(env) { + if (buffer.base == nullptr) return; + auto map = env->released_allocated_buffers(); + auto it = map->find(buffer.base); + CHECK_NE(it, map->end()); + backing_store_ = std::move(it->second); + map->erase(it); +} -inline void AllocatedBuffer::Resize(size_t len) { - // The `len` check is to make sure we don't end up with `nullptr` as our base. - char* new_data = env_->Reallocate(buffer_.base, buffer_.len, - len > 0 ? len : 1); - CHECK_NOT_NULL(new_data); - buffer_ = uv_buf_init(new_data, len); +void AllocatedBuffer::Resize(size_t len) { + if (len == 0) { + backing_store_ = v8::ArrayBuffer::NewBackingStore(env_->isolate(), 0); + return; + } + NoArrayBufferZeroFillScope no_zero_fill_scope(env_->isolate_data()); + backing_store_ = v8::BackingStore::Reallocate( + env_->isolate(), std::move(backing_store_), len); } inline uv_buf_t AllocatedBuffer::release() { - uv_buf_t ret = buffer_; - buffer_ = uv_buf_init(nullptr, 0); + if (data() == nullptr) return uv_buf_init(nullptr, 0); + + CHECK_NOT_NULL(env_); + uv_buf_t ret = uv_buf_init(data(), size()); + env_->released_allocated_buffers()->emplace( + ret.base, std::move(backing_store_)); return ret; } inline char* AllocatedBuffer::data() { - return buffer_.base; + if (!backing_store_) return nullptr; + return static_cast(backing_store_->Data()); } inline const char* AllocatedBuffer::data() const { - return buffer_.base; + if (!backing_store_) return nullptr; + return static_cast(backing_store_->Data()); } -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 size_t AllocatedBuffer::size() const { + if (!backing_store_) return 0; + return backing_store_->ByteLength(); } inline void AllocatedBuffer::clear() { - uv_buf_t buf = release(); - if (buf.base != nullptr) { - CHECK_NOT_NULL(env_); - env_->Free(buf.base, buf.len); - } + backing_store_.reset(); } inline v8::MaybeLocal AllocatedBuffer::ToBuffer() { - CHECK_NOT_NULL(env_); - v8::MaybeLocal obj = Buffer::New(env_, data(), size(), false); - if (!obj.IsEmpty()) release(); - return obj; + v8::Local ab = ToArrayBuffer(); + return Buffer::New(env_, ab, 0, ab->ByteLength()) + .FromMaybe(v8::Local()); } inline v8::Local AllocatedBuffer::ToArrayBuffer() { - CHECK_NOT_NULL(env_); - uv_buf_t buf = release(); - auto callback = [](void* data, size_t length, void* deleter_data){ - CHECK_NOT_NULL(deleter_data); - - static_cast(deleter_data) - ->Free(data, length); - }; - std::unique_ptr backing = - v8::ArrayBuffer::NewBackingStore(buf.base, - buf.len, - callback, - env_->isolate() - ->GetArrayBufferAllocator()); - return v8::ArrayBuffer::New(env_->isolate(), - std::move(backing)); + return v8::ArrayBuffer::New(env_->isolate(), std::move(backing_store_)); } } // namespace node diff --git a/src/allocated_buffer.h b/src/allocated_buffer.h index b54f74b2d278ff..c984a342a8b2b8 100644 --- a/src/allocated_buffer.h +++ b/src/allocated_buffer.h @@ -6,31 +6,43 @@ #include "base_object.h" #include "uv.h" #include "v8.h" +#include "env.h" namespace node { class Environment; +// Disables zero-filling for ArrayBuffer allocations in this scope. This is +// similar to how we implement Buffer.allocUnsafe() in JS land. +class NoArrayBufferZeroFillScope{ + public: + inline explicit NoArrayBufferZeroFillScope(IsolateData* isolate_data); + inline ~NoArrayBufferZeroFillScope(); + + private: + NodeArrayBufferAllocator* node_allocator_; + + friend class Environment; +}; + // A unique-pointer-ish object that is compatible with the JS engine's // ArrayBuffer::Allocator. +// TODO(addaleax): We may want to start phasing this out as it's only a +// thin wrapper around v8::BackingStore at this point struct AllocatedBuffer { public: - enum AllocateManagedFlags { - ALLOCATE_MANAGED_FLAG_NONE, - ALLOCATE_MANAGED_UNCHECKED - }; - // Utilities 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 static AllocatedBuffer AllocateManaged( - Environment* env, - size_t size, - int flags = ALLOCATE_MANAGED_FLAG_NONE); - - explicit inline AllocatedBuffer(Environment* env = nullptr); - inline AllocatedBuffer(Environment* env, uv_buf_t buf); - inline ~AllocatedBuffer(); + inline static AllocatedBuffer AllocateManaged(Environment* env, size_t size); + + AllocatedBuffer() = default; + inline AllocatedBuffer( + Environment* env, std::unique_ptr bs); + // For this constructor variant, `buffer` *must* come from an earlier call + // to .release + inline AllocatedBuffer(Environment* env, uv_buf_t buffer); + inline void Resize(size_t len); inline uv_buf_t release(); @@ -42,16 +54,14 @@ struct AllocatedBuffer { inline v8::MaybeLocal ToBuffer(); inline v8::Local ToArrayBuffer(); - inline AllocatedBuffer(AllocatedBuffer&& other); - inline AllocatedBuffer& operator=(AllocatedBuffer&& other); + AllocatedBuffer(AllocatedBuffer&& other) = default; + AllocatedBuffer& operator=(AllocatedBuffer&& other) = default; 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_; + Environment* env_ = nullptr; + std::unique_ptr backing_store_; friend class Environment; }; diff --git a/src/env-inl.h b/src/env-inl.h index 20f7f840c1f621..db5d63176c554d 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -867,29 +867,9 @@ inline IsolateData* Environment::isolate_data() const { return isolate_data_; } -inline char* Environment::AllocateUnchecked(size_t size) { - return static_cast( - 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); -} - -// 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 New(Environment* env, - char* data, - size_t length, - bool uses_malloc); +std::unordered_map>* + Environment::released_allocated_buffers() { + return &released_allocated_buffers_; } inline void Environment::ThrowError(const char* errmsg) { diff --git a/src/env.cc b/src/env.cc index 3ccc899f272f8e..3ca9181628816e 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1133,25 +1133,6 @@ void Environment::MemoryInfo(MemoryTracker* tracker) const { // node, we shift its sizeof() size out of the Environment node. } -char* Environment::Reallocate(char* data, size_t old_size, size_t size) { - if (old_size == size) return data; - // If we know that the allocator is our ArrayBufferAllocator, we can let - // if reallocate directly. - if (isolate_data()->uses_node_allocator()) { - return static_cast( - 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; -} - void Environment::RunWeakRefCleanup() { isolate()->ClearKeptObjects(); } diff --git a/src/env.h b/src/env.h index 5729b940ab151a..b0d6375d7db5df 100644 --- a/src/env.h +++ b/src/env.h @@ -925,11 +925,6 @@ class Environment : public MemoryRetainer { inline IsolateData* isolate_data() const; - 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); @@ -1216,6 +1211,9 @@ class Environment : public MemoryRetainer { void RunAndClearNativeImmediates(bool only_refed = false); void RunAndClearInterrupts(); + inline std::unordered_map>* + released_allocated_buffers(); + private: template inline void CreateImmediate(Fn&& cb, bool ref); @@ -1378,6 +1376,11 @@ class Environment : public MemoryRetainer { // We should probably find a way to just use plain `v8::String`s created from // the source passed to LoadEnvironment() directly instead. std::unique_ptr main_utf16_; + + // Used by AllocatedBuffer::release() to keep track of the BackingStore for + // a given pointer. + std::unordered_map> + released_allocated_buffers_; }; } // namespace node diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 4f239671280d94..98b9ed7a92b765 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -328,19 +328,8 @@ MaybeLocal New(Environment* env, size_t length) { return Local(); } - AllocatedBuffer ret(env); - if (length > 0) { - ret = AllocatedBuffer::AllocateManaged( - env, - length, - AllocatedBuffer::ALLOCATE_MANAGED_UNCHECKED); - if (ret.data() == nullptr) { - THROW_ERR_MEMORY_ALLOCATION_FAILED(env); - return Local(); - } - } - - return scope.EscapeMaybe(ret.ToBuffer()); + return scope.EscapeMaybe( + AllocatedBuffer::AllocateManaged(env, length).ToBuffer()); } @@ -367,17 +356,8 @@ MaybeLocal Copy(Environment* env, const char* data, size_t length) { return Local(); } - AllocatedBuffer ret(env); + AllocatedBuffer ret = AllocatedBuffer::AllocateManaged(env, length); if (length > 0) { - CHECK_NOT_NULL(data); - ret = AllocatedBuffer::AllocateManaged( - env, - length, - AllocatedBuffer::ALLOCATE_MANAGED_UNCHECKED); - if (ret.data() == nullptr) { - THROW_ERR_MEMORY_ALLOCATION_FAILED(env); - return Local(); - } memcpy(ret.data(), data, length); } @@ -452,53 +432,23 @@ MaybeLocal New(Isolate* isolate, char* data, size_t length) { return MaybeLocal(); } Local obj; - if (Buffer::New(env, data, length, true).ToLocal(&obj)) + if (Buffer::New(env, data, length).ToLocal(&obj)) return handle_scope.Escape(obj); return Local(); } -// Warning: If this call comes through the public node_buffer.h API, -// the contract for this function is that `data` is allocated with malloc() +// The contract for this function is that `data` is allocated with malloc() // and not necessarily isolate's ArrayBuffer::Allocator. MaybeLocal New(Environment* env, char* data, - size_t length, - bool uses_malloc) { + size_t length) { if (length > 0) { CHECK_NOT_NULL(data); CHECK(length <= kMaxLength); } - if (uses_malloc) { - if (!env->isolate_data()->uses_node_allocator()) { - // We don't know for sure that the allocator is malloc()-based, so we need - // to fall back to the FreeCallback variant. - auto free_callback = [](char* data, void* hint) { free(data); }; - return New(env, data, length, free_callback, nullptr); - } else { - // This is malloc()-based, so we can acquire it into our own - // ArrayBufferAllocator. - CHECK_NOT_NULL(env->isolate_data()->node_allocator()); - env->isolate_data()->node_allocator()->RegisterPointer(data, length); - } - } - - auto callback = [](void* data, size_t length, void* deleter_data){ - CHECK_NOT_NULL(deleter_data); - - static_cast(deleter_data) - ->Free(data, length); - }; - std::unique_ptr backing = - v8::ArrayBuffer::NewBackingStore(data, - length, - callback, - env->isolate() - ->GetArrayBufferAllocator()); - Local ab = ArrayBuffer::New(env->isolate(), - std::move(backing)); - - return Buffer::New(env, ab, 0, length).FromMaybe(Local()); + auto free_callback = [](char* data, void* hint) { free(data); }; + return New(env, data, length, free_callback, nullptr); } namespace { diff --git a/src/node_crypto.cc b/src/node_crypto.cc index cb3b53ecc3eae1..42e833e9d60935 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -4025,7 +4025,7 @@ bool CipherBase::Final(AllocatedBuffer* out) { bool ok; if (kind_ == kDecipher && mode == EVP_CIPH_CCM_MODE) { ok = !pending_auth_failed_; - *out = AllocatedBuffer(env()); // Empty buffer. + *out = AllocatedBuffer::AllocateManaged(env(), 0); // Empty buffer. } else { int out_len = out->size(); ok = EVP_CipherFinal_ex(ctx_.get(), diff --git a/src/node_internals.h b/src/node_internals.h index 0ae17f71ecbcb2..65a4edcbea6b81 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -160,8 +160,7 @@ v8::MaybeLocal New(Environment* env, // ArrayBuffer::Allocator(). v8::MaybeLocal New(Environment* env, char* data, - size_t length, - bool uses_malloc); + size_t length); // Creates a Buffer instance over an existing ArrayBuffer. v8::MaybeLocal New(Environment* env, v8::Local ab, @@ -183,7 +182,7 @@ static v8::MaybeLocal New(Environment* env, const size_t len_in_bytes = buf->length() * sizeof(buf->out()[0]); if (buf->IsAllocated()) - ret = New(env, src, len_in_bytes, true); + ret = New(env, src, len_in_bytes); else if (!buf->IsInvalidated()) ret = Copy(env, src, len_in_bytes); diff --git a/src/node_serdes.cc b/src/node_serdes.cc index 86fb822dd5bfa9..c7877215911f8e 100644 --- a/src/node_serdes.cc +++ b/src/node_serdes.cc @@ -206,8 +206,7 @@ void SerializerContext::ReleaseBuffer(const FunctionCallbackInfo& args) { std::pair ret = ctx->serializer_.Release(); auto buf = Buffer::New(ctx->env(), reinterpret_cast(ret.first), - ret.second, - true /* uses_malloc */); + ret.second); if (!buf.IsEmpty()) { args.GetReturnValue().Set(buf.ToLocalChecked());