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

http2: allocate on every chunk send #16669

Closed
wants to merge 2 commits into from
Closed

http2: allocate on every chunk send #16669

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
125e1b5
http2: allocate on every chunk send
jasnell Nov 1, 2017
1c6ec13
[Squash] address comments
jasnell Nov 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 19 additions & 18 deletions src/node_http2.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,32 +893,33 @@ int Http2Session::DoWrite(WriteWrap* req_wrap,
return 0;
}

void Http2Session::AllocateSend(uv_buf_t* buf) {
buf->base = stream_alloc();
buf->len = kAllocBufferSize;
WriteWrap* Http2Session::AllocateSend() {
HandleScope scope(env()->isolate());
auto AfterWrite = [](WriteWrap* req, int status) {
req->Dispose();
};
Local<Object> obj =
env()->write_wrap_constructor_function()
->NewInstance(env()->context()).ToLocalChecked();
// Base the amount allocated on the remote peers max frame size
uint32_t size =
nghttp2_session_get_remote_settings(
session(),
NGHTTP2_SETTINGS_MAX_FRAME_SIZE);
Copy link
Member

Choose a reason for hiding this comment

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

This makes it 4x smaller than previously (which was equivalent to default window size + frame headers). Is that ok?

It could also potentially be allocating more than it needs to if the current available window size isn't very big.

Even if that's all fine, I think it needs + 9 (could define this elsewhere to be less magicky) because this doesn't account for the frame header.

Copy link
Member Author

Choose a reason for hiding this comment

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

+1 on the +9. Going to keep benchmarking for a good allocation size

// Max frame size + 9 bytes for the header
return WriteWrap::New(env(), obj, this, AfterWrite, size + 9);
}

void Http2Session::Send(uv_buf_t* buf, size_t length) {
void Http2Session::Send(WriteWrap* req, char* buf, size_t length) {
DEBUG_HTTP2("Http2Session: Attempting to send data\n");
if (stream_ == nullptr || !stream_->IsAlive() || stream_->IsClosing()) {
return;
}
HandleScope scope(env()->isolate());
auto AfterWrite = [](WriteWrap* req_wrap, int status) {
req_wrap->Dispose();
};
Local<Object> req_wrap_obj =
env()->write_wrap_constructor_function()
->NewInstance(env()->context()).ToLocalChecked();
WriteWrap* write_req = WriteWrap::New(env(),
req_wrap_obj,
this,
AfterWrite);

chunks_sent_since_last_write_++;
uv_buf_t actual = uv_buf_init(buf->base, length);
if (stream_->DoWrite(write_req, &actual, 1, nullptr)) {
write_req->Dispose();
uv_buf_t actual = uv_buf_init(buf, length);
if (stream_->DoWrite(req, &actual, 1, nullptr)) {
req->Dispose();
}
}

Expand Down
5 changes: 3 additions & 2 deletions src/node_http2.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,6 @@ class Http2Session : public AsyncWrap,
nghttp2_headers_category cat,
uint8_t flags) override;
void OnStreamClose(int32_t id, uint32_t code) override;
void Send(uv_buf_t* bufs, size_t total) override;
void OnDataChunk(Nghttp2Stream* stream, uv_buf_t* chunk) override;
void OnSettings(bool ack) override;
void OnPriority(int32_t stream,
Expand All @@ -398,7 +397,9 @@ class Http2Session : public AsyncWrap,
void OnFrameError(int32_t id, uint8_t type, int error_code) override;
void OnTrailers(Nghttp2Stream* stream,
const SubmitTrailers& submit_trailers) override;
void AllocateSend(uv_buf_t* buf) override;

void Send(WriteWrap* req, char* buf, size_t length) override;
WriteWrap* AllocateSend();

int DoWrite(WriteWrap* w, uv_buf_t* bufs, size_t count,
uv_stream_t* send_handle) override;
Expand Down
23 changes: 15 additions & 8 deletions src/node_http2_core-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,17 +490,22 @@ inline void Nghttp2Session::SendPendingData() {
if (IsDestroying())
return;

uv_buf_t dest;
AllocateSend(&dest);
WriteWrap* req = nullptr;
char* dest = nullptr;
size_t destRemaining = 0;
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) {
if (req == nullptr) {
req = AllocateSend();
destRemaining = req->self_size();
dest = req->Extra();
}
DEBUG_HTTP2("Nghttp2Session %s: nghttp2 has %d bytes to send\n",
TypeName(), srcLength);
size_t srcRemaining = srcLength;
Expand All @@ -512,18 +517,20 @@ inline void Nghttp2Session::SendPendingData() {
while (srcRemaining > destRemaining) {
DEBUG_HTTP2("Nghttp2Session %s: pushing %d bytes to the socket\n",
TypeName(), destLength + destRemaining);
memcpy(dest.base + destOffset, src + srcOffset, destRemaining);
memcpy(dest + destOffset, src + srcOffset, destRemaining);
destLength += destRemaining;
Send(&dest, destLength);
Send(req, dest, destLength);
destOffset = 0;
destLength = 0;
srcRemaining -= destRemaining;
srcOffset += destRemaining;
destRemaining = dest.len;
req = AllocateSend();
destRemaining = req->self_size();
dest = req->Extra();
}

if (srcRemaining > 0) {
memcpy(dest.base + destOffset, src + srcOffset, srcRemaining);
memcpy(dest + destOffset, src + srcOffset, srcRemaining);
destLength += srcRemaining;
destOffset += srcRemaining;
destRemaining -= srcRemaining;
Expand All @@ -535,7 +542,7 @@ inline void Nghttp2Session::SendPendingData() {
if (destLength > 0) {
DEBUG_HTTP2("Nghttp2Session %s: pushing %d bytes to the socket\n",
TypeName(), destLength);
Send(&dest, destLength);
Send(req, dest, destLength);
}
}

Expand Down
9 changes: 5 additions & 4 deletions src/node_http2_core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include "stream_base.h"
#include "util-inl.h"
#include "uv.h"
#include "nghttp2/nghttp2.h"
Expand Down Expand Up @@ -153,7 +154,6 @@ class Nghttp2Session {
// Removes a stream instance from this session
inline void RemoveStream(int32_t id);

virtual void Send(uv_buf_t* buf, size_t length) {}
virtual void OnHeaders(
Nghttp2Stream* stream,
std::queue<nghttp2_header>* headers,
Expand All @@ -176,7 +176,10 @@ class Nghttp2Session {
int error_code) {}
virtual ssize_t GetPadding(size_t frameLength,
size_t maxFrameLength) { return 0; }
virtual void AllocateSend(uv_buf_t* buf) = 0;

inline void SendPendingData();
virtual void Send(WriteWrap* req, char* buf, size_t length) = 0;
virtual WriteWrap* AllocateSend() = 0;

virtual bool HasGetPaddingCallback() { return false; }

Expand All @@ -199,8 +202,6 @@ class Nghttp2Session {
virtual void OnTrailers(Nghttp2Stream* stream,
const SubmitTrailers& submit_trailers) {}

inline void SendPendingData();

virtual uv_loop_t* event_loop() const = 0;

virtual void Close();
Expand Down
2 changes: 0 additions & 2 deletions test/parallel/parallel.status
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,5 @@ test-npm-install: PASS,FLAKY
[$system==solaris] # Also applies to SmartOS

[$system==freebsd]
test-http2-compat-serverrequest-pipe: PASS,FLAKY
test-http2-pipe: PASS,FLAKY

[$system==aix]