From 952cf0d17ae51c644a434684dabf493ccf2ebf38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vinicius=20Louren=C3=A7o?= <12551007+H4ad@users.noreply.github.com> Date: Mon, 2 Oct 2023 09:56:39 -0300 Subject: [PATCH] lib: reduce overhead of validateObject PR-URL: https://github.com/nodejs/node/pull/49928 Reviewed-By: Yagiz Nizipli Reviewed-By: Benjamin Gruenbaum Reviewed-By: Antoine du Hamel --- benchmark/validators/validate-object.js | 75 +++++++++++++++++++++ lib/fs.js | 7 +- lib/internal/abort_controller.js | 4 +- lib/internal/encoding.js | 31 +++------ lib/internal/event_target.js | 13 ++-- lib/internal/fs/promises.js | 3 +- lib/internal/util/inspect.js | 3 +- lib/internal/validators.js | 65 ++++++++++-------- lib/internal/vm.js | 6 +- lib/internal/webstreams/readablestream.js | 4 +- test/benchmark/test-benchmark-validators.js | 10 +++ test/parallel/test-validators.js | 23 +++---- 12 files changed, 170 insertions(+), 74 deletions(-) create mode 100644 benchmark/validators/validate-object.js create mode 100644 test/benchmark/test-benchmark-validators.js diff --git a/benchmark/validators/validate-object.js b/benchmark/validators/validate-object.js new file mode 100644 index 00000000000000..50e722c52426a4 --- /dev/null +++ b/benchmark/validators/validate-object.js @@ -0,0 +1,75 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); + +const bench = common.createBenchmark(main, { + n: [1e5], + objectToTest: [ + 'object', + 'null', + 'array', + 'function', + ], +}, { + flags: ['--expose-internals'], +}); + +function getObjectToTest(objectToTest) { + switch (objectToTest) { + case 'object': + return { foo: 'bar' }; + case 'null': + return null; + case 'array': + return ['foo', 'bar']; + case 'function': + return () => 'foo'; + default: + throw new Error(`Value ${objectToTest} is not a valid objectToTest.`); + } +} + +function getOptions(objectToTest) { + const { + kValidateObjectAllowNullable, + kValidateObjectAllowArray, + kValidateObjectAllowFunction, + } = require('internal/validators'); + + switch (objectToTest) { + case 'object': + return 0; + case 'null': + return kValidateObjectAllowNullable; + case 'array': + return kValidateObjectAllowArray; + case 'function': + return kValidateObjectAllowFunction; + default: + throw new Error(`Value ${objectToTest} is not a valid objectToTest.`); + } +} + +let _validateResult; + +function main({ n, objectToTest }) { + const { + validateObject, + } = require('internal/validators'); + + const value = getObjectToTest(objectToTest); + const options = getOptions(objectToTest); + + bench.start(); + for (let i = 0; i < n; ++i) { + try { + _validateResult = validateObject(value, 'Object', options); + } catch { + _validateResult = undefined; + } + } + bench.end(n); + + assert.ok(!_validateResult); +} diff --git a/lib/fs.js b/lib/fs.js index d682759d8b0a14..3ff5cff2d3fb9a 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -140,6 +140,7 @@ const { validateInteger, validateObject, validateString, + kValidateObjectAllowNullable, } = require('internal/validators'); let truncateWarn = true; @@ -624,7 +625,7 @@ function read(fd, buffer, offsetOrOptions, length, position, callback) { if (arguments.length <= 4) { if (arguments.length === 4) { // This is fs.read(fd, buffer, options, callback) - validateObject(offsetOrOptions, 'options', { nullable: true }); + validateObject(offsetOrOptions, 'options', kValidateObjectAllowNullable); callback = length; params = offsetOrOptions; } else if (arguments.length === 3) { @@ -642,7 +643,7 @@ function read(fd, buffer, offsetOrOptions, length, position, callback) { } if (params !== undefined) { - validateObject(params, 'options', { nullable: true }); + validateObject(params, 'options', kValidateObjectAllowNullable); } ({ offset = 0, @@ -715,7 +716,7 @@ function readSync(fd, buffer, offsetOrOptions, length, position) { let offset = offsetOrOptions; if (arguments.length <= 3 || typeof offsetOrOptions === 'object') { if (offsetOrOptions !== undefined) { - validateObject(offsetOrOptions, 'options', { nullable: true }); + validateObject(offsetOrOptions, 'options', kValidateObjectAllowNullable); } ({ diff --git a/lib/internal/abort_controller.js b/lib/internal/abort_controller.js index 69e9c31efd14fc..b10fb35a1a6e4f 100644 --- a/lib/internal/abort_controller.js +++ b/lib/internal/abort_controller.js @@ -46,6 +46,8 @@ const { validateAbortSignalArray, validateObject, validateUint32, + kValidateObjectAllowArray, + kValidateObjectAllowFunction, } = require('internal/validators'); const { @@ -436,7 +438,7 @@ async function aborted(signal, resource) { throw new ERR_INVALID_ARG_TYPE('signal', 'AbortSignal', signal); } validateAbortSignal(signal, 'signal'); - validateObject(resource, 'resource', { nullable: false, allowFunction: true, allowArray: true }); + validateObject(resource, 'resource', kValidateObjectAllowArray | kValidateObjectAllowFunction); if (signal.aborted) return PromiseResolve(); const abortPromise = createDeferredPromise(); diff --git a/lib/internal/encoding.js b/lib/internal/encoding.js index a9bfb665c2f1e8..6ed89b3f9b15a4 100644 --- a/lib/internal/encoding.js +++ b/lib/internal/encoding.js @@ -47,6 +47,9 @@ const { const { validateString, validateObject, + kValidateObjectAllowNullable, + kValidateObjectAllowArray, + kValidateObjectAllowFunction, } = require('internal/validators'); const binding = internalBinding('encoding_binding'); const { @@ -390,6 +393,10 @@ const TextDecoder = makeTextDecoderICU() : makeTextDecoderJS(); +const kValidateObjectAllowObjectsAndNull = kValidateObjectAllowNullable | + kValidateObjectAllowArray | + kValidateObjectAllowFunction; + function makeTextDecoderICU() { const { decode: _decode, @@ -399,11 +406,7 @@ function makeTextDecoderICU() { class TextDecoder { constructor(encoding = 'utf-8', options = kEmptyObject) { encoding = `${encoding}`; - validateObject(options, 'options', { - nullable: true, - allowArray: true, - allowFunction: true, - }); + validateObject(options, 'options', kValidateObjectAllowObjectsAndNull); const enc = getEncodingFromLabel(encoding); if (enc === undefined) @@ -448,11 +451,7 @@ function makeTextDecoderICU() { this.#prepareConverter(); - validateObject(options, 'options', { - nullable: true, - allowArray: true, - allowFunction: true, - }); + validateObject(options, 'options', kValidateObjectAllowObjectsAndNull); let flags = 0; if (options !== null) @@ -482,11 +481,7 @@ function makeTextDecoderJS() { class TextDecoder { constructor(encoding = 'utf-8', options = kEmptyObject) { encoding = `${encoding}`; - validateObject(options, 'options', { - nullable: true, - allowArray: true, - allowFunction: true, - }); + validateObject(options, 'options', kValidateObjectAllowObjectsAndNull); const enc = getEncodingFromLabel(encoding); if (enc === undefined || !hasConverter(enc)) @@ -528,11 +523,7 @@ function makeTextDecoderJS() { ['ArrayBuffer', 'ArrayBufferView'], input); } - validateObject(options, 'options', { - nullable: true, - allowArray: true, - allowFunction: true, - }); + validateObject(options, 'options', kValidateObjectAllowObjectsAndNull); if (this[kFlags] & CONVERTER_FLAGS_FLUSH) { this[kBOMSeen] = false; diff --git a/lib/internal/event_target.js b/lib/internal/event_target.js index 5ab2a23f980549..0236f3a53c5276 100644 --- a/lib/internal/event_target.js +++ b/lib/internal/event_target.js @@ -30,7 +30,14 @@ const { ERR_INVALID_THIS, }, } = require('internal/errors'); -const { validateAbortSignal, validateObject, validateString, validateInternalField } = require('internal/validators'); +const { + validateAbortSignal, + validateObject, + validateString, + validateInternalField, + kValidateObjectAllowArray, + kValidateObjectAllowFunction, +} = require('internal/validators'); const { customInspectSymbol, @@ -1037,9 +1044,7 @@ function validateEventListenerOptions(options) { if (options === null) return kEmptyObject; - validateObject(options, 'options', { - allowArray: true, allowFunction: true, - }); + validateObject(options, 'options', kValidateObjectAllowArray | kValidateObjectAllowFunction); return { once: Boolean(options.once), capture: Boolean(options.capture), diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index a656ca411584b2..28fb0127d71f66 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -82,6 +82,7 @@ const { validateInteger, validateObject, validateString, + kValidateObjectAllowNullable, } = require('internal/validators'); const pathModule = require('path'); const { @@ -597,7 +598,7 @@ async function read(handle, bufferOrParams, offset, length, position) { if (!isArrayBufferView(buffer)) { // This is fh.read(params) if (bufferOrParams !== undefined) { - validateObject(bufferOrParams, 'options', { nullable: true }); + validateObject(bufferOrParams, 'options', kValidateObjectAllowNullable); } ({ buffer = Buffer.alloc(16384), diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js index 2e3e1b60be871c..73da4446f824fe 100644 --- a/lib/internal/util/inspect.js +++ b/lib/internal/util/inspect.js @@ -154,6 +154,7 @@ const { BuiltinModule } = require('internal/bootstrap/realm'); const { validateObject, validateString, + kValidateObjectAllowArray, } = require('internal/validators'); let hexSlice; @@ -2155,7 +2156,7 @@ function format(...args) { } function formatWithOptions(inspectOptions, ...args) { - validateObject(inspectOptions, 'inspectOptions', { allowArray: true }); + validateObject(inspectOptions, 'inspectOptions', kValidateObjectAllowArray); return formatWithOptionsInternal(inspectOptions, args); } diff --git a/lib/internal/validators.js b/lib/internal/validators.js index c73cb213ebe72b..088a0a668ba8b2 100644 --- a/lib/internal/validators.js +++ b/lib/internal/validators.js @@ -177,7 +177,7 @@ function validateNumber(value, name, min = undefined, max) { throw new ERR_INVALID_ARG_TYPE(name, 'number', value); if ((min != null && value < min) || (max != null && value > max) || - ((min != null || max != null) && NumberIsNaN(value))) { + ((min != null || max != null) && NumberIsNaN(value))) { throw new ERR_OUT_OF_RANGE( name, `${min != null ? `>= ${min}` : ''}${min != null && max != null ? ' && ' : ''}${max != null ? `<= ${max}` : ''}`, @@ -218,41 +218,48 @@ function validateBoolean(value, name) { throw new ERR_INVALID_ARG_TYPE(name, 'boolean', value); } -/** - * @param {any} options - * @param {string} key - * @param {boolean} defaultValue - * @returns {boolean} - */ -function getOwnPropertyValueOrDefault(options, key, defaultValue) { - return options == null || !ObjectPrototypeHasOwnProperty(options, key) ? - defaultValue : - options[key]; -} +const kValidateObjectNone = 0; +const kValidateObjectAllowNullable = 1 << 0; +const kValidateObjectAllowArray = 1 << 1; +const kValidateObjectAllowFunction = 1 << 2; /** * @callback validateObject * @param {*} value * @param {string} name - * @param {{ - * allowArray?: boolean, - * allowFunction?: boolean, - * nullable?: boolean - * }} [options] + * @param {number} [options] */ /** @type {validateObject} */ const validateObject = hideStackFrames( - (value, name, options = null) => { - const allowArray = getOwnPropertyValueOrDefault(options, 'allowArray', false); - const allowFunction = getOwnPropertyValueOrDefault(options, 'allowFunction', false); - const nullable = getOwnPropertyValueOrDefault(options, 'nullable', false); - if ((!nullable && value === null) || - (!allowArray && ArrayIsArray(value)) || - (typeof value !== 'object' && ( - !allowFunction || typeof value !== 'function' - ))) { - throw new ERR_INVALID_ARG_TYPE(name, 'Object', value); + (value, name, options = kValidateObjectNone) => { + if (options === kValidateObjectNone) { + if (value === null || ArrayIsArray(value)) { + throw new ERR_INVALID_ARG_TYPE(name, 'Object', value); + } + + if (typeof value !== 'object') { + throw new ERR_INVALID_ARG_TYPE(name, 'Object', value); + } + } else { + const throwOnNullable = (kValidateObjectAllowNullable & options) === 0; + + if (throwOnNullable && value === null) { + throw new ERR_INVALID_ARG_TYPE(name, 'Object', value); + } + + const throwOnArray = (kValidateObjectAllowArray & options) === 0; + + if (throwOnArray && ArrayIsArray(value)) { + throw new ERR_INVALID_ARG_TYPE(name, 'Object', value); + } + + const throwOnFunction = (kValidateObjectAllowFunction & options) === 0; + const typeofValue = typeof value; + + if (typeofValue !== 'object' && (throwOnFunction || typeofValue !== 'function')) { + throw new ERR_INVALID_ARG_TYPE(name, 'Object', value); + } } }); @@ -564,6 +571,10 @@ module.exports = { validateInteger, validateNumber, validateObject, + kValidateObjectNone, + kValidateObjectAllowNullable, + kValidateObjectAllowArray, + kValidateObjectAllowFunction, validateOneOf, validatePlainFunction, validatePort, diff --git a/lib/internal/vm.js b/lib/internal/vm.js index ba5e2324667374..100118e7b4dbb5 100644 --- a/lib/internal/vm.js +++ b/lib/internal/vm.js @@ -17,13 +17,15 @@ const { validateString, validateStringArray, validateUint32, + kValidateObjectAllowArray, + kValidateObjectAllowNullable, } = require('internal/validators'); const { ERR_INVALID_ARG_TYPE, } = require('internal/errors').codes; function isContext(object) { - validateObject(object, 'object', { __proto__: null, allowArray: true }); + validateObject(object, 'object', kValidateObjectAllowArray); return _isContext(object); } @@ -67,7 +69,7 @@ function internalCompileFunction(code, params, options) { validateArray(contextExtensions, 'options.contextExtensions'); ArrayPrototypeForEach(contextExtensions, (extension, i) => { const name = `options.contextExtensions[${i}]`; - validateObject(extension, name, { __proto__: null, nullable: true }); + validateObject(extension, name, kValidateObjectAllowNullable); }); const result = compileFunction( diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index 4bdd1fa4862a8f..8f074efe696455 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -59,6 +59,8 @@ const { validateAbortSignal, validateBuffer, validateObject, + kValidateObjectAllowNullable, + kValidateObjectAllowFunction, } = require('internal/validators'); const { @@ -345,7 +347,7 @@ class ReadableStream { getReader(options = kEmptyObject) { if (!isReadableStream(this)) throw new ERR_INVALID_THIS('ReadableStream'); - validateObject(options, 'options', { nullable: true, allowFunction: true }); + validateObject(options, 'options', kValidateObjectAllowNullable | kValidateObjectAllowFunction); const mode = options?.mode; if (mode === undefined) diff --git a/test/benchmark/test-benchmark-validators.js b/test/benchmark/test-benchmark-validators.js new file mode 100644 index 00000000000000..3a6caba9b0bb20 --- /dev/null +++ b/test/benchmark/test-benchmark-validators.js @@ -0,0 +1,10 @@ +'use strict'; + +require('../common'); + +// Minimal test for assert benchmarks. This makes sure the benchmarks aren't +// completely broken but nothing more than that. + +const runBenchmark = require('../common/benchmark'); + +runBenchmark('validators'); diff --git a/test/parallel/test-validators.js b/test/parallel/test-validators.js index a40139678eee65..848fa0c27bbebe 100644 --- a/test/parallel/test-validators.js +++ b/test/parallel/test-validators.js @@ -9,6 +9,9 @@ const { validateInteger, validateNumber, validateObject, + kValidateObjectAllowNullable, + kValidateObjectAllowArray, + kValidateObjectAllowFunction, validateString, validateInt32, validateUint32, @@ -106,10 +109,6 @@ const invalidArgValueError = { { // validateObject tests. - Object.prototype.nullable = true; - Object.prototype.allowArray = true; - Object.prototype.allowFunction = true; - validateObject({}, 'foo'); validateObject({ a: 42, b: 'foo' }, 'foo'); @@ -121,18 +120,14 @@ const invalidArgValueError = { }); // validateObject options tests: - validateObject(null, 'foo', { nullable: true }); - validateObject([], 'foo', { allowArray: true }); - validateObject(() => {}, 'foo', { allowFunction: true }); + validateObject(null, 'foo', kValidateObjectAllowNullable); + validateObject([], 'foo', kValidateObjectAllowArray); + validateObject(() => {}, 'foo', kValidateObjectAllowFunction); // validateObject should not be affected by Object.prototype tampering. - assert.throws(() => validateObject(null, 'foo', { allowArray: true }), invalidArgTypeError); - assert.throws(() => validateObject([], 'foo', { nullable: true }), invalidArgTypeError); - assert.throws(() => validateObject(() => {}, 'foo', { nullable: true }), invalidArgTypeError); - - delete Object.prototype.nullable; - delete Object.prototype.allowArray; - delete Object.prototype.allowFunction; + assert.throws(() => validateObject(null, 'foo', kValidateObjectAllowArray), invalidArgTypeError); + assert.throws(() => validateObject([], 'foo', kValidateObjectAllowNullable), invalidArgTypeError); + assert.throws(() => validateObject(() => {}, 'foo', kValidateObjectAllowNullable), invalidArgTypeError); } {