From 1daccf51c1f81c735bee3c49fe7dcd2c8fb48cd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Louren=C3=A7o?= Date: Thu, 5 Oct 2023 21:39:55 -0300 Subject: [PATCH 1/3] perf_hooks: reduce overhead of createHistogram PR-URL: https://github.com/nodejs/node/pull/50074 Backport-PR-URL: https://github.com/nodejs/node/pull/51306 Reviewed-By: Stephen Belanger Reviewed-By: Matteo Collina Reviewed-By: Rafael Gonzaga --- benchmark/perf_hooks/histogram-clone.js | 24 +++++++++++ benchmark/perf_hooks/histogram-create.js | 22 ++++++++++ lib/internal/histogram.js | 54 +++++++++++++++--------- 3 files changed, 81 insertions(+), 19 deletions(-) create mode 100644 benchmark/perf_hooks/histogram-clone.js create mode 100644 benchmark/perf_hooks/histogram-create.js diff --git a/benchmark/perf_hooks/histogram-clone.js b/benchmark/perf_hooks/histogram-clone.js new file mode 100644 index 00000000000000..4b610963fa2a0a --- /dev/null +++ b/benchmark/perf_hooks/histogram-clone.js @@ -0,0 +1,24 @@ +'use strict'; + +const assert = require('assert'); +const common = require('../common.js'); + +const { createHistogram } = require('perf_hooks'); + +const bench = common.createBenchmark(main, { + n: [1e5], +}); + +let _histogram; + +function main({ n }) { + const histogram = createHistogram(); + + bench.start(); + for (let i = 0; i < n; i++) + _histogram = structuredClone(histogram); + bench.end(n); + + // Avoid V8 deadcode (elimination) + assert.ok(_histogram); +} diff --git a/benchmark/perf_hooks/histogram-create.js b/benchmark/perf_hooks/histogram-create.js new file mode 100644 index 00000000000000..89ddad1fa79224 --- /dev/null +++ b/benchmark/perf_hooks/histogram-create.js @@ -0,0 +1,22 @@ +'use strict'; + +const assert = require('assert'); +const common = require('../common.js'); + +const { createHistogram } = require('perf_hooks'); + +const bench = common.createBenchmark(main, { + n: [1e5], +}); + +let _histogram; + +function main({ n }) { + bench.start(); + for (let i = 0; i < n; i++) + _histogram = createHistogram(); + bench.end(n); + + // Avoid V8 deadcode (elimination) + assert.ok(_histogram); +} diff --git a/lib/internal/histogram.js b/lib/internal/histogram.js index 37bde8195e4d34..62d72e7c99475c 100644 --- a/lib/internal/histogram.js +++ b/lib/internal/histogram.js @@ -52,9 +52,13 @@ function isHistogram(object) { return object?.[kHandle] !== undefined; } +const kSkipThrow = Symbol('kSkipThrow'); + class Histogram { - constructor() { - throw new ERR_ILLEGAL_CONSTRUCTOR(); + constructor(skipThrowSymbol = undefined) { + if (skipThrowSymbol !== kSkipThrow) { + throw new ERR_ILLEGAL_CONSTRUCTOR(); + } } [kInspect](depth, options) { @@ -242,7 +246,7 @@ class Histogram { const handle = this[kHandle]; return { data: { handle }, - deserializeInfo: 'internal/histogram:internalHistogram', + deserializeInfo: 'internal/histogram:ClonedHistogram', }; } @@ -264,8 +268,12 @@ class Histogram { } class RecordableHistogram extends Histogram { - constructor() { - throw new ERR_ILLEGAL_CONSTRUCTOR(); + constructor(skipThrowSymbol = undefined) { + if (skipThrowSymbol !== kSkipThrow) { + throw new ERR_ILLEGAL_CONSTRUCTOR(); + } + + super(skipThrowSymbol); } /** @@ -309,7 +317,7 @@ class RecordableHistogram extends Histogram { const handle = this[kHandle]; return { data: { handle }, - deserializeInfo: 'internal/histogram:internalRecordableHistogram', + deserializeInfo: 'internal/histogram:ClonedRecordableHistogram', }; } @@ -318,24 +326,32 @@ class RecordableHistogram extends Histogram { } } -function internalHistogram(handle) { +function ClonedHistogram(handle) { return makeTransferable(ReflectConstruct( function() { this[kHandle] = handle; this[kMap] = new SafeMap(); }, [], Histogram)); } -internalHistogram.prototype[kDeserialize] = () => {}; -function internalRecordableHistogram(handle) { - return makeTransferable(ReflectConstruct( - function() { - this[kHandle] = handle; - this[kMap] = new SafeMap(); - this[kRecordable] = true; - }, [], RecordableHistogram)); +ClonedHistogram.prototype[kDeserialize] = () => { }; + +function ClonedRecordableHistogram(handle) { + const histogram = new RecordableHistogram(kSkipThrow); + + histogram[kRecordable] = true; + histogram[kMap] = new SafeMap(); + histogram[kHandle] = handle; + histogram.constructor = RecordableHistogram; + + return makeTransferable(histogram); +} + +ClonedRecordableHistogram.prototype[kDeserialize] = () => { }; + +function createRecordableHistogram(handle) { + return new ClonedRecordableHistogram(handle); } -internalRecordableHistogram.prototype[kDeserialize] = () => {}; /** * @param {{ @@ -361,14 +377,14 @@ function createHistogram(options = kEmptyObject) { throw new ERR_INVALID_ARG_VALUE.RangeError('options.highest', highest); } validateInteger(figures, 'options.figures', 1, 5); - return internalRecordableHistogram(new _Histogram(lowest, highest, figures)); + return createRecordableHistogram(new _Histogram(lowest, highest, figures)); } module.exports = { Histogram, RecordableHistogram, - internalHistogram, - internalRecordableHistogram, + ClonedHistogram, + ClonedRecordableHistogram, isHistogram, kDestroy, kHandle, From 5ff73f3a3abf17d0ba7207c2085f31dd38165653 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Louren=C3=A7o?= Date: Tue, 17 Oct 2023 21:26:40 -0300 Subject: [PATCH 2/3] test: avoid v8 deadcode on performance function PR-URL: https://github.com/nodejs/node/pull/50074 Backport-PR-URL: https://github.com/nodejs/node/pull/51306 Reviewed-By: Stephen Belanger Reviewed-By: Matteo Collina Reviewed-By: Rafael Gonzaga --- test/parallel/test-performance-function.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-performance-function.js b/test/parallel/test-performance-function.js index 5f774d6c2adcf5..b69a21308cf048 100644 --- a/test/parallel/test-performance-function.js +++ b/test/parallel/test-performance-function.js @@ -90,14 +90,20 @@ const { } (async () => { + let _deadCode; + const histogram = createHistogram(); - const m = (a, b = 1) => {}; + const m = (a, b = 1) => { + for (let i = 0; i < 1e3; i++) + _deadCode = i; + }; const n = performance.timerify(m, { histogram }); assert.strictEqual(histogram.max, 0); for (let i = 0; i < 10; i++) { n(); await sleep(10); } + assert.ok(_deadCode >= 0); assert.notStrictEqual(histogram.max, 0); [1, '', {}, [], false].forEach((histogram) => { assert.throws(() => performance.timerify(m, { histogram }), { From d2ac2e46a00a8b1f1499cadb04321ce4ae0ee093 Mon Sep 17 00:00:00 2001 From: Pulkit Gupta Date: Fri, 15 Dec 2023 12:23:08 +0530 Subject: [PATCH 3/3] test_runner: format coverage report for tap reporter PR-URL: https://github.com/nodejs/node/pull/51119 Reviewed-By: Raz Luvaton Reviewed-By: Luigi Pinca Reviewed-By: Moshe Atlow Reviewed-By: Debadree Chatterjee --- lib/internal/test_runner/reporter/tap.js | 2 +- test/parallel/test-runner-coverage.js | 43 +++++++++++++----------- 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/lib/internal/test_runner/reporter/tap.js b/lib/internal/test_runner/reporter/tap.js index 74d198344c9802..cd0bbe8ca53003 100644 --- a/lib/internal/test_runner/reporter/tap.js +++ b/lib/internal/test_runner/reporter/tap.js @@ -58,7 +58,7 @@ async function * tapReporter(source) { yield `${indent(data.nesting)}# ${tapEscape(data.message)}\n`; break; case 'test:coverage': - yield getCoverageReport(indent(data.nesting), data.summary, '# ', ''); + yield getCoverageReport(indent(data.nesting), data.summary, '# ', '', true); break; } } diff --git a/test/parallel/test-runner-coverage.js b/test/parallel/test-runner-coverage.js index 1eed7454c88fea..d8181417205b46 100644 --- a/test/parallel/test-runner-coverage.js +++ b/test/parallel/test-runner-coverage.js @@ -22,16 +22,21 @@ function findCoverageFileForPid(pid) { } function getTapCoverageFixtureReport() { + /* eslint-disable max-len */ const report = [ '# start of coverage report', - '# file | line % | branch % | funcs % | uncovered lines', - '# test/fixtures/test-runner/coverage.js | 78.65 | 38.46 | 60.00 | 12, ' + - '13, 16, 17, 18, 19, 20, 21, 22, 27, 39, 43, 44, 61, 62, 66, 67, 71, 72', - '# test/fixtures/test-runner/invalid-tap.js | 100.00 | 100.00 | 100.00 | ', - '# test/fixtures/v8-coverage/throw.js | 71.43 | 50.00 | 100.00 | 5, 6', - '# all files | 78.35 | 43.75 | 60.00 |', + '# -------------------------------------------------------------------------------------------------------------------', + '# file | line % | branch % | funcs % | uncovered lines', + '# -------------------------------------------------------------------------------------------------------------------', + '# test/fixtures/test-runner/coverage.js | 78.65 | 38.46 | 60.00 | 12-13 16-22 27 39 43-44 61-62 66-67 71-72', + '# test/fixtures/test-runner/invalid-tap.js | 100.00 | 100.00 | 100.00 | ', + '# test/fixtures/v8-coverage/throw.js | 71.43 | 50.00 | 100.00 | 5-6', + '# -------------------------------------------------------------------------------------------------------------------', + '# all files | 78.35 | 43.75 | 60.00 |', + '# -------------------------------------------------------------------------------------------------------------------', '# end of coverage report', ].join('\n'); + /* eslint-enable max-len */ if (common.isWindows) { return report.replaceAll('/', '\\'); @@ -88,7 +93,6 @@ test('test tap coverage reporter', skipIfNoInspector, async (t) => { const options = { env: { ...process.env, NODE_V8_COVERAGE: tmpdir.path } }; const result = spawnSync(process.execPath, args, options); const report = getTapCoverageFixtureReport(); - assert(result.stdout.toString().includes(report)); assert.strictEqual(result.stderr.toString(), ''); assert.strictEqual(result.status, 0); @@ -152,16 +156,16 @@ test('single process coverage is the same with --test', skipIfNoInspector, () => test('coverage is combined for multiple processes', skipIfNoInspector, () => { let report = [ '# start of coverage report', - '# file | line % | branch % | funcs % | uncovered lines', - '# test/fixtures/v8-coverage/combined_coverage/common.js | 89.86 | ' + - '62.50 | 100.00 | 8, 13, 14, 18, 34, 35, 53', - '# test/fixtures/v8-coverage/combined_coverage/first.test.js | 83.33 | ' + - '100.00 | 50.00 | 5, 6', - '# test/fixtures/v8-coverage/combined_coverage/second.test.js | 100.00 ' + - '| 100.00 | 100.00 | ', - '# test/fixtures/v8-coverage/combined_coverage/third.test.js | 100.00 | ' + - '100.00 | 100.00 | ', - '# all files | 92.11 | 72.73 | 88.89 |', + '# -------------------------------------------------------------------', + '# file | line % | branch % | funcs % | uncovered lines', + '# -------------------------------------------------------------------', + '# common.js | 89.86 | 62.50 | 100.00 | 8 13-14 18 34-35 53', + '# first.test.js | 83.33 | 100.00 | 50.00 | 5-6', + '# second.test.js | 100.00 | 100.00 | 100.00 | ', + '# third.test.js | 100.00 | 100.00 | 100.00 | ', + '# -------------------------------------------------------------------', + '# all files | 92.11 | 72.73 | 88.89 |', + '# -------------------------------------------------------------------', '# end of coverage report', ].join('\n'); @@ -171,10 +175,11 @@ test('coverage is combined for multiple processes', skipIfNoInspector, () => { const fixture = fixtures.path('v8-coverage', 'combined_coverage'); const args = [ - '--test', '--experimental-test-coverage', '--test-reporter', 'tap', fixture, + '--test', '--experimental-test-coverage', '--test-reporter', 'tap', ]; const result = spawnSync(process.execPath, args, { - env: { ...process.env, NODE_TEST_TMPDIR: tmpdir.path } + env: { ...process.env, NODE_TEST_TMPDIR: tmpdir.path }, + cwd: fixture, }); assert.strictEqual(result.stderr.toString(), '');