iris: a dirty widget redrawn by its ancestor never freed its old primitives
`draw_inner` read `needs_redraw` without consuming it, and used it to skip the whole `if let Some(active)` block -- including the `remove(id, false)` that frees a redrawn widget's previous primitives. So a widget that was both already active and marked dirty, and was reached by an *ancestor's* draw rather than by `redraw_updates` picking it first, drew a second full set of primitives and then had `active.insert` overwrite the only handles that could ever have freed the first set. Those primitives stay in the layer's instance buffer for the life of the process, with a leaked move slot and leaked mask refs, drawn every frame at whatever region they last had -- and `List` sets no mask, so a row measured at `GENEROUS_PADDING` leaves its ghost outside the list's own box. That is the doubled `Compacted:` row in docs/bench/iris-phone-v2-2026-09-06.md: overlapping copies inside the transcript and one more below the composer. Fixed by consuming the mark (`needs_redraw.remove`) at the top of `draw_inner` -- this call *is* the redraw it asked for -- and freeing the old primitives on the dirty path too. Guarded so it cannot come back silently: `UiRenderState::orphaned_primitives` walks every layer's live instances and names any whose owner is no longer active or no longer holds a handle to them, and `update` `debug_assert!`s it empty every frame (debug builds only). New regression test `an_ancestor_redrawing_a_dirty_row_leaves_no_stale_copy` in list.rs fails on the pre-fix code with "1 primitive(s) survived their own widget's redraw". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
1 parent
3e72a4ef19
commit
76b1f99277
3 files changed
+170
-2
No files matched your search
@@ -6,6 +6,7 @@ use crate::{
|
|||||||
ArrBuf,
|
ArrBuf,
|
||||||
data::{MaskIdx, MoveIdx, PrimitiveInstance},
|
data::{MaskIdx, MoveIdx, PrimitiveInstance},
|
||||||
},
|
},
|
||||||
|
util::HashSet,
|
||||||
};
|
};
|
||||||
use bytemuck::Pod;
|
use bytemuck::Pod;
|
||||||
use wgpu::*;
|
use wgpu::*;
|
||||||
@@ -277,6 +278,23 @@ impl Primitives {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Every instance that is still bound for the GPU, as `(inst_idx,
|
||||||
|
/// owner, is_image)` -- everything except the slots already handed to
|
||||||
|
/// [`Self::free`] and waiting for [`Self::apply_free`] to compact them
|
||||||
|
/// away. Only [`crate::UiRenderState::orphaned_primitives`] uses this,
|
||||||
|
/// to check that every drawn primitive still belongs to a live widget.
|
||||||
|
pub fn live_instances(&self) -> impl Iterator<Item = (usize, WidgetId, bool)> + '_ {
|
||||||
|
let free: HashSet<usize> = self.free.iter().copied().collect();
|
||||||
|
let image_free: HashSet<usize> = self.image_free.iter().copied().collect();
|
||||||
|
let rects = (0..self.instances.len())
|
||||||
|
.filter(move |i| !free.contains(i))
|
||||||
|
.map(|i| (i, self.assoc[i], false));
|
||||||
|
let images = (0..self.images.len())
|
||||||
|
.filter(move |i| !image_free.contains(i))
|
||||||
|
.map(|i| (i, self.image_assoc[i], true));
|
||||||
|
rects.chain(images)
|
||||||
|
}
|
||||||
|
|
||||||
pub fn data(&self) -> &PrimitiveData {
|
pub fn data(&self) -> &PrimitiveData {
|
||||||
&self.data
|
&self.data
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,7 +1,7 @@
|
|||||||
use crate::{
|
use crate::{
|
||||||
ActiveData, IdLike, MaskIdx, MoveIdx, Painter, PixelRegion, PrimitiveLayers, RegionAlign,
|
ActiveData, IdLike, MaskIdx, MoveIdx, Painter, PixelRegion, PrimitiveLayers, RegionAlign,
|
||||||
StrongWidget, UiRegion, UiRsc, UiVec2, WidgetId, Widgets,
|
StrongWidget, UiRegion, UiRsc, UiVec2, WidgetId, Widgets,
|
||||||
render::MoveOffset,
|
render::{IMAGE_BINDING, MoveOffset},
|
||||||
util::{HashMap, HashSet, Id, Vec2},
|
util::{HashMap, HashSet, Id, Vec2},
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -139,6 +139,12 @@ impl UiRenderState {
|
|||||||
} else if rsc.widgets().has_updates() {
|
} else if rsc.widgets().has_updates() {
|
||||||
self.redraw_updates(rsc);
|
self.redraw_updates(rsc);
|
||||||
}
|
}
|
||||||
|
#[cfg(debug_assertions)]
|
||||||
|
debug_assert!(
|
||||||
|
self.orphaned_primitives().is_empty(),
|
||||||
|
"{}",
|
||||||
|
self.orphan_report(rsc),
|
||||||
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
fn redraw_all(&mut self, root: Option<&StrongWidget>, rsc: &mut dyn UiRsc) {
|
fn redraw_all(&mut self, root: Option<&StrongWidget>, rsc: &mut dyn UiRsc) {
|
||||||
@@ -192,8 +198,21 @@ impl UiRenderState {
|
|||||||
) {
|
) {
|
||||||
let mut old_children = old_children.unwrap_or_default();
|
let mut old_children = old_children.unwrap_or_default();
|
||||||
let mut old_move_slot = old_move_slot;
|
let mut old_move_slot = old_move_slot;
|
||||||
|
// 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
|
||||||
|
// in one frame (`List::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,
|
||||||
|
// so `active.insert` at the end overwrote the only handles that could
|
||||||
|
// ever have freed them. The result is a full second copy of the row,
|
||||||
|
// drawn every frame from then on at the oversized measurement region
|
||||||
|
// and, with `List` 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);
|
||||||
if let Some(active) = self.active.get_mut(&id)
|
if let Some(active) = self.active.get_mut(&id)
|
||||||
&& !rsc.widgets().needs_redraw.contains(&id)
|
&& !dirty
|
||||||
{
|
{
|
||||||
// check to see if we can skip drawing first
|
// check to see if we can skip drawing first
|
||||||
if active.region == region {
|
if active.region == region {
|
||||||
@@ -227,6 +246,14 @@ impl UiRenderState {
|
|||||||
let active = self.remove(id, false, rsc).unwrap();
|
let active = self.remove(id, false, rsc).unwrap();
|
||||||
old_children = active.children;
|
old_children = active.children;
|
||||||
old_move_slot = Some(active.move_slot);
|
old_move_slot = Some(active.move_slot);
|
||||||
|
} else if dirty && self.active.contains_key(&id) {
|
||||||
|
// 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
|
||||||
|
// thing that frees them. Same two lines, reached the other way.
|
||||||
|
let active = self.remove(id, false, rsc).unwrap();
|
||||||
|
old_children = active.children;
|
||||||
|
old_move_slot = Some(active.move_slot);
|
||||||
}
|
}
|
||||||
|
|
||||||
// draw widget
|
// draw widget
|
||||||
@@ -475,6 +502,66 @@ impl UiRenderState {
|
|||||||
self.active.len()
|
self.active.len()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Primitive instances still bound for the GPU whose owner is no
|
||||||
|
/// longer in `active`, or whose owner's `ActiveData` no longer names
|
||||||
|
/// them: a copy nothing can move, clip, resize or free, redrawn every
|
||||||
|
/// frame at whatever position it last had. `(layer, inst_idx, owner)`
|
||||||
|
/// each.
|
||||||
|
///
|
||||||
|
/// Asserted empty at the end of every [`Self::update`], because this
|
||||||
|
/// is exactly the shape of the duplicated transcript row on Iris's
|
||||||
|
/// phone (`docs/bench/iris-phone-v2-2026-09-06.md`): counting
|
||||||
|
/// `active` alone cannot see it, since the orphan's owner is very
|
||||||
|
/// much alive -- it is the *earlier* set of primitives that got
|
||||||
|
/// stranded when the widget was drawn a second time without the first
|
||||||
|
/// draw being freed. O(primitives), debug builds only.
|
||||||
|
pub fn orphaned_primitives(&self) -> Vec<(usize, usize, WidgetId)> {
|
||||||
|
let mut orphans = Vec::new();
|
||||||
|
for (layer, primitives) in self.layers.iter() {
|
||||||
|
for (inst_idx, owner, is_image) in primitives.live_instances() {
|
||||||
|
let owned = self.active.get(&owner).is_some_and(|a| {
|
||||||
|
a.primitives.iter().any(|h| {
|
||||||
|
h.layer == layer
|
||||||
|
&& h.inst_idx == inst_idx
|
||||||
|
&& (h.binding == IMAGE_BINDING) == is_image
|
||||||
|
})
|
||||||
|
});
|
||||||
|
if !owned {
|
||||||
|
orphans.push((layer, inst_idx, owner));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
orphans
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The message [`Self::update`]'s orphan assert prints -- built here
|
||||||
|
/// rather than inline so the (allocating, O(primitives)) work only
|
||||||
|
/// happens on the failing path.
|
||||||
|
#[cfg(debug_assertions)]
|
||||||
|
fn orphan_report(&self, rsc: &dyn UiRsc) -> String {
|
||||||
|
let orphans = self.orphaned_primitives();
|
||||||
|
let mut lines: Vec<String> = orphans
|
||||||
|
.iter()
|
||||||
|
.take(8)
|
||||||
|
.map(|(layer, idx, owner)| {
|
||||||
|
let alive = self.active.contains_key(owner);
|
||||||
|
format!(
|
||||||
|
" layer {layer} instance {idx}: owner '{}' ({owner:?}), owner still active: {alive}",
|
||||||
|
rsc.widgets().label(*owner),
|
||||||
|
)
|
||||||
|
})
|
||||||
|
.collect();
|
||||||
|
if orphans.len() > lines.len() {
|
||||||
|
lines.push(format!(" ... and {} more", orphans.len() - lines.len()));
|
||||||
|
}
|
||||||
|
format!(
|
||||||
|
"{} primitive(s) are drawn but owned by nobody -- a stale copy \
|
||||||
|
nothing will ever move or free:\n{}",
|
||||||
|
orphans.len(),
|
||||||
|
lines.join("\n"),
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
/// Give `id` exclusive pointer input from the next `run_sensors` call
|
/// Give `id` exclusive pointer input from the next `run_sensors` call
|
||||||
/// on -- see `captured`'s field doc. Overwrites any previous capture
|
/// on -- see `captured`'s field doc. Overwrites any previous capture
|
||||||
/// (a gesture that starts a new one has already decided the old one
|
/// (a gesture that starts a new one has already decided the old one
|
||||||
|
|||||||
@@ -1467,6 +1467,69 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The doubled `Compacted:` row from Iris's phone (docs/bench/
|
||||||
|
/// iris-phone-v2-2026-09-06.md), reproduced at its mechanism.
|
||||||
|
///
|
||||||
|
/// `replacing_the_last_row_many_times_does_not_leak_primitives` above
|
||||||
|
/// counts *widgets*, which is why it passed all along: the orphan's
|
||||||
|
/// owner is very much alive -- it is an earlier set of that same
|
||||||
|
/// widget's primitives that got stranded. What strands them is a row
|
||||||
|
/// marked dirty and then reached by its **ancestor's** redraw rather
|
||||||
|
/// than by its own: `draw_inner` only *read* the dirty mark, so the
|
||||||
|
/// whole branch that frees a redrawn widget's previous primitives was
|
||||||
|
/// skipped, and the fresh `ActiveData` overwrote the only handles that
|
||||||
|
/// could ever have freed them. `List` sets no mask, so that copy then
|
||||||
|
/// draws every frame at whatever region it last had -- including,
|
||||||
|
/// where the row was being measured at `GENEROUS_PADDING`, well below
|
||||||
|
/// the list's own box and under the composer.
|
||||||
|
///
|
||||||
|
/// 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
|
||||||
|
/// frame and so orphans a copy even with no ancestor involved).
|
||||||
|
#[test]
|
||||||
|
fn an_ancestor_redrawing_a_dirty_row_leaves_no_stale_copy() {
|
||||||
|
let mut rsc = TestRsc {
|
||||||
|
ui: UiData::default(),
|
||||||
|
};
|
||||||
|
let mut list = List::new(Axis::Y);
|
||||||
|
// Rows that own a primitive *at their own id* (a background rect),
|
||||||
|
// not only through a child: an orphan is a widget's own primitive
|
||||||
|
// outliving its own redraw, so a row whose top-level widget paints
|
||||||
|
// nothing itself cannot show one however broken the path is.
|
||||||
|
let mut rows = Vec::new();
|
||||||
|
for key in 0..5u64 {
|
||||||
|
let (bg_id, row) = background_styled_row(&mut rsc, 20.0);
|
||||||
|
rows.push((row.id(), bg_id));
|
||||||
|
list.push_back(ListRow::new(key, row));
|
||||||
|
}
|
||||||
|
let (list_weak, root) = add_list(&mut rsc, list);
|
||||||
|
|
||||||
|
let mut render = UiRenderState::new();
|
||||||
|
render.resize((100.0, 100.0));
|
||||||
|
render.update(&root, &mut rsc);
|
||||||
|
assert!(render.orphaned_primitives().is_empty());
|
||||||
|
|
||||||
|
// A streamed row's content changing: the row is marked dirty (any
|
||||||
|
// `.set()` on it does this)...
|
||||||
|
let (row2, row2_bg) = rows[2];
|
||||||
|
rsc.ui.widgets.get_dyn_mut(row2).unwrap();
|
||||||
|
rsc.ui.widgets.get_dyn_mut(row2_bg).unwrap();
|
||||||
|
|
||||||
|
// Redraw the *list* by name, so the dirty row is reached by its
|
||||||
|
// ancestor's draw rather than by `redraw_updates` happening to
|
||||||
|
// pick it first -- which is the order `HashSet` iteration makes
|
||||||
|
// arbitrary, and the reason this went unnoticed.
|
||||||
|
render.redraw(list_weak.id(), &mut rsc);
|
||||||
|
|
||||||
|
let orphans = render.orphaned_primitives();
|
||||||
|
assert!(
|
||||||
|
orphans.is_empty(),
|
||||||
|
"{} primitive(s) survived their own widget's redraw: {orphans:?}",
|
||||||
|
orphans.len(),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
/// Enough rows, tall enough, that a fling toward the start has real
|
/// Enough rows, tall enough, that a fling toward the start has real
|
||||||
/// room to travel before `at_start` clamps it -- shared by the fling
|
/// room to travel before `at_start` clamps it -- shared by the fling
|
||||||
/// tests below.
|
/// tests below.
|
||||||
|
|||||||
Reference in new issue
Block a user