From 8387bac51f55cb2dbb957ba6c307ba6cd50c05e2 Mon Sep 17 00:00:00 2001 From: camchenry <1514176+camchenry@users.noreply.github.com> Date: Tue, 22 Oct 2024 02:24:41 +0000 Subject: [PATCH 1/2] perf(linter): apply small file optimization, up to 30% faster (#6600) Theory: iterating over the rules three times has slightly worse cache locality, because the prior iterations have pushed `rule` out of the cache by the time we iterate over it again. By iterating over each rule only once, we improve cache performance (hopefully). We also don't need to collect rules to a Vec, so it saves some CPU/memory there too. In practice: the behavior here actually depends on the number of AST nodes that are in the program. If the number of nodes is large, then it's better to iterate over the nodes only once and iterate the rules multiple times. But if the number of nodes is small, then it's better to iterate over nodes multiple times and only iterate over the rules once. See this comment for more context: https://github.com/oxc-project/oxc/pull/6600#issuecomment-2427837715, as well as the comment inside the PR: https://github.com/oxc-project/oxc/pull/6600/files#diff-207225884c5e031ffd802bb99e4fbacbd8364b1343a1cec5485bf50f29186300R131-R143. In practice, this can make linting a file 1-45% faster, depending on the size of the file, number of AST nodes, number of files, CPU cache size, etc. To accommodate large and small files better, we have an explicit threshold of 200,000 AST nodes, which is an arbitrary number picked based on some benchmarks on my laptop. For large files, the linter behavior doesn't change. For small files, we switch to iterating over nodes in the inner loop and iterating over rules once in the outer loop. --- crates/oxc_linter/src/lib.rs | 76 ++++++++++++++++++++++++++++-------- 1 file changed, 60 insertions(+), 16 deletions(-) diff --git a/crates/oxc_linter/src/lib.rs b/crates/oxc_linter/src/lib.rs index 69665affba2074..e8a590e605edca 100644 --- a/crates/oxc_linter/src/lib.rs +++ b/crates/oxc_linter/src/lib.rs @@ -121,30 +121,74 @@ impl Linter { .rules .iter() .filter(|rule| rule.should_run(&ctx_host)) - .map(|rule| (rule, Rc::clone(&ctx_host).spawn(rule))) - .collect::>(); - - for (rule, ctx) in &rules { - rule.run_once(ctx); - } + .map(|rule| (rule, Rc::clone(&ctx_host).spawn(rule))); let semantic = ctx_host.semantic(); - for symbol in semantic.symbols().symbol_ids() { + + let should_run_on_jest_node = + self.options.plugins.has_test() && ctx_host.frameworks().is_test(); + + // IMPORTANT: We have two branches here for performance reasons: + // + // 1) Branch where we iterate over each node, then each rule + // 2) Branch where we iterate over each rule, then each node + // + // When the number of nodes is relatively small, most of them can fit + // in the cache and we can save iterating over the rules multiple times. + // But for large files, the number of nodes can be so large that it + // starts to not fit into the cache and pushes out other data, like the rules. + // So we end up thrashing the cache with each rule iteration. In this case, + // it's better to put rules in the inner loop, as the rules data is smaller + // and is more likely to fit in the cache. + // + // The threshold here is chosen to balance between performance improvement + // from not iterating over rules multiple times, but also ensuring that we + // don't thrash the cache too much. Feel free to tweak based on benchmarking. + // + // See https://github.com/oxc-project/oxc/pull/6600 for more context. + if semantic.stats().nodes > 200_000 { + // Collect rules into a Vec so that we can iterate over the rules multiple times + let rules = rules.collect::>(); + for (rule, ctx) in &rules { - rule.run_on_symbol(symbol, ctx); + rule.run_once(ctx); } - } - for node in semantic.nodes() { - for (rule, ctx) in &rules { - rule.run(node, ctx); + for symbol in semantic.symbols().symbol_ids() { + for (rule, ctx) in &rules { + rule.run_on_symbol(symbol, ctx); + } } - } - if ctx_host.frameworks().is_test() && self.options.plugins.has_test() { - for jest_node in iter_possible_jest_call_node(semantic) { + for node in semantic.nodes() { for (rule, ctx) in &rules { - rule.run_on_jest_node(&jest_node, ctx); + rule.run(node, ctx); + } + } + + if should_run_on_jest_node { + for jest_node in iter_possible_jest_call_node(semantic) { + for (rule, ctx) in &rules { + rule.run_on_jest_node(&jest_node, ctx); + } + } + } + } else { + for (rule, ref ctx) in rules { + rule.run_once(ctx); + + for symbol in semantic.symbols().symbol_ids() { + rule.run_on_symbol(symbol, ctx); + } + + for node in semantic.nodes() { + rule.run(node, ctx); + } + + if should_run_on_jest_node { + for jest_node in iter_possible_jest_call_node(semantic) { + rule.run_on_jest_node(&jest_node, ctx); + } } } } From dbe19722834d9433d2fba1d3489aeee97b8fadc4 Mon Sep 17 00:00:00 2001 From: Boshen <1430279+Boshen@users.noreply.github.com> Date: Tue, 22 Oct 2024 02:35:28 +0000 Subject: [PATCH 2/2] feat(linter): import/no-cycle should turn on ignore_types by default (#6761) closes #6759 --- crates/oxc_linter/src/rules/import/no_cycle.rs | 12 ++++++------ crates/oxc_linter/src/snapshots/no_cycle.snap | 9 --------- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/crates/oxc_linter/src/rules/import/no_cycle.rs b/crates/oxc_linter/src/rules/import/no_cycle.rs index 1a8bd8d40cb2dd..0487ee062736b2 100644 --- a/crates/oxc_linter/src/rules/import/no_cycle.rs +++ b/crates/oxc_linter/src/rules/import/no_cycle.rs @@ -37,7 +37,7 @@ impl Default for NoCycle { fn default() -> Self { Self { max_depth: u32::MAX, - ignore_types: false, + ignore_types: true, ignore_external: false, allow_unsafe_dynamic_cyclic_dependency: false, } @@ -93,24 +93,25 @@ declare_oxc_lint!( impl Rule for NoCycle { fn from_configuration(value: serde_json::Value) -> Self { let obj = value.get(0); + let default = NoCycle::default(); Self { max_depth: obj .and_then(|v| v.get("maxDepth")) .and_then(serde_json::Value::as_number) .and_then(serde_json::Number::as_u64) - .map_or(u32::MAX, |n| n as u32), + .map_or(default.max_depth, |n| n as u32), ignore_types: obj .and_then(|v| v.get("ignoreTypes")) .and_then(serde_json::Value::as_bool) - .unwrap_or_default(), + .unwrap_or(default.ignore_types), ignore_external: obj .and_then(|v| v.get("ignoreExternal")) .and_then(serde_json::Value::as_bool) - .unwrap_or_default(), + .unwrap_or(default.ignore_external), allow_unsafe_dynamic_cyclic_dependency: obj .and_then(|v| v.get("allowUnsafeDynamicCyclicDependency")) .and_then(serde_json::Value::as_bool) - .unwrap_or_default(), + .unwrap_or(default.allow_unsafe_dynamic_cyclic_dependency), } } @@ -356,7 +357,6 @@ fn test() { // (r#"import { bar } from "./flow-types-depth-one""#, None), (r#"import { foo } from "./intermediate-ignore""#, None), (r#"import { foo } from "./ignore""#, None), - (r#"import { foo } from "./typescript/ts-types-only-importing-type";"#, None), ( r#"import { foo } from "./typescript/ts-types-some-type-imports";"#, Some(json!([{"ignoreTypes":true}])), diff --git a/crates/oxc_linter/src/snapshots/no_cycle.snap b/crates/oxc_linter/src/snapshots/no_cycle.snap index c115e4558904d3..69d69e175fdc31 100644 --- a/crates/oxc_linter/src/snapshots/no_cycle.snap +++ b/crates/oxc_linter/src/snapshots/no_cycle.snap @@ -262,15 +262,6 @@ source: crates/oxc_linter/src/tester.rs -> ./ignore - fixtures/import/cycles/ignore/index.js -> ../depth-zero - fixtures/import/cycles/depth-zero.js - ⚠ eslint-plugin-import(no-cycle): Dependency cycle detected - ╭─[cycles/depth-zero.js:1:21] - 1 │ import { foo } from "./typescript/ts-types-only-importing-type"; - · ─────────────────────────────────────────── - ╰──── - help: These paths form a cycle: - -> ./typescript/ts-types-only-importing-type - fixtures/import/cycles/typescript/ts-types-only-importing-type.ts - -> ../depth-zero - fixtures/import/cycles/depth-zero.js - ⚠ eslint-plugin-import(no-cycle): Dependency cycle detected ╭─[cycles/depth-zero.js:1:21] 1 │ import { foo } from "./typescript/ts-types-some-type-imports";