From 146fae66b1ff82765333b67a9f6ea4117ac311ab Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 29 Sep 2017 13:00:06 -0700 Subject: [PATCH 1/4] http2: simplify TypeName --- src/node_http2_core-inl.h | 54 +++++++++++++++++++-------------------- src/node_http2_core.h | 4 +-- 2 files changed, 28 insertions(+), 30 deletions(-) diff --git a/src/node_http2_core-inl.h b/src/node_http2_core-inl.h index 0d2a697a9d73e0..a3efbe119191f5 100644 --- a/src/node_http2_core-inl.h +++ b/src/node_http2_core-inl.h @@ -40,7 +40,7 @@ inline int Nghttp2Session::OnNghttpError(nghttp2_session* session, void* user_data) { Nghttp2Session* handle = static_cast(user_data); DEBUG_HTTP2("Nghttp2Session %s: Error '%.*s'\n", - handle->TypeName(handle->type()), len, message); + handle->TypeName(), len, message); return 0; } #endif @@ -58,7 +58,7 @@ inline int Nghttp2Session::OnBeginHeadersCallback(nghttp2_session* session, frame->push_promise.promised_stream_id : frame->hd.stream_id; DEBUG_HTTP2("Nghttp2Session %s: beginning headers for stream %d\n", - handle->TypeName(handle->type()), id); + handle->TypeName(), id); Nghttp2Stream* stream = handle->FindStream(id); if (stream == nullptr) { @@ -103,7 +103,7 @@ inline int Nghttp2Session::OnFrameReceive(nghttp2_session* session, void* user_data) { Nghttp2Session* handle = static_cast(user_data); DEBUG_HTTP2("Nghttp2Session %s: complete frame received: type: %d\n", - handle->TypeName(handle->type()), frame->hd.type); + handle->TypeName(), frame->hd.type); bool ack; switch (frame->hd.type) { case NGHTTP2_DATA: @@ -135,7 +135,7 @@ inline int Nghttp2Session::OnFrameNotSent(nghttp2_session *session, void *user_data) { Nghttp2Session *handle = static_cast(user_data); DEBUG_HTTP2("Nghttp2Session %s: frame type %d was not sent, code: %d\n", - handle->TypeName(handle->type()), frame->hd.type, error_code); + handle->TypeName(), frame->hd.type, error_code); // Do not report if the frame was not sent due to the session closing if (error_code != NGHTTP2_ERR_SESSION_CLOSING && error_code != NGHTTP2_ERR_STREAM_CLOSED && @@ -162,7 +162,7 @@ inline int Nghttp2Session::OnStreamClose(nghttp2_session *session, void *user_data) { Nghttp2Session *handle = static_cast(user_data); DEBUG_HTTP2("Nghttp2Session %s: stream %d closed, code: %d\n", - handle->TypeName(handle->type()), id, code); + handle->TypeName(), id, code); Nghttp2Stream *stream = handle->FindStream(id); // Intentionally ignore the callback if the stream does not exist if (stream != nullptr) @@ -182,7 +182,7 @@ inline ssize_t Nghttp2Session::OnStreamReadFD(nghttp2_session *session, void *user_data) { Nghttp2Session *handle = static_cast(user_data); DEBUG_HTTP2("Nghttp2Session %s: reading outbound file data for stream %d\n", - handle->TypeName(handle->type()), id); + handle->TypeName(), id); Nghttp2Stream *stream = handle->FindStream(id); int fd = source->fd; @@ -218,7 +218,7 @@ inline ssize_t Nghttp2Session::OnStreamReadFD(nghttp2_session *session, // if numchars < length, assume that we are done. if (static_cast(numchars) < length || length <= 0) { DEBUG_HTTP2("Nghttp2Session %s: no more data for stream %d\n", - handle->TypeName(handle->type()), id); + handle->TypeName(), id); *flags |= NGHTTP2_DATA_FLAG_EOF; GetTrailers(session, handle, stream, flags); } @@ -238,7 +238,7 @@ inline ssize_t Nghttp2Session::OnStreamRead(nghttp2_session *session, void *user_data) { Nghttp2Session *handle = static_cast(user_data); DEBUG_HTTP2("Nghttp2Session %s: reading outbound data for stream %d\n", - handle->TypeName(handle->type()), id); + handle->TypeName(), id); Nghttp2Stream *stream = handle->FindStream(id); size_t remaining = length; size_t offset = 0; @@ -248,7 +248,7 @@ inline ssize_t Nghttp2Session::OnStreamRead(nghttp2_session *session, // calls this callback. while (stream->queue_head_ != nullptr) { DEBUG_HTTP2("Nghttp2Session %s: processing outbound data chunk\n", - handle->TypeName(handle->type())); + handle->TypeName()); nghttp2_stream_write_queue *head = stream->queue_head_; while (stream->queue_head_index_ < head->nbufs) { if (remaining == 0) @@ -289,12 +289,12 @@ inline ssize_t Nghttp2Session::OnStreamRead(nghttp2_session *session, int writable = stream->queue_head_ != nullptr || stream->IsWritable(); if (offset == 0 && writable && stream->queue_head_ == nullptr) { DEBUG_HTTP2("Nghttp2Session %s: deferring stream %d\n", - handle->TypeName(handle->type()), id); + handle->TypeName(), id); return NGHTTP2_ERR_DEFERRED; } if (!writable) { DEBUG_HTTP2("Nghttp2Session %s: no more data for stream %d\n", - handle->TypeName(handle->type()), id); + handle->TypeName(), id); *flags |= NGHTTP2_DATA_FLAG_EOF; GetTrailers(session, handle, stream, flags); @@ -313,7 +313,7 @@ inline ssize_t Nghttp2Session::OnSelectPadding(nghttp2_session *session, CHECK(handle->HasGetPaddingCallback()); ssize_t padding = handle->GetPadding(frame->hd.length, maxPayloadLen); DEBUG_HTTP2("Nghttp2Session %s: using padding, size: %d\n", - handle->TypeName(handle->type()), padding); + handle->TypeName(), padding); return padding; } @@ -326,7 +326,7 @@ inline int Nghttp2Session::OnDataChunkReceived(nghttp2_session *session, void *user_data) { Nghttp2Session *handle = static_cast(user_data); DEBUG_HTTP2("Nghttp2Session %s: buffering data chunk for stream %d, size: " - "%d, flags: %d\n", handle->TypeName(handle->type()), + "%d, flags: %d\n", handle->TypeName(), id, len, flags); Nghttp2Stream *stream = handle->FindStream(id); nghttp2_data_chunk_t *chunk = data_chunk_free_list.pop(); @@ -361,7 +361,7 @@ inline void Nghttp2Session::SubmitTrailers::Submit(nghttp2_nv *trailers, if (length == 0) return; DEBUG_HTTP2("Nghttp2Session %s: sending trailers for stream %d, " - "count: %d\n", handle_->TypeName(handle_->type()), + "count: %d\n", handle_->TypeName(), stream_->id(), length); *flags_ |= NGHTTP2_DATA_FLAG_NO_END_STREAM; nghttp2_submit_trailer(handle_->session_, @@ -373,7 +373,7 @@ inline void Nghttp2Session::SubmitTrailers::Submit(nghttp2_nv *trailers, // See: https://nghttp2.org/documentation/nghttp2_submit_shutdown_notice.html inline void Nghttp2Session::SubmitShutdownNotice() { DEBUG_HTTP2("Nghttp2Session %s: submitting shutdown notice\n", - TypeName(type())); + TypeName()); nghttp2_submit_shutdown_notice(session_); } @@ -383,7 +383,7 @@ inline void Nghttp2Session::SubmitShutdownNotice() { inline int Nghttp2Session::SubmitSettings(const nghttp2_settings_entry iv[], size_t niv) { DEBUG_HTTP2("Nghttp2Session %s: submitting settings, count: %d\n", - TypeName(type()), niv); + TypeName(), niv); return nghttp2_submit_settings(session_, NGHTTP2_FLAG_NONE, iv, niv); } @@ -392,11 +392,10 @@ inline Nghttp2Stream* Nghttp2Session::FindStream(int32_t id) { auto s = streams_.find(id); if (s != streams_.end()) { DEBUG_HTTP2("Nghttp2Session %s: stream %d found\n", - TypeName(type()), id); + TypeName(), id); return s->second; } else { - DEBUG_HTTP2("Nghttp2Session %s: stream %d not found\n", - TypeName(type()), id); + DEBUG_HTTP2("Nghttp2Session %s: stream %d not found\n", TypeName(), id); return nullptr; } } @@ -421,7 +420,7 @@ inline void Nghttp2Stream::FlushDataChunks(bool done) { inline void Nghttp2Session::HandleDataFrame(const nghttp2_frame* frame) { int32_t id = frame->hd.stream_id; DEBUG_HTTP2("Nghttp2Session %s: handling data frame for stream %d\n", - TypeName(type()), id); + TypeName(), id); Nghttp2Stream* stream = this->FindStream(id); // If the stream does not exist, something really bad happened CHECK_NE(stream, nullptr); @@ -437,7 +436,7 @@ inline void Nghttp2Session::HandleHeadersFrame(const nghttp2_frame* frame) { int32_t id = (frame->hd.type == NGHTTP2_PUSH_PROMISE) ? frame->push_promise.promised_stream_id : frame->hd.stream_id; DEBUG_HTTP2("Nghttp2Session %s: handling headers frame for stream %d\n", - TypeName(type()), id); + TypeName(), id); Nghttp2Stream* stream = FindStream(id); // If the stream does not exist, something really bad happened CHECK_NE(stream, nullptr); @@ -453,7 +452,7 @@ inline void Nghttp2Session::HandlePriorityFrame(const nghttp2_frame* frame) { nghttp2_priority priority_frame = frame->priority; int32_t id = frame->hd.stream_id; DEBUG_HTTP2("Nghttp2Session %s: handling priority frame for stream %d\n", - TypeName(type()), id); + TypeName(), id); // Ignore the priority frame if stream ID is <= 0 // This actually should never happen because nghttp2 should treat this as // an error condition that terminates the session. @@ -466,7 +465,7 @@ inline void Nghttp2Session::HandlePriorityFrame(const nghttp2_frame* frame) { // Notifies the JS layer that a GOAWAY frame has been received inline void Nghttp2Session::HandleGoawayFrame(const nghttp2_frame* frame) { nghttp2_goaway goaway_frame = frame->goaway; - DEBUG_HTTP2("Nghttp2Session %s: handling goaway frame\n", TypeName(type())); + DEBUG_HTTP2("Nghttp2Session %s: handling goaway frame\n", TypeName()); OnGoAway(goaway_frame.last_stream_id, goaway_frame.error_code, @@ -476,7 +475,7 @@ inline void Nghttp2Session::HandleGoawayFrame(const nghttp2_frame* frame) { // Prompts nghttp2 to flush the queue of pending data frames inline void Nghttp2Session::SendPendingData() { - DEBUG_HTTP2("Nghttp2Session %s: Sending pending data\n", TypeName(type())); + DEBUG_HTTP2("Nghttp2Session %s: Sending pending data\n", TypeName()); // Do not attempt to send data on the socket if the destroying flag has // been set. That means everything is shutting down and the socket // will not be usable. @@ -510,10 +509,9 @@ inline int Nghttp2Session::Init(uv_loop_t* loop, const nghttp2_session_type type, nghttp2_option* options, nghttp2_mem* mem) { - DEBUG_HTTP2("Nghttp2Session %s: initializing session\n", - TypeName(type)); loop_ = loop; session_type_ = type; + DEBUG_HTTP2("Nghttp2Session %s: initializing session\n", TypeName()); destroying_ = false; int ret = 0; @@ -565,7 +563,7 @@ inline void Nghttp2Session::MarkDestroying() { inline int Nghttp2Session::Free() { CHECK(session_ != nullptr); - DEBUG_HTTP2("Nghttp2Session %s: freeing session\n", TypeName(type())); + DEBUG_HTTP2("Nghttp2Session %s: freeing session\n", TypeName()); // Stop the loop uv_prepare_stop(&prep_); auto PrepClose = [](uv_handle_t* handle) { @@ -579,7 +577,7 @@ inline int Nghttp2Session::Free() { nghttp2_session_del(session_); session_ = nullptr; loop_ = nullptr; - DEBUG_HTTP2("Nghttp2Session %s: session freed\n", TypeName(type())); + DEBUG_HTTP2("Nghttp2Session %s: session freed\n", TypeName()); return 1; } diff --git a/src/node_http2_core.h b/src/node_http2_core.h index 69b9891d5871cb..32f73648a3fc00 100644 --- a/src/node_http2_core.h +++ b/src/node_http2_core.h @@ -105,8 +105,8 @@ class Nghttp2Session { return destroying_; } - inline const char* TypeName(nghttp2_session_type type) { - switch (type) { + inline const char* TypeName() { + switch (session_type_) { case NGHTTP2_SESSION_SERVER: return "server"; case NGHTTP2_SESSION_CLIENT: return "client"; default: From 58327664d63d7022a537eb02d628270235a0e7ee Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 29 Sep 2017 14:14:27 -0700 Subject: [PATCH 2/4] http2: refactor method arguments to avoid bools --- lib/internal/http2/core.js | 67 +++++++++++++++++++++----------------- src/node_http2.cc | 32 +++++++++--------- src/node_http2_core-inl.h | 34 +++++++++---------- src/node_http2_core.h | 18 +++++----- 4 files changed, 80 insertions(+), 71 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index bc41cd4bf913c0..31252556847725 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -126,7 +126,10 @@ const { HTTP_STATUS_OK, HTTP_STATUS_NO_CONTENT, HTTP_STATUS_NOT_MODIFIED, - HTTP_STATUS_SWITCHING_PROTOCOLS + HTTP_STATUS_SWITCHING_PROTOCOLS, + + STREAM_OPTION_EMPTY_PAYLOAD, + STREAM_OPTION_GET_TRAILERS } = constants; // Top level to avoid creating a closure @@ -412,20 +415,22 @@ function requestOnConnect(headers, options) { return; } - let getTrailers = false; + let streamOptions = 0; + if (options.endStream) + streamOptions |= STREAM_OPTION_EMPTY_PAYLOAD; + if (typeof options.getTrailers === 'function') { - getTrailers = true; + streamOptions |= STREAM_OPTION_GET_TRAILERS; this[kState].getTrailers = options.getTrailers; } // ret will be either the reserved stream ID (if positive) // or an error code (if negative) const ret = handle.submitRequest(headersList, - !!options.endStream, + streamOptions, options.parent | 0, options.weight | 0, - !!options.exclusive, - getTrailers); + !!options.exclusive); // In an error condition, one of three possible response codes will be // possible: @@ -1563,7 +1568,7 @@ function processHeaders(headers) { } function processRespondWithFD(fd, headers, offset = 0, length = -1, - getTrailers = false) { + streamOptions = 0) { const session = this[kSession]; const state = this[kState]; state.headersSent = true; @@ -1573,7 +1578,7 @@ function processRespondWithFD(fd, headers, offset = 0, length = -1, const handle = session[kHandle]; const ret = - handle.submitFile(this[kID], fd, headers, offset, length, getTrailers); + handle.submitFile(this[kID], fd, headers, offset, length, streamOptions); let err; switch (ret) { case NGHTTP2_ERR_NOMEM: @@ -1588,7 +1593,7 @@ function processRespondWithFD(fd, headers, offset = 0, length = -1, } } -function doSendFD(session, options, fd, headers, getTrailers, err, stat) { +function doSendFD(session, options, fd, headers, streamOptions, err, stat) { if (this.destroyed || session.destroyed) { abort(this); return; @@ -1617,10 +1622,10 @@ function doSendFD(session, options, fd, headers, getTrailers, err, stat) { processRespondWithFD.call(this, fd, headersList, statOptions.offset, statOptions.length, - getTrailers); + streamOptions); } -function doSendFileFD(session, options, fd, headers, getTrailers, err, stat) { +function doSendFileFD(session, options, fd, headers, streamOptions, err, stat) { if (this.destroyed || session.destroyed) { abort(this); return; @@ -1674,10 +1679,10 @@ function doSendFileFD(session, options, fd, headers, getTrailers, err, stat) { processRespondWithFD.call(this, fd, headersList, options.offset, options.length, - getTrailers); + streamOptions); } -function afterOpen(session, options, headers, getTrailers, err, fd) { +function afterOpen(session, options, headers, streamOptions, err, fd) { const state = this[kState]; const onError = options.onError; if (this.destroyed || session.destroyed) { @@ -1695,7 +1700,8 @@ function afterOpen(session, options, headers, getTrailers, err, fd) { state.fd = fd; fs.fstat(fd, - doSendFileFD.bind(this, session, options, fd, headers, getTrailers)); + doSendFileFD.bind(this, session, options, fd, + headers, streamOptions)); } function streamOnError(err) { @@ -1779,9 +1785,9 @@ class ServerHttp2Stream extends Http2Stream { throw headersList; } - const ret = handle.submitPushPromise(this[kID], - headersList, - options.endStream); + const streamOptions = options.endStream ? STREAM_OPTION_EMPTY_PAYLOAD : 0; + + const ret = handle.submitPushPromise(this[kID], headersList, streamOptions); let err; switch (ret) { case NGHTTP2_ERR_NOMEM: @@ -1837,14 +1843,17 @@ class ServerHttp2Stream extends Http2Stream { options = Object.assign(Object.create(null), options); options.endStream = !!options.endStream; - let getTrailers = false; + let streamOptions = 0; + if (options.endStream) + streamOptions |= STREAM_OPTION_EMPTY_PAYLOAD; + if (options.getTrailers !== undefined) { if (typeof options.getTrailers !== 'function') { throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'getTrailers', options.getTrailers); } - getTrailers = true; + streamOptions |= STREAM_OPTION_GET_TRAILERS; state.getTrailers = options.getTrailers; } @@ -1874,10 +1883,7 @@ class ServerHttp2Stream extends Http2Stream { const handle = session[kHandle]; const ret = - handle.submitResponse(this[kID], - headersList, - options.endStream, - getTrailers); + handle.submitResponse(this[kID], headersList, streamOptions); let err; switch (ret) { case NGHTTP2_ERR_NOMEM: @@ -1930,14 +1936,14 @@ class ServerHttp2Stream extends Http2Stream { options.statCheck); } - let getTrailers = false; + let streamOptions = 0; if (options.getTrailers !== undefined) { if (typeof options.getTrailers !== 'function') { throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'getTrailers', options.getTrailers); } - getTrailers = true; + streamOptions |= STREAM_OPTION_GET_TRAILERS; state.getTrailers = options.getTrailers; } @@ -1956,7 +1962,8 @@ class ServerHttp2Stream extends Http2Stream { if (options.statCheck !== undefined) { fs.fstat(fd, - doSendFD.bind(this, session, options, fd, headers, getTrailers)); + doSendFD.bind(this, session, options, fd, + headers, streamOptions)); return; } @@ -1969,7 +1976,7 @@ class ServerHttp2Stream extends Http2Stream { processRespondWithFD.call(this, fd, headersList, options.offset, options.length, - getTrailers); + streamOptions); } // Initiate a file response on this Http2Stream. The path is passed to @@ -2011,14 +2018,14 @@ class ServerHttp2Stream extends Http2Stream { options.statCheck); } - let getTrailers = false; + let streamOptions = 0; if (options.getTrailers !== undefined) { if (typeof options.getTrailers !== 'function') { throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'getTrailers', options.getTrailers); } - getTrailers = true; + streamOptions |= STREAM_OPTION_GET_TRAILERS; state.getTrailers = options.getTrailers; } @@ -2032,7 +2039,7 @@ class ServerHttp2Stream extends Http2Stream { } fs.open(path, 'r', - afterOpen.bind(this, session, options, headers, getTrailers)); + afterOpen.bind(this, session, options, headers, streamOptions)); } // Sends a block of informational headers. In theory, the HTTP/2 spec diff --git a/src/node_http2.cc b/src/node_http2.cc index c1e178f7175935..8f666feaf0955f 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -479,7 +479,7 @@ void Http2Session::SubmitRstStream(const FunctionCallbackInfo& args) { void Http2Session::SubmitRequest(const FunctionCallbackInfo& args) { // args[0] Array of headers - // args[1] endStream boolean + // args[1] options int // args[2] parentStream ID (for priority spec) // args[3] weight (for priority spec) // args[4] exclusive boolean (for priority spec) @@ -492,15 +492,14 @@ void Http2Session::SubmitRequest(const FunctionCallbackInfo& args) { Isolate* isolate = env->isolate(); Local headers = args[0].As(); - bool endStream = args[1]->BooleanValue(context).ToChecked(); + int options = args[1]->IntegerValue(context).ToChecked(); int32_t parent = args[2]->Int32Value(context).ToChecked(); int32_t weight = args[3]->Int32Value(context).ToChecked(); bool exclusive = args[4]->BooleanValue(context).ToChecked(); - bool getTrailers = args[5]->BooleanValue(context).ToChecked(); - DEBUG_HTTP2("Http2Session: submitting request: headers: %d, end-stream: %d, " + DEBUG_HTTP2("Http2Session: submitting request: headers: %d, options: %d, " "parent: %d, weight: %d, exclusive: %d\n", headers->Length(), - endStream, parent, weight, exclusive); + options, parent, weight, exclusive); nghttp2_priority_spec prispec; nghttp2_priority_spec_init(&prispec, parent, weight, exclusive ? 1 : 0); @@ -509,8 +508,7 @@ void Http2Session::SubmitRequest(const FunctionCallbackInfo& args) { int32_t ret = session->Nghttp2Session::SubmitRequest(&prispec, *list, list.length(), - nullptr, endStream, - getTrailers); + nullptr, options); DEBUG_HTTP2("Http2Session: request submitted, response: %d\n", ret); args.GetReturnValue().Set(ret); } @@ -529,11 +527,10 @@ void Http2Session::SubmitResponse(const FunctionCallbackInfo& args) { int32_t id = args[0]->Int32Value(context).ToChecked(); Local headers = args[1].As(); - bool endStream = args[2]->BooleanValue(context).ToChecked(); - bool getTrailers = args[3]->BooleanValue(context).ToChecked(); + int options = args[2]->IntegerValue(context).ToChecked(); DEBUG_HTTP2("Http2Session: submitting response for stream %d: headers: %d, " - "end-stream: %d\n", id, headers->Length(), endStream); + "options: %d\n", id, headers->Length(), options); if (!(stream = session->FindStream(id))) { return args.GetReturnValue().Set(NGHTTP2_ERR_INVALID_STREAM_ID); @@ -542,7 +539,7 @@ void Http2Session::SubmitResponse(const FunctionCallbackInfo& args) { Headers list(isolate, context, headers); args.GetReturnValue().Set( - stream->SubmitResponse(*list, list.length(), endStream, getTrailers)); + stream->SubmitResponse(*list, list.length(), options)); } void Http2Session::SubmitFile(const FunctionCallbackInfo& args) { @@ -566,7 +563,7 @@ void Http2Session::SubmitFile(const FunctionCallbackInfo& args) { int64_t offset = args[3]->IntegerValue(context).ToChecked(); int64_t length = args[4]->IntegerValue(context).ToChecked(); - bool getTrailers = args[5]->BooleanValue(context).ToChecked(); + int options = args[5]->IntegerValue(context).ToChecked(); CHECK_GE(offset, 0); @@ -580,7 +577,7 @@ void Http2Session::SubmitFile(const FunctionCallbackInfo& args) { Headers list(isolate, context, headers); args.GetReturnValue().Set(stream->SubmitFile(fd, *list, list.length(), - offset, length, getTrailers)); + offset, length, options)); } void Http2Session::SendHeaders(const FunctionCallbackInfo& args) { @@ -719,10 +716,10 @@ void Http2Session::SubmitPushPromise(const FunctionCallbackInfo& args) { Nghttp2Stream* parent; int32_t id = args[0]->Int32Value(context).ToChecked(); Local headers = args[1].As(); - bool endStream = args[2]->BooleanValue(context).ToChecked(); + int options = args[2]->IntegerValue(context).ToChecked(); DEBUG_HTTP2("Http2Session: submitting push promise for stream %d: " - "end-stream: %d, headers: %d\n", id, endStream, + "options: %d, headers: %d\n", id, options, headers->Length()); if (!(parent = session->FindStream(id))) { @@ -732,7 +729,7 @@ void Http2Session::SubmitPushPromise(const FunctionCallbackInfo& args) { Headers list(isolate, context, headers); int32_t ret = parent->SubmitPushPromise(*list, list.length(), - nullptr, endStream); + nullptr, options); DEBUG_HTTP2("Http2Session: push promise submitted, ret: %d\n", ret); args.GetReturnValue().Set(ret); } @@ -1250,6 +1247,9 @@ void Initialize(Local target, NODE_DEFINE_HIDDEN_CONSTANT(constants, NGHTTP2_ERR_STREAM_CLOSED); NODE_DEFINE_CONSTANT(constants, NGHTTP2_ERR_FRAME_SIZE_ERROR); + NODE_DEFINE_HIDDEN_CONSTANT(constants, STREAM_OPTION_EMPTY_PAYLOAD); + NODE_DEFINE_HIDDEN_CONSTANT(constants, STREAM_OPTION_GET_TRAILERS); + NODE_DEFINE_CONSTANT(constants, NGHTTP2_FLAG_NONE); NODE_DEFINE_CONSTANT(constants, NGHTTP2_FLAG_END_STREAM); NODE_DEFINE_CONSTANT(constants, NGHTTP2_FLAG_END_HEADERS); diff --git a/src/node_http2_core-inl.h b/src/node_http2_core-inl.h index a3efbe119191f5..29f5b063801d10 100644 --- a/src/node_http2_core-inl.h +++ b/src/node_http2_core-inl.h @@ -614,10 +614,10 @@ inline Nghttp2Stream* Nghttp2Stream::Init( int32_t id, Nghttp2Session* session, nghttp2_headers_category category, - bool getTrailers) { + int options) { DEBUG_HTTP2("Nghttp2Stream %d: initializing stream\n", id); Nghttp2Stream* stream = stream_free_list.pop(); - stream->ResetState(id, session, category, getTrailers); + stream->ResetState(id, session, category, options); session->AddStream(stream); return stream; } @@ -628,7 +628,7 @@ inline void Nghttp2Stream::ResetState( int32_t id, Nghttp2Session* session, nghttp2_headers_category category, - bool getTrailers) { + int options) { DEBUG_HTTP2("Nghttp2Stream %d: resetting stream state\n", id); session_ = session; queue_head_ = nullptr; @@ -644,7 +644,7 @@ inline void Nghttp2Stream::ResetState( prev_local_window_size_ = 65535; queue_head_index_ = 0; queue_head_offset_ = 0; - getTrailers_ = getTrailers; + getTrailers_ = options & STREAM_OPTION_GET_TRAILERS; } @@ -735,7 +735,7 @@ inline int32_t Nghttp2Stream::SubmitPushPromise( nghttp2_nv* nva, size_t len, Nghttp2Stream** assigned, - bool emptyPayload) { + int options) { CHECK_GT(len, 0); DEBUG_HTTP2("Nghttp2Stream %d: sending push promise\n", id_); int32_t ret = nghttp2_submit_push_promise(session_->session(), @@ -744,7 +744,8 @@ inline int32_t Nghttp2Stream::SubmitPushPromise( nullptr); if (ret > 0) { auto stream = Nghttp2Stream::Init(ret, session_); - if (emptyPayload) stream->Shutdown(); + if (options & STREAM_OPTION_EMPTY_PAYLOAD) + stream->Shutdown(); if (assigned != nullptr) *assigned = stream; } return ret; @@ -756,16 +757,15 @@ inline int32_t Nghttp2Stream::SubmitPushPromise( // be sent. inline int Nghttp2Stream::SubmitResponse(nghttp2_nv* nva, size_t len, - bool emptyPayload, - bool getTrailers) { + int options) { CHECK_GT(len, 0); DEBUG_HTTP2("Nghttp2Stream %d: submitting response\n", id_); - getTrailers_ = getTrailers; + getTrailers_ = options & STREAM_OPTION_GET_TRAILERS; nghttp2_data_provider* provider = nullptr; nghttp2_data_provider prov; prov.source.ptr = this; prov.read_callback = Nghttp2Session::OnStreamRead; - if (!emptyPayload && IsWritable()) + if (IsWritable() && !(options & STREAM_OPTION_EMPTY_PAYLOAD)) provider = &prov; return nghttp2_submit_response(session_->session(), id_, @@ -777,11 +777,11 @@ inline int Nghttp2Stream::SubmitFile(int fd, nghttp2_nv* nva, size_t len, int64_t offset, int64_t length, - bool getTrailers) { + int options) { CHECK_GT(len, 0); CHECK_GT(fd, 0); DEBUG_HTTP2("Nghttp2Stream %d: submitting file\n", id_); - getTrailers_ = getTrailers; + getTrailers_ = options & STREAM_OPTION_GET_TRAILERS; nghttp2_data_provider prov; prov.source.ptr = this; prov.source.fd = fd; @@ -802,15 +802,14 @@ inline int32_t Nghttp2Session::SubmitRequest( nghttp2_nv* nva, size_t len, Nghttp2Stream** assigned, - bool emptyPayload, - bool getTrailers) { + int options) { CHECK_GT(len, 0); DEBUG_HTTP2("Nghttp2Session: submitting request\n"); nghttp2_data_provider* provider = nullptr; nghttp2_data_provider prov; prov.source.ptr = this; prov.read_callback = OnStreamRead; - if (!emptyPayload) + if (!(options & STREAM_OPTION_EMPTY_PAYLOAD)) provider = &prov; int32_t ret = nghttp2_submit_request(session_, prispec, nva, len, @@ -819,8 +818,9 @@ inline int32_t Nghttp2Session::SubmitRequest( if (ret > 0) { Nghttp2Stream* stream = Nghttp2Stream::Init(ret, this, NGHTTP2_HCAT_HEADERS, - getTrailers); - if (emptyPayload) stream->Shutdown(); + options); + if (options & STREAM_OPTION_EMPTY_PAYLOAD) + stream->Shutdown(); if (assigned != nullptr) *assigned = stream; } return ret; diff --git a/src/node_http2_core.h b/src/node_http2_core.h index 32f73648a3fc00..ff2321d0cb68a2 100644 --- a/src/node_http2_core.h +++ b/src/node_http2_core.h @@ -68,6 +68,10 @@ enum nghttp2_stream_flags { NGHTTP2_STREAM_FLAG_DESTROYED = 0x10 }; +enum nghttp2_stream_options { + STREAM_OPTION_EMPTY_PAYLOAD = 0x1, + STREAM_OPTION_GET_TRAILERS = 0x2, +}; // Callbacks typedef void (*nghttp2_stream_write_cb)( @@ -127,8 +131,7 @@ class Nghttp2Session { nghttp2_nv* nva, size_t len, Nghttp2Stream** assigned = nullptr, - bool emptyPayload = true, - bool getTrailers = false); + int options = 0); // Submits a notice to the connected peer that the session is in the // process of shutting down. @@ -299,7 +302,7 @@ class Nghttp2Stream { int32_t id, Nghttp2Session* session, nghttp2_headers_category category = NGHTTP2_HCAT_HEADERS, - bool getTrailers = false); + int options = 0); inline ~Nghttp2Stream() { CHECK_EQ(session_, nullptr); @@ -319,7 +322,7 @@ class Nghttp2Stream { int32_t id, Nghttp2Session* session, nghttp2_headers_category category = NGHTTP2_HCAT_HEADERS, - bool getTrailers = false); + int options = 0); // Destroy this stream instance and free all held memory. // Note that this will free queued outbound and inbound @@ -347,15 +350,14 @@ class Nghttp2Stream { // Initiate a response on this stream. inline int SubmitResponse(nghttp2_nv* nva, size_t len, - bool emptyPayload = false, - bool getTrailers = false); + int options); // Send data read from a file descriptor as the response on this stream. inline int SubmitFile(int fd, nghttp2_nv* nva, size_t len, int64_t offset, int64_t length, - bool getTrailers = false); + int options); // Submit informational headers for this stream inline int SubmitInfo(nghttp2_nv* nva, size_t len); @@ -372,7 +374,7 @@ class Nghttp2Stream { nghttp2_nv* nva, size_t len, Nghttp2Stream** assigned = nullptr, - bool writable = true); + int options = 0); // Marks the Writable side of the stream as being shutdown inline void Shutdown() { From 954058586a7fe17205cfa658513deed67a00d3d3 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 29 Sep 2017 15:06:45 -0700 Subject: [PATCH 3/4] http2: eliminate dead code --- src/node_http2.cc | 3 --- src/node_http2_core-inl.h | 9 --------- src/node_http2_core.h | 8 -------- 3 files changed, 20 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 8f666feaf0955f..6175ad5a7f30e6 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -25,9 +25,6 @@ Freelist stream_free_list; Freelist header_free_list; -Freelist - data_chunks_free_list; - Nghttp2Session::Callbacks Nghttp2Session::callback_struct_saved[2] = { Callbacks(false), Callbacks(true)}; diff --git a/src/node_http2_core-inl.h b/src/node_http2_core-inl.h index 29f5b063801d10..15fb3d4f83494c 100644 --- a/src/node_http2_core-inl.h +++ b/src/node_http2_core-inl.h @@ -30,9 +30,6 @@ extern Freelist stream_free_list; extern Freelist header_free_list; -extern Freelist - data_chunks_free_list; - #ifdef NODE_DEBUG_HTTP2 inline int Nghttp2Session::OnNghttpError(nghttp2_session* session, const char* message, @@ -905,12 +902,6 @@ inline void Nghttp2Stream::ReadStop() { id_, 0); } -nghttp2_data_chunks_t::~nghttp2_data_chunks_t() { - for (unsigned int n = 0; n < nbufs; n++) { - free(buf[n].base); - } -} - Nghttp2Session::Callbacks::Callbacks(bool kHasGetPaddingCallback) { nghttp2_session_callbacks_new(&callbacks); nghttp2_session_callbacks_set_on_begin_headers_callback( diff --git a/src/node_http2_core.h b/src/node_http2_core.h index ff2321d0cb68a2..1a8e9a9f57b13d 100644 --- a/src/node_http2_core.h +++ b/src/node_http2_core.h @@ -40,7 +40,6 @@ class Nghttp2Stream; struct nghttp2_stream_write_t; struct nghttp2_data_chunk_t; -struct nghttp2_data_chunks_t; #define MAX_BUFFER_COUNT 10 #define SEND_BUFFER_RECOMMENDED_SIZE 4096 @@ -508,13 +507,6 @@ struct nghttp2_data_chunk_t { nghttp2_data_chunk_t* next = nullptr; }; -struct nghttp2_data_chunks_t { - unsigned int nbufs = 0; - uv_buf_t buf[MAX_BUFFER_COUNT]; - - inline ~nghttp2_data_chunks_t(); -}; - } // namespace http2 } // namespace node From 0b943c6554006b263ea41d4687459d20769cb0a0 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 29 Sep 2017 16:00:51 -0700 Subject: [PATCH 4/4] http2: making sending to the socket more efficient --- benchmark/http2/write.js | 14 +++++++-- src/node_http2_core-inl.h | 63 ++++++++++++++++++++++++++++----------- 2 files changed, 58 insertions(+), 19 deletions(-) diff --git a/benchmark/http2/write.js b/benchmark/http2/write.js index bee1c1b351632f..e4f64cc05dedb3 100644 --- a/benchmark/http2/write.js +++ b/benchmark/http2/write.js @@ -6,17 +6,27 @@ const PORT = common.PORT; const bench = common.createBenchmark(main, { streams: [100, 200, 1000], length: [64 * 1024, 128 * 1024, 256 * 1024, 1024 * 1024], + size: [100000] }, { flags: ['--no-warnings'] }); function main(conf) { const m = +conf.streams; const l = +conf.length; + const s = +conf.size; const http2 = require('http2'); const server = http2.createServer(); server.on('stream', (stream) => { stream.respond(); - stream.write('ü'.repeat(l)); - stream.end(); + let written = 0; + function write() { + stream.write('ü'.repeat(s)); + written += s; + if (written < l) + setImmediate(write); + else + stream.end(); + } + write(); }); server.listen(PORT, () => { bench.http({ diff --git a/src/node_http2_core-inl.h b/src/node_http2_core-inl.h index 15fb3d4f83494c..8158abeabc98da 100644 --- a/src/node_http2_core-inl.h +++ b/src/node_http2_core-inl.h @@ -478,25 +478,54 @@ inline void Nghttp2Session::SendPendingData() { // will not be usable. if (IsDestroying()) return; - const uint8_t* data; - ssize_t len = 0; - size_t ncopy = 0; - uv_buf_t buf; - AllocateSend(SEND_BUFFER_RECOMMENDED_SIZE, &buf); - while (nghttp2_session_want_write(session_)) { - len = nghttp2_session_mem_send(session_, &data); - CHECK_GE(len, 0); // If this is less than zero, we're out of memory - // While len is greater than 0, send a chunk - while (len > 0) { - ncopy = len; - if (ncopy > buf.len) - ncopy = buf.len; - memcpy(buf.base, data, ncopy); - Send(&buf, ncopy); - len -= ncopy; - CHECK_GE(len, 0); // This should never be less than zero + + uv_buf_t dest; + AllocateSend(SEND_BUFFER_RECOMMENDED_SIZE, &dest); + size_t destLength = 0; // amount of data stored in dest + size_t destRemaining = dest.len; // amount space remaining in dest + size_t destOffset = 0; // current write offset of dest + + const uint8_t* src; // pointer to the serialized data + ssize_t srcLength = 0; // length of serialized data chunk + + // While srcLength is greater than zero + while ((srcLength = nghttp2_session_mem_send(session_, &src)) > 0) { + DEBUG_HTTP2("Nghttp2Session %s: nghttp2 has %d bytes to send\n", + TypeName(), srcLength); + size_t srcRemaining = srcLength; + size_t srcOffset = 0; + + // The amount of data we have to copy is greater than the space + // remaining. Copy what we can into the remaining space, send it, + // the proceed with the rest. + while (srcRemaining > destRemaining) { + DEBUG_HTTP2("Nghttp2Session %s: pushing %d bytes to the socket\n", + TypeName(), destRemaining); + memcpy(dest.base + destOffset, src + srcOffset, destRemaining); + destLength += destRemaining; + Send(&dest, destLength); + destOffset = 0; + destLength = 0; + srcRemaining -= destRemaining; + srcOffset += destRemaining; + destRemaining = dest.len; + } + + if (srcRemaining > 0) { + memcpy(dest.base + destOffset, src + srcOffset, srcRemaining); + destLength += srcRemaining; + destOffset += srcRemaining; + destRemaining -= srcRemaining; + srcRemaining = 0; + srcOffset = 0; } } + + if (destLength > 0) { + DEBUG_HTTP2("Nghttp2Session %s: pushing %d bytes to the socket\n", + TypeName(), destLength); + Send(&dest, destLength); + } } // Initialize the Nghttp2Session handle by creating and