http2: improve perf of passing headers to JS #14808

addaleax · 2017-08-13T14:54:04Z

Make ExternalString::New return a MaybeLocal. Failing is left up to the caller.
Allow creating internalized strings for short header names to reduce memory consumption and increase performance.
Use persistent storage for statically allocated header names.

Each of the latter two gives about 2.5 % perf in the benchmark (which, again, does a lot more than just reading headers from the stream.)

$ ./node benchmark/compare.js --new ./node --old ./node-master-348dd66337e --runs 10 --filter headers http2 | Rscript benchmark/compare.R
[00:00:27|% 100| 1/1 files | 20/20 runs | 2/2 configs]: Done
                                      improvement confidence      p.value
 http2/headers.js nheaders=0 n=1000       4.01 %        *** 1.890685e-05
 http2/headers.js nheaders=10 n=1000      5.19 %        *** 8.936262e-06

This does not yet include any work regarding never-indexed header fields, but it should be sufficiently independent from that anyway.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

@nodejs/http2

Original commit message: lib: add nghttp2_rcbuf_is_static() Add a `nghttp2_rcbuf_is_static()` method to tell whether a rcbuf is statically allocated. This can be useful for language bindings that wish to avoid creating duplicate strings for these buffers; concretely, I am planning to use this in the Node HTTP/2 module that is being introduced. Ref: nghttp2/nghttp2@eb306f4

- Make `ExternalString::New` return a `MaybeLocal`. Failing is left up to the caller. - Allow creating internalized strings for short header names to reduce memory consumption and increase performance. - Use persistent storage for statically allocated header names.

TimothyGu · 2017-08-13T15:03:45Z

src/node_http2.h


-    if (str.IsEmpty()) {
+    ExternalHeader* h_str = new ExternalHeader(buf);
+    MaybeLocal<String> str = String::NewExternalOneByte(env->isolate(), h_str);


Is the AdjustAmountOfExternalAllocatedMemory call deliberately removed?

Yes, because we did not inform V8 about the reverse situation in which we freed the corresponding memory. We either need to do both calls or none of them.

It was a bogus measure anyway, the buffers are reference counted and often can be the same. A much better thing to do would be to provide custom malloc functions to nghttp2, so we can actually watch allocations happen and report them to V8, but that’s a different story.

jasnell · 2017-08-13T15:53:58Z

Nice :) side note: just in case you hadn't seen it yet, we can give nghttp2 a custom allocator.

addaleax · 2017-08-13T15:55:04Z

@jasnell Yes, but … so many things to do ;)

jasnell · 2017-08-13T15:55:23Z

Rather than cherry picking an upstream commit, we may as well update the entire library at this point.

addaleax · 2017-08-13T15:56:55Z

@jasnell Idk, I would be completely fine with that but don’t we usually wait for releases to do full upstream updates?

jasnell · 2017-08-13T15:59:34Z

Yeah, we're currently on 1.22 but the current is 1.24 I think.

mcollina

LGTM

addaleax · 2017-08-17T18:22:57Z

CI: https://ci.nodejs.org/job/node-test-commit/11865/

This should be ready.

addaleax · 2017-08-17T20:56:36Z

Landed in 4766fcb, 0d9afa0

Original commit message: lib: add nghttp2_rcbuf_is_static() Add a `nghttp2_rcbuf_is_static()` method to tell whether a rcbuf is statically allocated. This can be useful for language bindings that wish to avoid creating duplicate strings for these buffers; concretely, I am planning to use this in the Node HTTP/2 module that is being introduced. Ref: nghttp2/nghttp2@eb306f4 PR-URL: #14808 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

- Make `ExternalString::New` return a `MaybeLocal`. Failing is left up to the caller. - Allow creating internalized strings for short header names to reduce memory consumption and increase performance. - Use persistent storage for statically allocated header names. PR-URL: #14808 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

Original commit message: lib: add nghttp2_rcbuf_is_static() Add a `nghttp2_rcbuf_is_static()` method to tell whether a rcbuf is statically allocated. This can be useful for language bindings that wish to avoid creating duplicate strings for these buffers; concretely, I am planning to use this in the Node HTTP/2 module that is being introduced. Ref: nghttp2/nghttp2@eb306f4 PR-URL: #14808 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

- Make `ExternalString::New` return a `MaybeLocal`. Failing is left up to the caller. - Allow creating internalized strings for short header names to reduce memory consumption and increase performance. - Use persistent storage for statically allocated header names. PR-URL: #14808 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

Original commit message: lib: add nghttp2_rcbuf_is_static() Add a `nghttp2_rcbuf_is_static()` method to tell whether a rcbuf is statically allocated. This can be useful for language bindings that wish to avoid creating duplicate strings for these buffers; concretely, I am planning to use this in the Node HTTP/2 module that is being introduced. Ref: nghttp2/nghttp2@eb306f4 PR-URL: #14808 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

- Make `ExternalString::New` return a `MaybeLocal`. Failing is left up to the caller. - Allow creating internalized strings for short header names to reduce memory consumption and increase performance. - Use persistent storage for statically allocated header names. PR-URL: #14808 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

addaleax added 2 commits August 13, 2017 16:33

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 Aug 13, 2017

addaleax added http2 Issues or PRs related to the http2 subsystem. performance Issues and PRs related to the performance of Node.js. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 13, 2017

TimothyGu reviewed Aug 13, 2017

View reviewed changes

jasnell approved these changes Aug 13, 2017

View reviewed changes

TimothyGu approved these changes Aug 13, 2017

View reviewed changes

mcollina approved these changes Aug 16, 2017

View reviewed changes

addaleax closed this Aug 17, 2017

addaleax deleted the http2-headers-to-js branch August 17, 2017 20:56

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the dont-land-on-v6.x label Sep 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: improve perf of passing headers to JS #14808

http2: improve perf of passing headers to JS #14808

addaleax commented Aug 13, 2017 •

edited

Loading

TimothyGu Aug 13, 2017

addaleax Aug 13, 2017

jasnell commented Aug 13, 2017

addaleax commented Aug 13, 2017

jasnell commented Aug 13, 2017

addaleax commented Aug 13, 2017

jasnell commented Aug 13, 2017

mcollina left a comment

addaleax commented Aug 17, 2017

addaleax commented Aug 17, 2017

http2: improve perf of passing headers to JS #14808

http2: improve perf of passing headers to JS #14808

Conversation

addaleax commented Aug 13, 2017 • edited Loading

Checklist

Affected core subsystem(s)

TimothyGu Aug 13, 2017

Choose a reason for hiding this comment

addaleax Aug 13, 2017

Choose a reason for hiding this comment

jasnell commented Aug 13, 2017

addaleax commented Aug 13, 2017

jasnell commented Aug 13, 2017

addaleax commented Aug 13, 2017

jasnell commented Aug 13, 2017

mcollina left a comment

Choose a reason for hiding this comment

addaleax commented Aug 17, 2017

addaleax commented Aug 17, 2017

addaleax commented Aug 13, 2017 •

edited

Loading