Skip to content

http2: code style and performance improvements #16239

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
wants to merge 12 commits into from
Closed

http2: code style and performance improvements #16239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d8120c3
benchmark: improve http2 benchmark configs
jasnell Oct 16, 2017
791d4b4
http2: move CHECK statements into DEBUG checks
jasnell Oct 16, 2017
04179bf
http2: code style fix
jasnell Oct 16, 2017
18b194f
http2: improve code documentation slightly
jasnell Oct 16, 2017
1bf393e
http2: improve performance by removing branches
jasnell Oct 16, 2017
a3f5864
http2: refactor flow control for proper backpressure
jasnell Oct 17, 2017
04f2b2a
http2: use std::queue for inbound headers
jasnell Oct 18, 2017
e150087
http2: use std::queue for outbound data
jasnell Oct 18, 2017
c19d914
http2: remove now unnecessary FreeHeaders function
jasnell Oct 18, 2017
796e313
http2: expand comments and miscellaneous edits
jasnell Oct 18, 2017
948606f
http2: use std::queue instead of std::list
jasnell Oct 18, 2017
9086258
[Squash] Fix tests on Windows
jasnell Oct 22, 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
3 changes: 2 additions & 1 deletion benchmark/http2/headers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ const PORT = common.PORT;
const bench = common.createBenchmark(main, {
n: [1e3],
nheaders: [0, 10, 100, 1000],
}, { flags: ['--no-warnings'] });
benchmarker: ['h2load']
}, { flags: ['--no-warnings', '--expose-http2'] });
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason --expose-http2 is back in? Is it just so compare can be run against versions where that was required?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. It's for benchmark comparisons against v8.x


function main(conf) {
const n = +conf.n;
Expand Down
5 changes: 3 additions & 2 deletions benchmark/http2/respond-with-fd.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@ const file = path.join(path.resolve(__dirname, '../fixtures'), 'alice.html');
const bench = common.createBenchmark(main, {
requests: [100, 1000, 10000, 100000, 1000000],
streams: [100, 200, 1000],
clients: [1, 2]
}, { flags: ['--no-warnings'] });
clients: [1, 2],
benchmarker: ['h2load']
}, { flags: ['--no-warnings', '--expose-http2'] });

function main(conf) {

Expand Down
5 changes: 3 additions & 2 deletions benchmark/http2/simple.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,9 @@ const file = path.join(path.resolve(__dirname, '../fixtures'), 'alice.html');
const bench = common.createBenchmark(main, {
requests: [100, 1000, 10000, 100000],
streams: [100, 200, 1000],
clients: [1, 2]
}, { flags: ['--no-warnings'] });
clients: [1, 2],
benchmarker: ['h2load']
}, { flags: ['--no-warnings', '--expose-http2'] });

function main(conf) {
const n = +conf.requests;
Expand Down
5 changes: 3 additions & 2 deletions benchmark/http2/write.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@ const PORT = common.PORT;
const bench = common.createBenchmark(main, {
streams: [100, 200, 1000],
length: [64 * 1024, 128 * 1024, 256 * 1024, 1024 * 1024],
size: [100000]
}, { flags: ['--no-warnings'] });
size: [100000],
benchmarker: ['h2load']
}, { flags: ['--no-warnings', '--expose-http2'] });

function main(conf) {
const m = +conf.streams;
Expand Down
30 changes: 8 additions & 22 deletions lib/internal/http2/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,19 +278,13 @@ function onSessionRead(nread, buf, handle) {
assert(stream !== undefined,
'Internal HTTP/2 Failure. Stream does not exist. Please ' +
'report this as a bug in Node.js');
const state = stream[kState];
_unrefActive(owner); // Reset the session timeout timer
_unrefActive(stream); // Reset the stream timeout timer
if (nread >= 0 && !stream.destroyed)
return stream.push(buf);

if (nread >= 0 && !stream.destroyed) {
if (!stream.push(buf)) {
this.streamReadStop(id);
state.reading = false;
}
} else {
// Last chunk was received. End the readable side.
stream.push(null);
}
// Last chunk was received. End the readable side.
stream.push(null);
}

// Called when the remote peer settings have been updated.
Expand Down Expand Up @@ -1199,9 +1193,7 @@ function streamOnResume() {
return;
}
const session = this[kSession];
const state = this[kState];
if (session && !state.reading) {
state.reading = true;
if (session) {
assert(session[kHandle].streamReadStart(this[kID]) === undefined,
`HTTP/2 Stream ${this[kID]} does not exist. Please report this as ` +
'a bug in Node.js');
Expand All @@ -1210,9 +1202,7 @@ function streamOnResume() {

function streamOnPause() {
const session = this[kSession];
const state = this[kState];
if (session && state.reading) {
state.reading = false;
if (session) {
assert(session[kHandle].streamReadStop(this[kID]) === undefined,
`HTTP/2 Stream ${this[kID]} does not exist. Please report this as ' +
'a bug in Node.js`);
Expand Down Expand Up @@ -1406,12 +1396,8 @@ class Http2Stream extends Duplex {
return;
}
_unrefActive(this);
const state = this[kState];
if (state.reading)
return;
state.reading = true;
assert(this[kSession][kHandle].streamReadStart(this[kID]) === undefined,
`HTTP/2 Stream ${this[kID]} does not exist. Please report this as ` +
assert(this[kSession][kHandle].flushData(this[kID]) === undefined,
'HTTP/2 Stream #{this[kID]} does not exist. Please report this as ' +
'a bug in Node.js');
}

Expand Down
Loading