perf(linter): improve noImportCycles performance
#7454
Conversation
🦋 Changeset detectedLatest commit: 46a6eaa The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds a changeset to bump the patch version of @biomejs/biome and note a performance improvement for noImportCycles. In the linter rule implementation, switches internal path handling to biome_resolver::ResolvedPath, updates stack element types accordingly, and changes the Rule state and find_cycle return types from boxed slices/strings to Vec. Adjusts path collection and traversal logic to use the new path representation, and imports ResolvedPath. Suggested labels
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing Touches
🧪 Generate unit tests
Comment |
|
|
||
| if let Some(next_module_info) = ctx.module_info_for_path(path) { | ||
| stack.push((path.as_str().into(), module_info)); | ||
| stack.push((resolved_path.clone(), module_info)); |
There was a problem hiding this comment.
I think this one was the main culprit, because path.as_str().into() would do an allocation to store as a Box<str>, but resolved_path wraps an Arc, so cloning it is cheap.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.changeset/cynical-cyclops-cycles.md (1)
5-7: Add a docs link to the rule.When referencing a rule in a changeset, include a link to its documentation page. Keeps release notes navigable.
Suggested tweak:
-Greatly improved performance of `noImportCycles` by eliminating allocations. +Greatly improved performance of [`noImportCycles`](<link-to-rule-docs>) by eliminating allocations.crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs (2)
158-160: Prefer a boxed slice for rule state to avoid Vec capacity overhead.
State = Vec<String>grows for collection then carries unused capacity into the diagnostic. ABox<[String]>trims that, and matches prevailing style for payloads passed to diagnostics.Apply:
- type State = Vec<String>; + type State = Box<[String]>; @@ -) -> Option<Vec<String>> { +) -> Option<Box<[String]>> { @@ - let paths = Some(start_path.to_string()) - .into_iter() - .chain( - stack - .iter() - .filter_map(|(path, _)| path.as_path()) - .map(ToString::to_string), - ) - .chain(Some(path.to_string())) - .collect(); + let paths: Box<[String]> = std::iter::once(start_path.to_string()) + .chain( + stack + .iter() + .filter_map(|(path, _)| path.as_path()) + .map(|p| p.to_string()), + ) + .chain(std::iter::once(path.to_string())) + .collect(); return Some(paths);Also applies to: 231-267
256-265: Minor: avoidToString::to_stringand Option::into_iter ceremony.Slightly cleaner and just as fast to use
std::iter::onceand a closure.If you keep
Vec<String>:- let paths = Some(start_path.to_string()) - .into_iter() + let paths = std::iter::once(start_path.to_string()) .chain( stack .iter() .filter_map(|(path, _)| path.as_path()) - .map(ToString::to_string), + .map(|p| p.to_string()), ) - .chain(Some(path.to_string())) + .chain(std::iter::once(path.to_string())) .collect();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/cynical-cyclops-cycles.md(1 hunks)crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
.changeset/*.md: In changeset files, only use #### or ##### headers; avoid other header levels
Changeset descriptions should use past tense for what you did (e.g., "Added...")
Describe current Biome behavior in present tense within changesets (e.g., "Biome now supports...")
For bug fixes in changesets, start with a link to the issue (e.g., "Fixed #1234: ...")
When referencing rules or assists in changesets, include links to their documentation pages
Include a minimal code block in the changeset when applicable to demonstrate the change
End every sentence in the changeset description with a period
Files:
.changeset/cynical-cyclops-cycles.md
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format all Rust source files before committing (just f)
Files:
crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : In declare_lint_rule! metadata, set version: "next"
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/lint/nursery/**/*.rs : Place all new rules in the nursery group under biome_js_analyze/lib/src/lint/nursery
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs
📚 Learning: 2025-08-11T11:50:12.090Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:50:12.090Z
Learning: Applies to crates/biome_js_type_info/src/**/*.rs : Represent links between types using TypeReference (not Arc) to avoid cross-module retention and recursive structures; store type data in linear vectors
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs
📚 Learning: 2025-08-11T11:53:15.299Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_service/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:53:15.299Z
Learning: Use the `noImportCycles` rule to observe project layout and module graph state during debugging
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs
📚 Learning: 2025-08-11T11:50:12.090Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:50:12.090Z
Learning: Applies to crates/biome_js_type_info/{src,biome_module_graph/src}/**/*.rs : When pattern-matching on ResolvedTypeData via as_raw_data(), ensure any nested TypeReferences are subsequently resolved using the correct ResolverId; never use the raw data with a resolver without applying the right ResolverId to avoid panics
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs
📚 Learning: 2025-08-11T11:50:12.090Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:50:12.090Z
Learning: Applies to crates/biome_js_type_info/{src,resolver,biome_module_graph/src}/**/*.rs : Implement and use type resolution via the TypeResolver trait; resolvers own TypeStore vectors and provide fast by-id and hashed lookups
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : Prefer Box<[_]> over Vec<_> for Signals when emitting multiple diagnostics
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : Avoid deep indentation and unwrap/expect in CST navigation; prefer ?, map, filter, and_then
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : Avoid unnecessary string allocations in rules; compare against &str or TokenText instead of calling to_string()
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs
📚 Learning: 2025-08-11T11:50:12.090Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:50:12.090Z
Learning: Applies to crates/biome_js_type_info/biome_module_graph/src/js_module_info/scoped_resolver.rs : Full inference resolves TypeReference::Import across modules into TypeReference::Resolved using the module graph; do not cache full-inference results
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs
📚 Learning: 2025-09-05T09:13:58.901Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_rule_options/lib/**/*.rs : Prefer Box<[Box<str>]> over Vec<String> for lists of strings in options for memory efficiency
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs
📚 Learning: 2025-08-11T11:50:12.090Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:50:12.090Z
Learning: Applies to crates/biome_js_type_info/biome_module_graph/src/js_module_info/collector.rs : Thin (module-level) inference may resolve qualifiers to local declarations or globals (TypeReference::Resolved), mark imported bindings as TypeReference::Import, and set unresolved to TypeReference::Unknown; it must not look beyond the current module
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: Documentation
- GitHub Check: Test Node.js API
- GitHub Check: Bench (biome_package)
- GitHub Check: Bench (biome_html_formatter)
- GitHub Check: Check Dependencies
- GitHub Check: Bench (biome_json_formatter)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Bench (biome_graphql_formatter)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_css_formatter)
- GitHub Check: Bench (biome_json_analyze)
- GitHub Check: Bench (biome_graphql_parser)
- GitHub Check: Bench (biome_html_parser)
- GitHub Check: Bench (biome_css_analyze)
- GitHub Check: Bench (biome_css_parser)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_module_graph)
- GitHub Check: Bench (biome_json_parser)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: autofix
🔇 Additional comments (1)
crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs (1)
234-273: Nice DFS without recursion; good use of ResolvedPath to cut transient strings.The iterative traversal plus a
seenset looks solid and avoids the usual stack blow-ups. UsingResolvedPathon the stack pays off here.
CodSpeed Performance ReportMerging #7454 will not alter performanceComparing Summary
Footnotes |
Summary
Greatly improved performance of
noImportCyclesby eliminating allocations.In one repository, the total runtime of Biome with only
noImportCyclesenabled went from ~23s down to ~4s.Test Plan
Manually tested on the
unleashrepository.Docs
N/A