http2: improve perf of passing headers to C++ #14723

addaleax · 2017-08-09T23:18:53Z

By passing a single string rather than many small ones and a single block allocation for passing headers, save expensive interactions with JS values and memory allocations.

$ ./node benchmark/compare.js --new ./node --old ./node-master --runs 10 --filter headers http2 | Rscript benchmark/compare.R
[00:01:43|% 100| 1/1 files | 20/20 runs | 2/2 configs]: Done
                                        improvement confidence      p.value
 http2/headers.js nheaders=1000 n=1000     13.45 %        *** 5.548478e-12
 http2/headers.js nheaders=100 n=1000       5.11 %        *** 4.354236e-07

(Note: The benchmark has a lot of overhead since it does full request/response cycles. I don’t see any way around that right now, but the actual improvement of the changed code should be significantly higher than what’s displayed here.)

This is blocked by #14688 because the implementation for trailers isn’t changed here, but in that PR the two are unified so we won’t have to bother about that after that lands.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes (after src,http2: DRY header/trailer handling code up #14688)
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

http2

/cc @nodejs/http2

TimothyGu

Wonderful work!

TimothyGu · 2017-08-10T00:05:32Z

src/node_http2.cc

+
+  CHECK_EQ(StringBytes::Write(isolate,
+                              header_contents, header_string_len,
+                              header_string, ASCII), header_string_len);


I think using V8's String API should suffice (and be preferred) here, and it doesn't have the overhead StringBytes has.

Yes – done!

TimothyGu · 2017-08-10T00:09:43Z

src/node_http2.h

  }

 private:
-  MaybeStackBuffer<nghttp2_nv> headers_;
+  size_t count_;
+  MaybeStackBuffer<char> buf_;


It might be appropriate to enlarge the default allocated stack size, as the size is determined by the type and a constant. nghttp2_nv is obviously bigger than a char.

I think this buffer was really oversized before. I don’t know what the typical number of headers and their sizes would be, but I’m pretty sure 1024 headers were excessive :)

If this buffer is expected to contain the full string of headers, it may need to be quite a bit larger. There are real world cases of applications sending headers with > 1 MB of data.

but yes, 1024 header pairs is.... excessive ;-) (I'd just never got around to tuning the size of it)

What about alignment? A compound type like nghttp2_nv has more strict alignment requirements than char. It will probably work out alright in practice but by accident, not by design.

If this buffer is expected to contain the full string of headers, it may need to be quite a bit larger. There are real world cases of applications sending headers with > 1 MB of data.

It is expected to be able hold the full string of headers often enough. I’ve bumped this to 3000 bytes by default, that ought to be enough for everyone ;) Even if not, the worst things that happens is a single malloc() + free() combination.

What about alignment? A compound type like nghttp2_nv has more strict alignment requirements than char.

Yeah, I guess it would always work, but I’ve added code to account for that concern now anyway.

TimothyGu · 2017-08-10T00:12:03Z

src/node_http2.cc

+    if (n >= count_) {
+      // This can happen if a passed header contained a null byte. In that
+      // case, just provide nghttp2 with an invalid header to make it reject
+      // the headers list.


Would be good to have a test case against this.

There already is :) test/parallel/test-http2-client-unescaped-path.js, if you want to have a look.

Rather inserting a fake header, can we simply abort and set an invalid flag on the class that we can check for before using the Headers instance? That way we can return a proper error code back to the js layer without having to attempt sending the headers at all. Should be faster.

@jasnell Yes, we could do that, but is that really a case that we want to optimize for? Plus, it would create inconsistency with how we handle other invalid headers.

nah, you're right, this works.

jasnell · 2017-08-10T00:43:36Z

lib/internal/http2/util.js

@@ -415,16 +417,18 @@ function mapToHeaders(map,
      if (isArray) {
        for (var k = 0; k < value.length; k++) {
          val = String(value[k]);
-          ret.push([key, val]);
+          ret += `${key}\0${val}\0`;


This presupposes that neither the key nor the val contain null characters. That may not be a safe assumption to make

@jasnell #14723 (comment)

Yeah, saw that as I read further :-)

jasnell · 2017-08-10T00:52:31Z

There's one consideration we need to make on this before going this route. The HTTP/2 header compression mechanism allows the sender to decide, on a header-by-header basis, whether or not the header pair should be stored in the stateful header compression table (it's one of the flags that can be set on the nva->flags for each individual header entry. I had not yet implemented support for this in the API, causing all headers name value pairs to be stored in the table. There are very legitimate security reasons why a developer may not wish to have a header stored in the table and we will eventually need to provide some API for allowing users to specify the flag. This would need to be done in some way via the mapToHeaders function.

jasnell · 2017-08-10T00:54:49Z

lib/internal/http2/util.js

      } else {
        val = String(value);
-        ret.push([key, val]);


A big part of the reason this was an array is because there will be a third value that needs to be passed in at some point... specifically, a flag that indicates whether or not the header pair should be stored in the header compression table

I suppose we can add another NUL-delimited column if we were to implement that.

We could even just add a single character/byte per header to the string.

single byte (bit-flag) per header should work.

In fact, why don't we go ahead and add that extra byte now as a reserved field in the structure.

One idea for the API... right now headers are set as an object.. e.g.

{ ':status': 200, 'content-type': 'text/plain', 'foo': 'bar' }

We could (not saying should) allow the value to be wrapped somehow... e.g.

{ ':status': 200, 'content-type': http2.flaggedHeader('text/plain', { store: false }), 'foo': 'bar' }

This feels a bit awkward but it optimizes for the common case and API familiarity and only adds the weirdness when someone really does want to use the more advanced feature.

Or, alternatively... (just brainstorming here ;-) ...)

{ ':status': 200, 'foo': 'bar', [http2.noStore]: { 'content-type': 'text/plain' } }

Where 'http2.noStore is a Symbol. This still feels weird, but it eliminates the potential polymorphism in the default case.

The other option is to change the headers API entirely and use a Headers object... e.g.

const headers = new http2.Headers(); headers.set(':status', 200); headers.set('content-type', 'text/plain', { store: false }); headers.set('foo', 'bar');

This has it's whole own set of issues, including being slower than using an object literal.

@jasnell I do like the second approach, for the reason you mentioned. :) Do you think we might ever want to add more options than NGHTTP2_NV_FLAG_NO_INDEX? I don’t the NO_COPY flags would be set by userland, so we’re good on that, right?

The NO_INDEX flag is likely the only one we're going to need. I would be quite surprised if any new flag was added here.

@jasnell Prototyped this out at addaleax/node@bb2fb6c, but I’d say it’s different enough to leave it for another PR.

jasnell · 2017-08-10T01:02:36Z

test/parallel/test-http2-util-headers-list.js

+    mapToHeaders(headers),
+    [ ':path_abc_:status_200_abc_1_xyz_1_xyz_2_xyz_3_xyz_4_bar_1_'
+        .replace(/_/g, '\0'),
+      8 ]


using mapToHeaders(headers).split('\0') and keeping the second argument as an array (albeit not an array of arrays) may be easier to read

Using Array#join was a bit easier, but yes, good idea – done!

By passing a single string rather than many small ones and a single block allocation for passing headers, save expensive interactions with JS values and memory allocations.

addaleax · 2017-08-10T12:04:02Z

I had not yet implemented support for this in the API, causing all headers name value pairs to be stored in the table.

If you have suggestions for how that might be implemented in the API, I volunteer. :)

tniessen · 2017-08-10T12:03:52Z

lib/internal/http2/util.js

      }
    }
  }

-  return ret;
+  return [ ret, count ];


IMO we should go for consistent styling here (no spaces). By the way, I would actually like to enable linting against these with array-bracket-spacing: [error, never] (only 7 errors in lib/*, but quite some churn in test).

Ugh, done. I’d like to point out that, while I know arrays and objects are different things, we just enabled the opposite lint rule for objects.

I know, but as far as I know the house style is still [a, b, c]. To be honest, I personally prefer [ a, b, c ] over [a, b, c], but enforcing spacing within arrays would cause over 7400 errors within our code. But as the majority seems to prefer arrays without spaces anyway, I would go for consistency, even though @refack warned me that a PR might not be too successful, depending on the churn.

addaleax · 2017-08-10T12:08:55Z

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

jasnell · 2017-08-10T17:13:03Z

Another something to keep in mind, currently, we take the strings and copy them into the nghttp2_nv structure, pass those off to nghttp2, then free the structure allocations. Because nghttp2 may not immediately send those headers, by default it will copy the name and value into it's own internal buffer that is freed when it actually sends the header pairs. That means for every header pair, there are two copies made.

We can eliminate the need for the extra copy by managing that memory ourselves and telling nghttp2 not to copy the name and value using a flag on the nghttp2_nv struct. We would need to add a callback to know when the headers are sent so we know when to free, but it could help shave off some time. It's worth looking into.

jasnell · 2017-08-10T17:15:05Z

If you have suggestions for how that might be implemented in the API, I volunteer. :)

Heh... that's more difficult. It would completely change the way headers are currently set and make it inconsistent with the way they are handled in the http/1 API. I'd played around with this a bit early on and decided to back off for the time being. Essentially, if we want to support this, then we'll need to go with a new API design.

bnoordhuis · 2017-08-11T07:55:24Z

src/node_http2.cc

+  // Make sure the start address is aligned appropriately for an nghttp2_nv*.
+  char* start = reinterpret_cast<char*>(
+      ROUND_UP(reinterpret_cast<uintptr_t>(*buf_), alignof(nghttp2_nv)));
+  char* header_contents = start + (count_ * sizeof(nghttp2_nv));


Parens aren't necessary here. (They don't hurt either, of course.)

addaleax · 2017-08-12T19:36:26Z

@jasnell Did some reading up & thinking about noStore; and basically:

Support for it is not an optional part of HTTP/2, it’s something that’s necessary so that headers can contain sensitive information, e.g. cookies
Our APIs that provide headers read from a stream need to provide the information of whether the flag was set or not
The compatibility API needs to do something reasonable here, and that may be hard. My initial guess would be that we’d want a whitelist of the standard headers that are not usually expected to contain sensitive information, and set the flag for all other headers.

I’m landing this PR soon, I’ll be working a bit on the above, and we’ll see where we get.

addaleax · 2017-08-12T19:44:09Z

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

jasnell · 2017-08-12T22:11:17Z

Agreed on all points. The key issue will be the ergonomics of the api we expose.

mcollina · 2017-08-13T05:53:44Z

@addaleax can you link your research? I'm unconvinced that we should not store cookies in the compressed headers. I do not see an attack scenario where storying cookies would couse less security that not storying them (or using HTTP1). There are little best practices around HTTP2, so we can expect all of this to change.

IMHO we should not make any decisions for our users on both layers, but provide an API (this is going to be hard, I agree), to help them help themselves.

addaleax · 2017-08-13T14:08:39Z

can you link your research?

@mcollina That would be mostly going by https://http2.github.io/http2-spec/compression.html#compression.based.attacks, which does explicitly mention Cookies and Authorization headers as examples of headers that implementations may choose not to want to index.

An intermediary MUST NOT re-encode a value that uses the never-indexed literal representation with another representation that would index it. If HPACK is used for re-encoding, the never-indexed literal representation MUST be used.

I think this expectation makes support for the flag a requirement for a full-featured HTTP/2 API.

I'm unconvinced that we should not store cookies in the compressed headers.

I am not convinced either but would prefer to err on the side of caution in security issues.

IMHO we should not make any decisions for our users on both layers

I think for the compat API that’s literally impossible because we’re always adding a bit of information that is not present in the HTTP/1 API, no matter how implicitly or explicitly we do that. ;)

but provide an API (this is going to be hard, I agree), to help them help themselves.

I don’t think anybody is disagreeing with you on this :)

addaleax · 2017-08-13T14:18:15Z

I don’t mind continuing this discussion here, but to say something on-topic for a change:

Landed in 348dd66 ;)

By passing a single string rather than many small ones and a single block allocation for passing headers, save expensive interactions with JS values and memory allocations. PR-URL: #14723 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

By passing a single string rather than many small ones and a single block allocation for passing headers, save expensive interactions with JS values and memory allocations. PR-URL: nodejs#14723 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

By passing a single string rather than many small ones and a single block allocation for passing headers, save expensive interactions with JS values and memory allocations. PR-URL: #14723 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

addaleax added blocked PRs that are blocked by other issues or PRs. http2 Issues or PRs related to the http2 subsystem. performance Issues and PRs related to the performance of Node.js. labels Aug 9, 2017

addaleax requested a review from jasnell August 9, 2017 23:18

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. labels Aug 9, 2017

TimothyGu reviewed Aug 10, 2017

View reviewed changes

jasnell reviewed Aug 10, 2017

View reviewed changes

addaleax removed the blocked PRs that are blocked by other issues or PRs. label Aug 10, 2017

addaleax added 4 commits August 10, 2017 13:51

http2: improve perf of passing headers to C++

f6d3a00

By passing a single string rather than many small ones and a single block allocation for passing headers, save expensive interactions with JS values and memory allocations.

[squash] use WriteOneByte

327da38

[squash] bump buffer size to 3000 bytes

d02a8a7

[squash] address alignment concern

a9d9f6d

addaleax force-pushed the http2-better-header-passing branch from cbc6729 to a9d9f6d Compare August 10, 2017 11:54

[squash] use arrays in test again

eea3825

tniessen reviewed Aug 10, 2017

View reviewed changes

[squash] whitespace nit

b7080cf

[squash] fix linting (oops)

95964a1

jasnell approved these changes Aug 11, 2017

View reviewed changes

bnoordhuis approved these changes Aug 11, 2017

View reviewed changes

addaleax closed this Aug 13, 2017

addaleax deleted the http2-better-header-passing branch August 13, 2017 14:18

addaleax mentioned this pull request Aug 14, 2017

v8.4.0 proposal #14811

Merged

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

http2: improve perf of passing headers to C++ #14723

http2: improve perf of passing headers to C++ #14723

Conversation

addaleax commented Aug 9, 2017

Checklist

Affected core subsystem(s)

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

jasnell commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Aug 10, 2017

jasnell commented Aug 10, 2017

jasnell commented Aug 10, 2017

Choose a reason for hiding this comment

addaleax commented Aug 12, 2017 • edited Loading

addaleax commented Aug 12, 2017

jasnell commented Aug 12, 2017

mcollina commented Aug 13, 2017

addaleax commented Aug 13, 2017

addaleax commented Aug 13, 2017

jasnell Aug 10, 2017 •

edited

Loading

jasnell Aug 10, 2017 •

edited

Loading

addaleax commented Aug 12, 2017 •

edited

Loading