Skip to content

http2: allocate on every chunk send #16669

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

Closed
jasnell wants to merge 2 commits into from
Closed

http2: allocate on every chunk send #16669

Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
[Squash] address comments
  • Loading branch information
@jasnell
jasnell committed Nov 2, 2017
commit 1c6ec13d8027fbba6ff84bf75f55841358bda217
6 changes: 3 additions & 3 deletions src/node_http2.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,7 +893,7 @@ int Http2Session::DoWrite(WriteWrap* req_wrap,
return 0;
}

size_t Http2Session::AllocateSend(WriteWrap** req) {
WriteWrap* Http2Session::AllocateSend() {
HandleScope scope(env()->isolate());
auto AfterWrite = [](WriteWrap* req, int status) {
req->Dispose();
Expand All @@ -906,8 +906,8 @@ size_t Http2Session::AllocateSend(WriteWrap** req) {
nghttp2_session_get_remote_settings(
session(),
NGHTTP2_SETTINGS_MAX_FRAME_SIZE);
Copy link
Copy Markdown
Contributor

@apapirovski apapirovski Nov 1, 2017

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
Copy Markdown
Member Author

@jasnell jasnell Nov 1, 2017

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

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

void Http2Session::Send(WriteWrap* req, char* buf, size_t length) {
Expand Down
2 changes: 1 addition & 1 deletion src/node_http2.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ class Http2Session : public AsyncWrap,
const SubmitTrailers& submit_trailers) override;

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

int DoWrite(WriteWrap* w, uv_buf_t* bufs, size_t count,
uv_stream_t* send_handle) override;
Expand Down
6 changes: 4 additions & 2 deletions src/node_http2_core-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,8 @@ inline void Nghttp2Session::SendPendingData() {
// While srcLength is greater than zero
while ((srcLength = nghttp2_session_mem_send(session_, &src)) > 0) {
if (req == nullptr) {
destRemaining = AllocateSend(&req);
req = AllocateSend();
destRemaining = req->self_size();
dest = req->Extra();
}
DEBUG_HTTP2("Nghttp2Session %s: nghttp2 has %d bytes to send\n",
Expand All @@ -523,7 +524,8 @@ inline void Nghttp2Session::SendPendingData() {
destLength = 0;
srcRemaining -= destRemaining;
srcOffset += destRemaining;
destRemaining = AllocateSend(&req);
req = AllocateSend();
destRemaining = req->self_size();
dest = req->Extra();
}

Expand Down
2 changes: 1 addition & 1 deletion src/node_http2_core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ class Nghttp2Session {

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

virtual bool HasGetPaddingCallback() { return false; }

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]