Skip to content

Commit

Permalink
lib: revert primordials in a hot path
Browse files Browse the repository at this point in the history 
Evidence has shown that use of primordials have sometimes an impact of
performance. This commit reverts the changes who are most likely to be
responsible for performance regression in the HTTP response path.

PR-URL: #38248
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
@aduh95 @mcollina
aduh95 authored and mcollina committed Apr 19, 2021
1 parent 4e9212b commit ee9e2a2
Show file tree
Hide file tree
Showing 20 changed files with 182 additions and 222 deletions.
6 changes: 2 additions & 4 deletions lib/_http_common.js
View file
Expand Up @@ -22,11 +22,9 @@
'use strict';

const {
ArrayPrototypePushApply,
MathMin,
Symbol,
RegExpPrototypeTest,
TypedArrayPrototypeSlice,
} = primordials;
const { setImmediate } = require('timers');

Expand Down Expand Up @@ -66,7 +64,7 @@ function parserOnHeaders(headers, url) {
// Once we exceeded headers limit - stop collecting them
if (this.maxHeaderPairs <= 0 ||
this._headers.length < this.maxHeaderPairs) {
ArrayPrototypePushApply(this._headers, headers);
this._headers.push(...headers);
}
this._url += url;
}
Expand Down Expand Up @@ -138,7 +136,7 @@ function parserOnBody(b, start, len) {

// Pretend this was the result of a stream._read call.
if (len > 0 && !stream._dumped) {
const slice = TypedArrayPrototypeSlice(b, start, start + len);
const slice = b.slice(start, start + len);
const ret = stream.push(slice);
if (!ret)
readStop(this.socket);
Expand Down
6 changes: 2 additions & 4 deletions lib/_http_incoming.js
View file
Expand Up @@ -22,8 +22,6 @@
'use strict';

const {
ArrayPrototypePush,
FunctionPrototypeCall,
ObjectDefineProperty,
ObjectSetPrototypeOf,
StringPrototypeCharCodeAt,
Expand Down Expand Up @@ -59,7 +57,7 @@ function IncomingMessage(socket) {
};
}

FunctionPrototypeCall(Readable, this, streamOptions);
Readable.call(this, streamOptions);

this._readableState.readingMore = true;

Expand Down Expand Up @@ -350,7 +348,7 @@ function _addHeaderLine(field, value, dest) {
} else if (flag === 1) {
// Array header -- only Set-Cookie at the moment
if (dest['set-cookie'] !== undefined) {
ArrayPrototypePush(dest['set-cookie'], value);
dest['set-cookie'].push(value);
} else {
dest['set-cookie'] = [value];
}
Expand Down
23 changes: 9 additions & 14 deletions lib/_http_outgoing.js
View file
Expand Up @@ -24,13 +24,7 @@
const {
Array,
ArrayIsArray,
ArrayPrototypeForEach,
ArrayPrototypeJoin,
ArrayPrototypePush,
ArrayPrototypeUnshift,
FunctionPrototype,
FunctionPrototypeBind,
FunctionPrototypeCall,
MathFloor,
NumberPrototypeToString,
ObjectCreate,
Expand Down Expand Up @@ -88,7 +82,7 @@ const { CRLF } = common;

const kCorked = Symbol('corked');

const nop = FunctionPrototype;
const nop = () => {};

const RE_CONN_CLOSE = /(?:^|\W)close(?:$|\W)/i;
const RE_TE_CHUNKED = common.chunkExpression;
Expand All @@ -101,7 +95,7 @@ function isCookieField(s) {
}

function OutgoingMessage() {
FunctionPrototypeCall(Stream, this);
Stream.call(this);

// Queue that holds all currently pending data, until the response will be
// assigned to the socket (until it will its turn in the HTTP pipeline).
Expand Down Expand Up @@ -331,7 +325,7 @@ OutgoingMessage.prototype._send = function _send(data, encoding, callback) {
data = this._header + data;
} else {
const header = this._header;
ArrayPrototypeUnshift(this.outputData, {
this.outputData.unshift({
data: header,
encoding: 'latin1',
callback: null
Expand Down Expand Up @@ -368,7 +362,7 @@ function _writeRaw(data, encoding, callback) {
return conn.write(data, encoding, callback);
}
// Buffer, as long as we're not destroyed.
ArrayPrototypePush(this.outputData, { data, encoding, callback });
this.outputData.push({ data, encoding, callback });
this.outputSize += data.length;
this._onPendingData(data.length);
return this.outputSize < HIGH_WATER_MARK;
Expand Down Expand Up @@ -397,9 +391,10 @@ function _storeHeader(firstLine, headers) {
}
} else if (ArrayIsArray(headers)) {
if (headers.length && ArrayIsArray(headers[0])) {
ArrayPrototypeForEach(headers, (entry) =>
processHeader(this, state, entry[0], entry[1], true)
);
for (let i = 0; i < headers.length; i++) {
const entry = headers[i];
processHeader(this, state, entry[0], entry[1], true);
}
} else {
if (headers.length % 2 !== 0) {
throw new ERR_INVALID_ARG_VALUE('headers', headers);
Expand Down Expand Up @@ -877,7 +872,7 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) {
if (typeof callback === 'function')
this.once('finish', callback);

const finish = FunctionPrototypeBind(onFinish, undefined, this);
const finish = onFinish.bind(undefined, this);

if (this._hasBody && this.chunkedEncoding) {
this._send('0\r\n' + this._trailer + '\r\n', 'latin1', finish);
Expand Down
88 changes: 41 additions & 47 deletions lib/_http_server.js
View file
Expand Up @@ -23,20 +23,12 @@

const {
ArrayIsArray,
ArrayPrototypeForEach,
ArrayPrototypePush,
ArrayPrototypeShift,
Error,
FunctionPrototype,
FunctionPrototypeBind,
FunctionPrototypeCall,
ObjectKeys,
ObjectSetPrototypeOf,
ReflectApply,
RegExpPrototypeTest,
Symbol,
SymbolFor,
TypedArrayPrototypeSlice,
} = primordials;

const net = require('net');
Expand Down Expand Up @@ -185,7 +177,7 @@ class HTTPServerAsyncResource {
}

function ServerResponse(req) {
FunctionPrototypeCall(OutgoingMessage, this);
OutgoingMessage.call(this);

if (req.method === 'HEAD') this._hasBody = false;

Expand All @@ -212,7 +204,7 @@ ObjectSetPrototypeOf(ServerResponse, OutgoingMessage);
ServerResponse.prototype._finish = function _finish() {
DTRACE_HTTP_SERVER_RESPONSE(this.socket);
emitStatistics(this[kServerResponseStatistics]);
FunctionPrototypeCall(OutgoingMessage.prototype._finish, this);
OutgoingMessage.prototype._finish.call(this);
};


Expand Down Expand Up @@ -386,7 +378,7 @@ function Server(options, requestListener) {
validateBoolean(insecureHTTPParser, 'options.insecureHTTPParser');
this.insecureHTTPParser = insecureHTTPParser;

FunctionPrototypeCall(net.Server, this, { allowHalfOpen: true });
net.Server.call(this, { allowHalfOpen: true });

if (requestListener) {
this.on('request', requestListener);
Expand Down Expand Up @@ -422,17 +414,19 @@ Server.prototype[EE.captureRejectionSymbol] = function(err, event, ...args) {
const { 1: res } = args;
if (!res.headersSent && !res.writableEnded) {
// Don't leak headers.
ArrayPrototypeForEach(res.getHeaderNames(),
(name) => res.removeHeader(name));
const names = res.getHeaderNames();
for (let i = 0; i < names.length; i++) {
res.removeHeader(names[i]);
}
res.statusCode = 500;
res.end(STATUS_CODES[500]);
} else {
res.destroy();
}
break;
default:
ReflectApply(net.Server.prototype[SymbolFor('nodejs.rejection')],
this, arguments);
net.Server.prototype[SymbolFor('nodejs.rejection')]
.apply(this, arguments);
}
};

Expand Down Expand Up @@ -493,20 +487,20 @@ function connectionListenerInternal(server, socket) {
outgoingData: 0,
keepAliveTimeoutSet: false
};
state.onData = FunctionPrototypeBind(socketOnData, undefined,
server, socket, parser, state);
state.onEnd = FunctionPrototypeBind(socketOnEnd, undefined,
server, socket, parser, state);
state.onClose = FunctionPrototypeBind(socketOnClose, undefined,
socket, state);
state.onDrain = FunctionPrototypeBind(socketOnDrain, undefined,
socket, state);
state.onData = socketOnData.bind(undefined,
server, socket, parser, state);
state.onEnd = socketOnEnd.bind(undefined,
server, socket, parser, state);
state.onClose = socketOnClose.bind(undefined,
socket, state);
state.onDrain = socketOnDrain.bind(undefined,
socket, state);
socket.on('data', state.onData);
socket.on('error', socketOnError);
socket.on('end', state.onEnd);
socket.on('close', state.onClose);
socket.on('drain', state.onDrain);
parser.onIncoming = FunctionPrototypeBind(parserOnIncoming, undefined,
parser.onIncoming = parserOnIncoming.bind(undefined,
server, socket, state);

// We are consuming socket, so it won't get any actual data
Expand All @@ -527,18 +521,18 @@ function connectionListenerInternal(server, socket) {
parser.consume(socket._handle);
}
parser[kOnExecute] =
FunctionPrototypeBind(onParserExecute, undefined,
server, socket, parser, state);
onParserExecute.bind(undefined,
server, socket, parser, state);

parser[kOnTimeout] =
FunctionPrototypeBind(onParserTimeout, undefined,
server, socket);
onParserTimeout.bind(undefined,
server, socket);

// When receiving new requests on the same socket (pipelining or keep alive)
// make sure the requestTimeout is active.
parser[kOnMessageBegin] =
FunctionPrototypeBind(setRequestTimeout, undefined,
server, socket);
setRequestTimeout.bind(undefined,
server, socket);

// This protects from DOS attack where an attacker establish the connection
// without sending any data on applications where server.timeout is left to
Expand Down Expand Up @@ -594,7 +588,7 @@ function socketOnClose(socket, state) {

function abortIncoming(incoming) {
while (incoming.length) {
const req = ArrayPrototypeShift(incoming);
const req = incoming.shift();
req.destroy(connResetException('aborted'));
}
// Abort socket._httpMessage ?
Expand All @@ -606,7 +600,7 @@ function socketOnEnd(server, socket, parser, state) {
if (ret instanceof Error) {
debug('parse error');
// socketOnError has additional logic and will call socket.destroy(err).
FunctionPrototypeCall(socketOnError, socket, ret);
socketOnError.call(socket, ret);
} else if (!server.httpAllowHalfOpen) {
socket.end();
} else if (state.outgoing.length) {
Expand All @@ -629,7 +623,7 @@ function socketOnData(server, socket, parser, state, d) {
function onRequestTimeout(socket) {
socket[kRequestTimeout] = undefined;
// socketOnError has additional logic and will call socket.destroy(err).
ReflectApply(socketOnError, socket, [new ERR_HTTP_REQUEST_TIMEOUT()]);
socketOnError.call(socket, new ERR_HTTP_REQUEST_TIMEOUT());
}

function onParserExecute(server, socket, parser, state, ret) {
Expand All @@ -649,7 +643,7 @@ function onParserTimeout(server, socket) {
socket.destroy();
}

const noop = FunctionPrototype;
const noop = () => {};
const badRequestResponse = Buffer.from(
`HTTP/1.1 400 ${STATUS_CODES[400]}${CRLF}` +
`Connection: close${CRLF}${CRLF}`, 'ascii'
Expand Down Expand Up @@ -696,7 +690,7 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) {
prepareError(ret, parser, d);
ret.rawPacket = d || parser.getCurrentBuffer();
debug('parse error', ret);
FunctionPrototypeCall(socketOnError, socket, ret);
socketOnError.call(socket, ret);
} else if (parser.incoming && parser.incoming.upgrade) {
// Upgrade or CONNECT
const req = parser.incoming;
Expand All @@ -719,7 +713,7 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) {
const eventName = req.method === 'CONNECT' ? 'connect' : 'upgrade';
if (eventName === 'upgrade' || server.listenerCount(eventName) > 0) {
debug('SERVER have listener for %s', eventName);
const bodyHead = TypedArrayPrototypeSlice(d, ret, d.length);
const bodyHead = d.slice(ret, d.length);

socket.readableFlowing = null;

Expand All @@ -738,7 +732,7 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) {
// When receiving new requests on the same socket (pipelining or keep alive)
// make sure the requestTimeout is active.
parser[kOnMessageBegin] =
FunctionPrototypeBind(setRequestTimeout, undefined, server, socket);
setRequestTimeout.bind(undefined, server, socket);
}

if (socket._paused && socket.parser) {
Expand Down Expand Up @@ -802,7 +796,7 @@ function resOnFinish(req, res, socket, state, server) {
// array will be empty.
assert(state.incoming.length === 0 || state.incoming[0] === req);

ArrayPrototypeShift(state.incoming);
state.incoming.shift();

// If the user never called req.read(), and didn't pipe() or
// .resume() or .on('data'), then we call req._dump() so that the
Expand Down Expand Up @@ -835,7 +829,7 @@ function resOnFinish(req, res, socket, state, server) {
}
} else {
// Start sending the next message
const m = ArrayPrototypeShift(state.outgoing);
const m = state.outgoing.shift();
if (m) {
m.assignSocket(socket);
}
Expand All @@ -861,7 +855,7 @@ function parserOnIncoming(server, socket, state, req, keepAlive) {
return 2;
}

ArrayPrototypePush(state.incoming, req);
state.incoming.push(req);

// If the writable end isn't consuming, then stop reading
// so that we don't become overwhelmed by a flood of
Expand All @@ -879,8 +873,8 @@ function parserOnIncoming(server, socket, state, req, keepAlive) {

const res = new server[kServerResponse](req);
res._keepAliveTimeout = server.keepAliveTimeout;
res._onPendingData = FunctionPrototypeBind(updateOutgoingData, undefined,
socket, state);
res._onPendingData = updateOutgoingData.bind(undefined,
socket, state);

res.shouldKeepAlive = keepAlive;
DTRACE_HTTP_SERVER_REQUEST(req, socket);
Expand All @@ -896,16 +890,16 @@ function parserOnIncoming(server, socket, state, req, keepAlive) {

if (socket._httpMessage) {
// There are already pending outgoing res, append.
ArrayPrototypePush(state.outgoing, res);
state.outgoing.push(res);
} else {
res.assignSocket(socket);
}

// When we're finished writing the response, check if this is the last
// response, if so destroy the socket.
res.on('finish',
FunctionPrototypeBind(resOnFinish, undefined,
req, res, socket, state, server));
resOnFinish.bind(undefined,
req, res, socket, state, server));

if (req.headers.expect !== undefined &&
(req.httpVersionMajor === 1 && req.httpVersionMinor === 1)) {
Expand Down Expand Up @@ -977,8 +971,8 @@ function unconsume(parser, socket) {

function generateSocketListenerWrapper(originalFnName) {
return function socketListenerWrap(ev, fn) {
const res = ReflectApply(net.Socket.prototype[originalFnName], this,
[ev, fn]);
const res = net.Socket.prototype[originalFnName].call(this,
ev, fn);
if (!this.parser) {
this.on = net.Socket.prototype.on;
this.addListener = net.Socket.prototype.addListener;
Expand Down

0 comments on commit ee9e2a2

Please sign in to comment.