diff --git a/benchmark/async_hooks/async-resource-vs-destroy.js b/benchmark/async_hooks/async-resource-vs-destroy.js index 3bd9dd56934e..50085ddcf8a7 100644 --- a/benchmark/async_hooks/async-resource-vs-destroy.js +++ b/benchmark/async_hooks/async-resource-vs-destroy.js @@ -19,7 +19,6 @@ const bench = common.createBenchmark(main, { path: '/', connections: 500, duration: 5, - n: [1e6], }); function buildCurrentResource(getServe) { diff --git a/benchmark/eslint.config_partial.mjs b/benchmark/eslint.config_partial.mjs index 7f10460f7197..47ef14673908 100644 --- a/benchmark/eslint.config_partial.mjs +++ b/benchmark/eslint.config_partial.mjs @@ -9,6 +9,7 @@ export default [ }, }, rules: { + 'node-core/benchmark-n-config': 'error', 'prefer-arrow-callback': 'error', }, }, diff --git a/test/parallel/test-eslint-benchmark-n-config.js b/test/parallel/test-eslint-benchmark-n-config.js new file mode 100644 index 000000000000..98049592fc8e --- /dev/null +++ b/test/parallel/test-eslint-benchmark-n-config.js @@ -0,0 +1,82 @@ +'use strict'; + +const common = require('../common'); +if ((!common.hasCrypto) || (!common.hasIntl)) { + common.skip('ESLint tests require crypto and Intl'); +} + +common.skipIfEslintMissing(); + +const RuleTester = require('../../tools/eslint/node_modules/eslint').RuleTester; +const rule = require('../../tools/eslint-rules/benchmark-n-config'); + +const message = '`n` is configured but not read by the benchmark main ' + + 'function (destructure `{ n }` or read `conf.n`)'; + +new RuleTester().run('benchmark-n-config', rule, { + valid: [ + ` + const bench = common.createBenchmark(main, { len: [1], n: [100] }); + function main({ len, n }) { bench.end(n); } + `, + ` + const bench = common.createBenchmark(main, { n: [100] }); + function main(conf) { bench.end(conf.n); } + `, + ` + const bench = common.createBenchmark(main, { n: [100] }); + function main(conf) { const { n } = conf; bench.end(n); } + `, + ` + const bench = common.createBenchmark(({ n }) => bench.end(n), { n: [1] }); + `, + ` + const configs = { n: [100] }; + const bench = common.createBenchmark(main, configs); + function main({ n }) { bench.end(n); } + `, + // Configs without `n` (e.g. duration-driven benchmarks) are not checked. + ` + const bench = common.createBenchmark(main, { duration: [5] }); + function main({ duration }) {} + `, + // Non-static configs are skipped. + ` + const bench = common.createBenchmark(main, { ...base, n: [100] }); + function main({ len }) {} + `, + ], + invalid: [ + // nodejs/node#59872: the config key was renamed to `n`, but main() kept + // reading `num`. + { + code: ` + const bench = common.createBenchmark(main, { len: [1], n: [100] }); + function main({ len, num: n }) { bench.end(n); } + `, + errors: [{ message }], + }, + { + code: ` + const bench = common.createBenchmark(main, { n: [1e6], duration: [5] }); + function main({ duration }) {} + `, + errors: [{ message }], + }, + { + code: ` + const bench = common.createBenchmark(main, { n: [100] }); + function main(conf) { bench.end(conf.num); } + `, + errors: [{ message }], + }, + { + code: ` + const configs = { n: [100] }; + const bench = common.createBenchmark(main, configs); + function main({ count }) { bench.end(count); } + `, + errors: [{ message }], + }, + ], +}); diff --git a/tools/eslint-rules/benchmark-n-config.js b/tools/eslint-rules/benchmark-n-config.js new file mode 100644 index 000000000000..81a4fb30d96e --- /dev/null +++ b/tools/eslint-rules/benchmark-n-config.js @@ -0,0 +1,158 @@ +'use strict'; + +// Requires benchmarks that declare an `n` iteration count in their configs +// to actually read that value in the benchmark main function. +// +// This prevents regressions like nodejs/node#59872, where a config key was +// renamed to `n` while main() kept reading the old key, silently producing +// a near-zero result. + +function walk(node, visit) { + if (node === null || typeof node !== 'object') return; + if (Array.isArray(node)) { + for (const item of node) walk(item, visit); + return; + } + if (typeof node.type !== 'string') return; + visit(node); + for (const key of Object.keys(node)) { + if (key === 'parent' || key === 'loc' || key === 'range') continue; + walk(node[key], visit); + } +} + +// Collects the property names read by an ObjectPattern: `{ num: n }` reads +// `num`, `{ n }` reads `n`. +function objectPatternNames(pattern, out) { + for (const prop of pattern.properties) { + if (prop.type !== 'Property' || prop.computed) continue; + if (prop.key.type === 'Identifier') out.add(prop.key.name); + else if (prop.key.type === 'Literal' && typeof prop.key.value === 'string') { + out.add(prop.key.value); + } + if (prop.value && prop.value.type === 'ObjectPattern') { + objectPatternNames(prop.value, out); + } + } +} + +// Returns false when the object literal is not fully static. +function objectExpressionKeys(node, out) { + for (const prop of node.properties) { + if (prop.type === 'SpreadElement') return false; + if (prop.type !== 'Property' || prop.computed) continue; + if (prop.key.type === 'Identifier') out.add(prop.key.name); + else if (prop.key.type === 'Literal' && typeof prop.key.value === 'string') { + out.add(prop.key.value); + } + } + return true; +} + +module.exports = { + meta: { + type: 'problem', + docs: { + description: 'require benchmark main to read `n` when configs declare it', + }, + schema: [], + }, + + create(context) { + const { sourceCode } = context; + + let state = null; + + return { + CallExpression(node) { + if (state !== null) return; + const callee = node.callee; + if (callee.type !== 'MemberExpression' || callee.computed) return; + if (callee.property.name !== 'createBenchmark') return; + if (node.arguments.length < 2) return; + state = { + mainArg: node.arguments[0], + configArg: node.arguments[1], + }; + }, + + 'Program:exit'() { + if (state === null) return; + const { mainArg, configArg } = state; + + // Resolve config keys. + const configKeys = new Set(); + if (configArg.type === 'ObjectExpression') { + if (!objectExpressionKeys(configArg, configKeys)) return; + } else if (configArg.type === 'Identifier') { + let resolved = false; + walk(sourceCode.ast, (n) => { + if (resolved) return; + if (n.type === 'VariableDeclarator' && n.id.type === 'Identifier' && + n.id.name === configArg.name && n.init && + n.init.type === 'ObjectExpression') { + resolved = objectExpressionKeys(n.init, configKeys); + } + }); + if (!resolved) return; + } else { + return; + } + if (!configKeys.has('n')) return; + + // Resolve the main function node. + let fn = null; + if (mainArg.type === 'Identifier') { + walk(sourceCode.ast, (n) => { + if (fn !== null) return; + if (n.type === 'FunctionDeclaration' && n.id && n.id.name === mainArg.name) { + fn = n; + } + if (fn === null && n.type === 'VariableDeclarator' && n.id.type === 'Identifier' && + n.id.name === mainArg.name && n.init && + (n.init.type === 'FunctionExpression' || + n.init.type === 'ArrowFunctionExpression')) { + fn = n.init; + } + }); + } else if (mainArg.type === 'FunctionExpression' || + mainArg.type === 'ArrowFunctionExpression') { + fn = mainArg; + } + if (fn === null) return; + + // Collect every name through which `n` could be read in main(). + const paramNames = new Set(); + const readNames = new Set(); + for (const param of fn.params) { + if (param.type === 'ObjectPattern') { + objectPatternNames(param, readNames); + } else if (param.type === 'Identifier') { + paramNames.add(param.name); + } + } + walk(fn.body, (n) => { + // `const { a, b } = conf;` inside the body. + if (n.type === 'VariableDeclarator' && n.id.type === 'ObjectPattern' && + n.init && n.init.type === 'Identifier' && paramNames.has(n.init.name)) { + objectPatternNames(n.id, readNames); + } + // `conf.n` member access. + if (n.type === 'MemberExpression' && !n.computed && + n.object.type === 'Identifier' && paramNames.has(n.object.name) && + n.property.type === 'Identifier') { + readNames.add(n.property.name); + } + }); + + if (!readNames.has('n')) { + context.report({ + node: fn.id ?? fn, + message: '`n` is configured but not read by the benchmark main ' + + 'function (destructure `{ n }` or read `conf.n`)', + }); + } + }, + }; + }, +};