perf(formatter): use flat storage for BestFitting variants#18085
perf(formatter): use flat storage for BestFitting variants#18085
Conversation
Port of astral-sh/ruff#7411 to oxc_formatter. This optimizes memory allocation for `BestFitting` elements by using a flat storage structure with marker tags instead of nested allocations. Key changes: - Add `StartBestFittingEntry`/`EndBestFittingEntry` tags to delimit variants - Change `BestFittingElement` from `&[&[FormatElement]]` to `&[FormatElement]` - Add `BestFittingVariantsIter` iterator to parse variants from flat structure - Update printer to handle the new structure Benefits: - Reduces memory allocations by avoiding per-variant heap allocations - All variant content stored in a single contiguous buffer Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR optimizes memory allocation for BestFitting elements by replacing nested slice allocations with a flat storage structure using delimiter tags. This is a performance optimization that reduces per-variant heap allocations while maintaining the same functionality.
Changes:
- Added
StartBestFittingEntryandEndBestFittingEntrytags to delimit variants in flat storage - Refactored
BestFittingElementto store variants in a single flat slice with delimiter tags instead of nested slices - Added
BestFittingVariantsIterto parse variants from the flat structure - Updated all consumers (printer, document display, builders, and call arguments) to work with the new flat structure
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/oxc_formatter/src/formatter/format_element/tag.rs | Added new StartBestFittingEntry and EndBestFittingEntry tags and BestFittingEntry tag kind |
| crates/oxc_formatter/src/formatter/format_element/mod.rs | Changed BestFittingElement storage from nested slices to flat slice with delimiter tags, added iterator |
| crates/oxc_formatter/src/formatter/builders.rs | Updated BestFitting::fmt to write all variants to single buffer with tags, removed ArenaVec usage |
| crates/oxc_formatter/src/formatter/printer/mod.rs | Added print_best_fitting_entry function, updated best fitting logic to use iterator and new tags |
| crates/oxc_formatter/src/formatter/format_element/document.rs | Updated display logic to handle new BestFittingEntry tags |
| crates/oxc_formatter/src/print/call_like_expression/arguments.rs | Updated direct BestFittingElement construction to use flat storage with new tags |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Some(most_expanded) = variants.last() { | ||
| queue.extend_back(most_expanded); | ||
| } |
There was a problem hiding this comment.
The use of variants.last() wrapped in if let Some(...) at lines 381-382 suggests uncertainty about whether variants is non-empty. However, the safety invariants guarantee at least 2 variants exist. If the invariants hold, this check is unnecessary and variants.last().unwrap() would be clearer. If there's concern about the invariants not holding, a more explicit assertion or unwrap with a meaningful message would be better than silently doing nothing when variants.last() returns None.
| if let Some(most_expanded) = variants.last() { | |
| queue.extend_back(most_expanded); | |
| } | |
| let most_expanded = variants | |
| .last() | |
| .expect("BestFittingElement should have at least one variant"); | |
| queue.extend_back(most_expanded); |
| // Find the StartBestFittingEntry tag | ||
| let start_index = self.elements.iter().position(|element| { | ||
| matches!(element, FormatElement::Tag(Tag::StartBestFittingEntry)) | ||
| })?; | ||
|
|
There was a problem hiding this comment.
The iterator searches for the first StartBestFittingEntry using position(), but based on the construction logic in arguments.rs and builders.rs, the flat buffer should always start with StartBestFittingEntry at position 0. Consider checking if the first element is StartBestFittingEntry and returning early if not, rather than searching through the slice. This would make the common case faster and catch malformed data earlier.
| // Find the StartBestFittingEntry tag | |
| let start_index = self.elements.iter().position(|element| { | |
| matches!(element, FormatElement::Tag(Tag::StartBestFittingEntry)) | |
| })?; | |
| // The flat buffer is expected to start with StartBestFittingEntry at position 0. | |
| // If it does not, treat the data as malformed and terminate the iteration. | |
| if !matches!( | |
| self.elements[0], | |
| FormatElement::Tag(Tag::StartBestFittingEntry) | |
| ) { | |
| // Clear remaining elements so subsequent calls also return None. | |
| self.elements = &[]; | |
| return None; | |
| } | |
| let start_index = 0; |
Merging this PR will not alter performance
Comparing Footnotes
|
Summary
Port of astral-sh/ruff#7411 to oxc_formatter.
This optimizes memory allocation for
BestFittingelements by using a flat storage structure with marker tags instead of nested allocations.Key Changes
New Tags (
tag.rs):StartBestFittingEntryandEndBestFittingEntryto theTagenumBestFittingEntrytoTagKindenumFlat Storage Structure (
format_element/mod.rs):BestFittingElementfrom storing&'a [&'a [FormatElement<'a>]](nested slices) to&'a [FormatElement<'a>](flat slice)StartBestFittingEntry/EndBestFittingEntrytagsBestFittingVariantsIteriterator to parse variants from the flat structureBuilder Update (
builders.rs):BestFitting::fmtto write all variants to a single buffer with entry tagsPrinter Updates (
printer/mod.rs):print_best_fitting_entryfunction to handle the new entry tagsprint_best_fittingto use the iterator and call the new entry printerDocument Display (
document.rs):StartBestFittingEntry/EndBestFittingEntrytagsDirect API Usage (
arguments.rs):BestFittingElementto use the new flat structureBenefits
Test plan
🤖 Generated with Claude Code