From b29eba7a4a45cbacf9ab7e5d189b46f41a792969 Mon Sep 17 00:00:00 2001 From: Sergey Chernyshev Date: Thu, 25 Apr 2024 01:51:50 +0200 Subject: [PATCH] src: move AliasedBuffer implementation to -inl.h Drive-by: Replace the SFINAE with a static_assert because we don't have (or need) an implementation for non-scalar AliasedBufferBase otherwise. Add forward declarations to memory_tracker.h now that aliased-buffer.h no longer includes util-inl.h. PR-URL: https://github.com/nodejs/node/pull/46817 Reviewed-By: Anna Henningsen Reviewed-By: Matteo Collina --- graal-nodejs/node.gyp | 1 + graal-nodejs/src/aliased_buffer-inl.h | 213 ++++++++++++++++++ graal-nodejs/src/aliased_buffer.h | 201 ++++------------- graal-nodejs/src/env-inl.h | 2 +- graal-nodejs/src/memory_tracker-inl.h | 1 + graal-nodejs/src/memory_tracker.h | 7 + graal-nodejs/src/node_file.cc | 2 +- graal-nodejs/src/node_http2.cc | 2 +- graal-nodejs/src/node_perf.cc | 2 +- graal-nodejs/src/node_v8.cc | 1 + .../test/cctest/test_aliased_buffer.cc | 4 +- 11 files changed, 268 insertions(+), 168 deletions(-) create mode 100644 graal-nodejs/src/aliased_buffer-inl.h diff --git a/graal-nodejs/node.gyp b/graal-nodejs/node.gyp index c304d8d0a3a..681a8d08975 100644 --- a/graal-nodejs/node.gyp +++ b/graal-nodejs/node.gyp @@ -598,6 +598,7 @@ 'src/uv.cc', # headers to make for a more pleasant IDE experience 'src/aliased_buffer.h', + 'src/aliased_buffer-inl.h', 'src/aliased_struct.h', 'src/aliased_struct-inl.h', 'src/async_wrap.h', diff --git a/graal-nodejs/src/aliased_buffer-inl.h b/graal-nodejs/src/aliased_buffer-inl.h new file mode 100644 index 00000000000..a3bee2593ae --- /dev/null +++ b/graal-nodejs/src/aliased_buffer-inl.h @@ -0,0 +1,213 @@ +#ifndef SRC_ALIASED_BUFFER_INL_H_ +#define SRC_ALIASED_BUFFER_INL_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#include "aliased_buffer.h" +#include "util-inl.h" + +namespace node { + +typedef size_t AliasedBufferIndex; + +template +AliasedBufferBase::AliasedBufferBase( + v8::Isolate* isolate, const size_t count, const AliasedBufferIndex* index) + : isolate_(isolate), count_(count), byte_offset_(0), index_(index) { + CHECK_GT(count, 0); + if (index != nullptr) { + // Will be deserialized later. + return; + } + const v8::HandleScope handle_scope(isolate_); + const size_t size_in_bytes = + MultiplyWithOverflowCheck(sizeof(NativeT), count); + + // allocate v8 ArrayBuffer + v8::Local ab = v8::ArrayBuffer::New(isolate_, size_in_bytes); + buffer_ = static_cast(ab->Data()); + + // allocate v8 TypedArray + v8::Local js_array = V8T::New(ab, byte_offset_, count); + js_array_ = v8::Global(isolate, js_array); +} + +template +AliasedBufferBase::AliasedBufferBase( + v8::Isolate* isolate, + const size_t byte_offset, + const size_t count, + const AliasedBufferBase& backing_buffer, + const AliasedBufferIndex* index) + : isolate_(isolate), + count_(count), + byte_offset_(byte_offset), + index_(index) { + if (index != nullptr) { + // Will be deserialized later. + return; + } + const v8::HandleScope handle_scope(isolate_); + v8::Local ab = backing_buffer.GetArrayBuffer(); + + // validate that the byte_offset is aligned with sizeof(NativeT) + CHECK_EQ(byte_offset & (sizeof(NativeT) - 1), 0); + // validate this fits inside the backing buffer + CHECK_LE(MultiplyWithOverflowCheck(sizeof(NativeT), count), + ab->ByteLength() - byte_offset); + + buffer_ = reinterpret_cast( + const_cast(backing_buffer.GetNativeBuffer() + byte_offset)); + + v8::Local js_array = V8T::New(ab, byte_offset, count); + js_array_ = v8::Global(isolate, js_array); +} + +template +AliasedBufferBase::AliasedBufferBase( + const AliasedBufferBase& that) + : isolate_(that.isolate_), + count_(that.count_), + byte_offset_(that.byte_offset_), + buffer_(that.buffer_) { + DCHECK_NULL(index_); + js_array_ = v8::Global(that.isolate_, that.GetJSArray()); +} + +template +AliasedBufferIndex AliasedBufferBase::Serialize( + v8::Local context, v8::SnapshotCreator* creator) { + DCHECK_NULL(index_); + return creator->AddData(context, GetJSArray()); +} + +template +inline void AliasedBufferBase::Deserialize( + v8::Local context) { + DCHECK_NOT_NULL(index_); + v8::Local arr = + context->GetDataFromSnapshotOnce(*index_).ToLocalChecked(); + // These may not hold true for AliasedBuffers that have grown, so should + // be removed when we expand the snapshot support. + DCHECK_EQ(count_, arr->Length()); + DCHECK_EQ(byte_offset_, arr->ByteOffset()); + uint8_t* raw = static_cast(arr->Buffer()->Data()); + buffer_ = reinterpret_cast(raw + byte_offset_); + js_array_.Reset(isolate_, arr); + index_ = nullptr; +} + +template +AliasedBufferBase& AliasedBufferBase::operator=( + AliasedBufferBase&& that) noexcept { + DCHECK_NULL(index_); + this->~AliasedBufferBase(); + isolate_ = that.isolate_; + count_ = that.count_; + byte_offset_ = that.byte_offset_; + buffer_ = that.buffer_; + + js_array_.Reset(isolate_, that.js_array_.Get(isolate_)); + + that.buffer_ = nullptr; + that.js_array_.Reset(); + return *this; +} + +template +v8::Local AliasedBufferBase::GetJSArray() const { + DCHECK_NULL(index_); + return js_array_.Get(isolate_); +} + +template +void AliasedBufferBase::Release() { + DCHECK_NULL(index_); + js_array_.Reset(); +} + +template +v8::Local AliasedBufferBase::GetArrayBuffer() + const { + return GetJSArray()->Buffer(); +} + +template +inline const NativeT* AliasedBufferBase::GetNativeBuffer() const { + DCHECK_NULL(index_); + return buffer_; +} + +template +inline const NativeT* AliasedBufferBase::operator*() const { + return GetNativeBuffer(); +} + +template +inline void AliasedBufferBase::SetValue(const size_t index, + NativeT value) { + DCHECK_LT(index, count_); + DCHECK_NULL(index_); + buffer_[index] = value; +} + +template +inline const NativeT AliasedBufferBase::GetValue( + const size_t index) const { + DCHECK_NULL(index_); + DCHECK_LT(index, count_); + return buffer_[index]; +} + +template +typename AliasedBufferBase::Reference +AliasedBufferBase::operator[](size_t index) { + DCHECK_NULL(index_); + return Reference(this, index); +} + +template +NativeT AliasedBufferBase::operator[](size_t index) const { + return GetValue(index); +} + +template +size_t AliasedBufferBase::Length() const { + return count_; +} + +template +void AliasedBufferBase::reserve(size_t new_capacity) { + DCHECK_NULL(index_); + DCHECK_GE(new_capacity, count_); + DCHECK_EQ(byte_offset_, 0); + const v8::HandleScope handle_scope(isolate_); + + const size_t old_size_in_bytes = sizeof(NativeT) * count_; + const size_t new_size_in_bytes = + MultiplyWithOverflowCheck(sizeof(NativeT), new_capacity); + + // allocate v8 new ArrayBuffer + v8::Local ab = + v8::ArrayBuffer::New(isolate_, new_size_in_bytes); + + // allocate new native buffer + NativeT* new_buffer = static_cast(ab->Data()); + // copy old content + memcpy(new_buffer, buffer_, old_size_in_bytes); + + // allocate v8 TypedArray + v8::Local js_array = V8T::New(ab, byte_offset_, new_capacity); + + // move over old v8 TypedArray + js_array_ = std::move(v8::Global(isolate_, js_array)); + + buffer_ = new_buffer; + count_ = new_capacity; +} + +} // namespace node + +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#endif // SRC_ALIASED_BUFFER_INL_H_ diff --git a/graal-nodejs/src/aliased_buffer.h b/graal-nodejs/src/aliased_buffer.h index 98ea2d31feb..1b9336e1cd3 100644 --- a/graal-nodejs/src/aliased_buffer.h +++ b/graal-nodejs/src/aliased_buffer.h @@ -4,7 +4,6 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include -#include "util-inl.h" #include "v8.h" namespace node { @@ -28,34 +27,14 @@ typedef size_t AliasedBufferIndex; * The encapsulation herein provides a placeholder where such writes can be * observed. Any notification APIs will be left as a future exercise. */ -template ::value>> +template class AliasedBufferBase { public: + static_assert(std::is_scalar::value); + AliasedBufferBase(v8::Isolate* isolate, const size_t count, - const AliasedBufferIndex* index = nullptr) - : isolate_(isolate), count_(count), byte_offset_(0), index_(index) { - CHECK_GT(count, 0); - if (index != nullptr) { - // Will be deserialized later. - return; - } - const v8::HandleScope handle_scope(isolate_); - const size_t size_in_bytes = - MultiplyWithOverflowCheck(sizeof(NativeT), count); - - // allocate v8 ArrayBuffer - v8::Local ab = v8::ArrayBuffer::New( - isolate_, size_in_bytes); - buffer_ = static_cast(ab->Data()); - - // allocate v8 TypedArray - v8::Local js_array = V8T::New(ab, byte_offset_, count); - js_array_ = v8::Global(isolate, js_array); - } + const AliasedBufferIndex* index = nullptr); /** * Create an AliasedBufferBase over a sub-region of another aliased buffer. @@ -71,74 +50,16 @@ class AliasedBufferBase { const size_t byte_offset, const size_t count, const AliasedBufferBase& backing_buffer, - const AliasedBufferIndex* index = nullptr) - : isolate_(isolate), - count_(count), - byte_offset_(byte_offset), - index_(index) { - if (index != nullptr) { - // Will be deserialized later. - return; - } - const v8::HandleScope handle_scope(isolate_); - v8::Local ab = backing_buffer.GetArrayBuffer(); - - // validate that the byte_offset is aligned with sizeof(NativeT) - CHECK_EQ(byte_offset & (sizeof(NativeT) - 1), 0); - // validate this fits inside the backing buffer - CHECK_LE(MultiplyWithOverflowCheck(sizeof(NativeT), count), - ab->ByteLength() - byte_offset); - - buffer_ = reinterpret_cast( - const_cast(backing_buffer.GetNativeBuffer() + byte_offset)); - - v8::Local js_array = V8T::New(ab, byte_offset, count); - js_array_ = v8::Global(isolate, js_array); - } - - AliasedBufferBase(const AliasedBufferBase& that) - : isolate_(that.isolate_), - count_(that.count_), - byte_offset_(that.byte_offset_), - buffer_(that.buffer_) { - DCHECK_NULL(index_); - js_array_ = v8::Global(that.isolate_, that.GetJSArray()); - } + const AliasedBufferIndex* index = nullptr); + + AliasedBufferBase(const AliasedBufferBase& that); AliasedBufferIndex Serialize(v8::Local context, - v8::SnapshotCreator* creator) { - DCHECK_NULL(index_); - return creator->AddData(context, GetJSArray()); - } - - inline void Deserialize(v8::Local context) { - DCHECK_NOT_NULL(index_); - v8::Local arr = - context->GetDataFromSnapshotOnce(*index_).ToLocalChecked(); - // These may not hold true for AliasedBuffers that have grown, so should - // be removed when we expand the snapshot support. - DCHECK_EQ(count_, arr->Length()); - DCHECK_EQ(byte_offset_, arr->ByteOffset()); - uint8_t* raw = static_cast(arr->Buffer()->Data()); - buffer_ = reinterpret_cast(raw + byte_offset_); - js_array_.Reset(isolate_, arr); - index_ = nullptr; - } - - AliasedBufferBase& operator=(AliasedBufferBase&& that) noexcept { - DCHECK_NULL(index_); - this->~AliasedBufferBase(); - isolate_ = that.isolate_; - count_ = that.count_; - byte_offset_ = that.byte_offset_; - buffer_ = that.buffer_; - - js_array_.Reset(isolate_, that.js_array_.Get(isolate_)); - - that.buffer_ = nullptr; - that.js_array_.Reset(); - return *this; - } + v8::SnapshotCreator* creator); + + inline void Deserialize(v8::Local context); + + AliasedBufferBase& operator=(AliasedBufferBase&& that) noexcept; /** * Helper class that is returned from operator[] to support assignment into @@ -191,105 +112,50 @@ class AliasedBufferBase { /** * Get the underlying v8 TypedArray overlayed on top of the native buffer */ - v8::Local GetJSArray() const { - DCHECK_NULL(index_); - return js_array_.Get(isolate_); - } + v8::Local GetJSArray() const; - void Release() { - DCHECK_NULL(index_); - js_array_.Reset(); - } + void Release(); /** * Get the underlying v8::ArrayBuffer underlying the TypedArray and * overlaying the native buffer */ - v8::Local GetArrayBuffer() const { - return GetJSArray()->Buffer(); - } + v8::Local GetArrayBuffer() const; /** * Get the underlying native buffer. Note that all reads/writes should occur * through the GetValue/SetValue/operator[] methods */ - inline const NativeT* GetNativeBuffer() const { - DCHECK_NULL(index_); - return buffer_; - } + inline const NativeT* GetNativeBuffer() const; /** * Synonym for GetBuffer() */ - inline const NativeT* operator * () const { - return GetNativeBuffer(); - } + inline const NativeT* operator*() const; /** * Set position index to given value. */ - inline void SetValue(const size_t index, NativeT value) { - DCHECK_LT(index, count_); - DCHECK_NULL(index_); - buffer_[index] = value; - } + inline void SetValue(const size_t index, NativeT value); /** * Get value at position index */ - inline const NativeT GetValue(const size_t index) const { - DCHECK_NULL(index_); - DCHECK_LT(index, count_); - return buffer_[index]; - } + inline const NativeT GetValue(const size_t index) const; /** * Effectively, a synonym for GetValue/SetValue */ - Reference operator[](size_t index) { - DCHECK_NULL(index_); - return Reference(this, index); - } + Reference operator[](size_t index); - NativeT operator[](size_t index) const { - return GetValue(index); - } + NativeT operator[](size_t index) const; - size_t Length() const { - return count_; - } + size_t Length() const; // Should only be used to extend the array. // Should only be used on an owning array, not one created as a sub array of // an owning `AliasedBufferBase`. - void reserve(size_t new_capacity) { - DCHECK_NULL(index_); - DCHECK_GE(new_capacity, count_); - DCHECK_EQ(byte_offset_, 0); - const v8::HandleScope handle_scope(isolate_); - - const size_t old_size_in_bytes = sizeof(NativeT) * count_; - const size_t new_size_in_bytes = MultiplyWithOverflowCheck(sizeof(NativeT), - new_capacity); - - // allocate v8 new ArrayBuffer - v8::Local ab = v8::ArrayBuffer::New( - isolate_, new_size_in_bytes); - - // allocate new native buffer - NativeT* new_buffer = static_cast(ab->Data()); - // copy old content - memcpy(new_buffer, buffer_, old_size_in_bytes); - - // allocate v8 TypedArray - v8::Local js_array = V8T::New(ab, byte_offset_, new_capacity); - - // move over old v8 TypedArray - js_array_ = std::move(v8::Global(isolate_, js_array)); - - buffer_ = new_buffer; - count_ = new_capacity; - } + void reserve(size_t new_capacity); private: v8::Isolate* isolate_ = nullptr; @@ -302,11 +168,22 @@ class AliasedBufferBase { const AliasedBufferIndex* index_ = nullptr; }; -typedef AliasedBufferBase AliasedInt32Array; -typedef AliasedBufferBase AliasedUint8Array; -typedef AliasedBufferBase AliasedUint32Array; -typedef AliasedBufferBase AliasedFloat64Array; -typedef AliasedBufferBase AliasedBigInt64Array; +#define ALIASED_BUFFER_LIST(V) \ + V(int8_t, Int8Array) \ + V(uint8_t, Uint8Array) \ + V(int16_t, Int16Array) \ + V(uint16_t, Uint16Array) \ + V(int32_t, Int32Array) \ + V(uint32_t, Uint32Array) \ + V(float, Float32Array) \ + V(double, Float64Array) \ + V(int64_t, BigInt64Array) + +#define V(NativeT, V8T) \ + typedef AliasedBufferBase Aliased##V8T; +ALIASED_BUFFER_LIST(V) +#undef V + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/graal-nodejs/src/env-inl.h b/graal-nodejs/src/env-inl.h index 6b0f6f05cf0..b959fb48d29 100644 --- a/graal-nodejs/src/env-inl.h +++ b/graal-nodejs/src/env-inl.h @@ -24,7 +24,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS -#include "aliased_buffer.h" +#include "aliased_buffer-inl.h" #include "callback_queue-inl.h" #include "env.h" #include "node.h" diff --git a/graal-nodejs/src/memory_tracker-inl.h b/graal-nodejs/src/memory_tracker-inl.h index a4970d9392f..b8d2667d981 100644 --- a/graal-nodejs/src/memory_tracker-inl.h +++ b/graal-nodejs/src/memory_tracker-inl.h @@ -3,6 +3,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include "aliased_buffer-inl.h" #include "memory_tracker.h" namespace node { diff --git a/graal-nodejs/src/memory_tracker.h b/graal-nodejs/src/memory_tracker.h index 4682f25916e..ae3b0031790 100644 --- a/graal-nodejs/src/memory_tracker.h +++ b/graal-nodejs/src/memory_tracker.h @@ -13,8 +13,15 @@ #include #include +namespace v8 { +class BackingStore; +} + namespace node { +template +struct MallocedBuffer; + // Set the node name of a MemoryRetainer to klass #define SET_MEMORY_INFO_NAME(Klass) \ inline const char* MemoryInfoName() const override { return #Klass; } diff --git a/graal-nodejs/src/node_file.cc b/graal-nodejs/src/node_file.cc index cc077a9d43a..4dc4b91145d 100644 --- a/graal-nodejs/src/node_file.cc +++ b/graal-nodejs/src/node_file.cc @@ -20,7 +20,7 @@ // USE OR OTHER DEALINGS IN THE SOFTWARE. #include "node_file.h" // NOLINT(build/include_inline) #include "node_file-inl.h" -#include "aliased_buffer.h" +#include "aliased_buffer-inl.h" #include "memory_tracker-inl.h" #include "node_buffer.h" #include "node_external_reference.h" diff --git a/graal-nodejs/src/node_http2.cc b/graal-nodejs/src/node_http2.cc index d0733bb304c..528bf3aa58b 100644 --- a/graal-nodejs/src/node_http2.cc +++ b/graal-nodejs/src/node_http2.cc @@ -1,5 +1,5 @@ #include "node_http2.h" -#include "aliased_buffer.h" +#include "aliased_buffer-inl.h" #include "aliased_struct-inl.h" #include "debug_utils-inl.h" #include "histogram-inl.h" diff --git a/graal-nodejs/src/node_perf.cc b/graal-nodejs/src/node_perf.cc index 582a74ebd14..d816a0a4db4 100644 --- a/graal-nodejs/src/node_perf.cc +++ b/graal-nodejs/src/node_perf.cc @@ -1,5 +1,5 @@ #include "node_perf.h" -#include "aliased_buffer.h" +#include "aliased_buffer-inl.h" #include "env-inl.h" #include "histogram-inl.h" #include "memory_tracker-inl.h" diff --git a/graal-nodejs/src/node_v8.cc b/graal-nodejs/src/node_v8.cc index f4900328cfe..82ae6caa952 100644 --- a/graal-nodejs/src/node_v8.cc +++ b/graal-nodejs/src/node_v8.cc @@ -20,6 +20,7 @@ // USE OR OTHER DEALINGS IN THE SOFTWARE. #include "node_v8.h" +#include "aliased_buffer-inl.h" #include "base_object-inl.h" #include "env-inl.h" #include "memory_tracker-inl.h" diff --git a/graal-nodejs/test/cctest/test_aliased_buffer.cc b/graal-nodejs/test/cctest/test_aliased_buffer.cc index ba947700c1b..6d62444d42c 100644 --- a/graal-nodejs/test/cctest/test_aliased_buffer.cc +++ b/graal-nodejs/test/cctest/test_aliased_buffer.cc @@ -1,6 +1,6 @@ -#include "v8.h" -#include "aliased_buffer.h" +#include "aliased_buffer-inl.h" #include "node_test_fixture.h" +#include "v8.h" using node::AliasedBufferBase;