Skip to content

Commit

Permalink
src: remove usage of AllocatedBuffer from node_http2
Browse files Browse the repository at this point in the history 
Signed-off-by: Darshan Sen <darshan.sen@postman.com>

PR-URL: #40584
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
  • Loading branch information
@RaisinTen @BethGriggs
RaisinTen authored and BethGriggs committed Nov 25, 2021
1 parent f778fa2 commit 2c8a6ec
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 39 deletions.
92 changes: 57 additions & 35 deletions src/node_http2.cc
View file
Original file line number Diff line number Diff line change
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()),
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
Original file line number Diff line number Diff line change
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

0 comments on commit 2c8a6ec

Please sign in to comment.