From faf15805ea2c9819ed572c422f6fbe9569cc9e26 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Thu, 15 Apr 2021 15:05:42 +0200 Subject: [PATCH] lib: revert primordials in a hot path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/nodejs/node/pull/38248 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Matteo Collina Reviewed-By: Michaël Zasso Reviewed-By: Rich Trott --- lib/_http_outgoing.js | 3 ++- lib/_http_server.js | 7 ++++--- lib/events.js | 22 +++++++++------------- lib/internal/async_hooks.js | 20 ++++++++------------ lib/internal/per_context/primordials.js | 14 +++++++++----- lib/internal/streams/readable.js | 8 ++++---- lib/internal/timers.js | 2 +- lib/internal/util/debuglog.js | 16 ++++++++++++++-- lib/net.js | 24 ++++++++++-------------- 9 files changed, 61 insertions(+), 55 deletions(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 7736c31c4d41a8..5cfddc529562ba 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -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 { diff --git a/lib/_http_server.js b/lib/_http_server.js index 1890e665cb9b55..f037e9e97ceb31 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -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]); @@ -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); } }; diff --git a/lib/events.js b/lib/events.js index 18500888251b87..7d004b348fedfd 100644 --- a/lib/events.js +++ b/lib/events.js @@ -22,13 +22,10 @@ 'use strict'; const { - ArrayPrototypeForEach, - ArrayPrototypePush, ArrayPrototypeSlice, Boolean, Error, ErrorCaptureStackTrace, - FunctionPrototypeCall, MathMin, NumberIsNaN, ObjectCreate, @@ -39,7 +36,6 @@ const { Promise, PromiseReject, PromiseResolve, - ReflectApply, ReflectOwnKeys, String, Symbol, @@ -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; @@ -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; @@ -188,7 +184,7 @@ EventEmitter.setMaxListeners = ['EventEmitter', 'EventTarget'], target); } - }); + } } }; @@ -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); @@ -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 @@ -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 @@ -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 }); }); }, @@ -906,7 +902,7 @@ function on(emitter, event, options) { if (promise) { promise.resolve(createIterResult(args, false)); } else { - ArrayPrototypePush(unconsumedEvents, args); + unconsumedEvents.push(args); } } diff --git a/lib/internal/async_hooks.js b/lib/internal/async_hooks.js index 71ffd26396b590..3e97141ce4e18b 100644 --- a/lib/internal/async_hooks.js +++ b/lib/internal/async_hooks.js @@ -1,15 +1,11 @@ 'use strict'; const { - ArrayPrototypePop, ArrayPrototypeSlice, - ArrayPrototypeUnshift, ErrorCaptureStackTrace, - FunctionPrototypeBind, ObjectPrototypeHasOwnProperty, ObjectDefineProperty, Promise, - ReflectApply, Symbol, } = primordials; @@ -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; } @@ -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', { @@ -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; } @@ -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; } diff --git a/lib/internal/per_context/primordials.js b/lib/internal/per_context/primordials.js index 874fd224da274b..8086b9f60a2d67 100644 --- a/lib/internal/per_context/primordials.js +++ b/lib/internal/per_context/primordials.js @@ -6,11 +6,15 @@ // 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 + +const { + defineProperty: ReflectDefineProperty, + getOwnPropertyDescriptor: ReflectGetOwnPropertyDescriptor, + ownKeys: ReflectOwnKeys, +} = Reflect; // `uncurryThis` is equivalent to `func => Function.prototype.call.bind(func)`. // It is using `bind.bind(call)` to avoid using `Function.prototype.bind` diff --git a/lib/internal/streams/readable.js b/lib/internal/streams/readable.js index fd2a8635674899..fd18cf920b90a1 100644 --- a/lib/internal/streams/readable.js +++ b/lib/internal/streams/readable.js @@ -28,7 +28,7 @@ const { ObjectDefineProperties, ObjectSetPrototypeOf, Promise, - Set, + SafeSet, SymbolAsyncIterator, Symbol } = primordials; @@ -630,7 +630,7 @@ Readable.prototype.pipe = function(dest, pipeOpts) { if (state.pipes.length === 1) { if (!state.multiAwaitDrain) { state.multiAwaitDrain = true; - state.awaitDrainWriters = new Set( + state.awaitDrainWriters = new SafeSet( state.awaitDrainWriters ? [state.awaitDrainWriters] : [] ); } @@ -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; } diff --git a/lib/internal/timers.js b/lib/internal/timers.js index d1eca3d996722d..c3bc720390b06c 100644 --- a/lib/internal/timers.js +++ b/lib/internal/timers.js @@ -78,8 +78,8 @@ const { NumberIsFinite, NumberMIN_SAFE_INTEGER, ObjectCreate, - Symbol, ReflectApply, + Symbol, } = primordials; const { diff --git a/lib/internal/util/debuglog.js b/lib/internal/util/debuglog.js index a3b8a84772054e..8d17a89598e464 100644 --- a/lib/internal/util/debuglog.js +++ b/lib/internal/util/debuglog.js @@ -78,7 +78,12 @@ function debuglog(set, cb) { debug = debuglogImpl(enabled, set); if (typeof cb === 'function') cb(debug); - debug(...args); + switch (args.length) { + case 0: return debug(); + case 1: return debug(args[0]); + case 2: return debug(args[0], args[1]); + default: return debug(...new SafeArrayIterator(args)); + } }; let enabled; let test = () => { @@ -86,7 +91,14 @@ function debuglog(set, cb) { test = () => enabled; return enabled; }; - const logger = (...args) => debug(...args); + const logger = (...args) => { + switch (args.length) { + case 0: return debug(); + case 1: return debug(args[0]); + case 2: return debug(args[0], args[1]); + default: return debug(...new SafeArrayIterator(args)); + } + }; ObjectDefineProperty(logger, 'enabled', { get() { return test(); diff --git a/lib/net.js b/lib/net.js index 639395bcf162a8..3e7dfe44e1632b 100644 --- a/lib/net.js +++ b/lib/net.js @@ -24,18 +24,13 @@ const { ArrayIsArray, ArrayPrototypeIndexOf, - ArrayPrototypePush, - ArrayPrototypeSplice, Boolean, Error, - FunctionPrototype, - FunctionPrototypeCall, Number, NumberIsNaN, NumberParseInt, ObjectDefineProperty, ObjectSetPrototypeOf, - ReflectApply, Symbol, } = primordials; @@ -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; @@ -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); @@ -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; }; @@ -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); }; @@ -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); }; @@ -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); }; @@ -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; @@ -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); }); };