diff --git a/.gitignore b/.gitignore index ab29cbc0b3..cce2ab1446 100644 --- a/.gitignore +++ b/.gitignore @@ -19,3 +19,5 @@ test/versioned-external/TEMP_TESTS nr-security-home # Needed for testing !test/integration/moduleLoading/node_modules +# benchmark results +benchmark_results diff --git a/bin/compare-bench-results.js b/bin/compare-bench-results.js index 330c0f9eab..613b6a82fb 100644 --- a/bin/compare-bench-results.js +++ b/bin/compare-bench-results.js @@ -27,7 +27,7 @@ const processFile = async (file) => { } } -const reportResults = (resultFiles) => { +const reportResults = async (resultFiles) => { const baseline = resultFiles[0] const downstream = resultFiles[1] @@ -88,8 +88,7 @@ const reportResults = (resultFiles) => { allPassing = allPassing && filePassing return [ - '
', - `${testFile}: ${passMark(filePassing)}`, + `#### ${testFile}: ${passMark(filePassing)}`, '', results, '', @@ -105,11 +104,22 @@ const reportResults = (resultFiles) => { console.log('') } - console.log(`### Benchmark Results: ${passMark(allPassing)}`) - console.log('') - console.log('### Details') - console.log('_Lower is better._') - console.log(details) + const date = new Date() + let content = `### Benchmark Results: ${passMark(allPassing)}\n\n\n\n` + content += `${date.toISOString()}\n\n` + content += '### Details\n\n' + content += '_Lower is better._\n\n' + content += `${details}\n` + + const resultPath = 'benchmark_results' + try { + await fs.stat(resultPath) + } catch (e) { + await fs.mkdir(resultPath) + } + const fileName = `${resultPath}/comparison_${date.getTime()}.md` + await fs.writeFile(fileName, content) + console.log(`Done! Benchmark test comparison written to ${fileName}`) if (!allPassing) { process.exitCode = -1 @@ -118,9 +128,11 @@ const reportResults = (resultFiles) => { const iterate = async () => { const files = process.argv.slice(2) - const results = files.map(async (file) => { - return processFile(file) - }) + const results = await Promise.all( + files.map(async (file) => { + return await processFile(file) + }) + ) reportResults(results) } diff --git a/bin/run-bench.js b/bin/run-bench.js index dcc2c13c33..08d02cb022 100644 --- a/bin/run-bench.js +++ b/bin/run-bench.js @@ -5,29 +5,20 @@ 'use strict' -/* eslint sonarjs/cognitive-complexity: ["error", 21] -- TODO: https://issues.newrelic.com/browse/NEWRELIC-5252 */ - const cp = require('child_process') const glob = require('glob') const path = require('path') const { errorAndExit } = require('./utils') +const fs = require('fs/promises') const cwd = path.resolve(__dirname, '..') const benchpath = path.resolve(cwd, 'test/benchmark') const tests = [] +const testPromises = [] const globs = [] const opts = Object.create(null) -// replacement for former async-lib cb -const testCb = (err, payload) => { - if (err) { - console.error(err) - return - } - return payload -} - process.argv.slice(2).forEach(function forEachFileArg(file) { if (/^--/.test(file)) { opts[file.substring(2)] = true @@ -48,70 +39,81 @@ if (tests.length === 0 && globs.length === 0) { globs.push(path.join(benchpath, '*.bench.js'), path.join(benchpath, '**/*.bench.js')) } -class ConsolePrinter { - /* eslint-disable no-console */ - addTest(name, child) { - console.log(name) - child.stdout.on('data', (d) => process.stdout.write(d)) - child.stderr.on('data', (d) => process.stderr.write(d)) - child.once('exit', () => console.log('')) - } - - finish() { - console.log('') - } - /* eslint-enable no-console */ -} - -class JSONPrinter { +class Printer { constructor() { this._tests = Object.create(null) } addTest(name, child) { let output = '' - this._tests[name] = null child.stdout.on('data', (d) => (output += d.toString())) - child.stdout.on('end', () => (this._tests[name] = JSON.parse(output))) child.stderr.on('data', (d) => process.stderr.write(d)) + + this._tests[name] = new Promise((resolve) => { + child.stdout.on('end', () => { + try { + this._tests[name] = JSON.parse(output) + } catch (e) { + console.error(`Error parsing test results for ${name}`, e) + this._tests[name] = output + } + resolve() + }) + }) } - finish() { - /* eslint-disable no-console */ - console.log(JSON.stringify(this._tests, null, 2)) - /* eslint-enable no-console */ + async finish() { + if (opts.console) { + /* eslint-disable no-console */ + console.log(JSON.stringify(this._tests, null, 2)) + /* eslint-enable no-console */ + } + const resultPath = 'benchmark_results' + try { + await fs.stat(resultPath) + } catch (e) { + await fs.mkdir(resultPath) + } + const content = JSON.stringify(this._tests, null, 2) + const fileName = `${resultPath}/benchmark_${new Date().getTime()}.json` + await fs.writeFile(fileName, content) + console.log(`Done! Test output written to ${fileName}`) } } run() async function run() { - const printer = opts.json ? new JSONPrinter() : new ConsolePrinter() + const printer = new Printer() + let currentTest = 0 - const resolveGlobs = (cb) => { + const resolveGlobs = () => { if (!globs.length) { - cb() + console.error(`There aren't any globs to resolve.`) + return } - const afterGlobbing = (err, resolved) => { - if (err) { - errorAndExit(err, 'Failed to glob', -1) - cb(err) + const afterGlobbing = (resolved) => { + if (!resolved) { + return errorAndExit(new Error('Failed to glob'), 'Failed to glob', -1) } - resolved.forEach(function mergeResolved(files) { - files.forEach(function mergeFile(file) { - if (tests.indexOf(file) === -1) { - tests.push(file) - } - }) - }) - cb() // ambient scope + + function mergeFile(file) { + if (tests.indexOf(file) === -1) { + tests.push(file) + } + } + function mergeResolved(files) { + files.forEach(mergeFile) + } + + return resolved.forEach(mergeResolved) } const globbed = globs.map((item) => glob.sync(item)) - return afterGlobbing(null, globbed) + return afterGlobbing(globbed) } - const spawnEachFile = (file, spawnCb) => { + const spawnEachFile = async (file) => { const test = path.relative(benchpath, file) const args = [file] @@ -119,34 +121,36 @@ async function run() { args.unshift('--inspect-brk') } - const child = cp.spawn('node', args, { cwd: cwd, stdio: 'pipe' }) - printer.addTest(test, child) + const child = cp.spawn('node', args, { cwd: cwd, stdio: 'pipe', silent: true }) - child.on('error', spawnCb) + child.on('error', (err) => { + console.error(`Error in child test ${test}`, err) + throw err + }) child.on('exit', function onChildExit(code) { + currentTest = currentTest + 1 if (code) { - spawnCb(new Error('Benchmark exited with code ' + code)) + console.error(`(${currentTest}/${tests.length}) FAILED: ${test} exited with code ${code}`) + return } - spawnCb() + console.log(`(${currentTest}/${tests.length}) ${file} has completed`) }) + printer.addTest(test, child) } - const afterSpawnEachFile = (err, cb) => { - if (err) { - errorAndExit(err, 'Spawning failed:', -2) - return cb(err) - } - cb() - } - - const runBenchmarks = async (cb) => { + const runBenchmarks = async () => { tests.sort() - await tests.forEach((file) => spawnEachFile(file, testCb)) - await afterSpawnEachFile(null, testCb) - return cb() + for await (const file of tests) { + await spawnEachFile(file) + } + const keys = Object.keys(printer._tests) + for (const key of keys) { + testPromises.push(printer._tests[key]) + } } - await resolveGlobs(testCb) - await runBenchmarks(testCb) + await resolveGlobs() + await runBenchmarks() + await Promise.all(testPromises) printer.finish() } diff --git a/test/benchmark/async-hooks.bench.js b/test/benchmark/async-hooks.bench.js index 51871527c9..14d9599bff 100644 --- a/test/benchmark/async-hooks.bench.js +++ b/test/benchmark/async-hooks.bench.js @@ -9,7 +9,6 @@ const benchmark = require('../lib/benchmark') const suite = benchmark.createBenchmark({ name: 'async hooks', - async: true, fn: runBenchmark }) @@ -52,10 +51,11 @@ tests.forEach((test) => suite.add(test)) suite.run() -function runBenchmark(agent, cb) { +function runBenchmark() { let p = Promise.resolve() for (let i = 0; i < 300; ++i) { p = p.then(function noop() {}) } - p.then(cb) + + return p } diff --git a/test/benchmark/datastore-shim/recordBatch.bench.js b/test/benchmark/datastore-shim/recordBatch.bench.js index 66a7c35608..ad2552d185 100644 --- a/test/benchmark/datastore-shim/recordBatch.bench.js +++ b/test/benchmark/datastore-shim/recordBatch.bench.js @@ -19,31 +19,34 @@ function makeInit(instrumented) { suite.add({ name: 'instrumented operation', - async: true, initialize: makeInit(true), agent: {}, - fn: function (agent, done) { - testDatastore.testBatch('test', done) + fn: function () { + return new Promise((resolve) => { + testDatastore.testBatch('test', resolve) + }) } }) suite.add({ name: 'uninstrumented operation', initialize: makeInit(false), - async: true, - fn: function (agent, done) { - testDatastore.testBatch('test', done) + fn: function () { + return new Promise((resolve) => { + testDatastore.testBatch('test', resolve) + }) } }) suite.add({ name: 'instrumented operation in transaction', - async: true, agent: {}, initialize: makeInit(true), runInTransaction: true, - fn: function (agent, done) { - testDatastore.testBatch('test', done) + fn: function () { + return new Promise((resolve) => { + testDatastore.testBatch('test', resolve) + }) } }) diff --git a/test/benchmark/datastore-shim/recordOperation.bench.js b/test/benchmark/datastore-shim/recordOperation.bench.js index a81849743e..e5d0179557 100644 --- a/test/benchmark/datastore-shim/recordOperation.bench.js +++ b/test/benchmark/datastore-shim/recordOperation.bench.js @@ -19,31 +19,34 @@ function makeInit(instrumented) { suite.add({ name: 'instrumented operation in transaction', - async: true, agent: {}, initialize: makeInit(true), runInTransaction: true, - fn: function (agent, done) { - testDatastore.testOp(done) + fn: function () { + return new Promise((resolve) => { + testDatastore.testOp(resolve) + }) } }) suite.add({ name: 'instrumented operation', - async: true, initialize: makeInit(true), agent: {}, - fn: function (agent, done) { - testDatastore.testOp(done) + fn: function () { + return new Promise((resolve) => { + testDatastore.testOp(resolve) + }) } }) suite.add({ name: 'uninstrumented operation', initialize: makeInit(false), - async: true, - fn: function (agent, done) { - testDatastore.testOp(done) + fn: function () { + return new Promise((resolve) => { + testDatastore.testOp(resolve) + }) } }) diff --git a/test/benchmark/datastore-shim/recordQuery.bench.js b/test/benchmark/datastore-shim/recordQuery.bench.js index 02df4f2a6b..d49f388329 100644 --- a/test/benchmark/datastore-shim/recordQuery.bench.js +++ b/test/benchmark/datastore-shim/recordQuery.bench.js @@ -19,31 +19,34 @@ function makeInit(instrumented) { suite.add({ name: 'instrumented operation in transaction', - async: true, agent: {}, initialize: makeInit(true), runInTransaction: true, - fn: function (agent, done) { - testDatastore.testQuery('test', done) + fn: function () { + return new Promise((resolve) => { + testDatastore.testQuery('test', resolve) + }) } }) suite.add({ name: 'instrumented operation', - async: true, initialize: makeInit(true), agent: {}, - fn: function (agent, done) { - testDatastore.testQuery('test', done) + fn: function () { + return new Promise((resolve) => { + testDatastore.testQuery('test', resolve) + }) } }) suite.add({ name: 'uninstrumented operation', initialize: makeInit(false), - async: true, - fn: function (agent, done) { - testDatastore.testQuery('test', done) + fn: function () { + return new Promise((resolve) => { + testDatastore.testQuery('test', resolve) + }) } }) diff --git a/test/benchmark/datastore-shim/test-datastore.js b/test/benchmark/datastore-shim/test-datastore.js index 011f04b1ba..fc5ae81f23 100644 --- a/test/benchmark/datastore-shim/test-datastore.js +++ b/test/benchmark/datastore-shim/test-datastore.js @@ -7,24 +7,15 @@ class TestDatastore { testOp(cb) { - if (typeof cb === 'function') { - return setImmediate(cb) - } - return cb || 'testOp' + setImmediate(cb) } testQuery(query, cb) { - if (typeof cb === 'function') { - return setImmediate(cb) - } - return cb || 'testQuery' + setImmediate(cb) } testBatch(query, cb) { - if (typeof cb === 'function') { - return setImmediate(cb) - } - return cb || 'testBatch' + setImmediate(cb) } } diff --git a/test/benchmark/http/server.bench.js b/test/benchmark/http/server.bench.js index 1e0782026f..de1635ed34 100644 --- a/test/benchmark/http/server.bench.js +++ b/test/benchmark/http/server.bench.js @@ -10,40 +10,60 @@ const http = require('http') const suite = benchmark.createBenchmark({ name: 'http', runs: 5000 }) -let server = null -const PORT = 3000 +const HOST = 'localhost' +// manage the servers separately +// since we have to enqueue the server.close +// to avoid net connect errors +const servers = { + 3000: null, + 3001: null +} suite.add({ name: 'uninstrumented http.Server', - async: true, - initialize: createServer, - fn: (agent, done) => makeRequest(done), - teardown: closeServer + initialize: createServer(3000), + fn: setupRequest(3000), + teardown: closeServer(3000) }) suite.add({ name: 'instrumented http.Server', agent: {}, - async: true, - initialize: createServer, - fn: (agent, done) => makeRequest(done), - teardown: closeServer + initialize: createServer(3001), + fn: setupRequest(3001), + teardown: closeServer(3001) }) suite.run() -function createServer() { - server = http.createServer((req, res) => { - res.end() - }) - server.listen(PORT) +function createServer(port) { + return async function makeServer() { + return new Promise((resolve, reject) => { + servers[port] = http.createServer((req, res) => { + res.end() + }) + servers[port].listen(port, HOST, (err) => { + if (err) { + reject(err) + } + resolve() + }) + }) + } } -function closeServer() { - server && server.close() - server = null +function closeServer(port) { + return function close() { + setImmediate(() => { + servers[port].close() + }) + } } -function makeRequest(cb) { - http.request({ port: PORT }, cb).end() +function setupRequest(port) { + return async function makeRequest() { + return new Promise((resolve) => { + http.request({ host: HOST, port }, resolve).end() + }) + } } diff --git a/test/benchmark/promises/native.bench.js b/test/benchmark/promises/native.bench.js index 4a3e478ce6..714f1a2155 100644 --- a/test/benchmark/promises/native.bench.js +++ b/test/benchmark/promises/native.bench.js @@ -10,7 +10,6 @@ const shared = require('./shared') const suite = shared.makeSuite('Promises') shared.tests.forEach(function registerTest(testFn) { suite.add({ - defer: true, name: testFn.name, fn: testFn(Promise), agent: { diff --git a/test/benchmark/promises/shared.js b/test/benchmark/promises/shared.js index 8e4115d654..13aba3b122 100644 --- a/test/benchmark/promises/shared.js +++ b/test/benchmark/promises/shared.js @@ -8,14 +8,14 @@ const benchmark = require('../../lib/benchmark') function makeSuite(name) { - return benchmark.createBenchmark({ async: true, name: name, delay: 0.01 }) + return benchmark.createBenchmark({ name }) } const NUM_PROMISES = 300 const tests = [ function forkedTest(Promise) { - return function runTest(agent, cb) { + return function runTest() { // number of internal nodes on the binary tree of promises // this will produce a binary tree with NUM_PROMISES / 2 internal // nodes, and NUM_PROMIES / 2 + 1 leaves @@ -27,79 +27,57 @@ const tests = [ promises.push(prom.then(function first() {})) promises.push(prom.then(function second() {})) } - Promise.all(promises).then(() => { - if (typeof cb === 'function') { - return cb() - } - return cb || true - }) + return Promise.all(promises) } }, function longTest(Promise) { - return function runTest(agent, cb) { + return function runTest() { let prom = Promise.resolve() for (let i = 0; i < NUM_PROMISES; ++i) { prom = prom.then(function () {}) } - prom.then(() => { - if (typeof cb === 'function') { - return cb() - } - return cb || true - }) + return prom } }, function longTestWithCatches(Promise) { - return function runTest(agent, cb) { + return function runTest() { let prom = Promise.resolve() for (let i = 0; i < NUM_PROMISES / 2; ++i) { prom = prom.then(function () {}).catch(function () {}) } - prom.then(() => { - if (typeof cb === 'function') { - return cb() - } - return cb || true - }) + return prom } }, function longThrowToEnd(Promise) { - return function runTest(agent, cb) { + return function runTest() { let prom = Promise.reject() for (let i = 0; i < NUM_PROMISES - 1; ++i) { prom = prom.then(function () {}) } - prom - .catch(function () {}) - .then(() => { - if (typeof cb === 'function') { - return cb() - } - return cb || true - }) + return prom.catch(function () {}) } }, function promiseConstructor(Promise) { - return function runTest(agent, cb) { + return function runTest() { + const promises = [] for (let i = 0; i < NUM_PROMISES; ++i) { /* eslint-disable no-new */ - new Promise(function (res) { - res() - }) - } - if (typeof cb === 'function') { - return cb() + promises.push( + new Promise(function (res) { + res() + }) + ) } - return cb || true + return Promise.all(promises) } }, function promiseReturningPromise(Promise) { - return function runTest(agent, cb) { + return function runTest() { const promises = [] for (let i = 0; i < NUM_PROMISES / 2; ++i) { promises.push( @@ -112,17 +90,12 @@ const tests = [ }) ) } - Promise.all(promises).then(() => { - if (typeof cb === 'function') { - return cb() - } - return cb || true - }) + return Promise.all(promises) } }, function thenReturningPromise(Promise) { - return function runTest(agent, cb) { + return function runTest() { let prom = Promise.resolve() for (let i = 0; i < NUM_PROMISES / 2; ++i) { prom = prom.then(function () { @@ -131,24 +104,15 @@ const tests = [ }) }) } - prom.then(() => { - if (typeof cb === 'function') { - return cb() - } - return cb || true - }) + return prom } }, function promiseConstructorThrow(Promise) { - return function runTest(agent, cb) { - new Promise(function () { + return function runTest() { + return new Promise(function () { throw new Error('Whoops!') }).catch(() => {}) - if (typeof cb === 'function') { - return cb() - } - return cb || true } } ] diff --git a/test/lib/benchmark.js b/test/lib/benchmark.js index b8bfeeb615..8a3be41562 100644 --- a/test/lib/benchmark.js +++ b/test/lib/benchmark.js @@ -129,7 +129,7 @@ class Benchmark { } if (typeof test.initialize === 'function') { - test.initialize(agent) + await test.initialize(agent) } const samples = [] @@ -151,8 +151,7 @@ class Benchmark { class BenchmarkStats { constructor(samples, testName, sampleName) { if (samples.length < 1) { - console.log(`BenchmarkStats for ${testName} has no samples. SampleName: ${sampleName}`) - throw new Error('BenchmarkStats requires more than zero samples') + throw new Error(`BenchmarkStats for ${testName} has no samples. SampleName: ${sampleName}`) } let sortedSamples = samples.slice().sort((a, b) => a - b)