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.

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 compat performance #25567

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
35 changes: 35 additions & 0 deletions benchmark/http2/compat.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
'use strict';

const common = require('../common.js');
const path = require('path');
const fs = require('fs');
const file = path.join(path.resolve(__dirname, '../fixtures'), 'alice.html');

const bench = common.createBenchmark(main, {
requests: [100, 1000, 5000],
streams: [1, 10, 20, 40, 100, 200],
clients: [2],
benchmarker: ['h2load']
}, { flags: ['--no-warnings'] });

function main({ requests, streams, clients }) {
const http2 = require('http2');
const server = http2.createServer();
server.on('request', (req, res) => {
const out = fs.createReadStream(file);
res.setHeader('content-type', 'text/html');
out.pipe(res);
out.on('error', (err) => {
res.destroy();
});
});
server.listen(common.PORT, () => {
bench.http({
path: '/',
requests,
maxConcurrentStreams: streams,
clients,
threads: clients
}, () => { server.close(); });
});
}
4 changes: 1 addition & 3 deletions lib/internal/http2/compat.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,16 @@ const {
ERR_INVALID_HTTP_TOKEN
} = require('internal/errors').codes;
const { validateString } = require('internal/validators');
const { kSocket } = require('internal/http2/util');
const { kSocket, kRequest, kProxySocket } = require('internal/http2/util');

const kBeginSend = Symbol('begin-send');
const kState = Symbol('state');
const kStream = Symbol('stream');
const kRequest = Symbol('request');
const kResponse = Symbol('response');
const kHeaders = Symbol('headers');
const kRawHeaders = Symbol('rawHeaders');
const kTrailers = Symbol('trailers');
const kRawTrailers = Symbol('rawTrailers');
const kProxySocket = Symbol('proxySocket');
const kSetHeader = Symbol('setHeader');
const kAborted = Symbol('aborted');

Expand Down
26 changes: 22 additions & 4 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ const {
getStreamState,
isPayloadMeaningless,
kSocket,
kRequest,
kProxySocket,
mapToHeaders,
NghttpError,
sessionName,
Expand All @@ -119,6 +121,8 @@ const {
} = require('internal/timers');
const { isArrayBufferView } = require('internal/util/types');

const hasOwnProperty = Object.prototype.hasOwnProperty;

const { FileHandle } = internalBinding('fs');
const binding = internalBinding('http2');
const {
Expand Down Expand Up @@ -157,7 +161,6 @@ const kOwner = owner_symbol;
const kOrigin = Symbol('origin');
const kProceed = Symbol('proceed');
const kProtocol = Symbol('protocol');
const kProxySocket = Symbol('proxy-socket');
const kRemoteSettings = Symbol('remote-settings');
const kSelectPadding = Symbol('select-padding');
const kSentHeaders = Symbol('sent-headers');
Expand Down Expand Up @@ -1624,6 +1627,10 @@ class Http2Stream extends Duplex {
endAfterHeaders: false
};

// Fields used by the compat API to avoid megamorphisms.
this[kRequest] = null;
this[kProxySocket] = null;

this.on('pause', streamOnPause);
}

Expand Down Expand Up @@ -2001,9 +2008,20 @@ class Http2Stream extends Duplex {
}
}

function processHeaders(headers) {
assertIsObject(headers, 'headers');
headers = Object.assign(Object.create(null), headers);
function processHeaders(oldHeaders) {
assertIsObject(oldHeaders, 'headers');
const headers = Object.create(null);

if (oldHeaders !== null && oldHeaders !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

assertIsObject above makes this if useless

const hop = hasOwnProperty.bind(oldHeaders);
// This loop is here for performance reason. Do not change.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment referring to the benchmark that is the reason for “Do not change” (or instead of it)?

Copy link
Member Author

Choose a reason for hiding this comment

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

In frankness I can't. I've added the comment because I saw these types of loops being removed from other critical places. They shouldn't unless there is a key reason to do so.

Our benchmarks do not cover why the previous case were slower, and I struggled in creating one.

'use strict'

const fastify = require('fastify')({
  http2: true
})

fastify
  .get('/', function (req, reply) {
    reply.send({})
  })

fastify.listen(3000, err => {
  if (err) throw err
  console.log(`server listening on ${fastify.server.address().port}`)
})

I used this command:

h2load -c 1 -m 100 -D 10 http://localhost:3000

I'm getting 49k req/sec vs 45k req/sec between this PR and master.
(Note that each of the changes matters for a bit of the improvement and taken alone are hard to measure).

for (var key in oldHeaders) {
if (hop(key)) {
headers[key] = oldHeaders[key];
}
}
}

const statusCode =
headers[HTTP2_HEADER_STATUS] =
headers[HTTP2_HEADER_STATUS] | 0 || HTTP_STATUS_OK;
Expand Down
8 changes: 6 additions & 2 deletions lib/internal/http2/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ const {
} = require('internal/errors').codes;

const kSocket = Symbol('socket');
const kProxySocket = Symbol('proxySocket');
const kRequest = Symbol('request');

const {
NGHTTP2_SESSION_CLIENT,
Expand Down Expand Up @@ -499,12 +501,12 @@ class NghttpError extends Error {
}
}

function assertIsObject(value, name, types = 'Object') {
function assertIsObject(value, name, types) {
Copy link
Member

Choose a reason for hiding this comment

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

/cc @nodejs/v8 Is it a know issue that default parameters are slow?

if (value !== undefined &&
(value === null ||
typeof value !== 'object' ||
Array.isArray(value))) {
const err = new ERR_INVALID_ARG_TYPE(name, types, value);
const err = new ERR_INVALID_ARG_TYPE(name, types || 'Object', value);
Error.captureStackTrace(err, assertIsObject);
throw err;
}
Expand Down Expand Up @@ -592,6 +594,8 @@ module.exports = {
getStreamState,
isPayloadMeaningless,
kSocket,
kProxySocket,
kRequest,
mapToHeaders,
NghttpError,
sessionName,
Expand Down
20 changes: 19 additions & 1 deletion src/stream_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,29 @@ void LibuvStreamWrap::Initialize(Local<Object> target,
};
Local<FunctionTemplate> sw =
FunctionTemplate::New(env->isolate(), is_construct_call_callback);
sw->InstanceTemplate()->SetInternalFieldCount(StreamReq::kStreamReqField + 1);
sw->InstanceTemplate()->SetInternalFieldCount(
StreamReq::kStreamReqField + 1 + 3);
Local<String> wrapString =
FIXED_ONE_BYTE_STRING(env->isolate(), "ShutdownWrap");
sw->SetClassName(wrapString);

// we need to set handle and callback to null,
// so that those fields are created and functions
// do not become megamorphic
// Fields:
// - oncomplete
// - callback
// - handle
sw->InstanceTemplate()->Set(
FIXED_ONE_BYTE_STRING(env->isolate(), "oncomplete"),
v8::Null(env->isolate()));
sw->InstanceTemplate()->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "callback"),
v8::Null(env->isolate()));
sw->InstanceTemplate()->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "handle"),
v8::Null(env->isolate()));
Copy link
Member Author

Choose a reason for hiding this comment

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

@bmeurer could you review 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.

Thanks @bmeurer!

@addaleax we might want to do this trick in other places as well (WriteWrap).


sw->Inherit(AsyncWrap::GetConstructorTemplate(env));

target->Set(env->context(),
wrapString,
sw->GetFunction(env->context()).ToLocalChecked()).FromJust();
Expand Down