Skip to content

Commit

Permalink
http2: remove redundant write indirection
Browse files Browse the repository at this point in the history
`nghttp2_stream_write_t` was not a necessary redirection layer
and came with the cost of one additional allocation per stream write.

Also, having both `nghttp2_stream_write` and `nghttp2_stream_write_t`
as identifiers did not help with readability.

    $ ./node benchmark/compare.js --new ./node --old ./node-before --runs 100 --filter write --set streams=20 --set length=1024 --set size=16 http2 | Rscript benchmark/compare.R
    [00:26:09|% 100| 1/1 files | 200/200 runs | 1/1 configs]: Done
                                                                         improvement confidence     p.value
     http2/write.js benchmarker="h2load" size=16 length=1024 streams=20      0.58 %         ** 0.005535241
  • Loading branch information
addaleax committed Dec 18, 2017
1 parent e467286 commit beb929a
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 57 deletions.
54 changes: 18 additions & 36 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1369,30 +1369,6 @@ inline void Http2Stream::AddChunk(const uint8_t* data, size_t len) {
data_chunks_.emplace(uv_buf_init(buf, len));
}

// The Http2Stream class is a subclass of StreamBase. The DoWrite method
// receives outbound chunks of data to send as outbound DATA frames. These
// are queued in an internal linked list of uv_buf_t structs that are sent
// when nghttp2 is ready to serialize the data frame.
int Http2Stream::DoWrite(WriteWrap* req_wrap,
uv_buf_t* bufs,
size_t count,
uv_stream_t* send_handle) {
CHECK(!this->IsDestroyed());
session_->SetChunksSinceLastWrite();

nghttp2_stream_write_t* req = new nghttp2_stream_write_t;
req->data = req_wrap;

auto AfterWrite = [](nghttp2_stream_write_t* req, int status) {
WriteWrap* wrap = static_cast<WriteWrap*>(req->data);
wrap->Done(status);
delete req;
};
req_wrap->Dispatched();
Write(req, bufs, count, AfterWrite);
return 0;
}


inline void Http2Stream::Close(int32_t code) {
CHECK(!this->IsDestroyed());
Expand Down Expand Up @@ -1447,7 +1423,7 @@ inline void Http2Stream::Destroy() {
// we still have qeueued outbound writes.
while (!stream->queue_.empty()) {
nghttp2_stream_write* head = stream->queue_.front();
head->cb(head->req, UV_ECANCELED);
head->req_wrap->Done(UV_ECANCELED);
delete head;
stream->queue_.pop();
}
Expand Down Expand Up @@ -1616,26 +1592,32 @@ inline int Http2Stream::ReadStop() {
return 0;
}

// The Http2Stream class is a subclass of StreamBase. The DoWrite method
// receives outbound chunks of data to send as outbound DATA frames. These
// are queued in an internal linked list of uv_buf_t structs that are sent
// when nghttp2 is ready to serialize the data frame.
//
// Queue the given set of uv_but_t handles for writing to an
// nghttp2_stream. The callback will be invoked once the chunks
// of data have been flushed to the underlying nghttp2_session.
// nghttp2_stream. The WriteWrap's Done callback will be invoked once the
// chunks of data have been flushed to the underlying nghttp2_session.
// Note that this does *not* mean that the data has been flushed
// to the socket yet.
inline int Http2Stream::Write(nghttp2_stream_write_t* req,
const uv_buf_t bufs[],
unsigned int nbufs,
nghttp2_stream_write_cb cb) {
inline int Http2Stream::DoWrite(WriteWrap* req_wrap,
uv_buf_t* bufs,
size_t nbufs,
uv_stream_t* send_handle) {
CHECK(!this->IsDestroyed());
CHECK_EQ(send_handle, nullptr);
Http2Scope h2scope(this);
session_->SetChunksSinceLastWrite();
req_wrap->Dispatched();
if (!IsWritable()) {
if (cb != nullptr)
cb(req, UV_EOF);
req_wrap->Done(UV_EOF);
return 0;
}
DEBUG_HTTP2STREAM2(this, "queuing %d buffers to send", id_, nbufs);
nghttp2_stream_write* item = new nghttp2_stream_write;
item->cb = cb;
item->req = req;
item->req_wrap = req_wrap;
item->nbufs = nbufs;
item->bufs.AllocateSufficientStorage(nbufs);
memcpy(*(item->bufs), bufs, nbufs * sizeof(*bufs));
Expand Down Expand Up @@ -1824,7 +1806,7 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle,
stream->queue_offset_ = 0;
}
if (stream->queue_index_ == head->nbufs) {
head->cb(head->req, 0);
head->req_wrap->Done(0);
delete head;
stream->queue_.pop();
stream->queue_offset_ = 0;
Expand Down
22 changes: 1 addition & 21 deletions src/node_http2.h
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,6 @@ void inline debug_vfprintf(const char* format, ...) {

#define MAX_BUFFER_COUNT 16

struct nghttp2_stream_write_t;

enum nghttp2_session_type {
NGHTTP2_SESSION_SERVER,
NGHTTP2_SESSION_CLIENT
Expand Down Expand Up @@ -127,15 +125,9 @@ enum nghttp2_stream_options {
STREAM_OPTION_GET_TRAILERS = 0x2,
};

// Callbacks
typedef void (*nghttp2_stream_write_cb)(
nghttp2_stream_write_t* req,
int status);

struct nghttp2_stream_write {
unsigned int nbufs = 0;
nghttp2_stream_write_t* req = nullptr;
nghttp2_stream_write_cb cb = nullptr;
WriteWrap* req_wrap = nullptr;
MaybeStackBuffer<uv_buf_t, MAX_BUFFER_COUNT> bufs;
};

Expand All @@ -146,11 +138,6 @@ struct nghttp2_header {
};


struct nghttp2_stream_write_t {
void* data;
int status;
};

// Unlike the HTTP/1 implementation, the HTTP/2 implementation is not limited
// to a fixed number of known supported HTTP methods. These constants, therefore
// are provided strictly as a convenience to users and are exposed via the
Expand Down Expand Up @@ -558,13 +545,6 @@ class Http2Stream : public AsyncWrap,

Http2Session* session() { return session_; }

// Queue outbound chunks of data to be sent on this stream
inline int Write(
nghttp2_stream_write_t* req,
const uv_buf_t bufs[],
unsigned int nbufs,
nghttp2_stream_write_cb cb);

inline bool HasDataChunks(bool ignore_eos = false);

inline void AddChunk(const uint8_t* data, size_t len);
Expand Down

0 comments on commit beb929a

Please sign in to comment.