perf: optimise embedded formatter #7567
Conversation
|
WalkthroughAdds a crate-private Suggested labels: A-Project Suggested reviewers:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)crates/biome_*/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.rs📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
🧠 Learnings (6)📚 Learning: 2025-08-11T11:48:27.774ZApplied to files:
📚 Learning: 2025-08-11T11:48:52.001ZApplied to files:
📚 Learning: 2025-08-11T11:48:52.001ZApplied to files:
📚 Learning: 2025-08-11T11:48:27.774ZApplied to files:
📚 Learning: 2025-08-11T11:48:27.774ZApplied to files:
📚 Learning: 2025-08-11T11:48:27.774ZApplied to files:
🧬 Code graph analysis (1)crates/biome_formatter/src/lib.rs (3)
⏰ 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)
🔇 Additional comments (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/biome_formatter/src/format_element.rs(1 hunks)crates/biome_formatter/src/format_element/document.rs(1 hunks)crates/biome_formatter/src/lib.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_formatter/src/format_element.rscrates/biome_formatter/src/format_element/document.rscrates/biome_formatter/src/lib.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format all Rust source files before committing (just f)
Files:
crates/biome_formatter/src/format_element.rscrates/biome_formatter/src/format_element/document.rscrates/biome_formatter/src/lib.rs
🧠 Learnings (6)
📚 Learning: 2025-08-11T11:48:52.001Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:48:52.001Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : Use the generic Format trait and FormatNode for AST nodes when implementing the formatter
Applied to files:
crates/biome_formatter/src/lib.rs
📚 Learning: 2025-08-11T11:48:27.774Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:48:27.774Z
Learning: Applies to crates/biome_formatter/biome_html_formatter/src/lib.rs : Place the plumbing traits and impls (AsFormat, IntoFormat, FormattedIterExt, and their Iterator adapters) in the biome_html_formatter crate’s lib.rs
Applied to files:
crates/biome_formatter/src/lib.rs
📚 Learning: 2025-08-11T11:48:52.001Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:48:52.001Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : Import the FormatNode trait and implement it for your Node
Applied to files:
crates/biome_formatter/src/lib.rs
📚 Learning: 2025-08-11T11:48:27.774Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:48:27.774Z
Learning: Applies to crates/biome_formatter/biome_html_formatter/src/comments.rs : Define HtmlComments alias and HtmlCommentStyle in comments.rs, and implement CommentStyle for the language
Applied to files:
crates/biome_formatter/src/lib.rs
📚 Learning: 2025-08-11T11:48:27.774Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:48:27.774Z
Learning: Applies to crates/biome_formatter/biome_html_formatter/src/context.rs : Define HtmlFormatContext in context.rs with fields comments: Rc<HtmlComments> and source_map: Option<TransformSourceMap>, and implement FormatContext and CstFormatContext
Applied to files:
crates/biome_formatter/src/lib.rs
📚 Learning: 2025-08-11T11:48:27.774Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:48:27.774Z
Learning: Applies to crates/biome_formatter/biome_html_formatter/src/lib.rs : Expose a documented public function format_node(options: HtmlFormatOptions, root: &HtmlSyntaxNode) -> FormatResult<Formatted<HtmlFormatContext>> delegating to biome_formatter::format_node
Applied to files:
crates/biome_formatter/src/lib.rs
🧬 Code graph analysis (2)
crates/biome_formatter/src/format_element/document.rs (2)
crates/biome_formatter/src/buffer.rs (6)
elements(41-41)elements(145-147)elements(230-232)elements(360-362)elements(415-417)elements(664-666)crates/biome_formatter/src/format_element.rs (1)
new(129-131)
crates/biome_formatter/src/lib.rs (1)
crates/biome_service/src/file_handlers/html.rs (1)
format_embedded(434-513)
⏰ 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). (22)
- GitHub Check: autofix
- GitHub Check: Documentation
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Check Dependencies
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Bench (biome_json_analyze)
- GitHub Check: Bench (biome_json_formatter)
- GitHub Check: Bench (biome_html_parser)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_json_parser)
- GitHub Check: Bench (biome_module_graph)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_css_formatter)
- GitHub Check: Bench (biome_graphql_formatter)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_html_formatter)
- GitHub Check: Bench (biome_css_parser)
- GitHub Check: Bench (biome_css_analyze)
- GitHub Check: Bench (biome_graphql_parser)
🔇 Additional comments (3)
crates/biome_formatter/src/lib.rs (2)
53-53: Import change looks goodCorrectly narrows the import to
Document.
894-913: Re-run expand propagation after mutating the document; sanity‑check EndEmbedded→Hard breakCall self.document.propagate_expand() after the transform so group expansion flags are recomputed. Replacing every EndEmbedded with Line::Hard can introduce extra blank lines—add tests and consider a context‑sensitive replacement (e.g. soft break when the preceding element already breaks).
pub fn format_embedded<F>(&mut self, mut fn_format_embedded: F) where F: FnMut(TextRange) -> Option<Document>, { self.document .transform(move |element: &FormatElement| -> Option<FormatElement> { match element { FormatElement::Tag(Tag::StartEmbedded(range)) => fn_format_embedded(*range) .map(|document| { FormatElement::Interned(Interned::new(document.into_elements())) }), FormatElement::Tag(Tag::EndEmbedded) => { // FIXME: this might not play well for all cases, so we need to figure out // a nicer way to replace the tag Some(FormatElement::Line(LineMode::Hard)) } _ => None, } }); + // New embedded content can force expansion; recompute flags. + self.document.propagate_expand(); }
- Add tests where embedded content sits inside/next to groups to verify no double blank lines and correct expansion.
- If hard breaks are too aggressive, use a context‑sensitive replacement (soft break when appropriate).
crates/biome_formatter/src/format_element/document.rs (1)
149-164: API shift to closure transform is a nice simplificationThe semantics (don’t visit container nodes
Interned/BestFittingthemselves) are clear and match the docstring.Please confirm this matches the old visitor’s behaviour in all call sites that relied on visiting those container nodes.
| /// Iterates over each of the given `elements` and optionally replaces each | ||
| /// element with a new one. | ||
| /// | ||
| /// Nested data structures such as [FormatElement::Interned] and | ||
| /// [FormatElement::BestFitting] use recursion and call [transform_elements()] | ||
| /// again. The visitor is *not* invoked on these elements. | ||
| fn transform_elements( | ||
| elements: &mut [FormatElement], | ||
| visitor: &mut impl FnMut(&FormatElement) -> Option<FormatElement>, | ||
| ) { | ||
| for element in elements { | ||
| match element { | ||
| FormatElement::Interned(interned) => { | ||
| let mut nested_elements = interned.deref().to_vec(); | ||
| transform_elements(&mut nested_elements, visitor); | ||
| *element = FormatElement::Interned(Interned::new(nested_elements)); | ||
| } | ||
| FormatElement::BestFitting(best_fitting) => { | ||
| for variant in best_fitting.variants_mut() { | ||
| transform_elements(variant, visitor); | ||
| } | ||
| } | ||
| _ => { | ||
| if let Some(replacement) = visitor(element) { | ||
| *element = replacement; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
❓ Verification inconclusive
Unconditional cloning of Interned loses sharing and may regress perf
Every Interned is cloned into a fresh vector and re‑interned, even when no child changes. This breaks pointer identity (used by caches/diagnostics) and can balloon allocations in documents with repeated interned content.
Recommendations:
- Copy‑on‑write for
Interned: only materialise a mutableVecand re‑intern when a child actually changes; otherwise keep the originalInterned(preserves Rc pointer identity and avoids extra allocs). - Optionally add a
transformvariant with a flag to skip descending into interned content when the visitor is known not to touch it (fast path for common transforms).
Can you benchmark this PR on large inputs with heavy best_fitting!/interning to ensure we didn’t trade one hotspot for another?
Avoid unconditional cloning of Interned — preserve sharing and reduce allocations
Transform currently dereferences every Interned into a Vec and always re‑interns it; this breaks pointer identity (used by caches/diagnostics) and causes extra allocations.
Location: crates/biome_formatter/src/format_element/document.rs:166-196
- Implement copy‑on‑write: materialise a Vec, run transform_elements, and only re‑intern/replace the element when the Vec actually changed (or track a change flag) so Rc pointer identity is preserved.
- Optionally add a fast‑path/flag to skip descending into Interned when the visitor will not modify interned content.
- Benchmark this PR on large inputs heavy on best_fitting!/interning to confirm no regressions.
There was a problem hiding this comment.
I agree we could potentially speed this up even more, but then we'd need to recursively track such a flag too. It felt too much trouble for now 😅
CodSpeed Performance ReportMerging #7567 will not alter performanceComparing Summary
|
arendjr
force-pushed
the
optimise-embedded-formatter
branch
from
c8256e1 to
982d741
Compare
1 participant
Summary
This optimises the transformation on documents to format embedded elements. Instead of letting the transformation take a vector, map each element, and collect new vectors, this simply iterates over and updates the elements in-place.
The code as a whole is simplified in the process. The
ElementTransformerstruct has been cleaned up, and its only public methodtransform_document()has been moved to become the non-staticDocument::transform(). TheDocumentVisitortrait is replaced with a single closure that gets passed toDocument::transform().transform_elements()has become a simple stand-alone function, and no longer contains anunsafeblock.Test Plan
Everything should stay green.
Docs
N/A