From 0a9e69f2f5ebc1b41c80dd2f6896b03df2f73bc9 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 16 Mar 2018 12:33:30 -0700 Subject: [PATCH 1/4] http2: don't aggressively inline Most of the inlines were leftovers from a much older design iteration and are largely pointless or counter productive. --- src/node_http2.cc | 193 ++++++++++++++++++++++------------------------ src/node_http2.h | 107 ++++++++++++------------- 2 files changed, 145 insertions(+), 155 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 68a684025ce4a0..5f7c42b6902287 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -283,7 +283,7 @@ Http2Session::Http2Settings::~Http2Settings() { // Generates a Buffer that contains the serialized payload of a SETTINGS // frame. This can be used, for instance, to create the Base64-encoded // content of an Http2-Settings header field. -inline Local Http2Session::Http2Settings::Pack() { +Local Http2Session::Http2Settings::Pack() { const size_t len = count_ * 6; Local buf = Buffer::New(env(), len).ToLocalChecked(); ssize_t ret = @@ -298,9 +298,9 @@ inline Local Http2Session::Http2Settings::Pack() { // Updates the shared TypedArray with the current remote or local settings for // the session. -inline void Http2Session::Http2Settings::Update(Environment* env, - Http2Session* session, - get_setting fn) { +void Http2Session::Http2Settings::Update(Environment* env, + Http2Session* session, + get_setting fn) { AliasedBuffer& buffer = env->http2_state()->settings_buffer; buffer[IDX_SETTINGS_HEADER_TABLE_SIZE] = @@ -318,7 +318,7 @@ inline void Http2Session::Http2Settings::Update(Environment* env, } // Initializes the shared TypedArray with the default settings values. -inline void Http2Session::Http2Settings::RefreshDefaults(Environment* env) { +void Http2Session::Http2Settings::RefreshDefaults(Environment* env) { AliasedBuffer& buffer = env->http2_state()->settings_buffer; @@ -541,7 +541,7 @@ inline bool HasHttp2Observer(Environment* env) { return observers[performance::NODE_PERFORMANCE_ENTRY_TYPE_HTTP2] != 0; } -inline void Http2Stream::EmitStatistics() { +void Http2Stream::EmitStatistics() { if (!HasHttp2Observer(env())) return; Http2StreamPerformanceEntry* entry = @@ -579,7 +579,7 @@ inline void Http2Stream::EmitStatistics() { }, static_cast(entry)); } -inline void Http2Session::EmitStatistics() { +void Http2Session::EmitStatistics() { if (!HasHttp2Observer(env())) return; Http2SessionPerformanceEntry* entry = @@ -684,8 +684,8 @@ inline void Http2Session::RemoveStream(Http2Stream* stream) { // that the total frame size, including header bytes, are 8-byte aligned. // If maxPayloadLen is smaller than the number of bytes necessary to align, // will return maxPayloadLen instead. -inline ssize_t Http2Session::OnDWordAlignedPadding(size_t frameLen, - size_t maxPayloadLen) { +ssize_t Http2Session::OnDWordAlignedPadding(size_t frameLen, + size_t maxPayloadLen) { size_t r = (frameLen + 9) % 8; if (r == 0) return frameLen; // If already a multiple of 8, return. @@ -701,8 +701,8 @@ inline ssize_t Http2Session::OnDWordAlignedPadding(size_t frameLen, // Used as one of the Padding Strategy functions. Uses the maximum amount // of padding allowed for the current frame. -inline ssize_t Http2Session::OnMaxFrameSizePadding(size_t frameLen, - size_t maxPayloadLen) { +ssize_t Http2Session::OnMaxFrameSizePadding(size_t frameLen, + size_t maxPayloadLen) { DEBUG_HTTP2SESSION2(this, "using max frame size padding: %d", maxPayloadLen); return maxPayloadLen; } @@ -711,8 +711,8 @@ inline ssize_t Http2Session::OnMaxFrameSizePadding(size_t frameLen, // to determine the amount of padding for the current frame. This option is // rather more expensive because of the JS boundary cross. It generally should // not be the preferred option. -inline ssize_t Http2Session::OnCallbackPadding(size_t frameLen, - size_t maxPayloadLen) { +ssize_t Http2Session::OnCallbackPadding(size_t frameLen, + size_t maxPayloadLen) { if (frameLen == 0) return 0; DEBUG_HTTP2SESSION(this, "using callback to determine padding"); Isolate* isolate = env()->isolate(); @@ -743,7 +743,7 @@ inline ssize_t Http2Session::OnCallbackPadding(size_t frameLen, // various callback functions. Each of these will typically result in a call // out to JavaScript so this particular function is rather hot and can be // quite expensive. This is a potential performance optimization target later. -inline ssize_t Http2Session::Write(const uv_buf_t* bufs, size_t nbufs) { +ssize_t Http2Session::Write(const uv_buf_t* bufs, size_t nbufs) { size_t total = 0; // Note that nghttp2_session_mem_recv is a synchronous operation that // will trigger a number of other callbacks. Those will, in turn have @@ -783,9 +783,9 @@ inline int32_t GetFrameID(const nghttp2_frame* frame) { // callback to determine if a new stream is being created or if we are simply // adding a new block of headers to an existing stream. The header pairs // themselves are set in the OnHeaderCallback -inline int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle, - const nghttp2_frame* frame, - void* user_data) { +int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle, + const nghttp2_frame* frame, + void* user_data) { Http2Session* session = static_cast(user_data); int32_t id = GetFrameID(frame); DEBUG_HTTP2SESSION2(session, "beginning headers for stream %d", id); @@ -812,12 +812,12 @@ inline int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle, // Called by nghttp2 for each header name/value pair in a HEADERS block. // This had to have been preceded by a call to OnBeginHeadersCallback so // the Http2Stream is guaranteed to already exist. -inline int Http2Session::OnHeaderCallback(nghttp2_session* handle, - const nghttp2_frame* frame, - nghttp2_rcbuf* name, - nghttp2_rcbuf* value, - uint8_t flags, - void* user_data) { +int Http2Session::OnHeaderCallback(nghttp2_session* handle, + const nghttp2_frame* frame, + nghttp2_rcbuf* name, + nghttp2_rcbuf* value, + uint8_t flags, + void* user_data) { Http2Session* session = static_cast(user_data); int32_t id = GetFrameID(frame); Http2Stream* stream = session->FindStream(id); @@ -837,9 +837,9 @@ inline int Http2Session::OnHeaderCallback(nghttp2_session* handle, // Called by nghttp2 when a complete HTTP2 frame has been received. There are // only a handful of frame types tha we care about handling here. -inline int Http2Session::OnFrameReceive(nghttp2_session* handle, - const nghttp2_frame* frame, - void* user_data) { +int Http2Session::OnFrameReceive(nghttp2_session* handle, + const nghttp2_frame* frame, + void* user_data) { Http2Session* session = static_cast(user_data); session->statistics_.frame_count++; DEBUG_HTTP2SESSION2(session, "complete frame received: type: %d", @@ -874,10 +874,10 @@ inline int Http2Session::OnFrameReceive(nghttp2_session* handle, return 0; } -inline int Http2Session::OnInvalidFrame(nghttp2_session* handle, - const nghttp2_frame *frame, - int lib_error_code, - void* user_data) { +int Http2Session::OnInvalidFrame(nghttp2_session* handle, + const nghttp2_frame *frame, + int lib_error_code, + void* user_data) { Http2Session* session = static_cast(user_data); DEBUG_HTTP2SESSION2(session, "invalid frame received, code: %d", @@ -906,10 +906,10 @@ inline int Http2Session::OnInvalidFrame(nghttp2_session* handle, // really care about those and there's nothing we can reasonably do about it // anyway. Other types of failures are reported up to JavaScript. This should // be exceedingly rare. -inline int Http2Session::OnFrameNotSent(nghttp2_session* handle, - const nghttp2_frame* frame, - int error_code, - void* user_data) { +int Http2Session::OnFrameNotSent(nghttp2_session* handle, + const nghttp2_frame* frame, + int error_code, + void* user_data) { Http2Session* session = static_cast(user_data); Environment* env = session->env(); DEBUG_HTTP2SESSION2(session, "frame type %d was not sent, code: %d", @@ -933,19 +933,19 @@ inline int Http2Session::OnFrameNotSent(nghttp2_session* handle, return 0; } -inline int Http2Session::OnFrameSent(nghttp2_session* handle, - const nghttp2_frame* frame, - void* user_data) { +int Http2Session::OnFrameSent(nghttp2_session* handle, + const nghttp2_frame* frame, + void* user_data) { Http2Session* session = static_cast(user_data); session->statistics_.frame_sent += 1; return 0; } // Called by nghttp2 when a stream closes. -inline int Http2Session::OnStreamClose(nghttp2_session* handle, - int32_t id, - uint32_t code, - void* user_data) { +int Http2Session::OnStreamClose(nghttp2_session* handle, + int32_t id, + uint32_t code, + void* user_data) { Http2Session* session = static_cast(user_data); Environment* env = session->env(); Isolate* isolate = env->isolate(); @@ -982,12 +982,12 @@ inline int Http2Session::OnStreamClose(nghttp2_session* handle, // ignore these. If this callback was not provided, nghttp2 would handle // invalid headers strictly and would shut down the stream. We are intentionally // being more lenient here although we may want to revisit this choice later. -inline int Http2Session::OnInvalidHeader(nghttp2_session* session, - const nghttp2_frame* frame, - nghttp2_rcbuf* name, - nghttp2_rcbuf* value, - uint8_t flags, - void* user_data) { +int Http2Session::OnInvalidHeader(nghttp2_session* session, + const nghttp2_frame* frame, + nghttp2_rcbuf* name, + nghttp2_rcbuf* value, + uint8_t flags, + void* user_data) { // Ignore invalid header fields by default. return 0; } @@ -996,12 +996,12 @@ inline int Http2Session::OnInvalidHeader(nghttp2_session* session, // us in discrete chunks. We push these into a linked list stored in the // Http2Sttream which is flushed out to JavaScript as quickly as possible. // This can be a particularly hot path. -inline int Http2Session::OnDataChunkReceived(nghttp2_session* handle, - uint8_t flags, - int32_t id, - const uint8_t* data, - size_t len, - void* user_data) { +int Http2Session::OnDataChunkReceived(nghttp2_session* handle, + uint8_t flags, + int32_t id, + const uint8_t* data, + size_t len, + void* user_data) { Http2Session* session = static_cast(user_data); DEBUG_HTTP2SESSION2(session, "buffering data chunk for stream %d, size: " "%d, flags: %d", id, len, flags); @@ -1059,10 +1059,10 @@ inline int Http2Session::OnDataChunkReceived(nghttp2_session* handle, // Called by nghttp2 when it needs to determine how much padding to use in // a DATA or HEADERS frame. -inline ssize_t Http2Session::OnSelectPadding(nghttp2_session* handle, - const nghttp2_frame* frame, - size_t maxPayloadLen, - void* user_data) { +ssize_t Http2Session::OnSelectPadding(nghttp2_session* handle, + const nghttp2_frame* frame, + size_t maxPayloadLen, + void* user_data) { Http2Session* session = static_cast(user_data); ssize_t padding = frame->hd.length; @@ -1089,10 +1089,10 @@ inline ssize_t Http2Session::OnSelectPadding(nghttp2_session* handle, // We use this currently to determine when an attempt is made to use the http2 // protocol with a non-http2 peer. -inline int Http2Session::OnNghttpError(nghttp2_session* handle, - const char* message, - size_t len, - void* user_data) { +int Http2Session::OnNghttpError(nghttp2_session* handle, + const char* message, + size_t len, + void* user_data) { // Unfortunately, this is currently the only way for us to know if // the session errored because the peer is not an http2 peer. Http2Session* session = static_cast(user_data); @@ -1115,7 +1115,7 @@ inline int Http2Session::OnNghttpError(nghttp2_session* handle, // Once all of the DATA frames for a Stream have been sent, the GetTrailers // method calls out to JavaScript to fetch the trailing headers that need // to be sent. -inline void Http2Session::GetTrailers(Http2Stream* stream, uint32_t* flags) { +void Http2Session::GetTrailers(Http2Stream* stream, uint32_t* flags) { if (!stream->IsDestroyed() && stream->HasTrailers()) { Http2Stream::SubmitTrailers submit_trailers{this, stream, flags}; stream->OnTrailers(submit_trailers); @@ -1164,8 +1164,8 @@ Http2Stream::SubmitTrailers::SubmitTrailers( : session_(session), stream_(stream), flags_(flags) { } -inline void Http2Stream::SubmitTrailers::Submit(nghttp2_nv* trailers, - size_t length) const { +void Http2Stream::SubmitTrailers::Submit(nghttp2_nv* trailers, + size_t length) const { Http2Scope h2scope(session_); if (length == 0) return; @@ -1180,7 +1180,7 @@ inline void Http2Stream::SubmitTrailers::Submit(nghttp2_nv* trailers, // Called by OnFrameReceived to notify JavaScript land that a complete // HEADERS frame has been received and processed. This method converts the // received headers into a JavaScript array and pushes those out to JS. -inline void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) { +void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) { Isolate* isolate = env()->isolate(); HandleScope scope(isolate); Local context = env()->context(); @@ -1249,7 +1249,7 @@ inline void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) { // received. Notifies JS land about the priority change. Note that priorities // are considered advisory only, so this has no real effect other than to // simply let user code know that the priority has changed. -inline void Http2Session::HandlePriorityFrame(const nghttp2_frame* frame) { +void Http2Session::HandlePriorityFrame(const nghttp2_frame* frame) { Isolate* isolate = env()->isolate(); HandleScope scope(isolate); Local context = env()->context(); @@ -1274,7 +1274,7 @@ inline void Http2Session::HandlePriorityFrame(const nghttp2_frame* frame) { // Called by OnFrameReceived when a complete DATA frame has been received. // If we know that this was the last DATA frame (because the END_STREAM flag // is set), then we'll terminate the readable side of the StreamBase. -inline void Http2Session::HandleDataFrame(const nghttp2_frame* frame) { +void Http2Session::HandleDataFrame(const nghttp2_frame* frame) { int32_t id = GetFrameID(frame); DEBUG_HTTP2SESSION2(this, "handling data frame for stream %d", id); Http2Stream* stream = FindStream(id); @@ -1290,7 +1290,7 @@ inline void Http2Session::HandleDataFrame(const nghttp2_frame* frame) { // Called by OnFrameReceived when a complete GOAWAY frame has been received. -inline void Http2Session::HandleGoawayFrame(const nghttp2_frame* frame) { +void Http2Session::HandleGoawayFrame(const nghttp2_frame* frame) { Isolate* isolate = env()->isolate(); HandleScope scope(isolate); Local context = env()->context(); @@ -1316,7 +1316,7 @@ inline void Http2Session::HandleGoawayFrame(const nghttp2_frame* frame) { } // Called by OnFrameReceived when a complete ALTSVC frame has been received. -inline void Http2Session::HandleAltSvcFrame(const nghttp2_frame* frame) { +void Http2Session::HandleAltSvcFrame(const nghttp2_frame* frame) { Isolate* isolate = env()->isolate(); HandleScope scope(isolate); Local context = env()->context(); @@ -1344,7 +1344,7 @@ inline void Http2Session::HandleAltSvcFrame(const nghttp2_frame* frame) { } // Called by OnFrameReceived when a complete PING frame has been received. -inline void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { +void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK; if (ack) { Http2Ping* ping = PopPing(); @@ -1370,7 +1370,7 @@ inline void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { } // Called by OnFrameReceived when a complete SETTINGS frame has been received. -inline void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) { +void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) { bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK; if (ack) { // If this is an acknowledgement, we should have an Http2Settings @@ -1616,7 +1616,7 @@ int Http2Session::OnSendData( } // Creates a new Http2Stream and submits a new http2 request. -inline Http2Stream* Http2Session::SubmitRequest( +Http2Stream* Http2Session::SubmitRequest( nghttp2_priority_spec* prispec, nghttp2_nv* nva, size_t len, @@ -1805,7 +1805,7 @@ void Http2Stream::OnTrailers(const SubmitTrailers& submit_trailers) { } } -inline void Http2Stream::Close(int32_t code) { +void Http2Stream::Close(int32_t code) { CHECK(!this->IsDestroyed()); flags_ |= NGHTTP2_STREAM_FLAG_CLOSED; code_ = code; @@ -1830,7 +1830,7 @@ int Http2Stream::DoShutdown(ShutdownWrap* req_wrap) { // Destroy the Http2Stream and render it unusable. Actual resources for the // Stream will not be freed until the next tick of the Node.js event loop // using the SetImmediate queue. -inline void Http2Stream::Destroy() { +void Http2Stream::Destroy() { // Do nothing if this stream instance is already destroyed if (IsDestroyed()) return; @@ -1869,9 +1869,7 @@ inline void Http2Stream::Destroy() { // Initiates a response on the Http2Stream using data provided via the // StreamBase Streams API. -inline int Http2Stream::SubmitResponse(nghttp2_nv* nva, - size_t len, - int options) { +int Http2Stream::SubmitResponse(nghttp2_nv* nva, size_t len, int options) { CHECK(!this->IsDestroyed()); Http2Scope h2scope(this); DEBUG_HTTP2STREAM(this, "submitting response"); @@ -1889,7 +1887,7 @@ inline int Http2Stream::SubmitResponse(nghttp2_nv* nva, // Submit informational headers for a stream. -inline int Http2Stream::SubmitInfo(nghttp2_nv* nva, size_t len) { +int Http2Stream::SubmitInfo(nghttp2_nv* nva, size_t len) { CHECK(!this->IsDestroyed()); Http2Scope h2scope(this); DEBUG_HTTP2STREAM2(this, "sending %d informational headers", len); @@ -1902,8 +1900,8 @@ inline int Http2Stream::SubmitInfo(nghttp2_nv* nva, size_t len) { } // Submit a PRIORITY frame to the connected peer. -inline int Http2Stream::SubmitPriority(nghttp2_priority_spec* prispec, - bool silent) { +int Http2Stream::SubmitPriority(nghttp2_priority_spec* prispec, + bool silent) { CHECK(!this->IsDestroyed()); Http2Scope h2scope(this); DEBUG_HTTP2STREAM(this, "sending priority spec"); @@ -1919,7 +1917,7 @@ inline int Http2Stream::SubmitPriority(nghttp2_priority_spec* prispec, // Closes the Http2Stream by submitting an RST_STREAM frame to the connected // peer. -inline void Http2Stream::SubmitRstStream(const uint32_t code) { +void Http2Stream::SubmitRstStream(const uint32_t code) { CHECK(!this->IsDestroyed()); Http2Scope h2scope(this); // Force a purge of any currently pending data here to make sure @@ -1931,10 +1929,10 @@ inline void Http2Stream::SubmitRstStream(const uint32_t code) { // Submit a push promise and create the associated Http2Stream if successful. -inline Http2Stream* Http2Stream::SubmitPushPromise(nghttp2_nv* nva, - size_t len, - int32_t* ret, - int options) { +Http2Stream* Http2Stream::SubmitPushPromise(nghttp2_nv* nva, + size_t len, + int32_t* ret, + int options) { CHECK(!this->IsDestroyed()); Http2Scope h2scope(this); DEBUG_HTTP2STREAM(this, "sending push promise"); @@ -1950,7 +1948,7 @@ inline Http2Stream* Http2Stream::SubmitPushPromise(nghttp2_nv* nva, // Switch the StreamBase into flowing mode to begin pushing chunks of data // out to JS land. -inline int Http2Stream::ReadStart() { +int Http2Stream::ReadStart() { Http2Scope h2scope(this); CHECK(!this->IsDestroyed()); flags_ |= NGHTTP2_STREAM_FLAG_READ_START; @@ -1969,7 +1967,7 @@ inline int Http2Stream::ReadStart() { } // Switch the StreamBase into paused mode. -inline int Http2Stream::ReadStop() { +int Http2Stream::ReadStop() { CHECK(!this->IsDestroyed()); if (!IsReading()) return 0; @@ -1988,10 +1986,10 @@ inline int Http2Stream::ReadStop() { // 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::DoWrite(WriteWrap* req_wrap, - uv_buf_t* bufs, - size_t nbufs, - uv_stream_t* send_handle) { +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); @@ -2013,22 +2011,19 @@ inline int Http2Stream::DoWrite(WriteWrap* req_wrap, return 0; } -inline size_t GetBufferLength(nghttp2_rcbuf* buf) { - return nghttp2_rcbuf_get_buf(buf).len; -} - // Ads a header to the Http2Stream. Note that the header name and value are // provided using a buffer structure provided by nghttp2 that allows us to // avoid unnecessary memcpy's. Those buffers are ref counted. The ref count // is incremented here and are decremented when the header name and values // are garbage collected later. -inline bool Http2Stream::AddHeader(nghttp2_rcbuf* name, - nghttp2_rcbuf* value, - uint8_t flags) { +bool Http2Stream::AddHeader(nghttp2_rcbuf* name, + nghttp2_rcbuf* value, + uint8_t flags) { CHECK(!this->IsDestroyed()); if (this->statistics_.first_header == 0) this->statistics_.first_header = uv_hrtime(); - size_t length = GetBufferLength(name) + GetBufferLength(value) + 32; + size_t length = nghttp2_rcbuf_get_buf(name).len + + nghttp2_rcbuf_get_buf(value).len + 32; // A header can only be added if we have not exceeded the maximum number // of headers and the session has memory available for it. if (!session_->IsAvailableSessionMemory(length) || diff --git a/src/node_http2.h b/src/node_http2.h index 0fac6cca00f4db..d09dcc22dd6978 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -558,7 +558,7 @@ class Http2Stream : public AsyncWrap, Http2Session* session() { return session_; } - inline void EmitStatistics(); + void EmitStatistics(); // Process a Data Chunk void OnDataChunk(uv_buf_t* chunk); @@ -576,32 +576,29 @@ class Http2Stream : public AsyncWrap, bool HasWantsWrite() const override { return true; } // Initiate a response on this stream. - inline int SubmitResponse(nghttp2_nv* nva, - size_t len, - int options); + int SubmitResponse(nghttp2_nv* nva, size_t len, int options); // Submit informational headers for this stream - inline int SubmitInfo(nghttp2_nv* nva, size_t len); + int SubmitInfo(nghttp2_nv* nva, size_t len); // Submit a PRIORITY frame for this stream - inline int SubmitPriority(nghttp2_priority_spec* prispec, - bool silent = false); + int SubmitPriority(nghttp2_priority_spec* prispec, bool silent = false); // Submits an RST_STREAM frame using the given code - inline void SubmitRstStream(const uint32_t code); + void SubmitRstStream(const uint32_t code); // Submits a PUSH_PROMISE frame with this stream as the parent. - inline Http2Stream* SubmitPushPromise( + Http2Stream* SubmitPushPromise( nghttp2_nv* nva, size_t len, int32_t* ret, int options = 0); - inline void Close(int32_t code); + void Close(int32_t code); // Destroy this stream instance and free all held memory. - inline void Destroy(); + void Destroy(); inline bool IsDestroyed() const { return flags_ & NGHTTP2_STREAM_FLAG_DESTROYED; @@ -640,9 +637,7 @@ class Http2Stream : public AsyncWrap, inline void IncrementAvailableOutboundLength(size_t amount); inline void DecrementAvailableOutboundLength(size_t amount); - inline bool AddHeader(nghttp2_rcbuf* name, - nghttp2_rcbuf* value, - uint8_t flags); + bool AddHeader(nghttp2_rcbuf* name, nghttp2_rcbuf* value, uint8_t flags); inline nghttp2_header* headers() { return current_headers_.data(); @@ -678,11 +673,11 @@ class Http2Stream : public AsyncWrap, // Handling Trailer Headers class SubmitTrailers { public: - inline void Submit(nghttp2_nv* trailers, size_t length) const; + void Submit(nghttp2_nv* trailers, size_t length) const; - inline SubmitTrailers(Http2Session* sesion, - Http2Stream* stream, - uint32_t* flags); + SubmitTrailers(Http2Session* sesion, + Http2Stream* stream, + uint32_t* flags); private: Http2Session* const session_; @@ -795,7 +790,7 @@ class Http2Session : public AsyncWrap, public StreamListener { class Http2Ping; class Http2Settings; - inline void EmitStatistics(); + void EmitStatistics(); inline StreamBase* underlying_stream() { return static_cast(stream_); @@ -816,25 +811,25 @@ class Http2Session : public AsyncWrap, public StreamListener { bool Ping(v8::Local function); - inline void SendPendingData(); + void SendPendingData(); // Submits a new request. If the request is a success, assigned // will be a pointer to the Http2Stream instance assigned. // This only works if the session is a client session. - inline Http2Stream* SubmitRequest( + Http2Stream* SubmitRequest( nghttp2_priority_spec* prispec, nghttp2_nv* nva, size_t len, int32_t* ret, int options = 0); - nghttp2_session_type type() const { return session_type_; } + inline nghttp2_session_type type() const { return session_type_; } inline nghttp2_session* session() const { return session_; } - nghttp2_session* operator*() { return session_; } + inline nghttp2_session* operator*() { return session_; } - uint32_t GetMaxHeaderPairs() const { return max_header_pairs_; } + inline uint32_t GetMaxHeaderPairs() const { return max_header_pairs_; } inline const char* TypeName(); @@ -860,11 +855,11 @@ class Http2Session : public AsyncWrap, public StreamListener { bool HasWritesOnSocketForStream(Http2Stream* stream); // Write data to the session - inline ssize_t Write(const uv_buf_t* bufs, size_t nbufs); + ssize_t Write(const uv_buf_t* bufs, size_t nbufs); size_t self_size() const override { return sizeof(*this); } - inline void GetTrailers(Http2Stream* stream, uint32_t* flags); + void GetTrailers(Http2Stream* stream, uint32_t* flags); // Handle reads/writes from the underlying network transport. void OnStreamRead(ssize_t nread, const uv_buf_t& buf) override; @@ -942,84 +937,84 @@ class Http2Session : public AsyncWrap, public StreamListener { private: // Frame Padding Strategies - inline ssize_t OnDWordAlignedPadding(size_t frameLength, - size_t maxPayloadLen); - inline ssize_t OnMaxFrameSizePadding(size_t frameLength, - size_t maxPayloadLen); - inline ssize_t OnCallbackPadding(size_t frame, - size_t maxPayloadLen); + ssize_t OnDWordAlignedPadding(size_t frameLength, + size_t maxPayloadLen); + ssize_t OnMaxFrameSizePadding(size_t frameLength, + size_t maxPayloadLen); + ssize_t OnCallbackPadding(size_t frameLength, + size_t maxPayloadLen); // Frame Handler - inline void HandleDataFrame(const nghttp2_frame* frame); - inline void HandleGoawayFrame(const nghttp2_frame* frame); - inline void HandleHeadersFrame(const nghttp2_frame* frame); - inline void HandlePriorityFrame(const nghttp2_frame* frame); - inline void HandleSettingsFrame(const nghttp2_frame* frame); - inline void HandlePingFrame(const nghttp2_frame* frame); - inline void HandleAltSvcFrame(const nghttp2_frame* frame); + void HandleDataFrame(const nghttp2_frame* frame); + void HandleGoawayFrame(const nghttp2_frame* frame); + void HandleHeadersFrame(const nghttp2_frame* frame); + void HandlePriorityFrame(const nghttp2_frame* frame); + void HandleSettingsFrame(const nghttp2_frame* frame); + void HandlePingFrame(const nghttp2_frame* frame); + void HandleAltSvcFrame(const nghttp2_frame* frame); // nghttp2 callbacks - static inline int OnBeginHeadersCallback( + static int OnBeginHeadersCallback( nghttp2_session* session, const nghttp2_frame* frame, void* user_data); - static inline int OnHeaderCallback( + static int OnHeaderCallback( nghttp2_session* session, const nghttp2_frame* frame, nghttp2_rcbuf* name, nghttp2_rcbuf* value, uint8_t flags, void* user_data); - static inline int OnFrameReceive( + static int OnFrameReceive( nghttp2_session* session, const nghttp2_frame* frame, void* user_data); - static inline int OnFrameNotSent( + static int OnFrameNotSent( nghttp2_session* session, const nghttp2_frame* frame, int error_code, void* user_data); - static inline int OnFrameSent( + static int OnFrameSent( nghttp2_session* session, const nghttp2_frame* frame, void* user_data); - static inline int OnStreamClose( + static int OnStreamClose( nghttp2_session* session, int32_t id, uint32_t code, void* user_data); - static inline int OnInvalidHeader( + static int OnInvalidHeader( nghttp2_session* session, const nghttp2_frame* frame, nghttp2_rcbuf* name, nghttp2_rcbuf* value, uint8_t flags, void* user_data); - static inline int OnDataChunkReceived( + static int OnDataChunkReceived( nghttp2_session* session, uint8_t flags, int32_t id, const uint8_t* data, size_t len, void* user_data); - static inline ssize_t OnSelectPadding( + static ssize_t OnSelectPadding( nghttp2_session* session, const nghttp2_frame* frame, size_t maxPayloadLen, void* user_data); - static inline int OnNghttpError( + static int OnNghttpError( nghttp2_session* session, const char* message, size_t len, void* user_data); - static inline int OnSendData( + static int OnSendData( nghttp2_session* session, nghttp2_frame* frame, const uint8_t* framehd, size_t length, nghttp2_data_source* source, void* user_data); - static inline int OnInvalidFrame( + static int OnInvalidFrame( nghttp2_session* session, const nghttp2_frame *frame, int lib_error_code, @@ -1197,15 +1192,15 @@ class Http2Session::Http2Settings : public AsyncWrap { } // Returns a Buffer instance with the serialized SETTINGS payload - inline Local Pack(); + Local Pack(); // Resets the default values in the settings buffer - static inline void RefreshDefaults(Environment* env); + static void RefreshDefaults(Environment* env); // Update the local or remote settings for the given session - static inline void Update(Environment* env, - Http2Session* session, - get_setting fn); + static void Update(Environment* env, + Http2Session* session, + get_setting fn); private: void Init(); From 1382a182b7d5c0205c0f3c09f7fe8f2e8b9a2638 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 16 Mar 2018 13:28:51 -0700 Subject: [PATCH 2/4] http2: clean up Http2Settings Use of a MaybeStackBuffer was just silly. Fix a long standing todo Reduce code duplication a bit. --- src/node_http2.cc | 62 ++++++++++++----------------------------------- src/node_http2.h | 8 +----- 2 files changed, 16 insertions(+), 54 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 5f7c42b6902287..d6f1d6e7659707 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -190,60 +190,28 @@ Http2Options::Http2Options(Environment* env) { } void Http2Session::Http2Settings::Init() { - entries_.AllocateSufficientStorage(IDX_SETTINGS_COUNT); AliasedBuffer& buffer = env()->http2_state()->settings_buffer; uint32_t flags = buffer[IDX_SETTINGS_COUNT]; size_t n = 0; - if (flags & (1 << IDX_SETTINGS_HEADER_TABLE_SIZE)) { - uint32_t val = buffer[IDX_SETTINGS_HEADER_TABLE_SIZE]; - DEBUG_HTTP2SESSION2(session_, "setting header table size: %d\n", val); - entries_[n].settings_id = NGHTTP2_SETTINGS_HEADER_TABLE_SIZE; - entries_[n].value = val; - n++; +#define GRABSETTING(N, trace) \ + if (flags & (1 << IDX_SETTINGS_##N)) { \ + uint32_t val = buffer[IDX_SETTINGS_##N]; \ + DEBUG_HTTP2SESSION2(session_, "setting " trace ": %d\n", val); \ + entries_[n++] = \ + nghttp2_settings_entry {NGHTTP2_SETTINGS_##N, val}; \ } - if (flags & (1 << IDX_SETTINGS_MAX_CONCURRENT_STREAMS)) { - uint32_t val = buffer[IDX_SETTINGS_MAX_CONCURRENT_STREAMS]; - DEBUG_HTTP2SESSION2(session_, "setting max concurrent streams: %d\n", val); - entries_[n].settings_id = NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS; - entries_[n].value = val; - n++; - } - - if (flags & (1 << IDX_SETTINGS_MAX_FRAME_SIZE)) { - uint32_t val = buffer[IDX_SETTINGS_MAX_FRAME_SIZE]; - DEBUG_HTTP2SESSION2(session_, "setting max frame size: %d\n", val); - entries_[n].settings_id = NGHTTP2_SETTINGS_MAX_FRAME_SIZE; - entries_[n].value = val; - n++; - } - - if (flags & (1 << IDX_SETTINGS_INITIAL_WINDOW_SIZE)) { - uint32_t val = buffer[IDX_SETTINGS_INITIAL_WINDOW_SIZE]; - DEBUG_HTTP2SESSION2(session_, "setting initial window size: %d\n", val); - entries_[n].settings_id = NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE; - entries_[n].value = val; - n++; - } - - if (flags & (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE)) { - uint32_t val = buffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE]; - DEBUG_HTTP2SESSION2(session_, "setting max header list size: %d\n", val); - entries_[n].settings_id = NGHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE; - entries_[n].value = val; - n++; - } + GRABSETTING(HEADER_TABLE_SIZE, "header table size"); + GRABSETTING(MAX_CONCURRENT_STREAMS, "max concurrent streams"); + GRABSETTING(MAX_FRAME_SIZE, "max frame size"); + GRABSETTING(INITIAL_WINDOW_SIZE, "initial window size"); + GRABSETTING(MAX_HEADER_LIST_SIZE, "max header list size"); + GRABSETTING(ENABLE_PUSH, "enable push"); - if (flags & (1 << IDX_SETTINGS_ENABLE_PUSH)) { - uint32_t val = buffer[IDX_SETTINGS_ENABLE_PUSH]; - DEBUG_HTTP2SESSION2(session_, "setting enable push: %d\n", val); - entries_[n].settings_id = NGHTTP2_SETTINGS_ENABLE_PUSH; - entries_[n].value = val; - n++; - } +#undef GRABSETTING count_ = n; } @@ -289,7 +257,7 @@ Local Http2Session::Http2Settings::Pack() { ssize_t ret = nghttp2_pack_settings_payload( reinterpret_cast(Buffer::Data(buf)), len, - *entries_, count_); + &entries_[0], count_); if (ret >= 0) return buf; else @@ -344,7 +312,7 @@ void Http2Session::Http2Settings::RefreshDefaults(Environment* env) { void Http2Session::Http2Settings::Send() { Http2Scope h2scope(session_); CHECK_EQ(nghttp2_submit_settings(**session_, NGHTTP2_FLAG_NONE, - *entries_, length()), 0); + &entries_[0], count_), 0); } void Http2Session::Http2Settings::Done(bool ack) { diff --git a/src/node_http2.h b/src/node_http2.h index d09dcc22dd6978..9dd65667f90670 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -1185,12 +1185,6 @@ class Http2Session::Http2Settings : public AsyncWrap { void Send(); void Done(bool ack); - size_t length() const { return count_; } - - nghttp2_settings_entry* operator*() { - return *entries_; - } - // Returns a Buffer instance with the serialized SETTINGS payload Local Pack(); @@ -1207,7 +1201,7 @@ class Http2Session::Http2Settings : public AsyncWrap { Http2Session* session_; uint64_t startTime_; size_t count_ = 0; - MaybeStackBuffer entries_; + nghttp2_settings_entry entries_[IDX_SETTINGS_COUNT]; }; class ExternalHeader : From b1684a6c6a0670c4c9e94fabf4a90c01d0e01db0 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 16 Mar 2018 14:48:22 -0700 Subject: [PATCH 3/4] http2: some general code improvements --- src/node_http2.cc | 70 ++++++++++++++--------------------------------- 1 file changed, 21 insertions(+), 49 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index d6f1d6e7659707..ba6b09ef97a87a 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -595,9 +595,8 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { Http2Scope h2scope(this); DEBUG_HTTP2SESSION2(this, "terminating session with code %d", code); CHECK_EQ(nghttp2_session_terminate_session(session_, code), 0); - } else { - if (stream_ != nullptr) - stream_->RemoveStreamListener(this); + } else if (stream_ != nullptr) { + stream_->RemoveStreamListener(this); } // If there are outstanding pings, those will need to be canceled, do @@ -688,10 +687,6 @@ ssize_t Http2Session::OnCallbackPadding(size_t frameLen, Local context = env()->context(); Context::Scope context_scope(context); -#if defined(DEBUG) && DEBUG - CHECK(object()->Has(context, env()->ongetpadding_string()).FromJust()); -#endif - AliasedBuffer& buffer = env()->http2_state()->padding_buffer; buffer[PADDING_BUF_FRAME_LENGTH] = frameLen; @@ -760,18 +755,14 @@ int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle, Http2Stream* stream = session->FindStream(id); if (stream == nullptr) { - if (session->CanAddStream()) { - new Http2Stream(session, id, frame->headers.cat); - } else { + if (!session->CanAddStream()) { // Too many concurrent streams being opened nghttp2_submit_rst_stream(**session, NGHTTP2_FLAG_NONE, id, NGHTTP2_ENHANCE_YOUR_CALM); return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; } - } else { - // If the stream has already been destroyed, ignore. - if (stream->IsDestroyed()) - return 0; + new Http2Stream(session, id, frame->headers.cat); + } else if (!stream->IsDestroyed()) { stream->StartHeaders(frame->headers.cat); } return 0; @@ -791,9 +782,7 @@ int Http2Session::OnHeaderCallback(nghttp2_session* handle, Http2Stream* stream = session->FindStream(id); CHECK_NE(stream, nullptr); // If the stream has already been destroyed, ignore. - if (stream->IsDestroyed()) - return 0; - if (!stream->AddHeader(name, value, flags)) { + if (!stream->IsDestroyed() && !stream->AddHeader(name, value, flags)) { // This will only happen if the connected peer sends us more // than the allowed number of header items at any given time stream->SubmitRstStream(NGHTTP2_ENHANCE_YOUR_CALM); @@ -859,11 +848,8 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle, HandleScope scope(isolate); Local context = env->context(); Context::Scope context_scope(context); - - Local argv[1] = { - Integer::New(isolate, lib_error_code), - }; - session->MakeCallback(env->error_string(), arraysize(argv), argv); + Local argv = Integer::New(isolate, lib_error_code); + session->MakeCallback(env->error_string(), 1, &argv); } return 0; } @@ -1071,11 +1057,8 @@ int Http2Session::OnNghttpError(nghttp2_session* handle, HandleScope scope(isolate); Local context = env->context(); Context::Scope context_scope(context); - - Local argv[1] = { - Integer::New(isolate, NGHTTP2_ERR_PROTO), - }; - session->MakeCallback(env->error_string(), arraysize(argv), argv); + Local argv = Integer::New(isolate, NGHTTP2_ERR_PROTO); + session->MakeCallback(env->error_string(), 1, &argv); } return 0; } @@ -1247,13 +1230,8 @@ void Http2Session::HandleDataFrame(const nghttp2_frame* frame) { DEBUG_HTTP2SESSION2(this, "handling data frame for stream %d", id); Http2Stream* stream = FindStream(id); - // If the stream has already been destroyed, do nothing - if (stream->IsDestroyed()) - return; - - if (frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { + if (!stream->IsDestroyed() && frame->hd.flags & NGHTTP2_FLAG_END_STREAM) stream->EmitRead(UV_EOF); - } } @@ -1328,11 +1306,8 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { HandleScope scope(isolate); Local context = env()->context(); Context::Scope context_scope(context); - - Local argv[1] = { - Integer::New(isolate, NGHTTP2_ERR_PROTO), - }; - MakeCallback(env()->error_string(), arraysize(argv), argv); + Local argv = Integer::New(isolate, NGHTTP2_ERR_PROTO); + MakeCallback(env()->error_string(), 1, &argv); } } } @@ -1360,11 +1335,8 @@ void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) { HandleScope scope(isolate); Local context = env()->context(); Context::Scope context_scope(context); - - Local argv[1] = { - Integer::New(isolate, NGHTTP2_ERR_PROTO), - }; - MakeCallback(env()->error_string(), arraysize(argv), argv); + Local argv = Integer::New(isolate, NGHTTP2_ERR_PROTO); + MakeCallback(env()->error_string(), 1, &argv); } } else { // Otherwise, notify the session about a new settings @@ -1787,7 +1759,7 @@ int Http2Stream::DoShutdown(ShutdownWrap* req_wrap) { { Http2Scope h2scope(this); flags_ |= NGHTTP2_STREAM_FLAG_SHUT; - CHECK_NE(nghttp2_session_resume_data(session_->session(), id_), + CHECK_NE(nghttp2_session_resume_data(**session_, id_), NGHTTP2_ERR_NOMEM); DEBUG_HTTP2STREAM(this, "writable side shutdown"); } @@ -1848,7 +1820,7 @@ int Http2Stream::SubmitResponse(nghttp2_nv* nva, size_t len, int options) { options |= STREAM_OPTION_EMPTY_PAYLOAD; Http2Stream::Provider::Stream prov(this, options); - int ret = nghttp2_submit_response(session_->session(), id_, nva, len, *prov); + int ret = nghttp2_submit_response(**session_, id_, nva, len, *prov); CHECK_NE(ret, NGHTTP2_ERR_NOMEM); return ret; } @@ -1859,7 +1831,7 @@ int Http2Stream::SubmitInfo(nghttp2_nv* nva, size_t len) { CHECK(!this->IsDestroyed()); Http2Scope h2scope(this); DEBUG_HTTP2STREAM2(this, "sending %d informational headers", len); - int ret = nghttp2_submit_headers(session_->session(), + int ret = nghttp2_submit_headers(**session_, NGHTTP2_FLAG_NONE, id_, nullptr, nva, len, nullptr); @@ -1874,9 +1846,9 @@ int Http2Stream::SubmitPriority(nghttp2_priority_spec* prispec, Http2Scope h2scope(this); DEBUG_HTTP2STREAM(this, "sending priority spec"); int ret = silent ? - nghttp2_session_change_stream_priority(session_->session(), + nghttp2_session_change_stream_priority(**session_, id_, prispec) : - nghttp2_submit_priority(session_->session(), + nghttp2_submit_priority(**session_, NGHTTP2_FLAG_NONE, id_, prispec); CHECK_NE(ret, NGHTTP2_ERR_NOMEM); @@ -1926,7 +1898,7 @@ int Http2Stream::ReadStart() { // Tell nghttp2 about our consumption of the data that was handed // off to JS land. - nghttp2_session_consume_stream(session_->session(), + nghttp2_session_consume_stream(**session_, id_, inbound_consumed_data_while_paused_); inbound_consumed_data_while_paused_ = 0; From ad80ce96852412884db4ac0511d82433511b1cba Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 16 Mar 2018 15:25:31 -0700 Subject: [PATCH 4/4] [Squash] nit[Squash] nit --- src/node_http2.cc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index ba6b09ef97a87a..523fda730d2a4a 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -848,8 +848,8 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle, HandleScope scope(isolate); Local context = env->context(); Context::Scope context_scope(context); - Local argv = Integer::New(isolate, lib_error_code); - session->MakeCallback(env->error_string(), 1, &argv); + Local arg = Integer::New(isolate, lib_error_code); + session->MakeCallback(env->error_string(), 1, &arg); } return 0; } @@ -1057,8 +1057,8 @@ int Http2Session::OnNghttpError(nghttp2_session* handle, HandleScope scope(isolate); Local context = env->context(); Context::Scope context_scope(context); - Local argv = Integer::New(isolate, NGHTTP2_ERR_PROTO); - session->MakeCallback(env->error_string(), 1, &argv); + Local arg = Integer::New(isolate, NGHTTP2_ERR_PROTO); + session->MakeCallback(env->error_string(), 1, &arg); } return 0; } @@ -1306,8 +1306,8 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { HandleScope scope(isolate); Local context = env()->context(); Context::Scope context_scope(context); - Local argv = Integer::New(isolate, NGHTTP2_ERR_PROTO); - MakeCallback(env()->error_string(), 1, &argv); + Local arg = Integer::New(isolate, NGHTTP2_ERR_PROTO); + MakeCallback(env()->error_string(), 1, &arg); } } } @@ -1335,8 +1335,8 @@ void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) { HandleScope scope(isolate); Local context = env()->context(); Context::Scope context_scope(context); - Local argv = Integer::New(isolate, NGHTTP2_ERR_PROTO); - MakeCallback(env()->error_string(), 1, &argv); + Local arg = Integer::New(isolate, NGHTTP2_ERR_PROTO); + MakeCallback(env()->error_string(), 1, &arg); } } else { // Otherwise, notify the session about a new settings