From eb8f7ee634065b24888e4c5f28fb32d530eb7036 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 Backport-PR-URL: https://github.com/nodejs/node/pull/38972 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 | 24 ++++++++----------- lib/internal/async_hooks.js | 20 +++++++--------- lib/internal/per_context/primordials.js | 8 +++---- lib/internal/streams/readable.js | 4 ++-- lib/internal/timers.js | 2 +- lib/net.js | 24 ++++++++----------- test/parallel/test-stream-pipe-await-drain.js | 2 +- 9 files changed, 41 insertions(+), 53 deletions(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 27a1f4454c1581..dbd53365f06e51 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..77db8a28fdb241 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 @@ -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; @@ -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..6492da782a9edb 100644 --- a/lib/internal/per_context/primordials.js +++ b/lib/internal/per_context/primordials.js @@ -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` diff --git a/lib/internal/streams/readable.js b/lib/internal/streams/readable.js index fd2a8635674899..0b6a8f7b9cbd26 100644 --- a/lib/internal/streams/readable.js +++ b/lib/internal/streams/readable.js @@ -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/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); }); }; diff --git a/test/parallel/test-stream-pipe-await-drain.js b/test/parallel/test-stream-pipe-await-drain.js index 90d418a09783e3..35b86f67f99676 100644 --- a/test/parallel/test-stream-pipe-await-drain.js +++ b/test/parallel/test-stream-pipe-await-drain.js @@ -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