http2: allocate on every chunk send #16669

jasnell · 2017-11-01T18:14:56Z

Previously, we were using a shared stack allocated buffer to hold
the serialized outbound data but that runs into issues if the
outgoing stream does not write or copy immediately. Instead,
allocate a buffer each time. Slight additional overhead here,
but necessary.

Later on, once we've analyzed this more, we might be able to
switch to a stack allocated ring or slab buffer but that's a
bit more complicated than what we strictly need right now.

@apapirovski ... can you see if this resolves the issue you were having the other day with the frame size weirdness?

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

http2

apapirovski

👍 LGTM — just one minor thing that needs some discussion / to be addressed

apapirovski · 2017-11-01T20:47:48Z

src/node_http2.cc

+  uint32_t size =
+      nghttp2_session_get_remote_settings(
+          session(),
+          NGHTTP2_SETTINGS_MAX_FRAME_SIZE);


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.

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

apapirovski · 2017-11-01T21:02:18Z

Also, haven't tested on FreeBSD yet — will later tonight. If this does resolve it, I think it would be nice to have another commit in this PR that unmarks those tests as flaky.

apapirovski · 2017-11-01T22:24:10Z

@jasnell Can confirm this fixes FreeBSD issues. Could you add a commit that unmarks the two pipe tests as flaky?

addaleax · 2017-11-01T22:44:34Z

src/node_http2.cc

+          session(),
+          NGHTTP2_SETTINGS_MAX_FRAME_SIZE);
+  *req = WriteWrap::New(env(), obj, this, AfterWrite, size);
+  return size;


I would rather have this return the WriteWrap* and instead add a method on that class that returns size, if that sounds okay?

Yep, that works also

jasnell · 2017-11-01T23:14:49Z

Awesome, will unmark those tests!

Previously, we were using a shared stack allocated buffer to hold the serialized outbound data but that runs into issues if the outgoing stream does not write or copy immediately. Instead, allocate a buffer each time. Slight additional overhead here, but necessary. Later on, once we've analyzed this more, we might be able to switch to a stack allocated ring or slab buffer but that's a bit more complicated than what we strictly need right now.

jasnell · 2017-11-02T05:54:06Z

@apapirovski @addaleax ... updated :-)

jasnell · 2017-11-02T05:55:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11140/

mcollina

LGTM

Previously, we were using a shared stack allocated buffer to hold the serialized outbound data but that runs into issues if the outgoing stream does not write or copy immediately. Instead, allocate a buffer each time. Slight additional overhead here, but necessary. Later on, once we've analyzed this more, we might be able to switch to a stack allocated ring or slab buffer but that's a bit more complicated than what we strictly need right now. PR-URL: #16669 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell · 2017-11-03T17:30:02Z

Landed in 4db1bc8

richardlau · 2017-11-03T18:41:52Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11140/

@jasnell This appears to have landed with failing tests in the CI, e.g. https://ci.nodejs.org/job/node-test-commit-linuxone/9931/nodes=rhel72-s390x/console

jasnell · 2017-11-03T19:08:44Z

Hmmm ok, likely overlooked that due to some other flaky tests. Can check it out shortly. Finishing a flight currently. Will be back on terminal wifi in the next 30 minutes or so

`WriteWrap` instances may contain extra storage space. `self_size()` returns the size of the *entire* struct, member fields as well as storage space, so it is not an accurate measure for the storage space available. Add a method `ExtraSize()` (like the existing `Extra()` for accessing the storage memory) that yields the wanted value, and use it in the HTTP2 impl to fix a crash. Ref: nodejs#16669

addaleax · 2017-11-03T19:30:07Z

Fix should be in #16727

`WriteWrap` instances may contain extra storage space. `self_size()` returns the size of the *entire* struct, member fields as well as storage space, so it is not an accurate measure for the storage space available. Add a method `ExtraSize()` (like the existing `Extra()` for accessing the storage memory) that yields the wanted value, and use it in the HTTP2 impl to fix a crash. PR-URL: #16727 Refs: #16669 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Previously, we were using a shared stack allocated buffer to hold the serialized outbound data but that runs into issues if the outgoing stream does not write or copy immediately. Instead, allocate a buffer each time. Slight additional overhead here, but necessary. Later on, once we've analyzed this more, we might be able to switch to a stack allocated ring or slab buffer but that's a bit more complicated than what we strictly need right now. PR-URL: nodejs#16669 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

`WriteWrap` instances may contain extra storage space. `self_size()` returns the size of the *entire* struct, member fields as well as storage space, so it is not an accurate measure for the storage space available. Add a method `ExtraSize()` (like the existing `Extra()` for accessing the storage memory) that yields the wanted value, and use it in the HTTP2 impl to fix a crash. PR-URL: nodejs#16727 Refs: nodejs#16669 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Previously, we were using a shared stack allocated buffer to hold the serialized outbound data but that runs into issues if the outgoing stream does not write or copy immediately. Instead, allocate a buffer each time. Slight additional overhead here, but necessary. Later on, once we've analyzed this more, we might be able to switch to a stack allocated ring or slab buffer but that's a bit more complicated than what we strictly need right now. PR-URL: #16669 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

`WriteWrap` instances may contain extra storage space. `self_size()` returns the size of the *entire* struct, member fields as well as storage space, so it is not an accurate measure for the storage space available. Add a method `ExtraSize()` (like the existing `Extra()` for accessing the storage memory) that yields the wanted value, and use it in the HTTP2 impl to fix a crash. PR-URL: #16727 Refs: #16669 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Previously, we were using a shared stack allocated buffer to hold the serialized outbound data but that runs into issues if the outgoing stream does not write or copy immediately. Instead, allocate a buffer each time. Slight additional overhead here, but necessary. Later on, once we've analyzed this more, we might be able to switch to a stack allocated ring or slab buffer but that's a bit more complicated than what we strictly need right now. PR-URL: #16669 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

`WriteWrap` instances may contain extra storage space. `self_size()` returns the size of the *entire* struct, member fields as well as storage space, so it is not an accurate measure for the storage space available. Add a method `ExtraSize()` (like the existing `Extra()` for accessing the storage memory) that yields the wanted value, and use it in the HTTP2 impl to fix a crash. PR-URL: #16727 Refs: #16669 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 1, 2017

jasnell requested review from apapirovski, mcollina, sebdeckers and addaleax November 1, 2017 18:18

apapirovski approved these changes Nov 1, 2017

View reviewed changes

addaleax approved these changes Nov 1, 2017

View reviewed changes

addaleax added the lts-watch-v8.x label Nov 1, 2017

jasnell added 2 commits November 1, 2017 22:53

[Squash] address comments

1c6ec13

jasnell force-pushed the http2-refactor-send branch from afddd09 to 1c6ec13 Compare November 2, 2017 05:53

XadillaX approved these changes Nov 2, 2017

View reviewed changes

mcollina approved these changes Nov 2, 2017

View reviewed changes

jasnell closed this Nov 3, 2017

addaleax mentioned this pull request Nov 3, 2017

src: add method to compute storage in WriteWrap #16727

Closed

3 tasks

cjihrig mentioned this pull request Nov 6, 2017

v9.1.0 proposal #16851

Merged

MylesBorins added the dont-land-on-v6.x label Nov 14, 2017

MylesBorins added land-on-v8.x and removed lts-watch-v8.x labels Dec 7, 2017

MylesBorins mentioned this pull request Dec 7, 2017

v8.9.3 proposal #17532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: allocate on every chunk send #16669

http2: allocate on every chunk send #16669

jasnell commented Nov 1, 2017

apapirovski left a comment

apapirovski Nov 1, 2017

jasnell Nov 1, 2017

apapirovski commented Nov 1, 2017 •

edited

Loading

apapirovski commented Nov 1, 2017

addaleax Nov 1, 2017

jasnell Nov 1, 2017

jasnell commented Nov 1, 2017

jasnell commented Nov 2, 2017

jasnell commented Nov 2, 2017

mcollina left a comment

jasnell commented Nov 3, 2017

richardlau commented Nov 3, 2017

jasnell commented Nov 3, 2017

addaleax commented Nov 3, 2017

http2: allocate on every chunk send #16669

http2: allocate on every chunk send #16669

Conversation

jasnell commented Nov 1, 2017

Checklist

Affected core subsystem(s)

apapirovski left a comment

Choose a reason for hiding this comment

apapirovski Nov 1, 2017

Choose a reason for hiding this comment

jasnell Nov 1, 2017

Choose a reason for hiding this comment

apapirovski commented Nov 1, 2017 • edited Loading

apapirovski commented Nov 1, 2017

addaleax Nov 1, 2017

Choose a reason for hiding this comment

jasnell Nov 1, 2017

Choose a reason for hiding this comment

jasnell commented Nov 1, 2017

jasnell commented Nov 2, 2017

jasnell commented Nov 2, 2017

mcollina left a comment

Choose a reason for hiding this comment

jasnell commented Nov 3, 2017

richardlau commented Nov 3, 2017

jasnell commented Nov 3, 2017

addaleax commented Nov 3, 2017

apapirovski commented Nov 1, 2017 •

edited

Loading