Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
lib: revert primordials in a hot path
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
Backport-PR-URL: #38972
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 authored and targos committed Jun 11, 2021
1 parent 07c55d2 commit eb8f7ee
Show file tree
Hide file tree
Showing 9 changed files with 41 additions and 53 deletions.
3 changes: 2 additions & 1 deletion lib/_http_outgoing.js
Expand Up @@ -380,7 +380,8 @@ function _storeHeader(firstLine, headers) {
}
} else if (ArrayIsArray(headers)) {
if (headers.length && ArrayIsArray(headers[0])) {
for (const entry of headers) {
for (let i = 0; i < headers.length; i++) {
const entry = headers[i];
processHeader(this, state, entry[0], entry[1], true);
}
} else {
Expand Down
7 changes: 4 additions & 3 deletions lib/_http_server.js
Expand Up @@ -398,8 +398,9 @@ Server.prototype[EE.captureRejectionSymbol] = function(err, event, ...args) {
const [ , res] = args;
if (!res.headersSent && !res.writableEnded) {
// Don't leak headers.
for (const name of res.getHeaderNames()) {
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]);
Expand All @@ -409,7 +410,7 @@ Server.prototype[EE.captureRejectionSymbol] = function(err, event, ...args) {
break;
default:
net.Server.prototype[SymbolFor('nodejs.rejection')]
.call(this, err, event, ...args);
.apply(this, arguments);
}
};

Expand Down
24 changes: 10 additions & 14 deletions lib/events.js
Expand Up @@ -22,13 +22,10 @@
'use strict';

const {
ArrayPrototypeForEach,
ArrayPrototypePush,
ArrayPrototypeSlice,
Boolean,
Error,
ErrorCaptureStackTrace,
FunctionPrototypeCall,
MathMin,
NumberIsNaN,
ObjectCreate,
Expand All @@ -39,7 +36,6 @@ const {
Promise,
PromiseReject,
PromiseResolve,
ReflectApply,
ReflectOwnKeys,
String,
Symbol,
Expand Down Expand Up @@ -84,7 +80,7 @@ const lazyDOMException = hideStackFrames((message, name) => {


function EventEmitter(opts) {
FunctionPrototypeCall(EventEmitter.init, this, opts);
EventEmitter.init.call(this, opts);
}
module.exports = EventEmitter;
module.exports.once = once;
Expand Down Expand Up @@ -175,8 +171,8 @@ EventEmitter.setMaxListeners =
if (isEventTarget === undefined)
isEventTarget = require('internal/event_target').isEventTarget;

// Performance for forEach is now comparable with regular for-loop
ArrayPrototypeForEach(eventTargets, (target) => {
for (let i = 0; i < eventTargets.length; i++) {
const target = eventTargets[i];
if (isEventTarget(target)) {
target[kMaxEventTargetListeners] = n;
target[kMaxEventTargetListenersWarned] = false;
Expand All @@ -188,7 +184,7 @@ EventEmitter.setMaxListeners =
['EventEmitter', 'EventTarget'],
target);
}
});
}
}
};

Expand Down Expand Up @@ -227,7 +223,7 @@ function addCatch(that, promise, type, args) {
const then = promise.then;

if (typeof then === 'function') {
FunctionPrototypeCall(then, promise, undefined, function(err) {
then.call(promise, undefined, function(err) {
// The callback is called with nextTick to avoid a follow-up
// rejection from this promise.
process.nextTick(emitUnhandledRejectionOrErr, that, err, type, args);
Expand Down Expand Up @@ -376,7 +372,7 @@ EventEmitter.prototype.emit = function emit(type, ...args) {
return false;

if (typeof handler === 'function') {
const result = ReflectApply(handler, this, args);
const result = handler.apply(this, args);

// We check if result is undefined first because that
// is the most common case so we do not pay any perf
Expand All @@ -388,7 +384,7 @@ EventEmitter.prototype.emit = function emit(type, ...args) {
const len = handler.length;
const listeners = arrayClone(handler);
for (let i = 0; i < len; ++i) {
const result = ReflectApply(listeners[i], this, args);
const result = listeners[i].apply(this, args);

// We check if result is undefined first because that
// is the most common case so we do not pay any perf
Expand Down Expand Up @@ -698,7 +694,7 @@ function getEventListeners(emitterOrTarget, type) {
const listeners = [];
let handler = root?.next;
while (handler?.listener !== undefined) {
ArrayPrototypePush(listeners, handler.listener);
listeners.push(handler.listener);
handler = handler.next;
}
return listeners;
Expand Down Expand Up @@ -842,7 +838,7 @@ function on(emitter, event, options) {

// Wait until an event happens
return new Promise(function(resolve, reject) {
ArrayPrototypePush(unconsumedPromises, { resolve, reject });
unconsumedPromises.push({ resolve, reject });
});
},

Expand Down Expand Up @@ -906,7 +902,7 @@ function on(emitter, event, options) {
if (promise) {
promise.resolve(createIterResult(args, false));
} else {
ArrayPrototypePush(unconsumedEvents, args);
unconsumedEvents.push(args);
}
}

Expand Down
20 changes: 8 additions & 12 deletions lib/internal/async_hooks.js
@@ -1,15 +1,11 @@
'use strict';

const {
ArrayPrototypePop,
ArrayPrototypeSlice,
ArrayPrototypeUnshift,
ErrorCaptureStackTrace,
FunctionPrototypeBind,
ObjectPrototypeHasOwnProperty,
ObjectDefineProperty,
Promise,
ReflectApply,
Symbol,
} = primordials;

Expand Down Expand Up @@ -129,16 +125,16 @@ function callbackTrampoline(asyncId, resource, cb, ...args) {

let result;
if (asyncId === 0 && typeof domain_cb === 'function') {
ArrayPrototypeUnshift(args, cb);
result = ReflectApply(domain_cb, this, args);
args.unshift(cb);
result = domain_cb.apply(this, args);
} else {
result = ReflectApply(cb, this, args);
result = cb.apply(this, args);
}

if (asyncId !== 0 && hasHooks(kAfter))
emitAfterNative(asyncId);

ArrayPrototypePop(execution_async_resources);
execution_async_resources.pop();
return result;
}

Expand Down Expand Up @@ -256,7 +252,7 @@ function emitHook(symbol, asyncId) {
}

function emitHookFactory(symbol, name) {
const fn = FunctionPrototypeBind(emitHook, undefined, symbol);
const fn = emitHook.bind(undefined, symbol);

// Set the name property of the function as it looks good in the stack trace.
ObjectDefineProperty(fn, 'name', {
Expand Down Expand Up @@ -429,14 +425,14 @@ function clearDefaultTriggerAsyncId() {

function defaultTriggerAsyncIdScope(triggerAsyncId, block, ...args) {
if (triggerAsyncId === undefined)
return ReflectApply(block, null, args);
return block.apply(null, args);
// CHECK(NumberIsSafeInteger(triggerAsyncId))
// CHECK(triggerAsyncId > 0)
const oldDefaultTriggerAsyncId = async_id_fields[kDefaultTriggerAsyncId];
async_id_fields[kDefaultTriggerAsyncId] = triggerAsyncId;

try {
return ReflectApply(block, null, args);
return block.apply(null, args);
} finally {
async_id_fields[kDefaultTriggerAsyncId] = oldDefaultTriggerAsyncId;
}
Expand Down Expand Up @@ -533,7 +529,7 @@ function popAsyncContext(asyncId) {
const offset = stackLength - 1;
async_id_fields[kExecutionAsyncId] = async_wrap.async_ids_stack[2 * offset];
async_id_fields[kTriggerAsyncId] = async_wrap.async_ids_stack[2 * offset + 1];
ArrayPrototypePop(execution_async_resources);
execution_async_resources.pop();
async_hook_fields[kStackLength] = offset;
return offset > 0;
}
Expand Down
8 changes: 3 additions & 5 deletions lib/internal/per_context/primordials.js
Expand Up @@ -6,11 +6,9 @@
// so that Node.js's builtin modules do not need to later look these up from
// the global proxy, which can be mutated by users.

// TODO(joyeecheung): we can restrict access to these globals in builtin
// modules through the JS linter, for example: ban access such as `Object`
// (which falls back to a lookup in the global proxy) in favor of
// `primordials.Object` where `primordials` is a lexical variable passed
// by the native module compiler.
// Use of primordials have sometimes a dramatic impact on performance, please
// benchmark all changes made in performance-sensitive areas of the codebase.
// See: https://github.com/nodejs/node/pull/38248

// `uncurryThis` is equivalent to `func => Function.prototype.call.bind(func)`.
// It is using `bind.bind(call)` to avoid using `Function.prototype.bind`
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/streams/readable.js
Expand Up @@ -823,8 +823,8 @@ Readable.prototype.unpipe = function(dest) {
state.pipes = [];
this.pause();

for (const dest of dests)
dest.emit('unpipe', this, { hasUnpiped: false });
for (let i = 0; i < dests.length; i++)
dests[i].emit('unpipe', this, { hasUnpiped: false });
return this;
}

Expand Down
2 changes: 1 addition & 1 deletion lib/internal/timers.js
Expand Up @@ -78,8 +78,8 @@ const {
NumberIsFinite,
NumberMIN_SAFE_INTEGER,
ObjectCreate,
Symbol,
ReflectApply,
Symbol,
} = primordials;

const {
Expand Down
24 changes: 10 additions & 14 deletions lib/net.js
Expand Up @@ -24,18 +24,13 @@
const {
ArrayIsArray,
ArrayPrototypeIndexOf,
ArrayPrototypePush,
ArrayPrototypeSplice,
Boolean,
Error,
FunctionPrototype,
FunctionPrototypeCall,
Number,
NumberIsNaN,
NumberParseInt,
ObjectDefineProperty,
ObjectSetPrototypeOf,
ReflectApply,
Symbol,
} = primordials;

Expand Down Expand Up @@ -129,7 +124,7 @@ const DEFAULT_IPV6_ADDR = '::';

const isWindows = process.platform === 'win32';

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

function getFlags(ipv6Only) {
return ipv6Only === true ? TCPConstants.UV_TCP_IPV6ONLY : 0;
Expand Down Expand Up @@ -304,7 +299,7 @@ function Socket(options) {
options.autoDestroy = false;
// Handle strings directly.
options.decodeStrings = false;
ReflectApply(stream.Duplex, this, [options]);
stream.Duplex.call(this, options);

// Default to *not* allowing half open sockets.
this.allowHalfOpen = Boolean(allowHalfOpen);
Expand Down Expand Up @@ -594,7 +589,8 @@ Socket.prototype._read = function(n) {


Socket.prototype.end = function(data, encoding, callback) {
ReflectApply(stream.Duplex.prototype.end, this, [data, encoding, callback]);
stream.Duplex.prototype.end.call(this,
data, encoding, callback);
DTRACE_NET_STREAM_END(this);
return this;
};
Expand All @@ -610,7 +606,7 @@ Socket.prototype.pause = function() {
this.destroy(errnoException(err, 'read'));
}
}
return FunctionPrototypeCall(stream.Duplex.prototype.pause, this);
return stream.Duplex.prototype.pause.call(this);
};


Expand All @@ -619,7 +615,7 @@ Socket.prototype.resume = function() {
!this._handle.reading) {
tryReadStart(this);
}
return FunctionPrototypeCall(stream.Duplex.prototype.resume, this);
return stream.Duplex.prototype.resume.call(this);
};


Expand All @@ -628,7 +624,7 @@ Socket.prototype.read = function(n) {
!this._handle.reading) {
tryReadStart(this);
}
return ReflectApply(stream.Duplex.prototype.read, this, [n]);
return stream.Duplex.prototype.read.call(this, n);
};


Expand Down Expand Up @@ -1167,7 +1163,7 @@ function Server(options, connectionListener) {
if (!(this instanceof Server))
return new Server(options, connectionListener);

FunctionPrototypeCall(EventEmitter, this);
EventEmitter.call(this);

if (typeof options === 'function') {
connectionListener = options;
Expand Down Expand Up @@ -1693,10 +1689,10 @@ ObjectDefineProperty(Socket.prototype, '_handle', {

Server.prototype._setupWorker = function(socketList) {
this._usingWorkers = true;
ArrayPrototypePush(this._workers, socketList);
this._workers.push(socketList);
socketList.once('exit', (socketList) => {
const index = ArrayPrototypeIndexOf(this._workers, socketList);
ArrayPrototypeSplice(this._workers, index, 1);
this._workers.splice(index, 1);
});
};

Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-stream-pipe-await-drain.js
Expand Up @@ -27,7 +27,7 @@ writer1.once('chunk-received', () => {
reader._readableState.awaitDrainWriters.size,
0,
'awaitDrain initial value should be 0, actual is ' +
reader._readableState.awaitDrainWriters
reader._readableState.awaitDrainWriters.size
);
setImmediate(() => {
// This one should *not* get through to writer1 because writer2 is not
Expand Down

0 comments on commit eb8f7ee

Please sign in to comment.