Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions crates/biome_formatter/src/format_element.rs
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,10 @@ impl BestFittingElement {
&self.variants
}

pub fn variants_mut(&mut self) -> &mut [Box<[FormatElement]>] {
&mut self.variants
}

/// Returns the least expanded variant
pub fn most_flat(&self) -> &[FormatElement] {
self.variants.first().expect(
Expand Down
100 changes: 43 additions & 57 deletions crates/biome_formatter/src/format_element/document.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,66 +145,52 @@ impl Document {
pub fn as_elements(&self) -> &[FormatElement] {
&self.elements
}
}

pub trait DocumentVisitor {
/// Visit an element and optionally return a replacement
fn visit_element(&mut self, element: &FormatElement) -> Option<FormatElement>;
}

/// Applies a visitor to transform elements in the document
pub struct ElementTransformer;

impl ElementTransformer {
/// Visits a mutable [Document] and replaces its internal elements.
pub fn transform_document<V: DocumentVisitor>(document: &mut Document, visitor: &mut V) {
let elements = std::mem::take(&mut document.elements);
document.elements = Self::transform_elements(elements, visitor);
/// Transforms the document by visiting every element, optionally replacing
/// them.
///
/// Accepts a `visitor` that will be called to visit each element, and which
/// may optionally return a replacement.
///
/// Elements that contain nested elements, such as [FormatElement::Interned]
/// and [FormatElement::BestFitting], have the visitor called on their
/// nested elements, but not on the elements themselves.
pub(crate) fn transform(
&mut self,
mut visitor: impl FnMut(&FormatElement) -> Option<FormatElement>,
) {
transform_elements(&mut self.elements, &mut visitor);
}
}

/// Iterates over each element of the document and map each element to a new element. The new element is
/// optionally crated using [DocumentVisitor::visit_element]. If no element is returned, the original element is kept.
///
/// Nested data structures such as [FormatElement::Interned] and [FormatElement::BestFitting] use recursion and call
/// [Self::transform_elements] again.
fn transform_elements<V: DocumentVisitor>(
elements: Vec<FormatElement>,
visitor: &mut V,
) -> Vec<FormatElement> {
elements
.into_iter()
.map(|element| {
// Transform nested elements first
let transformed_element = match element {
FormatElement::Interned(interned) => {
let nested_elements = interned.deref().to_vec();
let transformed_nested = Self::transform_elements(nested_elements, visitor);
FormatElement::Interned(Interned::new(transformed_nested))
}
FormatElement::BestFitting(best_fitting) => {
let variants: Vec<Box<[FormatElement]>> = best_fitting
.variants()
.iter()
.map(|variant| {
Self::transform_elements(variant.to_vec(), visitor)
.into_boxed_slice()
})
.collect();
// SAFETY: Safe because the number of variants is the same after the transformation
unsafe {
FormatElement::BestFitting(BestFittingElement::from_vec_unchecked(
variants,
))
}
}
other => other,
};
// Then apply a visitor to the element itself
visitor
.visit_element(&transformed_element)
.unwrap_or(transformed_element)
})
.collect()
/// 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;
}
}
}
}
}

Comment on lines +166 to 196
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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 mutable Vec and re‑intern when a child actually changes; otherwise keep the original Interned (preserves Rc pointer identity and avoids extra allocs).
  • Optionally add a transform variant 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Expand Down
33 changes: 8 additions & 25 deletions crates/biome_formatter/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,8 @@ use std::fmt::{Debug, Display};
use crate::builders::syntax_token_cow_slice;
use crate::comments::{CommentStyle, Comments, SourceComment};
pub use crate::diagnostics::{ActualStart, FormatError, InvalidDocumentError, PrintError};
use crate::format_element::document::{Document, ElementTransformer};
use crate::format_element::document::Document;
use crate::format_element::{Interned, LineMode};
use crate::prelude::document::DocumentVisitor;
#[cfg(debug_assertions)]
use crate::printed_tokens::PrintedTokens;
use crate::printer::{Printer, PrinterOptions};
Expand Down Expand Up @@ -892,41 +891,25 @@ impl<Context> Formatted<Context> {

/// Visits each embedded element and replaces it with elements contained inside the [Document]
/// emitted by `fn_format_embedded`
pub fn format_embedded<F>(&mut self, fn_format_embedded: F)
pub fn format_embedded<F>(&mut self, mut fn_format_embedded: F)
where
F: FnMut(TextRange) -> Option<Document>,
{
let document = &mut self.document;

struct EmbeddedVisitor<F> {
fn_format_embedded: F,
}

impl<F> DocumentVisitor for EmbeddedVisitor<F>
where
F: FnMut(TextRange) -> Option<Document>,
{
fn visit_element(&mut self, element: &FormatElement) -> Option<FormatElement> {
self.document
.transform(move |element: &FormatElement| -> Option<FormatElement> {
match element {
FormatElement::Tag(Tag::StartEmbedded(range)) => {
(self.fn_format_embedded)(*range).map(|document| {
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,
}
}
}

ElementTransformer::transform_document(
document,
&mut EmbeddedVisitor { fn_format_embedded },
);
});
}

/// Returns the formatted document.
Expand Down
Loading