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

src: simplify AliasedBuffer lifetime management #26196

Closed
wants to merge 1 commit into from
Closed

src: simplify AliasedBuffer lifetime management #26196

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 14 additions & 29 deletions src/aliased_buffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,19 +30,17 @@ class AliasedBuffer {
AliasedBuffer(v8::Isolate* isolate, const size_t count)
: isolate_(isolate),
count_(count),
byte_offset_(0),
free_buffer_(true) {
byte_offset_(0) {
CHECK_GT(count, 0);
const v8::HandleScope handle_scope(isolate_);

const size_t size_in_bytes = sizeof(NativeT) * count;

// allocate native buffer
buffer_ = Calloc<NativeT>(count);
const size_t size_in_bytes =
MultiplyWithOverflowCheck(sizeof(NativeT), count);

// allocate v8 ArrayBuffer
v8::Local<v8::ArrayBuffer> ab = v8::ArrayBuffer::New(
isolate_, buffer_, size_in_bytes);
isolate_, size_in_bytes);
buffer_ = static_cast<NativeT*>(ab->GetContents().Data());

// allocate v8 TypedArray
v8::Local<V8T> js_array = V8T::New(ab, byte_offset_, count);
Expand All @@ -65,16 +63,16 @@ class AliasedBuffer {
v8::Uint8Array>& backing_buffer)
: isolate_(isolate),
count_(count),
byte_offset_(byte_offset),
free_buffer_(false) {
byte_offset_(byte_offset) {
const v8::HandleScope handle_scope(isolate_);

v8::Local<v8::ArrayBuffer> 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(sizeof(NativeT) * count, ab->ByteLength() - byte_offset);
CHECK_LE(MultiplyWithOverflowCheck(sizeof(NativeT), count),
ab->ByteLength() - byte_offset);

buffer_ = reinterpret_cast<NativeT*>(
const_cast<uint8_t*>(backing_buffer.GetNativeBuffer() + byte_offset));
Expand All @@ -87,25 +85,16 @@ class AliasedBuffer {
: isolate_(that.isolate_),
count_(that.count_),
byte_offset_(that.byte_offset_),
buffer_(that.buffer_),
free_buffer_(false) {
buffer_(that.buffer_) {
js_array_ = v8::Global<V8T>(that.isolate_, that.GetJSArray());
}

~AliasedBuffer() {
if (free_buffer_ && buffer_ != nullptr) {
free(buffer_);
}
js_array_.Reset();
}

AliasedBuffer& operator=(AliasedBuffer&& that) noexcept {
this->~AliasedBuffer();
isolate_ = that.isolate_;
count_ = that.count_;
byte_offset_ = that.byte_offset_;
buffer_ = that.buffer_;
free_buffer_ = that.free_buffer_;

js_array_.Reset(isolate_, that.js_array_.Get(isolate_));

Expand Down Expand Up @@ -231,29 +220,26 @@ class AliasedBuffer {
void reserve(size_t new_capacity) {
DCHECK_GE(new_capacity, count_);
DCHECK_EQ(byte_offset_, 0);
DCHECK(free_buffer_);
const v8::HandleScope handle_scope(isolate_);

const size_t old_size_in_bytes = sizeof(NativeT) * count_;
const size_t new_size_in_bytes = sizeof(NativeT) * new_capacity;

// allocate v8 new ArrayBuffer
v8::Local<v8::ArrayBuffer> ab = v8::ArrayBuffer::New(
isolate_, new_size_in_bytes);

// allocate new native buffer
NativeT* new_buffer = Calloc<NativeT>(new_capacity);
NativeT* new_buffer = static_cast<NativeT*>(ab->GetContents().Data());
Copy link
Member

@gengjiawen gengjiawen Feb 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto here ?

https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html#cast-expressions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've made a decision about being explicit if the type is not too verbose: https://github.com/nodejs/node/blob/master/CPP_STYLE_GUIDE.md#using-auto

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But in current codebase, it's already used this way

node/src/node_platform.cc

Line 235 in c70e853

auto platform_data = static_cast<PerIsolatePlatformData*>(handle->data);

Copy link
Member

@gengjiawen gengjiawen Feb 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also used by v8 by default

node/deps/v8/.clang-tidy

Line 7 in 9b4bf7d

modernize-use-auto,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gengjiawen The example in node_platform.cc you’re referring to uses a more complex type – I would agree with using auto for that. Generally, we’re not being super consistent here, and we’ve had debates about this before. ;)

I think auto would be okay here too, because the static_cast makes things a bit more obvious, but I still think NativeT* is simple enough as a type to be kept this way.

(Basically, I agree with what the linked clang docs are saying, I just think NativeT* is short enough for me.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an option MinTypeNameLength, disscuss it's value to make auto rule more consistent ?
https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html#options

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gengjiawen It’s just my opinion, but my gut feeling would be something like 10 or 12 characters or so, and only to use auto when the type is obvious (e.g. from new Something() or when using a cast).

// copy old content
memcpy(new_buffer, buffer_, old_size_in_bytes);

// allocate v8 new ArrayBuffer
v8::Local<v8::ArrayBuffer> ab = v8::ArrayBuffer::New(
isolate_, new_buffer, new_size_in_bytes);

// allocate v8 TypedArray
v8::Local<V8T> js_array = V8T::New(ab, byte_offset_, new_capacity);

// move over old v8 TypedArray
js_array_ = std::move(v8::Global<V8T>(isolate_, js_array));

// Free old buffer and set new values
free(buffer_);
buffer_ = new_buffer;
count_ = new_capacity;
}
Expand All @@ -264,7 +250,6 @@ class AliasedBuffer {
size_t byte_offset_;
NativeT* buffer_;
v8::Global<V8T> js_array_;
bool free_buffer_;
};
} // namespace node

Expand Down