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: improve perf of passing headers to C++ #14723

Closed
wants to merge 7 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f6d3a00
http2: improve perf of passing headers to C++
addaleax Aug 9, 2017
327da38
[squash] use WriteOneByte
addaleax Aug 10, 2017
d02a8a7
[squash] bump buffer size to 3000 bytes
addaleax Aug 10, 2017
a9d9f6d
[squash] address alignment concern
addaleax Aug 10, 2017
eea3825
[squash] use arrays in test again
addaleax Aug 10, 2017
b7080cf
[squash] whitespace nit
addaleax Aug 10, 2017
95964a1
[squash] fix linting (oops)
addaleax Aug 10, 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
47 changes: 47 additions & 0 deletions benchmark/http2/headers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
'use strict';

const common = require('../common.js');
const PORT = common.PORT;

var bench = common.createBenchmark(main, {
n: [1e3],
nheaders: [100, 1000],
}, { flags: ['--expose-http2', '--no-warnings'] });

function main(conf) {
const n = +conf.n;
const nheaders = +conf.nheaders;
const http2 = require('http2');
const server = http2.createServer();

const headersObject = { ':path': '/' };
for (var i = 0; i < nheaders; i++) {
headersObject[`foo${i}`] = `some header value ${i}`;
}

server.on('stream', (stream) => {
stream.respond();
stream.end('Hi!');
});
server.listen(PORT, () => {
const client = http2.connect(`http://localhost:${PORT}/`);

function doRequest(remaining) {
const req = client.request(headersObject);
req.end();
req.on('data', () => {});
req.on('end', () => {
if (remaining > 0) {
doRequest(remaining - 1);
} else {
bench.end(n);
server.close();
client.destroy();
}
});
}

bench.start();
doRequest(n);
});
}
14 changes: 9 additions & 5 deletions lib/internal/http2/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,8 @@ function assertValidPseudoHeaderTrailer(key) {

function mapToHeaders(map,
assertValuePseudoHeader = assertValidPseudoHeader) {
const ret = [];
let ret = '';
let count = 0;
const keys = Object.keys(map);
const singles = new Set();
for (var i = 0; i < keys.length; i++) {
Expand All @@ -402,7 +403,8 @@ function mapToHeaders(map,
const err = assertValuePseudoHeader(key);
if (err !== undefined)
return err;
ret.unshift([key, String(value)]);
ret = `${key}\0${String(value)}\0${ret}`;
count++;
} else {
if (kSingleValueHeaders.has(key)) {
if (singles.has(key))
Expand All @@ -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`;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@jasnell #14723 (comment)

Copy link
Member

@jasnell jasnell Aug 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, saw that as I read further :-)

}
count += value.length;
} else {
val = String(value);
ret.push([key, val]);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

ret += `${key}\0${val}\0`;
count++;
}
}
}

return ret;
return [ ret, count ];
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

}

class NghttpError extends Error {
Expand Down
65 changes: 65 additions & 0 deletions src/node_http2.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ using v8::Boolean;
using v8::Context;
using v8::Function;
using v8::Integer;
using v8::String;
using v8::Uint32;
using v8::Undefined;

namespace http2 {
Expand Down Expand Up @@ -1075,6 +1077,69 @@ void Http2Session::Unconsume() {
}


Headers::Headers(Isolate* isolate,
Local<Context> context,
Local<Array> headers) {
CHECK_EQ(headers->Length(), 2);
Local<Value> header_string = headers->Get(context, 0).ToLocalChecked();
Local<Value> header_count = headers->Get(context, 1).ToLocalChecked();
CHECK(header_string->IsString());
CHECK(header_count->IsUint32());
count_ = header_count.As<Uint32>()->Value();
int header_string_len = header_string.As<String>()->Length();

if (count_ == 0) {
CHECK_EQ(header_string_len, 0);
return;
}

// Allocate a single buffer with count_ nghttp2_nv structs, followed
// by the raw header data as passed from JS. This looks like:
// | possible padding | nghttp2_nv | nghttp2_nv | ... | header contents |
buf_.AllocateSufficientStorage((alignof(nghttp2_nv) - 1) +
count_ * sizeof(nghttp2_nv) +
header_string_len);
// 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));
Copy link
Member

Choose a reason for hiding this comment

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

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

nghttp2_nv* const nva = reinterpret_cast<nghttp2_nv*>(start);

CHECK_LE(header_contents + header_string_len, *buf_ + buf_.length());
CHECK_EQ(header_string.As<String>()
->WriteOneByte(reinterpret_cast<uint8_t*>(header_contents),
0, header_string_len,
String::NO_NULL_TERMINATION),
header_string_len);

size_t n = 0;
char* p;
for (p = header_contents; p < header_contents + header_string_len; n++) {
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.
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to have a test case against this.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

nah, you're right, this works.

static uint8_t zero = '\0';
nva[0].name = nva[0].value = &zero;
nva[0].namelen = nva[0].valuelen = 1;
count_ = 1;
return;
}

nva[n].flags = NGHTTP2_NV_FLAG_NONE;
nva[n].name = reinterpret_cast<uint8_t*>(p);
nva[n].namelen = strlen(p);
p += nva[n].namelen + 1;
nva[n].value = reinterpret_cast<uint8_t*>(p);
nva[n].valuelen = strlen(p);
p += nva[n].valuelen + 1;
}

CHECK_EQ(p, header_contents + header_string_len);
CHECK_EQ(n, count_);
}


void Initialize(Local<Object> target,
Local<Value> unused,
Local<Context> context,
Expand Down
46 changes: 6 additions & 40 deletions src/node_http2.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,54 +515,20 @@ class ExternalHeader :

class Headers {
public:
Headers(Isolate* isolate, Local<Context> context, Local<Array> headers) {
headers_.AllocateSufficientStorage(headers->Length());
Local<Value> item;
Local<Array> header;

for (size_t n = 0; n < headers->Length(); n++) {
item = headers->Get(context, n).ToLocalChecked();
CHECK(item->IsArray());
header = item.As<Array>();
Local<Value> key = header->Get(context, 0).ToLocalChecked();
Local<Value> value = header->Get(context, 1).ToLocalChecked();
CHECK(key->IsString());
CHECK(value->IsString());
size_t keylen = StringBytes::StorageSize(isolate, key, ASCII);
size_t valuelen = StringBytes::StorageSize(isolate, value, ASCII);
headers_[n].flags = NGHTTP2_NV_FLAG_NONE;
Local<Value> flag = header->Get(context, 2).ToLocalChecked();
if (flag->BooleanValue(context).ToChecked())
headers_[n].flags |= NGHTTP2_NV_FLAG_NO_INDEX;
uint8_t* buf = Malloc<uint8_t>(keylen + valuelen);
headers_[n].name = buf;
headers_[n].value = buf + keylen;
headers_[n].namelen =
StringBytes::Write(isolate,
reinterpret_cast<char*>(headers_[n].name),
keylen, key, ASCII);
headers_[n].valuelen =
StringBytes::Write(isolate,
reinterpret_cast<char*>(headers_[n].value),
valuelen, value, ASCII);
}
}

~Headers() {
for (size_t n = 0; n < headers_.length(); n++)
free(headers_[n].name);
}
Headers(Isolate* isolate, Local<Context> context, Local<Array> headers);
~Headers() {}

nghttp2_nv* operator*() {
return *headers_;
return reinterpret_cast<nghttp2_nv*>(*buf_);
}

size_t length() const {
return headers_.length();
return count_;
}

private:
MaybeStackBuffer<nghttp2_nv> headers_;
size_t count_;
MaybeStackBuffer<char, 3000> buf_;
};

} // namespace http2
Expand Down
58 changes: 22 additions & 36 deletions test/parallel/test-http2-util-headers-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,16 +82,12 @@ const {
'BAR': [1]
};

assert.deepStrictEqual(mapToHeaders(headers), [
[ ':path', 'abc' ],
[ ':status', '200' ],
[ 'abc', '1' ],
[ 'xyz', '1' ],
[ 'xyz', '2' ],
[ 'xyz', '3' ],
[ 'xyz', '4' ],
[ 'bar', '1' ]
]);
assert.deepStrictEqual(
mapToHeaders(headers),
[ ':path_abc_:status_200_abc_1_xyz_1_xyz_2_xyz_3_xyz_4_bar_1_'
.replace(/_/g, '\0'),
8 ]
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

);
}

{
Expand All @@ -103,15 +99,12 @@ const {
'xyz': [1, 2, 3, 4]
};

assert.deepStrictEqual(mapToHeaders(headers), [
[ ':status', '200' ],
[ ':path', 'abc' ],
[ 'abc', '1' ],
[ 'xyz', '1' ],
[ 'xyz', '2' ],
[ 'xyz', '3' ],
[ 'xyz', '4' ]
]);
assert.deepStrictEqual(
mapToHeaders(headers),
[ ':status_200_:path_abc_abc_1_xyz_1_xyz_2_xyz_3_xyz_4_'
.replace(/_/g, '\0'),
7 ]
);
}

{
Expand All @@ -124,15 +117,12 @@ const {
[Symbol('test')]: 1 // Symbol keys are ignored
};

assert.deepStrictEqual(mapToHeaders(headers), [
[ ':status', '200' ],
[ ':path', 'abc' ],
[ 'abc', '1' ],
[ 'xyz', '1' ],
[ 'xyz', '2' ],
[ 'xyz', '3' ],
[ 'xyz', '4' ]
]);
assert.deepStrictEqual(
mapToHeaders(headers),
[ ':status_200_:path_abc_abc_1_xyz_1_xyz_2_xyz_3_xyz_4_'
.replace(/_/g, '\0'),
7 ]
);
}

{
Expand All @@ -144,14 +134,10 @@ const {
headers.foo = [];
headers[':status'] = 200;

assert.deepStrictEqual(mapToHeaders(headers), [
[ ':status', '200' ],
[ ':path', 'abc' ],
[ 'xyz', '1' ],
[ 'xyz', '2' ],
[ 'xyz', '3' ],
[ 'xyz', '4' ]
]);
assert.deepStrictEqual(
mapToHeaders(headers),
[ ':status_200_:path_abc_xyz_1_xyz_2_xyz_3_xyz_4_'.replace(/_/g, '\0'), 6 ]
);
}

// The following are not allowed to have multiple values
Expand Down