From 36c886f9df89a8ff5d97971334321bdf8b20d979 Mon Sep 17 00:00:00 2001 From: FrankQiu Date: Wed, 27 Oct 2021 19:12:13 +0800 Subject: [PATCH 1/4] performance: fix timerify bug --- lib/internal/perf/timerify.js | 8 ++------ test/parallel/test-performance-function.js | 12 ++++++++++++ 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/lib/internal/perf/timerify.js b/lib/internal/perf/timerify.js index dae0b06bf80c8a..5e23e532bc01e5 100644 --- a/lib/internal/perf/timerify.js +++ b/lib/internal/perf/timerify.js @@ -21,10 +21,6 @@ const { isHistogram } = require('internal/histogram'); -const { - isConstructor, -} = internalBinding('util'); - const { codes: { ERR_INVALID_ARG_TYPE, @@ -73,11 +69,11 @@ function timerify(fn, options = {}) { if (fn[kTimerified]) return fn[kTimerified]; - const constructor = isConstructor(fn); + const isClass = (fn) => /^\s*class/.test(fn.toString()); function timerified(...args) { const start = now(); - const result = constructor ? + const result = isClass(fn) ? ReflectConstruct(fn, args, fn) : ReflectApply(fn, this, args); if (!constructor && typeof result?.finally === 'function') { diff --git a/test/parallel/test-performance-function.js b/test/parallel/test-performance-function.js index ea928028208e47..bcafe839b8c92f 100644 --- a/test/parallel/test-performance-function.js +++ b/test/parallel/test-performance-function.js @@ -13,6 +13,18 @@ const { setTimeout: sleep } = require('timers/promises'); +{ + const perf = performance.timerify(function foo() { + let sum = 0; + for (let i = 0; i < 101; i++) { + sum += i; + } + return sum; + }); + const result = perf(); + assert.strictEqual(result, 5050); +} + { // Intentional non-op. Do not wrap in common.mustCall(); const n = performance.timerify(function noop() {}); From 304d5df1ad170983f929583a539bda879f0811a0 Mon Sep 17 00:00:00 2001 From: FrankQiu Date: Thu, 28 Oct 2021 00:28:51 +0800 Subject: [PATCH 2/4] perf_hooks: add a test and drop isConstructor --- lib/internal/perf/timerify.js | 71 ++++++++++------------ src/node_util.cc | 7 --- test/parallel/test-performance-function.js | 12 ++++ 3 files changed, 44 insertions(+), 46 deletions(-) diff --git a/lib/internal/perf/timerify.js b/lib/internal/perf/timerify.js index 5e23e532bc01e5..5eb8c950f6823d 100644 --- a/lib/internal/perf/timerify.js +++ b/lib/internal/perf/timerify.js @@ -12,41 +12,30 @@ const { const { InternalPerformanceEntry } = require('internal/perf/performance_entry'); const { now } = require('internal/perf/utils'); -const { - validateFunction, - validateObject, -} = require('internal/validators'); +const { validateFunction, validateObject } = require('internal/validators'); -const { - isHistogram -} = require('internal/histogram'); +const { isHistogram } = require('internal/histogram'); const { - codes: { - ERR_INVALID_ARG_TYPE, - }, + codes: { ERR_INVALID_ARG_TYPE }, } = require('internal/errors'); -const { - enqueue, -} = require('internal/perf/observe'); +const { enqueue } = require('internal/perf/observe'); const kTimerified = Symbol('kTimerified'); function processComplete(name, start, args, histogram) { const duration = now() - start; - if (histogram !== undefined) - histogram.record(MathCeil(duration * 1e6)); - const entry = - new InternalPerformanceEntry( - name, - 'function', - start, - duration, - args); - - for (let n = 0; n < args.length; n++) - entry[n] = args[n]; + if (histogram !== undefined) histogram.record(MathCeil(duration * 1e6)); + const entry = new InternalPerformanceEntry( + name, + 'function', + start, + duration, + args + ); + + for (let n = 0; n < args.length; n++) entry[n] = args[n]; enqueue(entry); } @@ -55,27 +44,29 @@ function timerify(fn, options = {}) { validateFunction(fn, 'fn'); validateObject(options, 'options'); - const { - histogram - } = options; + const { histogram } = options; - if (histogram !== undefined && - (!isHistogram(histogram) || typeof histogram.record !== 'function')) { + if ( + histogram !== undefined && + (!isHistogram(histogram) || typeof histogram.record !== 'function') + ) { throw new ERR_INVALID_ARG_TYPE( 'options.histogram', 'RecordableHistogram', - histogram); + histogram + ); } if (fn[kTimerified]) return fn[kTimerified]; - const isClass = (fn) => /^\s*class/.test(fn.toString()); + // If the parameter is a function and it will be called directly + // or used as `new` operator. + const isCalledAsConstructor = (fn) => /^\s*class/.test(fn.toString()); function timerified(...args) { const start = now(); - const result = isClass(fn) ? - ReflectConstruct(fn, args, fn) : - ReflectApply(fn, this, args); + const result = isCalledAsConstructor(fn) ? + ReflectConstruct(fn, args, fn) : ReflectApply(fn, this, args); if (!constructor && typeof result?.finally === 'function') { return result.finally( FunctionPrototypeBind( @@ -84,7 +75,9 @@ function timerify(fn, options = {}) { fn.name, start, args, - histogram)); + histogram + ) + ); } processComplete(fn.name, start, args, histogram); return result; @@ -104,8 +97,8 @@ function timerify(fn, options = {}) { name: { configurable: false, enumerable: true, - value: `timerified ${fn.name}` - } + value: `timerified ${fn.name}`, + }, }); ObjectDefineProperties(fn, { @@ -113,7 +106,7 @@ function timerify(fn, options = {}) { configurable: false, enumerable: false, value: timerified, - } + }, }); return timerified; diff --git a/src/node_util.cc b/src/node_util.cc index 2db45bd1fb40af..5b5dab36f08fbf 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -289,11 +289,6 @@ static void GuessHandleType(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(OneByteString(env->isolate(), type)); } -static void IsConstructor(const FunctionCallbackInfo& args) { - CHECK(args[0]->IsFunction()); - args.GetReturnValue().Set(args[0].As()->IsConstructor()); -} - static void ToUSVString(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); CHECK_GE(args.Length(), 2); @@ -344,7 +339,6 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(WeakReference::IncRef); registry->Register(WeakReference::DecRef); registry->Register(GuessHandleType); - registry->Register(IsConstructor); registry->Register(ToUSVString); } @@ -384,7 +378,6 @@ void Initialize(Local target, env->SetMethodNoSideEffect(target, "getConstructorName", GetConstructorName); env->SetMethodNoSideEffect(target, "getExternalValue", GetExternalValue); env->SetMethod(target, "sleep", Sleep); - env->SetMethodNoSideEffect(target, "isConstructor", IsConstructor); env->SetMethod(target, "arrayBufferViewHasBuffer", ArrayBufferViewHasBuffer); Local constants = Object::New(env->isolate()); diff --git a/test/parallel/test-performance-function.js b/test/parallel/test-performance-function.js index bcafe839b8c92f..2b12138162cbbb 100644 --- a/test/parallel/test-performance-function.js +++ b/test/parallel/test-performance-function.js @@ -25,6 +25,18 @@ const { assert.strictEqual(result, 5050); } +{ + class Foo { + constructor() { + /* eslint-disable no-constructor-return */ + return Promise.resolve('foo'); + } + } + const perf = performance.timerify(Foo); + const result = perf(); + result.then((val) => assert.strictEqual(val, 'foo')); +} + { // Intentional non-op. Do not wrap in common.mustCall(); const n = performance.timerify(function noop() {}); From 47edde31753fd6f3a79f988beb668d4cf3d0836a Mon Sep 17 00:00:00 2001 From: FrankQiu Date: Thu, 28 Oct 2021 17:49:28 +0800 Subject: [PATCH 3/4] perf_hooks: alter the test and format timerify --- lib/internal/perf/timerify.js | 62 ++++++++++++---------- test/parallel/test-performance-function.js | 2 +- 2 files changed, 36 insertions(+), 28 deletions(-) diff --git a/lib/internal/perf/timerify.js b/lib/internal/perf/timerify.js index 5eb8c950f6823d..c9862edee68707 100644 --- a/lib/internal/perf/timerify.js +++ b/lib/internal/perf/timerify.js @@ -12,30 +12,41 @@ const { const { InternalPerformanceEntry } = require('internal/perf/performance_entry'); const { now } = require('internal/perf/utils'); -const { validateFunction, validateObject } = require('internal/validators'); +const { + validateFunction, + validateObject, +} = require('internal/validators'); -const { isHistogram } = require('internal/histogram'); +const { + isHistogram +} = require('internal/histogram'); const { - codes: { ERR_INVALID_ARG_TYPE }, + codes: { + ERR_INVALID_ARG_TYPE, + }, } = require('internal/errors'); -const { enqueue } = require('internal/perf/observe'); +const { + enqueue, +} = require('internal/perf/observe'); const kTimerified = Symbol('kTimerified'); function processComplete(name, start, args, histogram) { const duration = now() - start; - if (histogram !== undefined) histogram.record(MathCeil(duration * 1e6)); - const entry = new InternalPerformanceEntry( - name, - 'function', - start, - duration, - args - ); - - for (let n = 0; n < args.length; n++) entry[n] = args[n]; + if (histogram !== undefined) + histogram.record(MathCeil(duration * 1e6)); + const entry = + new InternalPerformanceEntry( + name, + 'function', + start, + duration, + args); + + for (let n = 0; n < args.length; n++) + entry[n] = args[n]; enqueue(entry); } @@ -44,17 +55,16 @@ function timerify(fn, options = {}) { validateFunction(fn, 'fn'); validateObject(options, 'options'); - const { histogram } = options; + const { + histogram + } = options; - if ( - histogram !== undefined && - (!isHistogram(histogram) || typeof histogram.record !== 'function') - ) { + if (histogram !== undefined && + (!isHistogram(histogram) || typeof histogram.record !== 'function')) { throw new ERR_INVALID_ARG_TYPE( 'options.histogram', 'RecordableHistogram', - histogram - ); + histogram); } if (fn[kTimerified]) return fn[kTimerified]; @@ -75,9 +85,7 @@ function timerify(fn, options = {}) { fn.name, start, args, - histogram - ) - ); + histogram)); } processComplete(fn.name, start, args, histogram); return result; @@ -97,8 +105,8 @@ function timerify(fn, options = {}) { name: { configurable: false, enumerable: true, - value: `timerified ${fn.name}`, - }, + value: `timerified ${fn.name}` + } }); ObjectDefineProperties(fn, { @@ -106,7 +114,7 @@ function timerify(fn, options = {}) { configurable: false, enumerable: false, value: timerified, - }, + } }); return timerified; diff --git a/test/parallel/test-performance-function.js b/test/parallel/test-performance-function.js index 2b12138162cbbb..2f6b8e2ef59802 100644 --- a/test/parallel/test-performance-function.js +++ b/test/parallel/test-performance-function.js @@ -34,7 +34,7 @@ const { } const perf = performance.timerify(Foo); const result = perf(); - result.then((val) => assert.strictEqual(val, 'foo')); + result.then(common.mustCall((val) => assert.strictEqual(val, 'foo'))); } { From 591f94b760f98adf47eec2bc61950903597d6fe4 Mon Sep 17 00:00:00 2001 From: FrankQiu Date: Sat, 30 Oct 2021 21:56:32 +0800 Subject: [PATCH 4/4] doc: add a comment to the fn --- doc/api/perf_hooks.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/api/perf_hooks.md b/doc/api/perf_hooks.md index 34d2a58bd017c1..6d98a26896d809 100644 --- a/doc/api/perf_hooks.md +++ b/doc/api/perf_hooks.md @@ -232,7 +232,9 @@ changes: to time async functions. --> -* `fn` {Function} +* `fn` {Function} Only accept function and class. If the fn is a function( +includes "old style" class) will be executed directly or will be executed +by the `new` operator. * `options` {Object} * `histogram` {RecordableHistogram} A histogram object created using `perf_hooks.createHistogram()` that will record runtime durations in