Conversation
nicoburns
marked this pull request as ready for review
September 28, 2026 20:33
staging-devin-ai-integration
Bot
force-pushed
the
devin/1790175038-lineboxmetrics-copy
branch
2 times, most recently
from
September 29, 2026 12:58
110f5d3 to
47877e6
Compare
LineState is cloned at every soft-break opportunity, and the SmallVec<[SubtreeExtents; 2]> inside LineBoxMetrics made that a non-trivial clone/drop plus a 128-byte move in reset_to/take(). The root subtree's extents now live inline as a plain Copy field. Non-root (vertical-align: top/bottom) subtrees are recorded in an append-only Vec<SubtreeExtents> owned by BreakerState next to the `contributed` buffer, and cleared with it in reset_line. Entries at indices below the most recent saved break opportunity (LineBoxMetrics::saved_subtrees) are never mutated: growing such a subtree pushes a new copy instead, so truncating the log in reset_to restores the exact extents at that opportunity. Later entries are updated in place, and nothing is pushed when the extents don't change. The current extents of a subtree are the last log entry for its root; earlier entries are stale and ignored when finishing a line. Plain prose never touches the log. Span boxes, text run boxes and inline boxes all grow a subtree through add_to_subtree, which takes their BoxMetrics. `append_atom_to_line` and `LineBoxMetrics::add_text` are #[inline(always)] so they fold back into break_remaining. Adds tests for rolling back a tall top-aligned span at a break and for two aligned subtrees whose extents grow across several words/lines. Break phase (250KB prose, 800/400px alternating, min of 5 interleaved rounds): plain 4.52 -> 3.46 ms, styled 5.69 -> 4.57 ms, styled+vertical-align 6.25 -> 4.90 ms; layout output unchanged.
Keep height-limit checks constant-time while preserving Copy and restoring cached heights with regular and emergency break checkpoints. Validated library and integration tests, 1,000 differential layouts, formatting, and std/no-std Clippy. The 8,192-box finite-height probe drops from 16.22 ms to 0.38 ms. LLM Contributions: Generated with Devin.
staging-devin-ai-integration
Bot
force-pushed
the
devin/1790175038-lineboxmetrics-copy
branch
from
September 29, 2026 13:14
47877e6 to
2cd1543
Compare
DJMcNab
reviewed
Oct 1, 2026
DJMcNab
left a comment
DJMcNab
left a comment
Member
There was a problem hiding this comment.
This PR is extremely hard to review. I don't know if there's a simpler model which exists, but the in-code explanations of this seem to obfuscate/spread about the details.
Comment on lines
+111
to
+113
| /// This is plain `Copy` data so that saving a line-breaking opportunity (which clones | ||
| /// [`LineState`]) is a memcpy; the growable per-line buffers live in [`BreakerState`]. | ||
| /// |
Member
There was a problem hiding this comment.
Is this comment really needed? I guess it's only internal docs, so maybe it's valuable...
Comment on lines
+131
to
+132
| /// Length of [`BreakerState::subtrees`] at the most recently saved line-breaking opportunity. | ||
| /// Entries below it may be part of that opportunity's state and are never modified. |
Member
There was a problem hiding this comment.
Below here is ambiguous - I don't know if it means before or after. I presume it has to be after?
| .add(baseline_offset, metrics.ascent, metrics.descent); | ||
| } | ||
|
|
||
| fn same_as(&self, other: &Self) -> bool { |
Member
There was a problem hiding this comment.
Hmm...
Seems like this should be a PartialEq
Comment on lines
+282
to
+291
| /// Grow the extents of the non-root subtree rooted at `root` in the log `subtrees` (see | ||
| /// [`BreakerState::subtrees`]). | ||
| #[inline] | ||
| fn grow_subtree( | ||
| &mut self, | ||
| subtrees: &mut Vec<SubtreeExtents>, | ||
| root: u16, | ||
| baseline_offset: f32, | ||
| metrics: BoxMetrics, | ||
| ) { |
Member
There was a problem hiding this comment.
Why is this function split out? It has a very confusing doc/semantics. I presumed that it would be a cold path for add_to_subtree, but it being marked as inline seems to undermine that.
Comment on lines
+234
to
242
| /// The current extents of each non-root subtree in the log `subtrees` (see | ||
| /// [`BreakerState::subtrees`]), in order of first appearance. | ||
| fn current_subtrees(subtrees: &[SubtreeExtents]) -> impl Iterator<Item = SubtreeExtents> + '_ { | ||
| subtrees | ||
| .iter() | ||
| .enumerate() | ||
| .filter(|(i, s)| subtrees.iter().rposition(|t| t.root == s.root) == Some(*i)) | ||
| .map(|(_, s)| *s) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
LLM Contributions: Generated with Fable 5.1 Low
Depends on:
contributedinBreakerState::reset_line#854Context
This is a follow-up to #766 (
vertical-align) that gets us back to performance parity with before that PR was merged. It was purposefully left as a follow-up to make things easier to review, but we probably need to land this or some alternative performance fix before we can release.Summary
Eliminate
SmallVec<[SubtreeExtents; 2]>onLineBoxMetrics, allowing it to implCopywhich is performance-critical because it is copied at every line breaking opportunity.New design
LineBoxMetricsloses the generalSmallVec<[SubtreeExtents; 2]>and gainsroot: SubtreeExtents. So it still stores metrics for the root subtree on the line but extra subtrees generated byvertical-align: top | bottommove out.BreakerStategainsVec<SubtreeExtents>for the extra subtrees generated byvertical-align: top | bottom. This is stored is per-line state that is processed infinish_lineand then reset for the next line. The length of the vector will be roughly the number of items on the line that belong to non-root subtrees.LineBoxMetricsalso gainssaved_subtrees: usizewhich records the length of theVec<SubtreeExtents>at the time that each line-breaking opportunity is encountered. This is used to revert to that state when taking the line-breaking opportunity.SubtreeExtentsentry was pushed, we can update the entry in place. However, we cannot update the extents of subtree across line-breaking opportunity (as we would have no way to revert in case the opportunity is taken). Sosaved_subtreesis also used to limit deduplication to only "uncommited" subtree records.Changelog: None (performance improvement for never-published regression)