mirror of
https://github.com/oxc-project/oxc.git
synced 2026-11-04 10:29:58 +00:00
perf(linter/import/no-cycle): skip the graph walk for acyclic modules (#25003)
This is the change from #24963, but as its own PR to avoid dependency on the larger refactor PR it was targeted at. Benchmarks have been re-run to ensure that the info is correct, but I'm pretty confident in this change now. AI Disclosure: Generated with Claude Code, Opus 5. Reviewed and tested manually be me. ## What `run_once` walks the module graph once per direct import, and each walk explores the module's whole reachable subgraph before concluding "no cycle". On `vscode/src` that is 78,760 walks producing 1,566 diagnostics, so **98% of them are pure overhead**. A module is in a cycle **if and only if** some module reachable from it imports it back. So one traversal of that subgraph answers the question for every direct import at once. When it says no, skip all the walks. ## Why it can't change what's reported The prefilter only gates the walks — they still decide what is reported and reconstruct the cycle path for the diagnostic. It cannot skip a cycle a walk would have found: - it traverses the same edges under the same `should_traverse_module` filter; - it is unbounded where the walks are bounded by `max_depth`, so its reachable set is a strict superset; - it compares by resolved path, not `Arc` identity, matching what the walks report a cycle on — a file split into multiple sections has one record per section but one path. It runs only under the default unbounded `max_depth`, where the walks cover the whole subgraph anyway and the prefilter replaces work rather than adding to it. With a small `max_depth` the walks are shallow enough that an unbounded prefilter could cost more than it saves, so behaviour there is exactly as before. ## Measurements `vscode/src` — 7,368 files, 81,512 direct imports, 78,760 walks — with `import/no-cycle` as the only enabled rule (`number_of_rules: 1`). Paired interleaved runs with per-round order alternation, n=15, `--silent`, release builds with `--features allocator`. ### Wall clock | | mean | sd | | --------------------- | --------- | -------- | | `main` | 3217.3 ms | 169.3 ms | | this PR | 1178.3 ms | 56.1 ms | | **paired delta** | **−2038.9 ms** | 127.6 ms | t = +61.9, faster in **15/15** paired rounds. Subtracting the parse-only baseline, rule-attributable time goes **3110.5 ms → 1073.3 ms, 2.9× faster**. The parse-only control (same binaries, no rules enabled) is flat — 106.8 ms vs 105.0 ms, paired delta +1.8 ms, t = +0.49 — so the delta is attributable to the rule and not to binary layout or the harness. ### Allocations Counted with a global-allocator shim; rule-attributable = total minus a parse-only run on the same binary. | | allocations | bytes | | ------- | ----------- | -------- | | `main` | 331.0M | 22.52 GB | | this PR | 86.8M | 6.05 GB | | | **−73.8%** | **−73.1%** | Counts are stable to within 0.05% across repetitions, and the two parse-only baselines (separately built binaries) agree to 0.003%, which is what makes that subtraction meaningful. ## Correctness - 1,566 diagnostics on `main`, 1,566 on this branch, **identical** after sorting by `(filename, labels, message, code)`. - Full `oxc_linter` test suite passes (1,175 tests) with no snapshot changes; `cargo clippy -p oxc_linter --all-features --all-targets -D warnings` is clean. ### Cross-codebase parity Both binaries were also run over five TypeScript codebases, under the default options plus two variants chosen to stress what is most likely to break: `ignoreTypes: false` changes which edges `should_traverse_module` admits, so the prefilter has to apply the identical filter, and `maxDepth: 5` disables the prefilter entirely. | codebase | files | config | `main` | this PR | identical | | --- | --- | --- | --- | --- | --- | | vscode | 11,674 | default | 2,789 | 2,789 | ✅ | | actualbudget/actual | 1,789 | default | 188 | 188 | ✅ | | actualbudget/actual | 1,789 | `ignoreTypes: false` | 591 | 591 | ✅ | | actualbudget/actual | 1,789 | `maxDepth: 5` | 47 | 47 | ✅ | | storybook | 4,050 | default | 96 | 96 | ✅ | | storybook | 4,050 | `ignoreTypes: false` | 430 | 430 | ✅ | | storybook | 4,050 | `maxDepth: 5` | 40 | 40 | ✅ | | rolldown | 6,411 | default | 77 | 77 | ✅ | | rolldown | 6,411 | `ignoreTypes: false` | 239 | 239 | ✅ | | rolldown | 6,411 | `maxDepth: 5` | 73 | 73 | ✅ | | microsoft/TypeScript | 39,333 | default | 729 | 729 | ✅ | | microsoft/TypeScript | 39,333 | `ignoreTypes: false` | 729 | 729 | ✅ | | microsoft/TypeScript | 39,333 | `maxDepth: 5` | 242 | 242 | ✅ | Counts are `import(no-cycle)` diagnostics. The comparison is stricter than the counts: in all 13 runs the **entire** diagnostic array is element-wise identical after sorting by `(filename, labels, message, code)` — including the 30,713 parse errors the TypeScript repo's test fixtures produce — so no diagnostic changed span or cycle path either. These are full repository checkouts, which is why vscode is 11,674 files here rather than the 7,368 measured above.
This commit is contained in:
1 parent
ee344bd00b
commit
79ddb181ce
1 file changed
+57
@@ -9,6 +9,7 @@ use oxc_diagnostics::OxcDiagnostic;
|
||||
use oxc_macros::declare_oxc_lint;
|
||||
use oxc_span::Span;
|
||||
use oxc_str::CompactStr;
|
||||
use rustc_hash::FxHashSet;
|
||||
use schemars::JsonSchema;
|
||||
use serde::Deserialize;
|
||||
|
||||
@@ -146,6 +147,13 @@ impl Rule for NoCycle {
|
||||
fn run_once(&self, ctx: &LintContext<'_>) {
|
||||
let module_record = ctx.module_record();
|
||||
|
||||
// Gated on unbounded `max_depth` (the default), where the walks below explore the whole
|
||||
// reachable subgraph anyway so the prefilter replaces their work. Under a shallow depth cap
|
||||
// they are cheap and an unbounded prefilter could cost more than it saves.
|
||||
if self.max_depth == u32::MAX && !self.can_be_in_cycle(module_record) {
|
||||
return;
|
||||
}
|
||||
|
||||
let needle = &module_record.resolved_absolute_path;
|
||||
let mut direct_imports = module_record
|
||||
.loaded_modules()
|
||||
@@ -200,6 +208,55 @@ impl Rule for NoCycle {
|
||||
}
|
||||
|
||||
impl NoCycle {
|
||||
/// Is `root` in a cycle at all? True iff some module reachable from it imports it back — one
|
||||
/// traversal answering for every direct import at once, instead of one walk per import.
|
||||
///
|
||||
/// This only gates those walks, which still decide what is reported, so it must never miss a
|
||||
/// cycle they would find: it follows the same edges under the same filter, and is unbounded
|
||||
/// where they stop at `max_depth`.
|
||||
fn can_be_in_cycle(&self, root: &ModuleRecord) -> bool {
|
||||
// Pointer identity, to avoid cloning a `PathBuf` per node. `root` is never inserted: an
|
||||
// edge back to it is the answer, not a node to expand.
|
||||
let mut visited = FxHashSet::<usize>::default();
|
||||
let mut stack = Vec::<Arc<ModuleRecord>>::new();
|
||||
|
||||
if self.visit_dependencies(root, root, &mut visited, &mut stack) {
|
||||
return true;
|
||||
}
|
||||
while let Some(module_record) = stack.pop() {
|
||||
if self.visit_dependencies(&module_record, root, &mut visited, &mut stack) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
false
|
||||
}
|
||||
|
||||
/// Pushes `module_record`'s unvisited traversable dependencies onto `stack`; `true` if one of
|
||||
/// them is `root`, closing a cycle.
|
||||
fn visit_dependencies(
|
||||
&self,
|
||||
module_record: &ModuleRecord,
|
||||
root: &ModuleRecord,
|
||||
visited: &mut FxHashSet<usize>,
|
||||
stack: &mut Vec<Arc<ModuleRecord>>,
|
||||
) -> bool {
|
||||
for (specifier, weak_module_record) in module_record.loaded_modules().iter() {
|
||||
let loaded_module_record = weak_module_record.upgrade().unwrap();
|
||||
if !self.should_traverse_module(specifier, &loaded_module_record, module_record) {
|
||||
continue;
|
||||
}
|
||||
// By path, not pointer: that is what the walks report on, and one file can have
|
||||
// several records (one per section).
|
||||
if loaded_module_record.resolved_absolute_path == root.resolved_absolute_path {
|
||||
return true;
|
||||
}
|
||||
if visited.insert(Arc::as_ptr(&loaded_module_record) as usize) {
|
||||
stack.push(loaded_module_record);
|
||||
}
|
||||
}
|
||||
false
|
||||
}
|
||||
|
||||
fn should_traverse_module(
|
||||
&self,
|
||||
key: &CompactStr,
|
||||
|
||||
Reference in new issue
Block a user