Repository navigation
Conversation
Move the extents of the non-root aligned subtrees (those rooted at a `vertical-align: top | bottom` box) out of `LineBoxMetrics` into a `SubtreeHistory` on `BreakerState`, so that saving a line-breaking opportunity no longer clones a `SmallVec`. The history is strictly append-only: growing a subtree pushes its new extents. A saved opportunity records the history's length and reverting truncates to it, the same way `contributed` is already handled.
|
I'll fix CI failures and address comments from users with write access that start with 'Devin'.
|
|
Reviewed — LGTM. This is a nice simplification of linebender#855: no Correctness checks I did by hand (all hold):
Locally verified on this branch: Two minor things:
|
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
| let is_current = |(i, s): &(usize, &SubtreeExtents)| { | ||
| let newer = &self.entries[i + 1..]; | ||
| newer.iter().all(|n| n.root != s.root) | ||
| }; |
There was a problem hiding this comment.
If you want to drop the quadratic wart, a reverse scan with a seen-set makes this O(entries):
fn iter(&self) -> impl Iterator<Item = SubtreeExtents> + '_ {
let mut seen: Vec<u16> = Vec::new();
self.entries
.iter()
.rev()
.filter(move |s| {
if seen.contains(&s.root) {
return false;
}
seen.push(s.root);
true
})
.copied()
}Yields entries in last-push order rather than this version's first-push order, but every consumer is order-independent (max accumulation in finish_line, find by root in aligned_subtree_offset). It also slightly favors the common case where a subtree's newest entry is near the end. A SmallVec<[u16; 4]> (or a bitset, since roots are u16) avoids the alloc; Vec is fine given this runs twice per line and the existing entries Vec already allocates.
Same goes for grow's get scan — O(history) per non-root add_box vs. the old position() over live subtrees only. Both bounded per line since clear() runs in reset_line, so either way is defensible.
Replace SubtreeHistory::iter, which was filtered twice per line, with SubtreeHistory::current, which walks the history once in reverse and collects the latest entry of each root into a SmallVec.
finish_line only takes maxima over the list, and the per-line offsets it pushes are looked up by root, so the order doesn't matter.
Growing a subtree only has to push a new entry if its current one was there at the last saved line-breaking opportunity. Otherwise overwrite it, so that the history doesn't fill up with obsolete entries when there are few opportunities (e.g. without wrapping). The history now tracks which entries are frozen itself, behind save/restore instead of len/truncate.
| if aligned_subtree == 0 { | ||
| self.root.add(baseline_offset, metrics); | ||
| } else { | ||
| let subtree = subtrees.grow(aligned_subtree, baseline_offset, metrics); | ||
| self.non_root_height = self.non_root_height.max(subtree.line_box.height()); |
There was a problem hiding this comment.
Metrics for the main (root) aligned subtree on a line get stored on LineBoxMetrics. Metrics for all other aligned subtrees (if any) go into the SubtreeHistory
| fn grow(&mut self, root: u16, baseline_offset: f32, metrics: BoxMetrics) -> SubtreeExtents { | ||
| let index = self.entries.iter().rposition(|s| s.root == root); | ||
| let old = index.map(|i| self.entries[i]); | ||
| let mut new = old.unwrap_or(SubtreeExtents::new(root)); | ||
| new.add(baseline_offset, metrics); | ||
| if old != Some(new) { | ||
| match index { | ||
| Some(i) if i >= self.frozen_count => self.entries[i] = new, | ||
| _ => self.entries.push(new), | ||
| } | ||
| } | ||
| new | ||
| } |
There was a problem hiding this comment.
If there is a non-frozen entry for the aligned subtree with root root, then modify it's extents in-place. If there is no entry or the entry is frozen, push a new entry.
CSS defines an aligned subtree for every inline box, recursively, but layout only ever deals with the ones that aren't part of a larger one: those rooted at the root span box or at a box with `vertical-align: top | bottom`. Name those "independent aligned subtrees" (the specifications have no term for them), say that this is what "aligned subtree" means elsewhere in the crate, and say explicitly which of them `SubtreeHistory` holds. Also fix the definition, which included every parent-relative descendant rather than stopping at `top`/`bottom` boxes.
Follow-up to the module doc change: comments that said "non-root", "top-level" or just "aligned subtree" where the distinction matters now use the terms defined in `style_metrics`. Comments and one test name only; no behaviour change.
The field, parameters and locals of this name hold the style index of the root of an independent aligned subtree, not a subtree. No behaviour change.
1 participant
LLM Contributions: Code and this description were generated by Devin.
Fork-internal PR for review. Upstream's policy requires the description to be in the author's own words, so this text needs rewriting before anything goes to linebender/parley.
Alternative implementation of linebender#855, written in response to the review feedback there that the subtree log was hard to follow. Same goal: remove the
SmallVec<[SubtreeExtents; 2]>fromLineBoxMetrics, which is cloned at every line-breaking opportunity. Based on upstreammainat 718dcb7 (base/upstream-main-718dcb7), which linebender#855 currently conflicts with.Design
LineBoxMetricsisCopy. It keepsroot: SubtreeExtentsand a cachednon_root_height.vertical-align: top | bottom) move to a newSubtreeHistoryonBreakerState.SubtreeHistoryis strictly append-only.growpushes the subtree's new extents rather than overwriting, and skips the push if nothing changed. The current extents of a subtree are the last entry with its root.PrevBoundaryStategainssubtrees_len;reset_totruncates to it. This is the same pattern as the existingcontributed/contributed_len.Differences from linebender#855
LineBoxMetrics::saved_subtreesand the three-way branch ingrow_subtreeare gone.same_asis replaced by a derivedPartialEqonSubtreeExtents/Extents.add_to_subtree/grow_subtreebecomeLineBoxMetrics::add_boxandSubtreeHistory::grow.Known wart
SubtreeHistory::iter(used twice per line infinish_line) still filters with "no newer entry has the same root", which is quadratic in the number of history entries on the line. That is the number of times atop/bottomsubtree grew on that line.Testing
LineBoxMetricsCopylinebender/parley#855 (lines_revert_restores_aligned_subtree_line_height,lines_aligned_subtrees_grow_across_breaks).subtree_extents_restored_at_break_opportunities, ported from MakeLineBoxMetricsCopylinebender/parley#855's unit test; it fails if the truncate inreset_tois removed.cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace --all-features. Thecargo hackfeature matrix and non-host targets were not run.vertical-align, inline boxes, finite max line height) gave line metrics and item positions identical to the base commit.Changelog: None (performance fix for an unreleased regression)
Link to Devin session: https://dioxus.staging.devinenterprise.com/sessions/ebf23f033e594293a023cb44a2c87744
Open in Devin Desktop: https://dioxus.staging.devinenterprise.com/desktop/session/ebf23f033e594293a023cb44a2c87744?variant=devin-insiders
Requested by: @nicoburns