diff --git a/docs/RUST.md b/docs/RUST.md index f0e3ab0..c968671 100644 --- a/docs/RUST.md +++ b/docs/RUST.md @@ -674,6 +674,34 @@ changed. `PrimitiveVec::set` and `Primitives::set_instance` compare before marking, and `arena_churn` prints both numbers so the gap cannot reopen unnoticed. +**A measurement is a mode, not a discarded draw (added later the same +day).** `Painter::draw_twice(child, first, |used| second)` became +`Painter::measure` + an ordinary draw, at Iris's request: her objection +was the shape it forced on the caller, since the arithmetic that picks +the real region had to happen inside a closure and anything it wanted to +keep came back out through a captured `&mut`. Two statements now say it +in the order it happens. + +`DrawMode::Measure` is that draw with everything it *writes* switched +off -- no arena slot, no mask, no move slot, nothing left in `active`, +nothing marked dirty -- so the real draw that follows is an ordinary one +and cannot be short-circuited by the measurement having "already drawn" +the widget at that region. A `debug_assert` at the end of `draw_inner` +catches a `Painter` method that forgets to check the mode, because the +failure would otherwise be one leaked primitive per measured widget per +frame. + +The amplification this removes, measured: a streamed frame makes **1,083 +`Widget::draw` calls over 113 distinct widgets**, and the worst widgets +are drawn **11 times** at nesting depth 7-8. It is not two draws, it is +two to the power of how many measuring ancestors a widget has. Only the +*writes* go away, not the traversals -- the walk and the region +arithmetic still happen 11 times, and removing those needs a size that +can be answered without drawing, which is what LAYOUT.md section 5 rules +out. Worth what it cost: the streamed frame went p50 1.39ms -> 1.22ms +and p99 4.75ms -> 3.58ms, and the upload numbers did not move, because +recycling had already made the discarded writes free in arena terms. + **What is left, and it is a layout question rather than an upload one.** Stream instances upload 72.7%, which *is* the floor: the list is pinned to the newest end, so a growing reply moves every row, and a row's instances diff --git a/iris/core/src/render/primitive.rs b/iris/core/src/render/primitive.rs index f3e0210..d8bacae 100644 --- a/iris/core/src/render/primitive.rs +++ b/iris/core/src/render/primitive.rs @@ -271,8 +271,9 @@ impl Primitives { /// **Why a redraw must be able to do this.** Freed slots do not /// become reusable until the end of the frame (`freed`), so a widget /// that frees its primitives and immediately draws again takes fresh - /// slots every time. Since `Painter::draw_twice` is how a container - /// learns a child's size, and containers nest, that made the arena's + /// slots every time. Since a container learns a child's size by + /// drawing it (`Painter::measure`, and the real draw that follows) + /// and containers nest, that made the arena's /// high-water the *transient* push count rather than the live one: /// measured over the bench fixture's 401 streamed deltas /// (`scripts/rigs/ui-profile`'s `arena_churn`) at 17 million pushes diff --git a/iris/core/src/ui/painter.rs b/iris/core/src/ui/painter.rs index 5b54b6a..199966d 100644 --- a/iris/core/src/ui/painter.rs +++ b/iris/core/src/ui/painter.rs @@ -1,10 +1,11 @@ use crate::{ - Color, RenderedText, Size, StrongWidget, TextAttrs, TextBuffer, TextData, TextureHandle, - UiRegion, UiRenderState, UiRsc, UiScalar, UiVec2, WidgetId, + Color, DrawMode, RenderedText, Size, StrongWidget, TextAttrs, TextBuffer, TextData, + TextureHandle, UiRegion, UiRenderState, UiRsc, UiScalar, UiVec2, WidgetId, render::{ Drawn, GlyphPrimitive, IMAGE_BINDING, Mask, MaskIdx, MoveIdx, NOT_DRAWN, Primitive, PrimitiveHandle, PrimitiveInst, RectPrimitive, }, + ui::render_state::Retained, util::Vec2, }; @@ -36,9 +37,23 @@ pub struct Painter<'a> { pub(super) children: Vec, pub layer: usize, pub(super) id: WidgetId, + /// Whether this draw produces what goes on screen or only a size -- + /// see [`crate::DrawMode`]. Inherited by every child this widget + /// draws, so one `measure` at the top makes the whole subtree + /// write-free. + pub(super) mode: DrawMode, } impl<'a> Painter<'a> { + /// True while this draw is only being asked how big the widget would + /// be. **Every method here that writes anything must return early on + /// it** -- a widget's own `draw` never has to check, which is the + /// point: measuring is a property of the painter, not something each + /// widget re-implements. + pub fn measuring(&self) -> bool { + self.mode == DrawMode::Measure + } + fn primitive_at(&mut self, primitive: P, region: UiRegion) { self.write_primitive(primitive, region, Drawn::Yes); } @@ -74,6 +89,9 @@ impl<'a> Painter<'a> { region: UiRegion, drawn: Drawn, ) -> u32 { + if self.measuring() { + return u32::MAX; + } let inst = PrimitiveInst { id: self.id, primitive, @@ -139,6 +157,13 @@ impl<'a> Painter<'a> { /// so keeps pointing at whichever slot it was drawn under. See /// `ActiveData::own_mask` for what pushing a fresh one cost. pub fn set_mask(&mut self, region: UiRegion) { + // Clipping changes no widget's reported size, so a measurement + // skips it whole -- not just the shape primitive, but the mask + // slot and its refs, which would otherwise be a leaked slot per + // masked widget per measured frame. + if self.measuring() { + return; + } let shape = self.write_primitive(RectPrimitive::color(Color::NONE), region, Drawn::No); self.set_mask_to(shape); } @@ -151,6 +176,12 @@ impl<'a> Painter<'a> { /// with no radius argument anywhere that could fall out of step with /// the one being drawn. pub fn set_mask_to_widget(&mut self, shape: &StrongWidget) { + // Same as `set_mask`, and doubly so: a measurement leaves nothing + // in `active`, so the shape widget has drawn no primitive to + // point at and this would panic on its own message. + if self.measuring() { + return; + } let slot = self.state.first_primitive(shape.id()).unwrap_or_else(|| { panic!( "'{}' was given as a mask's shape but drew no primitive, so there is nothing to \ @@ -246,14 +277,47 @@ impl<'a> Painter<'a> { Some(self.id), self.move_slot.idx() as u32, self.mask, - Default::default(), + self.mode, + Retained::default(), self.rsc, - ); - self.state - .active - .get(&id.id()) - .map(|a| a.size) - .unwrap_or_default() + ) + } + + /// Ask `widget` how big it would be in `region`, **writing nothing** + /// -- see [`DrawMode::Measure`]. For the container that cannot choose + /// what to offer a child without already knowing the child's size: + /// measure, work out the real region, then draw it for real. + /// + /// ```ignore + /// let used = painter.measure(&child, generous); + /// painter.widget_within(&child, self.box_for(used)); + /// ``` + /// + /// This replaced a `draw_twice(child, first, |used| second)`, which + /// made the same two draws but had the caller express the second + /// region as a closure returning it -- so the interesting arithmetic + /// happened inside a callback and anything it wanted to keep had to + /// be written out through a captured `&mut`. Two statements say the + /// same thing in the order it happens (CODE_RULES' "compose + /// linearly"), and the measurement costs no arena slot now rather + /// than allocating one and freeing it. + /// + /// The measured widget is left exactly as it was -- not in `active` + /// if it was not there before, and untouched if it was -- so the draw + /// that follows is an ordinary one and cannot be short-circuited by + /// the measurement having "already drawn" it at that region. + pub fn measure(&mut self, id: &StrongWidget, region: UiRegion) -> Size { + self.state.draw_inner( + self.layer, + id.id(), + region.within(&self.region), + Some(self.id), + self.move_slot.idx() as u32, + self.mask, + DrawMode::Measure, + Retained::default(), + self.rsc, + ) } /// Move an already-drawn child from wherever it currently sits to @@ -266,27 +330,15 @@ impl<'a> Painter<'a> { /// (which detects that from the stored region) does the right thing /// instead. pub fn reposition(&mut self, id: &StrongWidget, region: UiRegion) { + // Moves an *already-drawn* child, of which a measurement has + // none. + if self.measuring() { + return; + } let region = region.within(&self.region); self.state.reposition(id.id(), region, self.rsc); } - /// Draw `child` at a provisional region to learn its size under one - /// axis's worth of assumption, discard everything it wrote, then draw - /// it again at the region that assumption produced. For the rare - /// parent that cannot pick an offered size without already knowing the - /// answer. Twice the cost of one `draw`; every other case in this file - /// avoids it. - pub fn draw_twice( - &mut self, - id: &StrongWidget, - first: UiRegion, - second: impl FnOnce(Size) -> UiRegion, - ) -> Size { - let used = self.widget_within(id, first); - let region = second(used); - self.widget_within(id, region) - } - pub fn texture_within(&mut self, handle: &TextureHandle, region: UiRegion) { self.textures.push(handle.clone()); self.write_image(handle.image_index(), region.within(&self.region)); @@ -306,6 +358,9 @@ impl<'a> Painter<'a> { /// the layer's one instanced draw, so it goes through /// `Primitives::write_image` instead of `primitive_at`/`Primitive::vec`. fn write_image(&mut self, texture_idx: u32, region: UiRegion) { + if self.measuring() { + return; + } let h = match self.take_recycled(IMAGE_BINDING, Drawn::Yes) { Some(h) => { self.state.primitives.recycle_image( diff --git a/iris/core/src/ui/render_state.rs b/iris/core/src/ui/render_state.rs index 4fe4fc0..740008c 100644 --- a/iris/core/src/ui/render_state.rs +++ b/iris/core/src/ui/render_state.rs @@ -2,7 +2,7 @@ use std::sync::Mutex; use std::time::{Duration, Instant}; use crate::{ - ActiveData, IdLike, MaskIdx, MoveIdx, Painter, PixelRegion, PrimitiveLayers, RegionAlign, + ActiveData, IdLike, MaskIdx, MoveIdx, Painter, PixelRegion, PrimitiveLayers, RegionAlign, Size, StrongWidget, UiRegion, UiRsc, UiVec2, WidgetId, Widgets, render::{ Drawn, MoveOffset, NOT_DRAWN, Primitive, PrimitiveHandle, PrimitiveInst, Primitives, @@ -94,6 +94,37 @@ pub struct UiRenderState { last_input_at: Mutex>, } +/// Whether a draw is producing what goes on screen, or only asking a +/// widget how big it would be. +/// +/// **There is no size query without a draw** (LAYOUT.md section 5): +/// `Widget::draw` reports the size it used, and nothing else can answer +/// it. A container that cannot choose what to offer a child without +/// already knowing the child's size therefore has to draw it -- so +/// [`Self::Measure`] is that draw with everything it *writes* switched +/// off. It allocates no arena slot, no mask, no move slot, leaves nothing +/// in `active` and marks nothing dirty; the widget is walked and its text +/// is shaped (which is memoized, and is the expensive half anyway), and +/// only the returned `Size` survives. +/// +/// Because it leaves no trace, the real draw that follows is an ordinary +/// first draw or redraw and cannot be short-circuited by the measurement +/// having "already drawn" the widget at that region -- which is the trap +/// the discarded-draw approach it replaced had to work around. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum DrawMode { + /// Write primitives, keep the result in `active`. + Draw, + /// Report a size and write nothing. + Measure, +} + +impl DrawMode { + fn measuring(self) -> bool { + self == Self::Measure + } +} + /// What a widget being redrawn keeps from the draw it is replacing. /// /// These four always travel together -- they are read off one @@ -102,7 +133,7 @@ pub struct UiRenderState { /// parameters of [`UiRenderState::draw_inner`] until 2026-09-09, next to /// six others. [`Default`] is the "nothing to keep" case: a widget drawn /// for the first time, and the root of a full relayout. -pub(super) struct Retained { +pub(crate) struct Retained { /// So children this draw does not draw again can be retired. pub children: Vec, /// Reused in place with its delta reset, never reallocated: a @@ -422,6 +453,7 @@ impl UiRenderState { None, MoveOffset::NONE_PARENT, MaskIdx::NONE, + DrawMode::Draw, Retained::default(), rsc, ); @@ -455,9 +487,10 @@ impl UiRenderState { parent: Option, parent_move_slot: u32, mask: MaskIdx, + mode: DrawMode, retained: Retained, rsc: &mut dyn UiRsc, - ) { + ) -> Size { let Retained { children: mut old_children, move_slot: mut old_move_slot, @@ -466,7 +499,7 @@ impl UiRenderState { } = retained; // Consumed here, not merely read: this call *is* the redraw the mark // asked for, and leaving the mark set is what stranded a widget's - // primitives. `Painter::draw_twice` calls this twice for the same id + // primitives. A measure-then-draw reaches this twice for the same id // in one frame (`LazySpan::place`'s measurement pass), and on the second // call the still-set mark took the whole `if let` below -- including // the `remove` that frees the first draw's primitives -- out of play, @@ -476,18 +509,24 @@ impl UiRenderState { // and, with `LazySpan` setting no mask, outside the list's own bounds: // the doubled `Compacted:` row in docs/bench/iris-phone-v2-2026-09-06.md. // The same shape reaches any dirty widget an ancestor redraws first. - let dirty = rsc.widgets_mut().needs_redraw.remove(&id); + // A measurement consumes no redraw mark and takes none of the + // fast paths: it is not the redraw the mark asked for, and + // "already drawn at this region" would make it return without + // reporting a size at all. + let dirty = !mode.measuring() && rsc.widgets_mut().needs_redraw.remove(&id); if let Some(active) = self.active.get_mut(&id) && !dirty + && !mode.measuring() { // check to see if we can skip drawing first if active.region == region { - return; + return active.size; } else if active.region.size() == region.size() { // TODO: epsilon? let from = active.region; + let size = active.size; self.mov(id, from, region, rsc); - return; + return size; } else if rsc .widgets() .get_dyn(id) @@ -515,7 +554,7 @@ impl UiRenderState { // exactly this step. See `ActiveData::move_applied`, and // `a_size_independent_widget_moved_by_its_parent_has_the_hit_box_it_is_drawn_at`. active.region = region; - return; + return active.size; } // if not, then maintain resize and track old children to remove unneeded let active = self.remove(id, false, true, rsc).unwrap(); @@ -524,6 +563,7 @@ impl UiRenderState { own_mask = active.own_mask; recycle = active.primitives; } else if dirty && self.active.contains_key(&id) { + debug_assert!(!mode.measuring()); // Dirty and already drawn: none of the fast paths above may be // taken (the widget's own content changed, so its old primitives // say nothing about its new ones), but they are also the only @@ -543,31 +583,12 @@ impl UiRenderState { the second draw's primitives would orphan the first's" ); - let move_slot = match old_move_slot { - // Reused across a real redraw of the same id: the fresh - // geometry this draw is about to write is placed at its - // correct absolute position by `region` itself, so any delta - // accumulated before this redraw is now stale and would - // double-offset it if left in place. The chain link (`parent`) - // is untouched -- the logical parent has not changed. - Some(slot) => { - let entry = rsc.ui_mut().move_offsets.get_mut(slot); - entry.delta = [0.0, 0.0]; - slot - } - None => { - let slot = rsc - .ui_mut() - .move_offsets - .push(MoveOffset::new([0.0, 0.0], parent_move_slot)); - rsc.ui_mut().move_offsets.push_ref(slot); - if parent_move_slot != MoveOffset::NONE_PARENT { - rsc.ui_mut() - .move_offsets - .push_ref(Id::preset(parent_move_slot)); - } - slot - } + // A measurement writes no primitive, so nothing ever reads this + // -- and allocating one would leak a slot per measured widget per + // frame, since `move_offsets` only frees on a widget's removal. + let move_slot = match mode { + DrawMode::Measure => Id::preset(MoveOffset::NONE_PARENT), + DrawMode::Draw => Self::move_slot_for(old_move_slot, parent_move_slot, rsc), }; // The mask this widget was drawn *under*, kept aside because @@ -591,6 +612,7 @@ impl UiRenderState { primitives: Vec::new(), recycle: recycle.into_iter().peekable(), children: Vec::new(), + mode, rsc, }; @@ -623,8 +645,27 @@ impl UiRenderState { children, layer, id, + mode: _, } = painter; + if mode.measuring() { + // Nothing to unwind: a measurement allocates no slot, no + // mask, no move offset and no `ActiveData`, so the size is + // the whole of what it produced. Asserted rather than + // assumed, because a `Painter` method that forgot to check + // the mode would otherwise leak silently -- one primitive per + // measured widget per frame, which a screen redrawn every + // frame turns into an arena that grows without bound. + debug_assert!( + primitives.is_empty() && textures.is_empty(), + "measuring {id:?} wrote {} primitive(s) and {} texture(s); \ + every `Painter` write must check `Painter::measuring`", + primitives.len(), + textures.len(), + ); + return size; + } + // Whatever the draw did not claim is genuinely gone: this draw // wrote fewer primitives than the last one, or stopped matching // part way. Freeing it here rather than in `remove` is what lets @@ -660,6 +701,39 @@ impl UiRenderState { rsc.on_draw(&active); self.active.insert(id, active); + size + } + + /// This widget's slot in `move_offsets`: the one it already had if it + /// is being redrawn, or a fresh one linked to its parent's. + /// + /// A redraw **reuses the slot in place with its delta reset**, never + /// reallocates: the geometry this draw is about to write is already + /// at its correct absolute position, so a delta accumulated before it + /// would double-offset it -- while the chain link (`parent`) is left + /// alone, because the logical parent has not changed and a descendant + /// that is not itself redrawn still points here. See LAYOUT.md + /// section 2. + fn move_slot_for(old: Option, parent_move_slot: u32, rsc: &mut dyn UiRsc) -> MoveIdx { + match old { + Some(slot) => { + rsc.ui_mut().move_offsets.get_mut(slot).delta = [0.0, 0.0]; + slot + } + None => { + let slot = rsc + .ui_mut() + .move_offsets + .push(MoveOffset::new([0.0, 0.0], parent_move_slot)); + rsc.ui_mut().move_offsets.push_ref(slot); + if parent_move_slot != MoveOffset::NONE_PARENT { + rsc.ui_mut() + .move_offsets + .push_ref(Id::preset(parent_move_slot)); + } + slot + } + } } /// O(1): write the delta for this widget's own slot in @@ -1217,6 +1291,7 @@ impl UiRenderState { parent, parent_move_slot, active.mask, + DrawMode::Draw, Retained { children: active.children, move_slot: Some(active.move_slot), diff --git a/iris/src/widget/position/lazy_span.rs b/iris/src/widget/position/lazy_span.rs index 9dc856e..87f9f49 100644 --- a/iris/src/widget/position/lazy_span.rs +++ b/iris/src/widget/position/lazy_span.rs @@ -119,7 +119,7 @@ //! placement of that row -- not merely an optimisation: see `place`'s doc //! for why a row that fills whatever it is offered (a `.background(rect //! (...))`) needs this to ever be placed at the right size at all, and why -//! reusing `draw_twice` every frame instead would defeat `draw_inner`'s own +//! re-measuring every frame instead would defeat `draw_inner`'s own //! skip-or-move caching. Only a row's first-ever appearance pays the //! two-draw measurement; nothing here estimates a height for an off-screen //! row that has never been measured, so this stays independent of how many @@ -1014,7 +1014,7 @@ impl LazySpan { /// region. A row seen for the first time has no cached height to place /// it *at*, so it is measured first (an oversized, fixed-size region) /// and then drawn a *second* time at the tight box that measurement - /// implies, via `Painter::draw_twice` -- not `reposition` (a pure + /// implies, via `Painter::measure` -- not `reposition` (a pure /// translation, no resize). This distinction is required, not just an /// optimisation: a row is not always plain wrapped text -- /// `.background(rect(tint))` is an ordinary way to style one, and @@ -1023,7 +1023,7 @@ impl LazySpan { /// Measuring such a row at the oversized box has it paint an oversized /// rect there; `reposition` only ever writes an offset, never a size, /// so an every-frame reposition-only scheme would leave that primitive - /// oversized forever. Using `draw_twice` for *every* frame would fix + /// oversized forever. Measuring on *every* frame would fix /// that but break the opposite property: its two calls use two /// different regions, so whichever one `ActiveData.region` ends up /// holding always disagrees with the *next* frame's first call, @@ -1116,8 +1116,9 @@ impl LazySpan { } // Never measured, so there is no height to place it at: it is // measured at an oversized region first and drawn again at the - // box that measurement implies (`draw_twice`, not - // `reposition`, which writes an offset and never a size). + // box that measurement implies (`Painter::measure` then a + // real draw, not `reposition`, which writes an offset and + // never a size). // // A bottom-known row measures at a *zero-anchored* region // rather than at its own box: using the real box would make @@ -1132,12 +1133,9 @@ impl LazySpan { Placement::Trailing(_) => 0.0, }; let first = Self::abs_region(dir, measure_from, measure_from + GENEROUS_PADDING); - let mut height = 0.0; - painter.draw_twice(widget, first, |used| { - height = resolve(used); - let (lead, trail) = placement.edges(height); - Self::abs_region(dir, lead, trail) - }); + let height = resolve(painter.measure(widget, first)); + let (lead, trail) = placement.edges(height); + painter.widget_within(widget, Self::abs_region(dir, lead, trail)); height } }; @@ -2102,7 +2100,7 @@ mod tests { /// `ReplaceLast` case drives up to 400 times during a streamed reply /// (`bench_client.rs`'s stream phase): the last slot's widget is /// swapped for a brand-new one, same key, and (since a fresh widget - /// has no cached height) placed via `place`'s `draw_twice` path every + /// has no cached height) placed via `place`'s measure-then-draw path every /// time -- the provisional-then-real two-draw sequence LAYOUT.md /// documents as the one place in this crate that deliberately draws a /// widget twice. If `draw_inner`'s old-children diffing or @@ -2131,7 +2129,7 @@ mod tests { let before = render.active_widgets(); for i in 0..400u32 { - // A varying height keeps every replace on the `draw_twice` + // A varying height keeps every replace on the measure-then-draw // (cache-miss) path rather than settling into the O(1) // same-size `mov` fast path once the height happens to repeat. let (_bg_id, new_row) = background_styled_row(&mut rsc, 20.0 + (i % 3) as f32); @@ -2171,7 +2169,7 @@ mod tests { /// /// Two rows, two shapes of the same fault: row 2 has a cached height /// (one `widget_within`), row 4 is replaced so it has none (`place`'s - /// `draw_twice`, which reaches `draw_inner` twice for one id in one + /// measure-then-draw, which reaches `draw_inner` twice for one id in one /// frame and so orphans a copy even with no ancestor involved). #[test] fn an_ancestor_redrawing_a_dirty_row_leaves_no_stale_copy() {