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: remove usage of AllocatedBuffer from node_http2 #40584

Closed
wants to merge 1 commit into from
Closed

src: remove usage of AllocatedBuffer from node_http2 #40584

Show file tree
Hide file tree
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
92 changes: 57 additions & 35 deletions src/node_http2.cc
View file
Open in desktop
Expand Up @@ -21,9 +21,11 @@ namespace node {
using v8::Array;
using v8::ArrayBuffer;
using v8::ArrayBufferView;
using v8::BackingStore;
using v8::Boolean;
using v8::Context;
using v8::EscapableHandleScope;
using v8::False;
using v8::Function;
using v8::FunctionCallbackInfo;
using v8::FunctionTemplate;
Expand All @@ -37,6 +39,7 @@ using v8::Number;
using v8::Object;
using v8::ObjectTemplate;
using v8::String;
using v8::True;
using v8::Uint8Array;
using v8::Undefined;
using v8::Value;
Expand Down Expand Up @@ -267,17 +270,20 @@ Local<Value> Http2Settings::Pack(
size_t count,
const nghttp2_settings_entry* entries) {
EscapableHandleScope scope(env->isolate());
const size_t size = count * 6;
AllocatedBuffer buffer = AllocatedBuffer::AllocateManaged(env, size);
ssize_t ret =
nghttp2_pack_settings_payload(
reinterpret_cast<uint8_t*>(buffer.data()),
size,
entries,
count);
Local<Value> buf = Undefined(env->isolate());
if (ret >= 0) buf = buffer.ToBuffer().ToLocalChecked();
return scope.Escape(buf);
std::unique_ptr<BackingStore> bs;
{
NoArrayBufferZeroFillScope no_zero_fill_scope(env->isolate_data());
bs = ArrayBuffer::NewBackingStore(env->isolate(), count * 6);
}
if (nghttp2_pack_settings_payload(static_cast<uint8_t*>(bs->Data()),
bs->ByteLength(),
entries,
count) < 0) {
return scope.Escape(Undefined(env->isolate()));
}
Local<ArrayBuffer> ab = ArrayBuffer::New(env->isolate(), std::move(bs));
return scope.Escape(Buffer::New(env, ab, 0, ab->ByteLength())
.FromMaybe(Local<Value>()));
}

// Updates the shared TypedArray with the current remote or local settings for
Expand Down Expand Up @@ -323,7 +329,7 @@ void Http2Settings::Done(bool ack) {
double duration = (end - startTime_) / 1e6;

Local<Value> argv[] = {
ack ? v8::True(env()->isolate()) : v8::False(env()->isolate()),
ack ? True(env()->isolate()) : False(env()->isolate()),
Copy link
Member

Choose a reason for hiding this comment

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

nit: for these two in particular I tend to prefer keeping the v8:: prefix. Not blocking tho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasnell is it because these are similar to true and false?

Number::New(env()->isolate(), duration)
};
MakeCallback(callback(), arraysize(argv), argv);
Expand Down Expand Up @@ -368,19 +374,23 @@ Origins::Origins(
return;
}

buf_ = AllocatedBuffer::AllocateManaged(
env,
(alignof(nghttp2_origin_entry) - 1) +
count_ * sizeof(nghttp2_origin_entry) +
origin_string_len);
{
NoArrayBufferZeroFillScope no_zero_fill_scope(env->isolate_data());
bs_ = ArrayBuffer::NewBackingStore(env->isolate(),
alignof(nghttp2_origin_entry) - 1 +
count_ * sizeof(nghttp2_origin_entry) +
origin_string_len);
}

// Make sure the start address is aligned appropriately for an nghttp2_nv*.
char* start = AlignUp(buf_.data(), alignof(nghttp2_origin_entry));
char* start = AlignUp(static_cast<char*>(bs_->Data()),
alignof(nghttp2_origin_entry));
char* origin_contents = start + (count_ * sizeof(nghttp2_origin_entry));
nghttp2_origin_entry* const nva =
reinterpret_cast<nghttp2_origin_entry*>(start);

CHECK_LE(origin_contents + origin_string_len, buf_.data() + buf_.size());
CHECK_LE(origin_contents + origin_string_len,
static_cast<char*>(bs_->Data()) + bs_->ByteLength());
CHECK_EQ(origin_string->WriteOneByte(
env->isolate(),
reinterpret_cast<uint8_t*>(origin_contents),
Expand Down Expand Up @@ -819,7 +829,7 @@ void Http2Session::ConsumeHTTP2Data() {
DecrementCurrentSessionMemory(stream_buf_.len);
stream_buf_offset_ = 0;
stream_buf_ab_.Reset();
stream_buf_allocation_.clear();
stream_buf_allocation_.reset();
stream_buf_ = uv_buf_init(nullptr, 0);

// Send any data that was queued up while processing the received data.
Expand Down Expand Up @@ -1247,7 +1257,8 @@ void Http2StreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {

Local<ArrayBuffer> ab;
if (session->stream_buf_ab_.IsEmpty()) {
ab = session->stream_buf_allocation_.ToArrayBuffer();
ab = ArrayBuffer::New(env->isolate(),
std::move(session->stream_buf_allocation_));
session->stream_buf_ab_.Reset(env->isolate(), ab);
} else {
ab = PersistentToLocal::Strong(session->stream_buf_ab_);
Expand Down Expand Up @@ -1823,7 +1834,7 @@ Http2Stream* Http2Session::SubmitRequest(
}

uv_buf_t Http2Session::OnStreamAlloc(size_t suggested_size) {
return AllocatedBuffer::AllocateManaged(env(), suggested_size).release();
return env()->allocate_managed_buffer(suggested_size);
}

// Callback used to receive inbound data from the i/o stream
Expand All @@ -1833,7 +1844,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) {
Http2Scope h2scope(this);
CHECK_NOT_NULL(stream_);
Debug(this, "receiving %d bytes, offset %d", nread, stream_buf_offset_);
AllocatedBuffer buf(env(), buf_);
std::unique_ptr<BackingStore> bs = env()->release_managed_buffer(buf_);

// Only pass data on if nread > 0
if (nread <= 0) {
Expand All @@ -1843,24 +1854,34 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) {
return;
}

CHECK_LE(static_cast<size_t>(nread), bs->ByteLength());

statistics_.data_received += nread;

if (LIKELY(stream_buf_offset_ == 0)) {
// Shrink to the actual amount of used data.
buf.Resize(nread);
bs = BackingStore::Reallocate(env()->isolate(), std::move(bs), nread);
} else {
// This is a very unlikely case, and should only happen if the ReadStart()
// call in OnStreamAfterWrite() immediately provides data. If that does
// happen, we concatenate the data we received with the already-stored
// pending input data, slicing off the already processed part.
size_t pending_len = stream_buf_.len - stream_buf_offset_;
AllocatedBuffer new_buf =
AllocatedBuffer::AllocateManaged(env(), pending_len + nread);
memcpy(new_buf.data(), stream_buf_.base + stream_buf_offset_, pending_len);
memcpy(new_buf.data() + pending_len, buf.data(), nread);

buf = std::move(new_buf);
nread = buf.size();
std::unique_ptr<BackingStore> new_bs;
{
NoArrayBufferZeroFillScope no_zero_fill_scope(env()->isolate_data());
new_bs = ArrayBuffer::NewBackingStore(env()->isolate(),
pending_len + nread);
}
memcpy(static_cast<char*>(new_bs->Data()),
stream_buf_.base + stream_buf_offset_,
pending_len);
memcpy(static_cast<char*>(new_bs->Data()) + pending_len,
bs->Data(),
nread);

bs = std::move(new_bs);
nread = bs->ByteLength();
stream_buf_offset_ = 0;
stream_buf_ab_.Reset();

Expand All @@ -1873,12 +1894,13 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) {

// Remember the current buffer, so that OnDataChunkReceived knows the
// offset of a DATA frame's data into the socket read buffer.
stream_buf_ = uv_buf_init(buf.data(), static_cast<unsigned int>(nread));
stream_buf_ = uv_buf_init(static_cast<char*>(bs->Data()),
static_cast<unsigned int>(nread));

// Store this so we can create an ArrayBuffer for read data from it.
// DATA frames will be emitted as slices of that ArrayBuffer to avoid having
// to copy memory.
stream_buf_allocation_ = std::move(buf);
stream_buf_allocation_ = std::move(bs);

ConsumeHTTP2Data();

Expand Down Expand Up @@ -2023,7 +2045,7 @@ void Http2Stream::Close(int32_t code) {
Debug(this, "closed with code %d", code);
}

ShutdownWrap* Http2Stream::CreateShutdownWrap(v8::Local<v8::Object> object) {
ShutdownWrap* Http2Stream::CreateShutdownWrap(Local<Object> object) {
// DoShutdown() always finishes synchronously, so there's no need to create
// a structure to store asynchronous context.
return nullptr;
Expand Down Expand Up @@ -3049,7 +3071,7 @@ void Http2Ping::Done(bool ack, const uint8_t* payload) {
}

Local<Value> argv[] = {
ack ? v8::True(isolate) : v8::False(isolate),
ack ? True(isolate) : False(isolate),
Number::New(isolate, duration_ms),
buf
};
Expand Down
7 changes: 3 additions & 4 deletions src/node_http2.h
View file
Open in desktop
Expand Up @@ -8,7 +8,6 @@
#include "nghttp2/nghttp2.h"

#include "env.h"
#include "allocated_buffer.h"
#include "aliased_struct.h"
#include "node_http2_state.h"
#include "node_http_common.h"
Expand Down Expand Up @@ -897,7 +896,7 @@ class Http2Session : public AsyncWrap,
// When processing input data, either stream_buf_ab_ or stream_buf_allocation_
// will be set. stream_buf_ab_ is lazily created from stream_buf_allocation_.
v8::Global<v8::ArrayBuffer> stream_buf_ab_;
AllocatedBuffer stream_buf_allocation_;
std::unique_ptr<v8::BackingStore> stream_buf_allocation_;
size_t stream_buf_offset_ = 0;
// Custom error code for errors that originated inside one of the callbacks
// called by nghttp2_session_mem_recv.
Expand Down Expand Up @@ -1040,7 +1039,7 @@ class Origins {
~Origins() = default;

const nghttp2_origin_entry* operator*() const {
return reinterpret_cast<const nghttp2_origin_entry*>(buf_.data());
return static_cast<const nghttp2_origin_entry*>(bs_->Data());
}

size_t length() const {
Expand All @@ -1049,7 +1048,7 @@ class Origins {

private:
size_t count_;
AllocatedBuffer buf_;
std::unique_ptr<v8::BackingStore> bs_;
};

#define HTTP2_HIDDEN_CONSTANTS(V) \
Expand Down