Skip to content

Commit 520dd77

Browse files
nzakasclaude
andauthored
perf: Implement fast paths in critical areas (#21210)
* chore: Improve CLI perf * fix: revert lazy-getter exports in lib/api.js, drop unused export The lib/api.js lazy-getter rewrite broke Node's CJS-to-ESM named-export interop: cjs-module-lexer cannot statically detect getter-based exports, so `import { RuleTester } from "eslint"` (and ESLint/SourceCode/Linter) threw a SyntaxError at runtime for ESM consumers. This was caught by the ecosystem plugin tests (@eslint/css, @eslint/json, @eslint/markdown, eslint-plugin-unicorn) and the "Are the types wrong?" check, none of which are exercised by this repo's CJS-only unit test suite. Reverting to eager top-level requires restores the Loading benchmark to baseline, but that's the correct tradeoff over breaking every ESM consumer of the package. Also drop the `attributeError` export from source-code-visitor.js: it's only used internally within that module now, and knip correctly flagged it as an unused export. * perf: lazily construct the shared ajv instance in Config ajv pulls in ~40 modules that are only needed once a rule's schema actually needs validating. Constructing it lazily on first use (instead of eagerly at module load time) recovers most of the Loading benchmark improvement lost when lib/api.js reverted to eager requires, without touching the public export surface: this is purely internal to lib/config/config.js. * fix: address review feedback on CLI perf changes Correctness fixes from PR review, each with a regression test that fails without the corresponding fix: - stylish: the `stringLength()` fast path only checked for the 7-bit ESC introducer, but `util.stripVTControlCharacters()` also strips sequences introduced by the 8-bit CSI character (U+009B). A message containing only CSI sequences had its control bytes counted as visible characters, misaligning table columns. Check both introducers. - linter/source-code-visitor: the direct-listener fast path tags the listener function object with its rule ID, but two rules can register the same function object (for example, a shared visitor returned from a helper). The second registration overwrote the first rule's tag, so errors thrown from the first rule were attributed to the second. Fall back to the wrapper when a function is already tagged for a different rule. - source-code-visitor: `attributeError()` preserved a pre-existing `ruleId` on the thrown error, but the wrapper it replaced always overwrote it with the rule whose listener threw. Assign unconditionally to keep error attribution unchanged. Also make `ajv` lazy in RuleTester. `lib/api.js` eagerly requires RuleTester, so the eager `ajv` construction there pulled ~40 modules into the dependency graph of every consumer of the `eslint` package. Together with the existing lazy `ajv` in Config (both are required, since api.js -> linter -> config is a separate path to it), this cuts the `require("eslint")` graph from 177 modules to 131. * test: add unit tests for SourceCodeVisitor error attribution The `ruleId` overwrite behavior change was caught in review rather than by a failing test, because `callSync()` had no direct coverage of error propagation at all -- the attribution contract lives in this module but was only exercised indirectly through `Linter`. Adds unit tests covering the contract directly: an error from an untagged function propagates without a `ruleId`, an error from a tagged function is attributed to that rule, an existing `ruleId` is overwritten by the rule that actually threw, attribution works on both the single-argument fast path and the multi-argument path, and functions registered after a thrower are not called. The overwrite tests fail against the previous preserve-if-unset behavior. * fix: use a WeakMap for listener rule IDs, address review feedback Replaces the symbol tag on listener functions with a module-scoped `WeakMap`. Tagging wrote a property onto a function object owned by the rule, which the rule's author has no reason to expect; a `WeakMap` keeps that association external. The hot path is unaffected either way, since the mapping is only consulted when a listener throws. This also lets the `Object.isExtensible()` guard go away: a `WeakMap` can key a frozen function, so frozen listeners now take the fast path instead of falling back to a wrapper. Added tests covering a frozen listener and a non-function listener value, which are the two remaining conditions on that branch. Guards the `ruleId` assignment against non-object thrown values. Rules can throw primitives, and assigning to one throws a strict-mode `TypeError` that replaces whatever the rule actually threw. Note this only fixes the assignment in this module -- `SourceCodeTraverser` and `_verifyWithFlatConfigArray` decorate thrown values the same way and still mask primitives, which is pre-existing behavior left alone here. Moves the lazy `ajv` state and helper in `Config` and `RuleTester` out from between the `require` calls and into the "Private Members" and "Helpers" sections. * perf: route all rule lookups in Config through the cache `#normalizeRulesConfig()` and `validateRulesConfig()` called `getRuleFromConfig()` directly, bypassing the cache added for `getRuleDefinition()`. Since both run over every configured rule, each rule was looked up three times -- repeating the rule ID parsing and the plugin proxy traps each time. Routing them through `getRuleDefinition()` leaves `getRuleFromConfig()` with a single caller, so the cache is the only lookup path. With all core rules enabled, `getRuleFromConfig()` executions during a lint drop from 876 to 292 (one per rule instead of three). * perf: don't materialize non-array traversal steps Addresses review feedback on #21210. `traverseSync()` converted a non-array `steps` iterable with `Array.from()` so it could iterate by index. That is slower than just iterating the iterable, and it fully materializes a lazy one. Measured over 50k steps on Node v24.18.1: an array iterator went from 0.817 ms to 2.198 ms and a generator from 2.353 ms to 3.550 ms. This is not hypothetical -- `@eslint/markdown` returns `steps.values()` from `traverse()`, so every markdown file paid for a full copy of the steps array. Arrays are still iterated by index; everything else now uses `for...of`. The step body moved into a local function so both loops can share it, which keeps most of the win on the array path (0.262 ms vs 0.165 ms inlined, against 0.879 ms for `for...of`). An interleaved A/B lint of `tests/performance/jshint.js` with all core rules shows no measurable change (3.385 s vs 3.340 s, within noise). `createFastMatcher()` also drops three checks that couldn't affect it: * `left.subject` / `right.subject` / `field.subject` -- `esquery` only reads `subject` in its `sibling` and `adjacent` matchers and in `esquery.query()`, none of which are reachable here, so a subject indicator has no effect on `esquery.matches()` for a `child` selector. These made the fast path bail on selectors it handles identically. * the `compound[field, wildcard]` branch -- unreachable, because `*` is a legal identifier character in `esquery`'s grammar, so `.field*` parses as a field named `field*`. An exhaustive check of every 4-token selector over a representative alphabet parsed 10,282 selectors and produced no such compound. Adds tests asserting that the fast matcher agrees with `esquery.matches()` across these selector shapes, and that non-array steps are consumed one at a time rather than all at once. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C4gazgTwRgBSJKkYRQdhW3 * Add comment about string optimization --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 9ecfdc5 commit 520dd77

16 files changed

Lines changed: 990 additions & 119 deletions

File tree

‎lib/cli-engine/formatters/stylish.js‎

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,25 @@ function getStyleText(color) {
2828
return (_, text) => text;
2929
}
3030

31+
/**
32+
* Computes the visible width of a string, avoiding the cost of stripping
33+
* VT control characters when the string doesn't contain any.
34+
*
35+
* VT sequences can be introduced either by the 7-bit `ESC` character or by
36+
* the 8-bit `CSI` character, both of which `util.stripVTControlCharacters()`
37+
* removes, so both must be checked before taking the fast path.
38+
* @todo This optimization is not needed in Node.js v24+ because it's baked
39+
* into util.stripVTControlCharacters(). When we update our supported
40+
* Node.js versions we can remove this optimization.
41+
* @param {string} str The string to measure.
42+
* @returns {number} The number of visible characters.
43+
*/
44+
function stringLength(str) {
45+
return str.includes("\u001b") || str.includes("\u009b")
46+
? util.stripVTControlCharacters(str).length
47+
: str.length;
48+
}
49+
3150
/**
3251
* Given a word and a count, append an s if count is not one.
3352
* @param {string} word A word in its singular form.
@@ -77,20 +96,33 @@ module.exports = function (results, data) {
7796
messageType = styleText("yellow", "warning");
7897
}
7998
99+
/*
100+
* Strip a trailing period unless it's preceded by a space.
101+
* This is equivalent to `.replace(/([^ ])\.$/u, "$1")` but
102+
* avoids running a regex on every message.
103+
*/
104+
let messageText = message.message;
105+
106+
if (
107+
messageText.length > 1 &&
108+
messageText.endsWith(".") &&
109+
messageText.at(-2) !== " "
110+
) {
111+
messageText = messageText.slice(0, -1);
112+
}
113+
80114
return [
81115
"",
82116
String(message.line || 0),
83117
String(message.column || 0),
84118
messageType,
85-
message.message.replace(/([^ ])\.$/u, "$1"),
119+
messageText,
86120
message.ruleId ? styleText("dim", message.ruleId) : "",
87121
];
88122
}),
89123
{
90124
align: ["", "r", "l"],
91-
stringLength(str) {
92-
return util.stripVTControlCharacters(str).length;
93-
},
125+
stringLength,
94126
},
95127
)
96128
.split("\n")

‎lib/config/config.js‎

Lines changed: 40 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,6 @@
1212
const { deepMergeArrays } = require("../shared/deep-merge-arrays");
1313
const { flatConfigSchema, hasMethod } = require("./flat-config-schema");
1414
const { ObjectSchema } = require("@eslint/config-array");
15-
const ajvImport = require("../shared/ajv");
16-
const ajv = ajvImport();
1715
const ruleReplacements = require("../../conf/replacements.json");
1816

1917
//-----------------------------------------------------------------------------
@@ -52,10 +50,31 @@ const severities = new Map([
5250
*/
5351
const validators = new WeakMap();
5452

53+
/**
54+
* The shared ajv instance, constructed on first use.
55+
*
56+
* `ajv` pulls in a large dependency graph (~40 modules) that's only needed
57+
* once a rule's options actually need schema validation, so it's constructed
58+
* lazily instead of eagerly at module load time.
59+
* @type {import("ajv").Ajv|undefined}
60+
*/
61+
let ajv;
62+
5563
//-----------------------------------------------------------------------------
5664
// Helpers
5765
//-----------------------------------------------------------------------------
5866

67+
/**
68+
* Gets the shared ajv instance, constructing it on first use.
69+
* @returns {import("ajv").Ajv} The ajv instance.
70+
*/
71+
function getAjv() {
72+
if (!ajv) {
73+
ajv = require("../shared/ajv")();
74+
}
75+
return ajv;
76+
}
77+
5978
/**
6079
* Throws a helpful error when a rule cannot be found.
6180
* @param {Object} ruleId The rule identifier.
@@ -413,7 +432,7 @@ function getOrCreateValidator(rule, ruleId) {
413432
const schema = getRuleOptionsSchema(rule);
414433

415434
if (schema) {
416-
validators.set(rule, ajv.compile(schema));
435+
validators.set(rule, getAjv().compile(schema));
417436
}
418437
} catch (err) {
419438
throw new InvalidRuleOptionsSchemaError(ruleId, err);
@@ -443,6 +462,14 @@ class Config {
443462
*/
444463
#processorName;
445464

465+
/**
466+
* Cache of rule definitions by rule ID. Rule lookups involve string
467+
* parsing and multiple property accesses, so the results are cached
468+
* because they're requested repeatedly for the same rule IDs.
469+
* @type {Map<string, RuleDefinition|undefined>}
470+
*/
471+
#ruleDefinitions = new Map();
472+
446473
/**
447474
* Creates a new instance.
448475
* @param {Object} config The configuration object.
@@ -596,7 +623,14 @@ class Config {
596623
* @returns {RuleDefinition|undefined} The rule definition from the plugin, or `undefined` if the rule is not found.
597624
*/
598625
getRuleDefinition(ruleId) {
599-
return getRuleFromConfig(ruleId, this);
626+
if (this.#ruleDefinitions.has(ruleId)) {
627+
return this.#ruleDefinitions.get(ruleId);
628+
}
629+
630+
const rule = getRuleFromConfig(ruleId, this);
631+
632+
this.#ruleDefinitions.set(ruleId, rule);
633+
return rule;
600634
}
601635

602636
/**
@@ -615,7 +649,7 @@ class Config {
615649
// normalize severity
616650
ruleConfig[0] = severities.get(ruleConfig[0]);
617651

618-
const rule = getRuleFromConfig(ruleId, this);
652+
const rule = this.getRuleDefinition(ruleId);
619653

620654
// apply meta.defaultOptions
621655
const slicedOptions = ruleConfig.slice(1);
@@ -685,7 +719,7 @@ class Config {
685719
continue;
686720
}
687721

688-
const rule = getRuleFromConfig(ruleId, this);
722+
const rule = this.getRuleDefinition(ruleId);
689723

690724
if (!rule) {
691725
throwRuleNotFoundError(parseRuleId(ruleId), this);

‎lib/linter/apply-disable-directives.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,14 @@ function collectUsedEnableDirectives(directives) {
309309
* of problems (including suppressed ones) and unused eslint-disable directives
310310
*/
311311
function applyDirectives(options) {
312+
// Fast path: without directives there's nothing to suppress or report.
313+
if (options.directives.length === 0) {
314+
return {
315+
problems: options.problems.slice(),
316+
unusedDirectives: [],
317+
};
318+
}
319+
312320
const problems = [];
313321
const usedDisableDirectives = new Set();
314322
const { sourceCode } = options;

‎lib/linter/esquery.js‎

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,23 @@ class ESQueryParsedSelector {
6464
*/
6565
identifierCount;
6666

67+
/**
68+
* Whether this selector is guaranteed to match any node it is tested
69+
* against after node-type-based filtering, making an `esquery.matches()`
70+
* call unnecessary. This is `true` only for selectors that consist solely
71+
* of node type identifiers, wildcards, or unions thereof.
72+
* @type {boolean}
73+
*/
74+
alwaysMatches;
75+
76+
/**
77+
* A specialized matcher function that is equivalent to calling
78+
* `esquery.matches()` for this selector but faster, or `null` if no
79+
* specialized matcher is available for this selector.
80+
* @type {((node: Object, ancestry: Object[], options: ESQueryOptions) => boolean)|null}
81+
*/
82+
fastMatch;
83+
6784
/**
6885
* Creates a new parsed selector.
6986
* @param {string} source The raw selector string that was parsed
@@ -72,6 +89,8 @@ class ESQueryParsedSelector {
7289
* @param {string[]|null} nodeTypes The node types that could possibly trigger this selector, or `null` if all node types could trigger it
7390
* @param {number} attributeCount The number of class, pseudo-class, and attribute queries in this selector
7491
* @param {number} identifierCount The number of identifier queries in this selector
92+
* @param {boolean} alwaysMatches Whether this selector always matches nodes it is tested against after node-type-based filtering
93+
* @param {Function|null} fastMatch A specialized matcher function for this selector, or `null` if not available
7594
*/
7695
constructor(
7796
source,
@@ -80,13 +99,17 @@ class ESQueryParsedSelector {
8099
nodeTypes,
81100
attributeCount,
82101
identifierCount,
102+
alwaysMatches,
103+
fastMatch,
83104
) {
84105
this.source = source;
85106
this.isExit = isExit;
86107
this.root = root;
87108
this.nodeTypes = nodeTypes;
88109
this.attributeCount = attributeCount;
89110
this.identifierCount = identifierCount;
111+
this.alwaysMatches = alwaysMatches;
112+
this.fastMatch = fastMatch;
90113
}
91114

92115
/**
@@ -230,6 +253,104 @@ function analyzeParsedSelector(parsedSelector) {
230253
};
231254
}
232255

256+
/**
257+
* Determines whether a parsed selector consists solely of node type
258+
* identifiers, wildcards, or unions thereof. Such selectors are guaranteed to
259+
* match any node they are tested against after node-type-based filtering, so
260+
* calling `esquery.matches()` for them is unnecessary.
261+
* @param {ESQuerySelector} selector The parsed selector to check.
262+
* @returns {boolean} `true` if the selector always matches nodes it is tested against after node-type-based filtering.
263+
*/
264+
function isAlwaysMatchingSelector(selector) {
265+
switch (selector.type) {
266+
case "identifier":
267+
case "wildcard":
268+
return true;
269+
270+
case "matches":
271+
return selector.selectors.every(isAlwaysMatchingSelector);
272+
273+
default:
274+
return false;
275+
}
276+
}
277+
278+
/**
279+
* Creates a specialized matcher function for selectors of the shape
280+
* `ParentType > .field` or `ParentType > *.field`, which are common in core
281+
* rules but can't be bucketed by node type. The matcher replicates `esquery`'s
282+
* matching behavior for these selectors: the parent (`ancestry[0]`) must have
283+
* the given node type (case-insensitively, as `esquery` matches identifiers
284+
* case-insensitively), and the node must be the value of the parent's given
285+
* field (or an element of it, if the field value is an array).
286+
* @param {ESQuerySelector} root The parsed selector to create a matcher for.
287+
* @returns {Function|null} A matcher function, or `null` if the selector doesn't have the expected shape.
288+
*/
289+
function createFastMatcher(root) {
290+
if (root.type !== "child") {
291+
return null;
292+
}
293+
294+
const { left, right } = root;
295+
296+
/*
297+
* Subject indicators (`!`) are intentionally not checked here. `esquery`
298+
* only consults `subject` in its `sibling` and `adjacent` matchers (and in
299+
* `esquery.query()`, which ESLint doesn't use), so for the `child`
300+
* selectors handled here a subject indicator has no effect on
301+
* `esquery.matches()`.
302+
*/
303+
if (left.type !== "identifier") {
304+
return null;
305+
}
306+
307+
let field = null;
308+
309+
if (right.type === "field") {
310+
field = right;
311+
} else if (right.type === "compound" && right.selectors.length === 2) {
312+
const [first, second] = right.selectors;
313+
314+
if (first.type === "wildcard" && second.type === "field") {
315+
field = second;
316+
}
317+
}
318+
319+
/*
320+
* Fields with a dotted name match against `ancestry[path.length - 1]`
321+
* rather than the immediate parent, so they aren't handled here.
322+
*/
323+
if (!field || field.name.includes(".")) {
324+
return null;
325+
}
326+
327+
const parentType = left.value;
328+
const parentTypeLowercase = parentType.toLowerCase();
329+
const fieldName = field.name;
330+
331+
return (node, ancestry, options) => {
332+
const parent = ancestry[0];
333+
334+
if (!parent) {
335+
return false;
336+
}
337+
338+
const parentTypeActual = parent[options?.nodeTypeKey || "type"];
339+
340+
if (
341+
parentTypeActual !== parentType &&
342+
(typeof parentTypeActual !== "string" ||
343+
parentTypeActual.toLowerCase() !== parentTypeLowercase)
344+
) {
345+
return false;
346+
}
347+
348+
const value = parent[fieldName];
349+
350+
return value === node || (Array.isArray(value) && value.includes(node));
351+
};
352+
}
353+
233354
/**
234355
* Tries to parse a simple selector string, such as a single identifier or wildcard.
235356
* This saves time by avoiding the overhead of esquery parsing for simple cases.
@@ -303,6 +424,8 @@ function parse(source) {
303424
nodeTypes,
304425
attributeCount,
305426
identifierCount,
427+
isAlwaysMatchingSelector(parsedSelector),
428+
createFastMatcher(parsedSelector),
306429
);
307430

308431
selectorCache.set(source, result);

0 commit comments

Comments
 (0)