Skip to content

Commit 8175dc3

Browse files
GroophyLifeforGroophyLifefor
authored
tools: add lint rule for unread benchmark n
If a benchmark declares `n` in its configs, main() must read it. #59872 renamed a config key from `num` to `n` but left `{ num: n }` in main(), so `n` resolved to undefined and the benchmark silently reported near-zero results until #62084 fixed it. The rule also found `async_hooks/async-resource-vs-destroy.js`, which has declared `n: [1e6]` since it was added but never read it. Remove the unused key. Refs: #59872 Refs: #62084 Signed-off-by: Murat Kirazkaya <groophylifefor@gmail.com> Assisted-by: Claude Code PR-URL: #66807 Refs: nodejs/performance#187 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
1 parent bb3f917 commit 8175dc3

4 files changed

Lines changed: 241 additions & 1 deletion

File tree

‎benchmark/async_hooks/async-resource-vs-destroy.js‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ const bench = common.createBenchmark(main, {
1919
path: '/',
2020
connections: 500,
2121
duration: 5,
22-
n: [1e6],
2322
});
2423

2524
function buildCurrentResource(getServe) {

‎benchmark/eslint.config_partial.mjs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ export default [
99
},
1010
},
1111
rules: {
12+
'node-core/benchmark-n-config': 'error',
1213
'prefer-arrow-callback': 'error',
1314
},
1415
},

‎test/parallel/test-eslint-benchmark-n-config.js‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if ((!common.hasCrypto) || (!common.hasIntl)) {
5+
common.skip('ESLint tests require crypto and Intl');
6+
}
7+
8+
common.skipIfEslintMissing();
9+
10+
const RuleTester = require('../../tools/eslint/node_modules/eslint').RuleTester;
11+
const rule = require('../../tools/eslint-rules/benchmark-n-config');
12+
13+
const message = '`n` is configured but not read by the benchmark main ' +
14+
'function (destructure `{ n }` or read `conf.n`)';
15+
16+
new RuleTester().run('benchmark-n-config', rule, {
17+
valid: [
18+
`
19+
const bench = common.createBenchmark(main, { len: [1], n: [100] });
20+
function main({ len, n }) { bench.end(n); }
21+
`,
22+
`
23+
const bench = common.createBenchmark(main, { n: [100] });
24+
function main(conf) { bench.end(conf.n); }
25+
`,
26+
`
27+
const bench = common.createBenchmark(main, { n: [100] });
28+
function main(conf) { const { n } = conf; bench.end(n); }
29+
`,
30+
`
31+
const bench = common.createBenchmark(({ n }) => bench.end(n), { n: [1] });
32+
`,
33+
`
34+
const configs = { n: [100] };
35+
const bench = common.createBenchmark(main, configs);
36+
function main({ n }) { bench.end(n); }
37+
`,
38+
// Configs without `n` (e.g. duration-driven benchmarks) are not checked.
39+
`
40+
const bench = common.createBenchmark(main, { duration: [5] });
41+
function main({ duration }) {}
42+
`,
43+
// Non-static configs are skipped.
44+
`
45+
const bench = common.createBenchmark(main, { ...base, n: [100] });
46+
function main({ len }) {}
47+
`,
48+
],
49+
invalid: [
50+
// nodejs/node#59872: the config key was renamed to `n`, but main() kept
51+
// reading `num`.
52+
{
53+
code: `
54+
const bench = common.createBenchmark(main, { len: [1], n: [100] });
55+
function main({ len, num: n }) { bench.end(n); }
56+
`,
57+
errors: [{ message }],
58+
},
59+
{
60+
code: `
61+
const bench = common.createBenchmark(main, { n: [1e6], duration: [5] });
62+
function main({ duration }) {}
63+
`,
64+
errors: [{ message }],
65+
},
66+
{
67+
code: `
68+
const bench = common.createBenchmark(main, { n: [100] });
69+
function main(conf) { bench.end(conf.num); }
70+
`,
71+
errors: [{ message }],
72+
},
73+
{
74+
code: `
75+
const configs = { n: [100] };
76+
const bench = common.createBenchmark(main, configs);
77+
function main({ count }) { bench.end(count); }
78+
`,
79+
errors: [{ message }],
80+
},
81+
],
82+
});

‎tools/eslint-rules/benchmark-n-config.js‎

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
'use strict';
2+
3+
// Requires benchmarks that declare an `n` iteration count in their configs
4+
// to actually read that value in the benchmark main function.
5+
//
6+
// This prevents regressions like nodejs/node#59872, where a config key was
7+
// renamed to `n` while main() kept reading the old key, silently producing
8+
// a near-zero result.
9+
10+
function walk(node, visit) {
11+
if (node === null || typeof node !== 'object') return;
12+
if (Array.isArray(node)) {
13+
for (const item of node) walk(item, visit);
14+
return;
15+
}
16+
if (typeof node.type !== 'string') return;
17+
visit(node);
18+
for (const key of Object.keys(node)) {
19+
if (key === 'parent' || key === 'loc' || key === 'range') continue;
20+
walk(node[key], visit);
21+
}
22+
}
23+
24+
// Collects the property names read by an ObjectPattern: `{ num: n }` reads
25+
// `num`, `{ n }` reads `n`.
26+
function objectPatternNames(pattern, out) {
27+
for (const prop of pattern.properties) {
28+
if (prop.type !== 'Property' || prop.computed) continue;
29+
if (prop.key.type === 'Identifier') out.add(prop.key.name);
30+
else if (prop.key.type === 'Literal' && typeof prop.key.value === 'string') {
31+
out.add(prop.key.value);
32+
}
33+
if (prop.value && prop.value.type === 'ObjectPattern') {
34+
objectPatternNames(prop.value, out);
35+
}
36+
}
37+
}
38+
39+
// Returns false when the object literal is not fully static.
40+
function objectExpressionKeys(node, out) {
41+
for (const prop of node.properties) {
42+
if (prop.type === 'SpreadElement') return false;
43+
if (prop.type !== 'Property' || prop.computed) continue;
44+
if (prop.key.type === 'Identifier') out.add(prop.key.name);
45+
else if (prop.key.type === 'Literal' && typeof prop.key.value === 'string') {
46+
out.add(prop.key.value);
47+
}
48+
}
49+
return true;
50+
}
51+
52+
module.exports = {
53+
meta: {
54+
type: 'problem',
55+
docs: {
56+
description: 'require benchmark main to read `n` when configs declare it',
57+
},
58+
schema: [],
59+
},
60+
61+
create(context) {
62+
const { sourceCode } = context;
63+
64+
let state = null;
65+
66+
return {
67+
CallExpression(node) {
68+
if (state !== null) return;
69+
const callee = node.callee;
70+
if (callee.type !== 'MemberExpression' || callee.computed) return;
71+
if (callee.property.name !== 'createBenchmark') return;
72+
if (node.arguments.length < 2) return;
73+
state = {
74+
mainArg: node.arguments[0],
75+
configArg: node.arguments[1],
76+
};
77+
},
78+
79+
'Program:exit'() {
80+
if (state === null) return;
81+
const { mainArg, configArg } = state;
82+
83+
// Resolve config keys.
84+
const configKeys = new Set();
85+
if (configArg.type === 'ObjectExpression') {
86+
if (!objectExpressionKeys(configArg, configKeys)) return;
87+
} else if (configArg.type === 'Identifier') {
88+
let resolved = false;
89+
walk(sourceCode.ast, (n) => {
90+
if (resolved) return;
91+
if (n.type === 'VariableDeclarator' && n.id.type === 'Identifier' &&
92+
n.id.name === configArg.name && n.init &&
93+
n.init.type === 'ObjectExpression') {
94+
resolved = objectExpressionKeys(n.init, configKeys);
95+
}
96+
});
97+
if (!resolved) return;
98+
} else {
99+
return;
100+
}
101+
if (!configKeys.has('n')) return;
102+
103+
// Resolve the main function node.
104+
let fn = null;
105+
if (mainArg.type === 'Identifier') {
106+
walk(sourceCode.ast, (n) => {
107+
if (fn !== null) return;
108+
if (n.type === 'FunctionDeclaration' && n.id && n.id.name === mainArg.name) {
109+
fn = n;
110+
}
111+
if (fn === null && n.type === 'VariableDeclarator' && n.id.type === 'Identifier' &&
112+
n.id.name === mainArg.name && n.init &&
113+
(n.init.type === 'FunctionExpression' ||
114+
n.init.type === 'ArrowFunctionExpression')) {
115+
fn = n.init;
116+
}
117+
});
118+
} else if (mainArg.type === 'FunctionExpression' ||
119+
mainArg.type === 'ArrowFunctionExpression') {
120+
fn = mainArg;
121+
}
122+
if (fn === null) return;
123+
124+
// Collect every name through which `n` could be read in main().
125+
const paramNames = new Set();
126+
const readNames = new Set();
127+
for (const param of fn.params) {
128+
if (param.type === 'ObjectPattern') {
129+
objectPatternNames(param, readNames);
130+
} else if (param.type === 'Identifier') {
131+
paramNames.add(param.name);
132+
}
133+
}
134+
walk(fn.body, (n) => {
135+
// `const { a, b } = conf;` inside the body.
136+
if (n.type === 'VariableDeclarator' && n.id.type === 'ObjectPattern' &&
137+
n.init && n.init.type === 'Identifier' && paramNames.has(n.init.name)) {
138+
objectPatternNames(n.id, readNames);
139+
}
140+
// `conf.n` member access.
141+
if (n.type === 'MemberExpression' && !n.computed &&
142+
n.object.type === 'Identifier' && paramNames.has(n.object.name) &&
143+
n.property.type === 'Identifier') {
144+
readNames.add(n.property.name);
145+
}
146+
});
147+
148+
if (!readNames.has('n')) {
149+
context.report({
150+
node: fn.id ?? fn,
151+
message: '`n` is configured but not read by the benchmark main ' +
152+
'function (destructure `{ n }` or read `conf.n`)',
153+
});
154+
}
155+
},
156+
};
157+
},
158+
};

0 commit comments

Comments
 (0)