1676 lines
99 KiB
Markdown
1676 lines
99 KiB
Markdown
# Layout findings log
|
||
|
||
What the sessions reviewing Iris's retained layout found, kept so that
|
||
nothing here is rediscovered. Each entry says who found it and when.
|
||
**Delete this file when #19 lands and its fixes are in**; what must outlive
|
||
it (settled design, the measurement method) belongs in `docs/LAYOUT.md`, and
|
||
the current plan is in `docs/HANDOFF.md`.
|
||
|
||
## Fourteenth sweep: allocation, and the cost per child (2026-09-20)
|
||
|
||
Over the whole branch again, aimed first at what allocates -- `Vec`s, `Arc`s,
|
||
capacity dropped and re-grown -- and then at whatever else the reading turned
|
||
up. Nine findings.
|
||
|
||
- **A container's draw was quadratic in its children.** Every per-child step
|
||
in one draw asked "have I done this one already?" by searching a list:
|
||
`widget_at` searched `children` and then `under`, `place_at` searched
|
||
`children`, `draw_at` searched it once per old child and once per size read,
|
||
and `depend_on` searched `size_deps`. At a hundred children none of that
|
||
shows; at a thousand it is most of the frame. Profiled at 1,600 children,
|
||
70% of the redraw was in those searches -- `place_at` alone 15.6%, and it
|
||
does nothing else linear.
|
||
|
||
The question each of them asks is answered in one read now: a draw takes a
|
||
`DrawId` and leaves it on every widget it asks about, and a widget carries
|
||
the draw that last asked. One note per widget is enough because one widget
|
||
is asked about by one container -- the handle a container holds a child by
|
||
cannot be cloned. `under` is still searched, but only on the rare re-ask,
|
||
since it is added to in step with the child list. `depend_on` no longer
|
||
dedupes at all: a child that answered with a hint may have no record to note
|
||
it on until the draw ends, and the loop `size_deps` drives asks the same of a
|
||
widget twice as of it once. A debug assertion in `widget_at` checks the note
|
||
against the list it claims to be in, which is the cheap guard for the pair.
|
||
|
||
`tests/children_cost.rs` is new and is the only rig here that varies width;
|
||
every other one varies depth, the window, or what changed. 40 full redraws of
|
||
one span, per child in the last column:
|
||
|
||
| children | before | after | per child before | after |
|
||
| --- | --- | --- | --- | --- |
|
||
| 100 | 0.068 ms | 0.053 ms | 0.0007 ms | 0.0005 ms |
|
||
| 200 | 0.143 ms | 0.101 ms | 0.0007 ms | 0.0005 ms |
|
||
| 400 | 0.386 ms | 0.213 ms | 0.0010 ms | 0.0005 ms |
|
||
| 800 | 1.158 ms | 0.410 ms | 0.0014 ms | 0.0005 ms |
|
||
| 1600 | 3.680 ms | 0.811 ms | 0.0023 ms | 0.0005 ms |
|
||
|
||
Flat per child after, 4.5x at 1,600. A transcript is exactly this shape.
|
||
|
||
- **A mask's rectangle was resolved once per fragment.** `masked()` in the
|
||
prelude called `resolve_move` -- a walk up to `CHAIN_LIMIT` (64) links
|
||
long -- for every fragment of every masked primitive, although the rectangle
|
||
is the same for all of them. It is resolved in `vs_main` now and handed on as
|
||
two flat `vec2`s, which also takes `masks` and `move_offsets` out of the
|
||
fragment stage entirely: both bindings are `ShaderStages::VERTEX` now, and
|
||
the wrong visibility is what the GPU rig caught first.
|
||
|
||
`chain_cost.rs` could not see this: its instances are two pixels wide on
|
||
purpose, so vertex work dominates. It has a second fixture now -- one
|
||
screenful of rows, each clipped by a mask whose own chain is that deep --
|
||
and the two tables are what tell the stages apart. GPU timestamps, best of 8
|
||
batches, 1024x1024:
|
||
|
||
| mask chain | before | after |
|
||
| --- | --- | --- |
|
||
| 1 | 66.5 us | 66.2 us |
|
||
| 4 | 91.4 us | 67.6 us |
|
||
| 8 | 141.2 us | 69.4 us |
|
||
| 16 | 240.8 us | 73.1 us |
|
||
| 64 | 838.4 us | 95.7 us |
|
||
|
||
8.8x at depth 64 and 2.0x at depth 8; unchanged at depth 1, which is the
|
||
check that nothing else moved. What is left at depth 64 is the instances' own
|
||
vertex walk, which this does not touch.
|
||
|
||
- **`TextBuffer::shape` copied the attrs before the check that would not need
|
||
them.** It built a whole `LayoutKey` first and compared that, so every text
|
||
draw copied its `TextAttrs` -- a heap allocation for any text naming its font
|
||
family, which is what the app does for every icon and every monospace run --
|
||
and then compared the attrs up to three times over. The comparisons are
|
||
asked of the borrowed attrs now and the key is built where it is kept.
|
||
Measured over 100 redraws of 8 named-family texts at one width: 800
|
||
allocations before, 0 after, which is one per text per frame.
|
||
|
||
`tests/allocation_cost.rs` had no text case at all, so none of this was
|
||
visible to the rig whose whole job is to say the steady state allocates
|
||
nothing. It has one now, asserting zero for a text redrawn at the width it
|
||
already has.
|
||
|
||
- **`cargo test --release` failed on this branch**, and every rig here is run
|
||
in release. `a_clipping_widget_reporting_more_than_its_box_is_caught` is
|
||
`#[should_panic]` on a `debug_assert`, which a release build does not
|
||
compile, so the test cannot pass there -- and a `should_panic` that does not
|
||
panic fails rather than passing vacuously. It is `#[cfg(debug_assertions)]`
|
||
now.
|
||
|
||
- **`Fixed::div` and `Div for Fixed` were reached only by their own test.**
|
||
Nothing in the framework divides one length by another; `div_int` and `ratio`
|
||
are what the layout uses. Deleted with the test that was its only caller,
|
||
which also settles the open item about `div` answering `MIN`/`MAX` for a zero
|
||
divisor where `ratio` answers `ZERO`: there is one of them now.
|
||
|
||
- **`Moves::remove` marked the entries changed.** Freeing a slot changes no
|
||
byte the GPU holds -- the slot keeps what it had, nothing names it until it
|
||
is handed out again, and whoever is handed it writes it then -- so every
|
||
frame that retired a region node re-uploaded the whole array for nothing.
|
||
|
||
- **A comment saying `widget_trait!` takes no attributes**, which this branch's
|
||
own `76aaf06` made false when it taught the macro to forward them to both the
|
||
trait and the impl. The comment was written on 2026-09-16 and was true then;
|
||
it is the doc comment it says it cannot be now.
|
||
|
||
- **A comment saying `Scroll` clips.** `Masked` explained reporting its own box
|
||
"for the reason `Scroll` reports the same: it clips what is inside to that
|
||
box". Nothing calls `set_mask` for a `Scroll`, and `scrollable()` only makes
|
||
its inner a region node: a scroll area positions its content by a move and
|
||
does not clip it, because masking is a capability a caller opts into by
|
||
putting a `Masked` around it. The code is right; the reason was not.
|
||
|
||
- **`StrongWidget` carried a `RefCounter(Arc<AtomicU32>)` it could never
|
||
use.** It deliberately has no `Clone`, so the count never rose above zero,
|
||
`RefCounter::drop` always answered true, and `StrongWidget::refs` had no
|
||
caller -- while every widget made paid a heap allocation and every one
|
||
created or dropped paid two atomic read-modify-writes. Removed: the handle is
|
||
the id, the sender and the type, and `Drop` sends. It is 32 bytes rather than
|
||
40, and 40 rather than 48 for a `dyn` one. Raised as something to leave for
|
||
the pre-gate pass, `handle.rs` being outside #19, and Bryan asked for it here
|
||
(2026-09-21): not being `Clone` is what makes one handle the only one, which
|
||
is the same fact the child note above rests on.
|
||
|
||
`RefCounter` stays for `TextureHandle`, which does clone -- several widgets
|
||
showing one picture share its slot -- and is down to what that needs:
|
||
`quiet_clone` and `refs` had no callers at all, and `new` was `Default` spelt
|
||
out, so the default is derived now and says in one line that zero means one
|
||
handle.
|
||
|
||
Four things the sweep **looked at and left**:
|
||
|
||
- **Text allocates about six times per re-broken paragraph per frame**, and it
|
||
is not worth removing. One is the whole text copied into the placement store
|
||
(`keep_placed`), and the other five are `place` growing its glyph list from
|
||
empty, 4 through 64 entries for a 58-glyph paragraph. Both would go with a
|
||
free list fed by the store's own evictions, which happen exactly when a
|
||
placement needs one. Measured what that would buy, with the copy removed and
|
||
the list given its capacity outright: 3,710,680,668 instructions before and
|
||
3,708,100,546 after over 500 sweeping resize frames of 40 paragraphs, which
|
||
is **0.07%**. The machinery is two pools and a changed `place` signature for
|
||
that, so the count is recorded here instead. Allocation count is not the
|
||
same quantity as cost, and this is where the two part.
|
||
- **More for that same pass, all dead and all older than #19**:
|
||
`Size::to_uivec2`, `Size::rel`, `Size::leftover`, `Len::to_uivec2`,
|
||
`Vec2::with_x`, `Vec2::with_y`, `Vec2::ceil`, `Layers::iter_orderless_mut`.
|
||
Left out of this sweep's diff because they are not this branch's, not
|
||
because they should stay.
|
||
- **`request_readers` keeps an empty `HashSet` for a widget whose readers have
|
||
all gone.** `remove` takes the reader out of the set and leaves the set; only
|
||
freeing the widget itself takes the entry. Bounded by the number of live
|
||
widgets and one allocation each, so it is a cost that does not grow with
|
||
time; left as it stands.
|
||
- **`Nodes::graft` copies a shared sub-expression once per reference.** A node
|
||
can be named twice only where a caller reused a `SizeRequest` it cloned, so
|
||
the duplication is bounded by what the caller wrote rather than by anything
|
||
the arena does on its own.
|
||
|
||
One more thing **looked at and left** in passing: `RefCounter` decrements with
|
||
`Ordering::Release` and has no acquire on the last drop, which is the shape an
|
||
`Arc` gets wrong when handles cross threads. Left because nothing here is
|
||
threaded and the orderings are not this sweep's to guess at.
|
||
|
||
Verified at the sweep's tip: format, workspace clippy under `-D warnings` with
|
||
and without `layout-diagnostics`, **190** ordinary, **193** diagnostic and
|
||
**189** release tests (188 and 191 before, with release failing), and the cold
|
||
dump byte-identical to `cbccfb6` across all **34,986** boxes. All three seed
|
||
scans pass -- 400 depth 5 (64.42s), 1,000 depth 6 (160.99s), 2,000 depth 4
|
||
(301.45s) -- as do 400 depth-5 trees in each of the three deferred corpora
|
||
(206.31s); the depth-5 scan was run again after the text change (91.40s). The
|
||
new `mask_clip.rs` reads the clipped pixels back off the GPU and gives the same
|
||
6,000 pixels in the same bounds before and after the shader change.
|
||
|
||
## Thirteenth sweep: a full pass over #19 (2026-09-20)
|
||
|
||
Over the whole branch rather than one commit, and the first review of
|
||
`f48e04e`. Seven findings, all in `cbccfb6`.
|
||
|
||
- **A length of zero printed as nothing.** `Display for LayoutLen` leaves out
|
||
each part that is zero, so `LayoutLen::ZERO` is the empty string -- and
|
||
`f48e04e` pointed `Debug` at `Display`, so the four `assert_eq!`s in
|
||
`cases/deferred.rs` print nothing where a request of zero is. Worse in
|
||
`scenario::describe`, which prints a `.width(0)` rule as `-` -- the same
|
||
thing it prints for a widget with no rule. That function exists so a tree a
|
||
fuzzer found "can be written out by hand"; a value it cannot say is a hole
|
||
in the one thing it is for. Measured before the fix:
|
||
`format!("{}", px(0).min(px(40)))` was `""`, and `Size::ZERO` was `"(, )"`.
|
||
It is `0 px;` now, with a test over all four shapes of length.
|
||
- **A ceiling that stepped off the top of the grid.** `ceil_from_f32` takes
|
||
`next_up` of a `from_f32` that has already clamped, and `next_up` wraps, so
|
||
`Px::ceil_from_f32(1e12)` was `Px::MIN` -- the most negative length there
|
||
is, from the largest measurement. `from_f32` clamps deliberately ("a float
|
||
has further to come from"); the ceiling is the other way in from a float
|
||
and the rule governs both. The assertion goes beside the one `from_f32`
|
||
already had, which is where the class lives.
|
||
- **`Moves::depth` walked the chain a second way**, with its own copy of
|
||
`CHAIN_LIMIT` and without the debug assertion `walk` makes. It is
|
||
`self.walk(idx, |_| depth += 1)` now, so the CPU counts a move chain in one
|
||
place and the constant the shader is handed reaches both.
|
||
- **A harness setter that did not do what it said.** `Harness::set_len` said
|
||
"the way `.width()` sets one" and called `set_size_rule`, which writes the
|
||
whole rule -- so it dropped any bound beside the length, where
|
||
`Widgets::set_len` keeps one on purpose. A case that set a bound and then a
|
||
length would have passed with no bound at all. It calls `Widgets::set_len`.
|
||
- **Three spellings of "one seed, or a range of them."** `generated.rs`,
|
||
`shrink.rs` and `deferred_generated.rs` each wrote the `std::env::var(..)
|
||
.parse()` match by hand -- the class the eleventh sweep closed for *reading*
|
||
a parameter and not for this one. One `rig::seeds`, carrying an
|
||
`#[allow(dead_code)]` because the module is compiled into each rig target
|
||
and the measurement rigs choose no seeds.
|
||
- **A refusal that could go uncounted.** `diag::outside` writes out
|
||
`AxisHolds::contains`'s four clauses to say *which* one refused a reuse. A
|
||
fifth clause added there and not here would leave a refusal counted by
|
||
`ReuseOutside` and explained by nothing; a debug assertion catches that now,
|
||
which is the cheap guard rather than machinery to derive the reasons.
|
||
- **`cases/deferred` sat last** in `suite.rs`'s otherwise alphabetical list.
|
||
|
||
**A stale record, re-measured.** `Sow::bound` grows bounds in pixels and its
|
||
comment said why: a fraction survives a `place_at` into a different rel base,
|
||
"seeds 4 (shuffle-all-but-first) and 196 (resize-size) at depth 5 are where
|
||
that showed". But `generated.rs` says in its own comment that a seed names a
|
||
tree only while the generator draws the same things in the same order, and
|
||
that adding images to the leaves moved every one of them -- which is
|
||
`2dba90b`, after that hole was recorded. Re-measured: 600 depth-5 trees over
|
||
all sixteen cases agree warm against cold with every bound a fraction
|
||
(93.18s, release). The generator still grows pixels, because
|
||
`deferred_generated.rs` already varies that dimension in its relative-bound
|
||
corpus and growing fractions here would move every box in the cold dump for
|
||
overlap; the comment now says that rather than describing an open defect.
|
||
|
||
Four things the sweep **looked at and left**, and one it withdrew:
|
||
|
||
- **`independent_order` compares two lengths by `leftover` where their pixels
|
||
and fractions are equal**, which is only sound while a leftover resolves at
|
||
a nonnegative ratio. Worked through: `allocate` starts at `Ratio::ZERO` and
|
||
breaks before moving when the fixed parts already exceed the room, so `at`
|
||
is never negative and a larger weight is never a shorter length. Recorded so
|
||
the next reader does not derive it again.
|
||
- **`Bound::outside`'s assertion that a floor does not sit over a cap fires
|
||
only for some boxes.** With `min` 100 and `max` 50 it fires where the box is
|
||
under 100 and passes where it is over, because the floor only binds on one
|
||
side. Left: the two ends can be a fraction and a pixel length, so which is
|
||
larger is not a question `set_min_len` can answer, and the box is where it
|
||
becomes one.
|
||
- **A bound has no remover.** `set_min_len`/`set_max_len` only ever write
|
||
`Some`, and clearing one means writing the whole rule -- which drops the
|
||
preferred length, the mirror of the defect fixed above. Left because
|
||
nothing asks for it yet and the preferred length has no remover either; the
|
||
asymmetry is uniform.
|
||
- **`Moves::clear` replaces its arena rather than clearing it.** Raised as
|
||
the shape `RequestArena::reset` deliberately avoids, and withdrawn: that
|
||
one keeps its capacity because it runs every frame and the allocation rig
|
||
checks the steady state allocates nothing, where `Moves::clear` runs only
|
||
when the root changes, next to a rebuild of the whole tree that re-grows it
|
||
anyway. `Arena` also holds an `IdTracker` that a clear has to reset, which
|
||
`Arena::default()` says in one line. Frequency is what separates the two,
|
||
and the code looks the same either way.
|
||
- **`TextEditCtx::apply_event` reads `if undo && let Some(..)` with an `else
|
||
if` after it**, so an undo with an empty history now falls into the branch
|
||
that pushes history. It is equivalent only because an undo command does not
|
||
itself change the text, so the `!=` guarding that push is false. Left as
|
||
correct; noted because the guard is somebody else's invariant.
|
||
|
||
Verified at `cbccfb6`: format, workspace clippy under `-D warnings` with and
|
||
without `layout-diagnostics`, 208 ordinary and 212 diagnostic tests (207 and
|
||
211 before, plus the one this adds), and the cold dump byte-identical to
|
||
`f48e04e` across all **34,986** boxes. The three seed scans were not run:
|
||
nothing here can move a box, which the dump confirms.
|
||
|
||
## Twelfth sweep: the request arena (2026-09-20)
|
||
|
||
Over `05e6ced`, which no earlier round reviewed -- one node type for a
|
||
request, an arena per owner, and `Widgets::edit_bound`. 316 inserted lines
|
||
over four files. Four findings, all in `f48e04e`.
|
||
|
||
- **The one path that copies a request into another was run by nothing.**
|
||
`SizeRequest::join` grafts the other side's nodes into this side's arena,
|
||
which happens only where both sides of a comparison are expressions --
|
||
`a.min(b).max(c.min(d))`, or a `clamp` whose ends are expressions.
|
||
`random.rs` builds only lengths and bounds, and every expression in the
|
||
three deferred corpora and in `cases/deferred.rs` compares an expression
|
||
against a plain length, so nothing reached it: measured, all 206 tests pass
|
||
with a `panic!` in that arm. It is where a missed renumbering would be
|
||
silent, because an operand copied without remapping still names a node that
|
||
exists -- just the wrong one. There is now a fixture at two window widths
|
||
with absolute geometry (`min(1 leftover, 40)` against
|
||
`max(min(2 leftover, 70), 10)`, which is 60 of 90 and 70 of 300), and a
|
||
fourth arm in `deferred_requests_agree_warm_and_cold` that puts an
|
||
expression on both sides across the whole corpus. Both were checked to
|
||
reach the arm by instrumenting it again. The path is correct as written:
|
||
this is a guard, not a fix.
|
||
- **The derived `Debug` the commit rejected was still what every `{:?}`
|
||
printed.** `SizeRequest` grew a `Display` because "a derived `Debug` of an
|
||
arena is not something a tree can be rebuilt from", and `describe` moved
|
||
onto it -- but `Debug` stayed derived, so the four `assert_eq!`s in
|
||
`cases/deferred.rs`, the only place a request is compared, print the arena
|
||
on failure, which is the case a reader has. Measured, `leftover(1).min(40)`
|
||
as derived `Debug` is `Expr(Expr { nodes: Nodes([Node { op: Min, a:
|
||
Linear(LayoutLen { px: 0, rel: 0, leftover: 1 }), b: Linear(LayoutLen { px:
|
||
40, rel: 0, leftover: 0 }), leftover: true }]), root: 0 })` -- 179
|
||
characters for what `Display` writes as `min(1 leftover;, 40 px;)`. That is
|
||
the class the eleventh sweep found one commit earlier and fixed at the call
|
||
site in hand. `Debug` forwards to `Display`, so the reason given for one
|
||
now governs both.
|
||
- **A method that asked nothing of its receiver.** `Nodes::linear(&self, at:
|
||
Operand)` never touched the arena, and `Nodes::leftover` beside it does, so
|
||
the pair reads as though both answers depend on it. It is `Operand`'s
|
||
question, the way `RequestedLen::linear` is `RequestedLen`'s; `combine`
|
||
opens `if let (Some(x), Some(y)) = (a.linear(), b.linear())`.
|
||
- **A closure parameter shadowing what it was called on.** `describe`'s
|
||
`|r| format!("{r}")` sits inside `let rule = |r: &SizeRule|`, so `r` means
|
||
two things four lines apart. It is `ToString::to_string`.
|
||
|
||
Four things the sweep **looked at and left**:
|
||
|
||
- **`RequestedLen`'s `leftover` repeats `Node`'s.** The same bit is stored on
|
||
the node and copied onto every handle made from it. Left because the handle
|
||
is the only form that leaves the arena and `Span` asks it where it has no
|
||
arena to ask -- `lens.iter().any(|len| len.has_leftover())` in
|
||
`position/span.rs` is widget code holding a slice of handles. Removing the
|
||
copy would push arena access into every widget that asks.
|
||
- **Nodes an arena can never reach.** `graft` copies a subtree and then
|
||
`combine` may fold the result away, which would leave the copy unreachable.
|
||
Worked through and it cannot happen: a fold discards an operand only where
|
||
both are `Operand::Linear` or the two are equal, and a grafted operand is
|
||
either a length or a fresh number that no existing node has, so it never
|
||
equals the side it is being combined with. Recorded so the next reader does
|
||
not derive it again.
|
||
- **`RequestArena::reset` reaches into `self.nodes.0`**, the one raw field
|
||
access outside `impl Nodes`. Left: a `clear` method is three lines to save
|
||
a `.0`, and `reset` must clear rather than replace, because the pass's
|
||
arena keeps its capacity across frames -- which is what the allocation rig
|
||
checks.
|
||
- **Three "compare, write, mark redraw" bodies in `Widgets`.** `set_len` and
|
||
`edit_bound` join `set_size_rule`, `set_alignment` and `set_region_node` in
|
||
writing that shape out. Unifying the first two needs a before-and-after
|
||
comparison of the whole rule, which is the clone this commit removed; the
|
||
duplication is the price of not having it.
|
||
|
||
Verified at `f48e04e`: format, workspace clippy under `-D warnings` with and
|
||
without `layout-diagnostics`, 207 ordinary and 211 diagnostic tests (206 and
|
||
210 before, plus the new fixture), the cold dump byte-identical to `05e6ced`
|
||
across all **34,986** boxes, and 400 depth-5 trees in each of the three
|
||
deferred corpora in 200.95s. The corpora were run because one of them
|
||
changed; the three seed scans were not, because nothing here can move a box
|
||
-- a `Debug` impl, a method moved between two types, and tests. `bounds_cost`
|
||
still prints `rule_bytes=40`.
|
||
|
||
## One node type for a request, and no refcount (2026-09-20)
|
||
|
||
`05e6ced`. `SizeRequest` was a second expression shape beside the one the
|
||
layout pass already had, and the `Arc` in it was how that showed. Bryan asked
|
||
the questions in this order: it is not exposed outside widget code, so it must
|
||
not be an `Arc`; why is it not a `Box`; where are the clones and can they be
|
||
references; is the tree duplicating one that exists; and ideally an arena,
|
||
because it is a single type.
|
||
|
||
**What was duplicated.** `SizeRequest` held `Sum`/`Min`/`Max` over
|
||
`Arc<(Self, Self)>`. `RequestArena` held the same three operators as
|
||
`Op` plus `Node { op, a, b }` in a `Vec`, reached through
|
||
`RequestedLen::Deferred { index, epoch, leftover }`. The constant fold was
|
||
written twice -- `SizeRequest::min`/`max`/`Add` and `RequestArena::combine`
|
||
both summed two linears, both resolved a comparison through
|
||
`independent_order`, and both collapsed equal operands -- and
|
||
`RequestArena::import` existed only to walk the first and rebuild it as the
|
||
second, recursively, at every ask.
|
||
|
||
**What it is now.** One `Node`, one `Op`, one fold in `Nodes::combine`, and
|
||
two kinds of owner: a rule's expression holds a small arena for as long as the
|
||
rule lasts, and `RequestArena` holds the pass's. `import` is
|
||
`Nodes::graft`, which copies nodes between two arenas with each length passed
|
||
through a `resolve` closure -- `within_len(base)` when importing, the identity
|
||
when the builder joins two expressions. A node's operand is an `Operand`, a
|
||
number within its own arena; `RequestedLen` is that plus the epoch saying
|
||
which pass numbered it, and is the only form that leaves an arena. The epoch
|
||
is therefore checked once where a handle comes back in (`RequestArena::operand`)
|
||
rather than at every level of the walk it starts, which also catches a stale
|
||
handle at `combine` rather than waiting for `segment`.
|
||
|
||
**No refcount replaces the `Arc`.** `SizeRequest::Linear(LayoutLen)` stays
|
||
inline and only `Expr(Box<Expr>)` allocates, so `size_of::<SizeRule>()` is 40
|
||
before and after -- the arena costs a widget nothing, because nearly every
|
||
rule holds a plain length. The only clones of a request in the framework were
|
||
`SizeRule::at_least` and `at_most`, and both read a rule out of a slot, moved
|
||
one end of its bound, and wrote it back into the same slot;
|
||
`Widgets::edit_bound` does that where it sits, and the two are gone. Every
|
||
read was already a reference (`size_rules` borrows, `deferred` returns
|
||
`Option<&SizeRequest>`, `import` takes `&SizeRequest`), and `WidgetData` is
|
||
not `Clone`. What is left is `src/random.rs`'s shrinker, where a deep copy of
|
||
a two-node expression at shrink time is nothing.
|
||
|
||
`SizeRequest` grew a `Display`, because the shrinker prints a rule and a
|
||
derived `Debug` of an arena is not something a tree can be rebuilt from. An
|
||
expression reads `min(30 px;1 leftover;, 2 leftover;)<0.5 rel;`.
|
||
|
||
**Measured**, medians of three release runs under `perf stat -e
|
||
instructions:u`, each set within 0.005% of its median: `bounds_cost`
|
||
`MODE=cap FRAMES=2000` is 5.665B against 5.743B (**-1.34%**), and
|
||
`revision_cost` resize `ROWS=40 FRAMES=500` is 4.855B against 4.893B
|
||
(**-0.79%**). Small, but consistent and well past the noise floor here.
|
||
|
||
Verified: format, workspace clippy under `-D warnings` with and without
|
||
`layout-diagnostics`, 206 ordinary and 210 diagnostic tests, the cold dump
|
||
byte-identical to `2ac0843` across all **34,986** boxes, all three seed scans
|
||
(400 at depth 5 in 64.24s, 1,000 at depth 6 in 160.35s, 2,000 at depth 4 in
|
||
298.82s), and 400 depth-5 trees in each of the three deferred-request corpora
|
||
in 205.42s. The scans were run because this replaces the allocator's data
|
||
structure, which is the one thing they exist to check.
|
||
|
||
## Eleventh sweep: the built-in bounds work (2026-09-20)
|
||
|
||
Over `2ac0843`, which no earlier round reviewed -- `SizeRule` from an enum to
|
||
a preferred length beside an independent `Bound`, the offer constrained in
|
||
`Placing::ask`, and `MaxSize` removed. 554 inserted lines over 18 files. Eight
|
||
findings, all in `ea1f836`.
|
||
|
||
- **A hint read that answered "cannot say" was counted as no read at all.**
|
||
The bound check moved out of `Painter::size_request` and into
|
||
`Painter::size_hint`, where it returns above `diag::hint_read` and the
|
||
`HintHits`/`HintMisses` bump. So a bounded child was neither a hit nor a
|
||
miss and left no trace event, in the one place the counters exist to watch.
|
||
It is a miss now -- the bound makes the hint `None` rather than returning
|
||
early -- with the reason written on it, since the obvious simplification
|
||
back to an early return silently loses the counter again.
|
||
- **Two loops over both axes, writing the same value.** `draw_widget` grew a
|
||
second `for axis in Axis::BOTH` when the rel-base pin moved up to join the
|
||
bound's, separated only by a comment. One loop; the pin goes first because
|
||
the bound block `continue`s on a share.
|
||
- **A twelve-line comment left describing the wrong line.** The new
|
||
one-liner about combining the ask's holds was inserted between the comment
|
||
about holding an answer to its bound and the code that comment is about.
|
||
- **Three things nothing reads.** `Declared::from_axes` lost its only caller
|
||
with `Widgets::declared_lens`; `Bounds::from_axes` and `SizeRule::declared`
|
||
never had one. `PlaceDesc::from_axes` is the only member of that family
|
||
anything calls, so nothing is left half-complete.
|
||
- **The shrinker printed a rule with derived `Debug`.** `describe`'s
|
||
hand-written printer was replaced by `format!("{r:?}")` and the comment
|
||
saying why it was hand-written deleted. Measured: one axis prints as
|
||
`SizeRule { request: Some(Linear(LayoutLen { px: 0, rel: 0, leftover: 1 })),
|
||
bound: Bound { min: None, max: Some(Len { rel: 0, px: 80 }) } }` -- 130
|
||
characters, twice per widget, in a line that joins every ancestor with
|
||
`" < "`. That is the one function whose stated job is output a tree can be
|
||
rebuilt from. It prints its parts again: `[x:1 leftover;<80 px;,y:-]`.
|
||
- **Five spellings of reading one environment variable.** `bounds_cost` wrote
|
||
three in one function (`var().unwrap_or_else`, `var().is_ok_and(== "1")`,
|
||
`var().ok().and_then(parse)`) where an identical `fn env<T: FromStr>`
|
||
already stood in `layout_dump.rs`, `layout_diagnostics.rs`,
|
||
`revision_cost.rs` and `scenario/mod.rs`. This is the class the tenth sweep
|
||
found one commit earlier and fixed one instance of. It is now one function
|
||
in `tests/rig/`, used by all six rigs, and a switch is `env(NAME, 0_u8) != 0`
|
||
everywhere.
|
||
- **A measurement rig verifying inside its measured loop.** `bounds_cost`
|
||
asserted 128 regions every frame, where `revision_cost` prints its geometry
|
||
once before the loop and asserts nothing inside it. Measured at `MODE=cap
|
||
FRAMES=2000`: 5.780B instructions with the per-frame assertions against
|
||
5.743B without, three runs each, stable to 0.005% -- 0.65%, which is far
|
||
less than it looked and still not layout. The check now runs once on each
|
||
side of the crossing before the loop, and the number is in the comment so
|
||
nobody re-adds it or deletes it for the wrong reason. Its module comment
|
||
also said it compares against "the former wrapper", which this tree no
|
||
longer contains, without saying the other side has to be run at an earlier
|
||
commit; and it documented `MODE` and `REDRAW` but not `FRAMES`, and gave no
|
||
invocation line where every neighbouring rig gives one.
|
||
- **The same fixture built by two tests, and a half-test that could no longer
|
||
decide anything.** `a_bound_holds_what_a_widget_answers` and
|
||
`a_cap_holds_an_answer_that_overflowed_its_box` both built a 250 window
|
||
holding two 200-wide rects in a row under a 300 cap; the first lost its doc
|
||
comment in `2ac0843` and the second kept one. They are one test, with the
|
||
corners assertion folded into the cap arm and a corrected explanation --
|
||
corrected because the old one said a bound "leaves the box alone", which is
|
||
no longer true in general and is true of this fixture only because 300 does
|
||
not bind a 250 box. Separately, the second half of
|
||
`a_cap_attribute_narrows_the_widgets_box` lost the assertion that
|
||
distinguished it when `MaxSize` went (the wrapper measuring 400 while its
|
||
child measured 300), leaving `rect().max_width(300)` at root in a 400
|
||
window -- which is the opening of
|
||
`a_cap_attribute_is_decided_again_on_either_side_of_the_crossing`, verbatim.
|
||
It is now `width(leftover(1)).max_width(300)`, the allocator's path, which
|
||
nothing covered at the root.
|
||
|
||
Two things the sweep **looked at and left**:
|
||
|
||
- **`SizeRule` now holds `Option<SizeRequest>` inline** where the enum held
|
||
`Request(Arc<SizeRequest>)`, so `at_least` and `at_most` clone the request
|
||
to edit a bound. Left because `SizeRequest` is `Linear(LayoutLen)` or an
|
||
`Arc` pair, so that clone is a discriminant copy or a refcount bump, never
|
||
a deep copy. `size_of::<SizeRule>()` is 40 bytes, which the rig prints.
|
||
- **`Widgets::set_len` writes the `SizeRule` literally** where `set_min_len`
|
||
and `set_max_len` go through `SizeRule::at_least`/`at_most`. A matching
|
||
builder was considered and rejected: the two bound helpers exist because a
|
||
bound is half of a pair that has to be preserved, and a request has no
|
||
other half to preserve -- a method would be a name in front of one field
|
||
assignment.
|
||
|
||
Verified at `ea1f836`: format, workspace clippy under `-D warnings` with and
|
||
without `layout-diagnostics`, 206 ordinary and 210 diagnostic tests (207 and
|
||
211 before, less the merged test), 400 depth-5 trees warm against cold in
|
||
64.19s, and the cold dump byte-identical to `2ac0843` across all **34,986**
|
||
boxes. The scan was run because the `size_hint` restructure touches what a
|
||
container records as a dependency; the dump because nothing here was meant to
|
||
move a box.
|
||
|
||
## Tenth sweep: the deferred request system (2026-09-20)
|
||
|
||
Over the part of #19 no earlier round reviewed -- `76aaf06` and `de1eb7e`
|
||
(the bound rules and the rule/widget split), `8780b40` (deferred comparisons)
|
||
and `0e838e9` (the invalidation fix) -- 2,243 inserted lines over 32 files,
|
||
with `core/src/widget/request.rs`, `MaxSize` and the deferred cases new. Ten
|
||
findings, all in `4cb6f68`.
|
||
|
||
- **A bound's held length was looked up a second time, through a value that
|
||
could not say which end.** `Bound::outside` returned `Option<Outside>` and
|
||
the caller asked `Bound::at(outside)` for the length, which `expect`s an end
|
||
the enum says nothing about -- `at(Shorter)` on a bound with no floor
|
||
panics, and only the pairing of the two calls kept it from happening.
|
||
`outside` already had the length in hand (`held`), so it now returns it;
|
||
`Outside` and `Bound::at` are deleted, and the state that could panic
|
||
cannot be written.
|
||
- **A pixel bound pinned the rel base it was not read against.**
|
||
`Painter::measured_request` recorded a rel-base dependency for any bound at
|
||
all, so a measured share under `Min(px(80))` was invalidated by a change to
|
||
a base its answer does not depend on. This is the class `0e838e9` fixed one
|
||
instance of; `Placing::ask` already asked the narrower question inline, so
|
||
the question is now `Bound::has_fraction` and all three callers ask it.
|
||
- **One question about a pair, written out three times.** `[bound.min,
|
||
bound.max].into_iter().flatten().any(|len| len.rel != Rel::ZERO)` appeared
|
||
in `SizeRule::has_fraction` and again in `Placing::ask`, over a pair the
|
||
framework names. It is `Bound::has_fraction` once.
|
||
- **Three buffers reused for their capacity, on an invariant nothing
|
||
stated.** `draw_at` now hands the painter the old draw's `textures`,
|
||
`primitives` and `request_deps`, which is only sound because every path to
|
||
it goes through `remove`, which drains them. A drawing over a buffer that
|
||
still held primitives would record them twice and free them once. A
|
||
`debug_assert` says so where they are taken.
|
||
- **Two names and two spellings for one switch in the rigs.** The walk that
|
||
drops a generated tree's bounds was written out in `layout_dump.rs` and
|
||
`layout_diagnostics.rs`, under `IRIS_DUMP_UNBOUNDED` read with
|
||
`var_os().is_some()` in one and `IRIS_UNBOUNDED` read with the file's own
|
||
`env` helper in the other. It is `Plan::drop_bounds` and `IRIS_UNBOUNDED`
|
||
in both, and the dump's module comment says so.
|
||
- **`Span` wrote its gaps twice and read its allocation three ways.** The gap
|
||
total is `Span::gaps`. In the placement loop `len`, `shares` and "not drawn
|
||
at all" were three separate matches on whether the row was allocated, two
|
||
of them deciding one child's length: one match now gives all three, so the
|
||
allocated and plain rules are read side by side.
|
||
- **`Stack` spelled "the child that sizes it" a second way** in
|
||
`size_request`, with two arms answering `LEFTOVER`, where `draw` resolves
|
||
the same thing once.
|
||
- **`Pad` spelled its own padding a second way.** `Padding::along(axis)` is
|
||
the sum of the two sides, which `draw` adds back and `size_request` insets
|
||
by.
|
||
- **Comments that described something else.** `with_requests` said nested
|
||
painters keep discovery from overwriting its buffers, where the hazard is a
|
||
child *drawn* mid-row; `Painter::allocate`'s doc described the window it
|
||
holds for rather than what it does; `minimum_request` had none;
|
||
`RequestArena::allocate` said "one scope of nonnegative shares", which is
|
||
not this codebase's vocabulary; and `redraw_updates` was left mid-rewrap,
|
||
with "the set" naming a `BTreeSet` that is now a `BinaryHeap`.
|
||
- **A live explanation deleted with the path it was not about.** The comment
|
||
saying a span carries its children's leftover weight whole rather than
|
||
collapsing it per level -- the rule the handoff still lists as wanting
|
||
confirmation -- was replaced by one about discovery. Both paths run; both
|
||
are now described.
|
||
|
||
Two findings of the `as i32` kind were **looked at and left**. The allocator
|
||
narrows i128 prefix sums and i64 segment totals to `Px` with `as i32` rather
|
||
than `fixed::narrow`, which clamps; but `Fixed::add`, `sub` and `mul` all
|
||
wrap deliberately (`MAX` is documented as "compared against, never added
|
||
to"), so wrapping is what the ordinary path does with the same overflow, and
|
||
`narrow` is for ranges. The epoch check in `RequestArena::segment` stays a
|
||
release `assert`: a retained `RequestedLen` would otherwise read a node
|
||
belonging to another widget and answer silently, which is worse than the
|
||
compare it costs. The negative-weight check beside it is a caller bug like
|
||
`div_int`'s and is now a `debug_assert`.
|
||
|
||
Two things the sweep did not change and somebody should decide:
|
||
|
||
- **The generator's leftover density halved** when `76aaf06` grew bounds:
|
||
`Sow::rule` draws `LEFTOVER` 1 time in 8 where `Sow::len` drew it 1 in 4,
|
||
and `Free` 2 in 8 where it was 1 in 2. The seed scans are the main defence
|
||
for share logic, so the corpus now exercises it half as often. Restoring it
|
||
costs a new dump baseline and three fresh scans.
|
||
- **`MaxSize` holds `x` and `y` with a hand-written axis match**, where the
|
||
framework names every other pair (`Bounds`, `Declared`, `Size`) and indexes
|
||
it. Left because the fields are the constructor surface `max_width` and
|
||
`max_height` build, and `impl_axis_index!` on a widget reads oddly; it is
|
||
three call sites inside one file.
|
||
|
||
Verified at `4cb6f68`: format, workspace clippy under `-D warnings` with and
|
||
without `layout-diagnostics`, 197 ordinary and 201 diagnostic workspace
|
||
tests, the cold dump byte-identical to `0e838e9` across all 34,986 boxes, and
|
||
all three seed scans (400 at depth 5 in 69.02s, 1,000 at depth 6 in 174.87s,
|
||
2,000 at depth 4 in 330.39s). The scans were run because the rel-base fix
|
||
changes what is invalidated, which is exactly what warm-against-cold checks.
|
||
|
||
## Performance sweep of #19 (2026-09-20)
|
||
|
||
`0e838e9`, by Codex.
|
||
|
||
The 5.89% text-resize increase at `8780b40` was real, but was not extra
|
||
shaping: `revision_cost` at both `de1eb7e` and `8780b40` drew 168 widgets,
|
||
rendered seven texts and broke seven layouts per resize frame. Sampling the
|
||
latter put most CPU work in relocating retained primitives. This is a CPU
|
||
layout/recording fixture, not a GPU text-rendering benchmark.
|
||
|
||
**Discarded discovery invalidated things it did not decide.** Both
|
||
`Painter::size_request` and `measured_request` kept dependencies and pinned
|
||
the rel base even when they discarded the discovered answer and used the
|
||
ordinary measured size. Repainting one unchanged paragraph consequently
|
||
redrew its enclosing span and remapped the other 39 paragraphs. Discovery now
|
||
keeps those dependencies only when its answer is used; the measured path
|
||
already records the size it reads. Used hints still register request readers:
|
||
adding a cap to a previously unbounded hinted share must reach its allocator.
|
||
The same edit is checked for an unhinted share too. `Widgets::size_rules` now
|
||
borrows its rules instead of cloning both axes at every lookup; callers that
|
||
need an owned snapshot clone explicitly.
|
||
|
||
All instruction measurements below are medians of nine executable runs with
|
||
`perf stat -e instructions:u`, release builds without diagnostics. Every set
|
||
kept all nine readings within 2% of its median. Totals include initialization
|
||
and the cold frame. Resize uses 40 rows and 1,000 frames; the other phases use
|
||
40 paragraphs in a scroll and 2,000 frames. Edit alternates a short suffix,
|
||
rather than growing the workload every frame. `SWEEP=1` cycles 256 widths;
|
||
the default toggles two widths. Billion retired instructions:
|
||
|
||
| Workload | Before deferred requests (`de1eb7e`) | Incoming (`8780b40`) | After this fix | After vs before deferred |
|
||
| --- | ---: | ---: | ---: | ---: |
|
||
| Text resize, two widths | 9.340 | 9.890 | 9.432 | +0.99% |
|
||
| Text resize, width sweep | 7.044 | 7.478 | 7.131 | +1.23% |
|
||
| Unchanged paragraph repaint | 0.376 | 3.010 | 0.370 | -1.58% |
|
||
| Paragraph edit | 3.485 | 3.607 | 3.492 | +0.22% |
|
||
| Scroll | 0.428 | 0.430 | 0.428 | +0.10% |
|
||
| Idle | 0.306 | 0.306 | 0.306 | +0.04% |
|
||
|
||
The repaint regression was eightfold in these process totals; diagnostics
|
||
now show one text draw and no span draw. The remaining roughly 1% resize
|
||
cost is discovery around content that still needs measurement. It is not
|
||
necessary to weaken wrapping, overflow or bounds correctness to recover the
|
||
regression. Differences below about half a percent should not be ranked:
|
||
rebuilding changes instruction counts too.
|
||
|
||
The full PR is substantially cheaper than its upstream base `ca2b4b2` in
|
||
these fixtures: that base takes 124.072B instructions for the two-width resize,
|
||
13.516B for repaint, 14.155B for edit and 13.517B for scroll. Its geometry also
|
||
differs (paragraphs run past the 900-pixel output), so this is a whole-version
|
||
comparison, not equal-work evidence for a single optimization. The fixture
|
||
was identical apart from spelling the fixed width `Len::abs(40.0)` there and
|
||
omitting unavailable diagnostic calls.
|
||
|
||
**The generated trees are not all faster.** At depth 8, 1,000 frames, seeds
|
||
1 and 13, the incoming branch's bounded resize workloads take 0.561B and
|
||
4.297B instructions; the fix takes 0.539B and 4.016B. The pre-deferred branch
|
||
uses 0.359B and 1.898B, but does different work: seed 13 draws 67 widgets per
|
||
resize there and 170 with deferred requests, and even the active cold tree
|
||
changes. Those are not equal-work comparisons. Removing intrinsic bounds
|
||
with `IRIS_UNBOUNDED=1` isolates the ordinary path: seed 1 resize is 0.359B
|
||
before vs 0.363B after, and seed 13 is 1.897B vs 1.296B. Unbounded seed 13's
|
||
multi-widget repaint still rises from 0.980B to 1.208B. A consumed request's
|
||
conservative invalidation remains a cost; eliminating it would require
|
||
checking whether the request changed before invalidating its allocator.
|
||
This sweep does not establish a universal speedup or make that cost inevitable.
|
||
|
||
**Storage and limits.** The forced-redraw plain and clamped allocation rigs
|
||
still allocate zero times after warm-up. A single 2,000-row stress run
|
||
(10,001 widgets, all retained) settles at 133,400 KiB RSS before deferred
|
||
requests, 141,912 KiB incoming and 138,820 KiB after this fix. These process
|
||
RSS snapshots show the retained bookkeeping cost, not a per-widget allocation
|
||
size or a leak. The spare glyph store still copies text when keeping a prior
|
||
width; its 128-entry bound does not bound bytes. The allocator visits expression
|
||
nodes at each crossing; many distinct caps can require quadratic work. Neither
|
||
case should be mistaken for the small fixed-width text-update workload above.
|
||
|
||
**GPU work is a separate tradeoff.** The first attempt detected RADV on the
|
||
RX 7900 XT but could not allocate its 64 MiB device buffer (`No space left on
|
||
device`). After allocation became available, both rigs ran successfully.
|
||
Across three timestamp runs of `chain_cost` (200,000 two-pixel rectangles),
|
||
depth 1 took 48–78 µs, depth 4 took 76–79 µs, depth 16 took 145–150 µs, and
|
||
depth 64 took 430–443 µs. The first run's depth-2 result was anomalous; the two
|
||
repeats agreed on approximately 78 µs for depths 1–4. Chain depth counts opted-in
|
||
movable regions, not every widget. Deep chains have a real GPU cost, but 200,000
|
||
small quads through 64 movable ancestors is a stress case, not a measurement
|
||
of a normal paragraph or a phone. The CPU recording-only `draw_cost` rig also
|
||
passed; its wall times under concurrent load are not used as regression evidence.
|
||
No renderer or shader behavior changed in this performance fix.
|
||
|
||
Verification is recorded in `HANDOFF.md`. The app's Iris pin is unchanged.
|
||
|
||
## One ask, the root's included (2026-09-20)
|
||
|
||
`0d03267`. `root_layout` read the root's declared lengths against the window
|
||
while every other widget's box came of `Painter::widget_at`, where a rule of
|
||
the widget's own can take the box past what its parent offered. Bryan asked
|
||
for the two paths unified, and ruled out doing it by putting a widget above
|
||
the root: a root sized in pixels would make every fraction under it a fraction
|
||
of a px length, which pins the whole tree to those pixels and re-asks it on
|
||
every resize.
|
||
|
||
`Placing::WINDOW` is the unification -- the full output, fractions of the full
|
||
output, no move entry and no mask -- and `Placing::ask` is the one place a box
|
||
is decided, called by the painter, by a local redraw, and by the root's first
|
||
draw. What is left of the root's own path is the bookkeeping a widget with no
|
||
parent keeps.
|
||
|
||
Two things fell out. `DrawInfo::asked` is now the place the parent offered
|
||
rather than the place the ask came to, so a local redraw re-decides a rule
|
||
instead of re-reading its decision; the two were the same until a rule could
|
||
move the box. And the root's `is_region_node` is read, where the old path
|
||
passed `false`.
|
||
|
||
The comparison a rule makes is kept on the widget asked about rather than on
|
||
the asker. A window range means the same thing at either end of an ask and
|
||
`in_parent` passes one up unchanged, so the asker still ends up holding it
|
||
through the child's drawing -- and the root, which has no asker, needs nothing
|
||
of its own, since `resize` already checks its record.
|
||
|
||
## Bounds are a rule that reads its box, and the oracle refuses one (2026-09-20)
|
||
|
||
On the branch `layout/bounds` (`76aaf06`), not in #19. `SizeRule::{Min, Max,
|
||
Clamp}` is the capability `MaxSize` had on the app's pin and nothing on this
|
||
branch has. Every hand-written test passes, including a capped scroll taking
|
||
its viewport from the cap; the 400-seed depth-5 scan does not.
|
||
|
||
The finding is general and worth keeping whatever is decided. A bound is the
|
||
first rule whose effect depends on the box its parent gives it, and the
|
||
retained machinery hands a widget a box by paths that never ask it again
|
||
(`place_in` from a re-placing parent, `reposition` after a parent's box moved).
|
||
A decision made when the box was one length survives into a box of another, so
|
||
warm and cold disagree about a tree they agree on structurally. Four readings
|
||
measured over 400 seeds at depth 5:
|
||
|
||
- decided at every ask and kept: seeds 291, 1, 120, 178, 64 differ.
|
||
- re-decided at `place_in` as well: seeds 1, 362, 188, 254, 156, because that
|
||
path's box is the one the answer chose, not the one the widget was asked in.
|
||
- skipping a place the parent decided outright, which is the rule the share
|
||
follows: worse, since the same widget then gets two decisions by two paths.
|
||
- the bound as an answer rule only, leaving the box alone: seeds 4 and 196 --
|
||
much the closest.
|
||
|
||
The share is the one existing rule of this kind, and it is stable for two
|
||
reasons that do not generalise: `place_at` re-asks a child whose rel base it
|
||
narrows, and a share that binds is baked into the retained place as a `Sized`
|
||
length. A bound that binds is a length of the rel base, and `Sized` cannot say
|
||
"this slot, narrowed" for a `Within` place.
|
||
|
||
Neutering the bounds in the generator while leaving its random draws in place
|
||
puts the same shapes back to green, so the divergence is the bounds and not
|
||
the new trees. Two findings from the branch are worth having whatever happens
|
||
to it: a rule that is a fraction must pin its rel base, which `Exact` was
|
||
already missing for bounds-shaped reasons; and `widget_trait!` dropped
|
||
attributes, so no method it defines could carry a doc comment.
|
||
|
||
**Settled (Bryan, 2026-09-20): a rule holds the answer, a widget holds the
|
||
box.** `de1eb7e`. `SizeRule::{Min, Max, Clamp}` holds the length a widget
|
||
answers and leaves its box alone, which is stable because an answer is a
|
||
function of what the widget drew. `MaxSize` is the box version and is a widget
|
||
because a widget is drawn again whenever its own box changes, so the
|
||
comparison is made where the answer can be kept. `.max_width`/`.max_height`
|
||
are that widget; `.min_width`/`.min_height` stay a rule. All three seed scans
|
||
pass. The capped scroll -- 400 px of content under a 100 px cap, a 100 px
|
||
viewport and 300 to scroll -- is the app's `MaxSize` capability, and it comes
|
||
back through the widget.
|
||
|
||
One hole is left and is **older than bounds**: `place_at` hands a parent a
|
||
retained answer without checking that the answer still holds for the rel base
|
||
that place gives, so a fraction resolved against one rel base survives into
|
||
another. A fraction in a bound is the first thing to reach it (seeds 4 and 196
|
||
at depth 5), which is why the generated trees grow bounds in pixels only, said
|
||
where the tree is grown. An `Exact` rule that is a fraction can reach it too.
|
||
The fix is a holds check at the re-place site, not anything about bounds.
|
||
|
||
## Images in the generated trees (2026-09-20)
|
||
|
||
`Image` is the only widget here whose size hint is a length in pixels, so it is
|
||
the only one that exercises a rule beside a hint -- which is what the two
|
||
findings above are about, and why neither was reachable from a generated tree.
|
||
Bryan asked for one in the trees, as a committed 64x64 checkerboard reusing one
|
||
texture handle. `2dba90b`: `Kind::Image` is a fifth leaf drawn one time in five,
|
||
stepping to a plain rect under the shrinker; the picture is
|
||
`src/assets/checkerboard.png`, purple and black in 8 px cells, included rather
|
||
than opened; and a `TextureHandle` is a counted reference, so the first image in
|
||
a tree uploads and the rest clone it. Measured: seed 1 at depth 4 grows 13
|
||
images and holds 1 texture, and `a_tree_of_images_uploads_one_texture` asserts
|
||
it.
|
||
|
||
The cost is that a seed no longer names the tree it used to, so the corpus in
|
||
`generated.rs` is a corpus and not a set of regression cases -- 20 and 86 no
|
||
longer grow the trees whose defects they caught. Both live on as shrunk fixtures
|
||
in `unsettled.rs`, which are trees rather than numbers, and both files now say
|
||
so. New dump baseline: **34,571** boxes over the 400 depth-5 trees. All three
|
||
scans pass over the new trees (400 at depth 5 in 62.79s, 1,000 at depth 6 in
|
||
160.20s, 2,000 at depth 4 in 299.58s).
|
||
|
||
## Ninth sweep: the widget vocabulary, and the eighth sweep's fix (2026-09-20)
|
||
|
||
Over the part no earlier round named -- `src/widget/trait_fns.rs` and
|
||
`wrapper.rs`, `core/src/widget/widgets.rs`, the `util` additions, `examples/`
|
||
and the two manifests -- and once more over `77ed7a2`, which was the eighth
|
||
sweep's own commit. Eight findings, all in `c2b8bf8`. The cold dump over 400
|
||
depth-5 trees is byte-identical to `77ed7a2` across all **34,488** boxes, and
|
||
all three seed scans pass (400 at depth 5 in 63.27s, 1,000 at depth 6 in
|
||
160.45s, 2,000 at depth 4 in 302.52s).
|
||
|
||
**A hint overrode a rule.** `Widgets::declared_lens` asked
|
||
`rules[axis].declared()` and fell through to the widget's own `size_hint`
|
||
whenever that answered `None` -- which it does for a share, because a share is
|
||
not a declaration. So a widget carrying `width(leftover(1))` *and* hinting a
|
||
pixel length of its own was given a box of the hint, against the rule and
|
||
against the comment inside the function ("a hint still narrows the box where no
|
||
rule does"). `Painter::size_hint` spells the same rule-else-hint step three
|
||
hundred lines up and gets it right, with the reason written on it; both read
|
||
`Widgets::exact_len` now, and `declared_lens` is the part of its answer that
|
||
needs nobody to divide it.
|
||
|
||
`Image` is the only widget in the repository whose hint is a declared length
|
||
(every other hints `LEFTOVER`, whose `declared()` is `None` either way), and
|
||
neither the tests nor the generator builds one, so nothing here could reach the
|
||
difference -- which is why the dump is unchanged, and why
|
||
`a_share_rule_beats_the_widgets_own_pixel_size` builds a widget of its own. It
|
||
records the box it was asked in: 400 with the rule and 50 without, and 50
|
||
either way at `77ed7a2`. The alternative reading -- a hint narrows even under a
|
||
share rule -- would mean changing that comment instead, and is Bryan's to
|
||
prefer if he does.
|
||
|
||
**What a leftover means where nothing divides it** (Bryan, 2026-09-20, on the
|
||
finding above). A leftover under a parent that does not divide is still a
|
||
leftover, acting as a minimum: where the `px` and `rel` parts come to less than
|
||
the box it fills the rest, and where they come to more they overflow as normal.
|
||
The length is `max(box, px + rel*box)` -- which is the same `max` he gave for
|
||
`Scroll`'s content length on 2026-09-18, generalised to every non-dividing
|
||
parent.
|
||
|
||
Measured at `c2b8bf8`, a probe recording the box it is asked in, in a 400 px
|
||
window, under `.wrapper()` against a one-child span:
|
||
|
||
rule nothing divides a span divides
|
||
leftover(1) 400 400
|
||
px(50) + leftover(1) 400 400
|
||
px(500) + leftover(1) 400 500
|
||
rel(0.5) + leftover(1) 400 400
|
||
px(500), no leftover 500 500
|
||
|
||
One row disagrees. A leftover whose fixed part is longer than the box loses the
|
||
overflow where nothing divides it. The span is right and says so in place --
|
||
"One that also asked for pixels or a fraction keeps those and overflows" -- and
|
||
`only_a_pure_leftover_child_disappears_when_nothing_is_left` asserts it at
|
||
100..120 of a 100 px row. The cause is that `LayoutLen::declared` refuses to
|
||
answer for anything carrying leftover weight, so the non-dividing path never
|
||
learns the fixed part and falls back to the offer. The same length without the
|
||
share does overflow (drawn -50..450, its alignment centring it), so what
|
||
swallows it is the share and not the overflow.
|
||
|
||
**Fixed in `b295c8b`**, after Bryan asked for it if it was cheap and pointed out
|
||
that the machinery is a minimum size of `rel(1.0)` and should share a path with
|
||
one. What made it cheap was saying it as the place the parent gives rather than
|
||
as a declaration: where the fixed part is the longer, `widget_at` hands the
|
||
child `fixed.as_desc().fills()`, which is what a declared length already comes
|
||
to, and `active.placed` keeps it -- so nothing about `Declared`, `ActiveData` or
|
||
the resize path had to change. A place that is already the child's placement is
|
||
skipped, because a parent that divides has given the share whatever it was owed;
|
||
without that guard a span's slot was re-placed and its child moved from 0..500
|
||
to -50..450.
|
||
|
||
The comparison is one operation now, `Painter::longer_than`, and both callers
|
||
share it: the span's room for the shares it divides, and a share past the box it
|
||
was given. It narrows the widget's window range where the span replaced it,
|
||
since a comparison the framework makes on an arbitrary parent's behalf is one
|
||
more reason its drawing holds rather than the only one. `SizeRule::Min` of
|
||
`rel(1.0)` is the same operation and belongs on the same path when it lands.
|
||
|
||
Two things were tried and dropped. An escalation for a changed rule
|
||
(`shares_past_a_length`) turned out to buy nothing: the reported size is the
|
||
rule resolved, so a changed rule changes the answer and the parent refuses its
|
||
own drawing -- the tests pass without it. And `Holds::crossing`, a named
|
||
constructor for the range, had one caller and read better spelled there.
|
||
|
||
**The root still reads the old way.** `root_layout` asks
|
||
`Widgets::declared_lens` with no painter, so a mixed share on the root is the
|
||
window rather than the longer of the two. It would mean the comparison in a
|
||
second place, and the root's box is the window and is relaid out on every
|
||
resize; nothing here puts a share on a root.
|
||
|
||
**Marking a widget for redraw had no name.** Twenty-one sites under `tests/`
|
||
said it as `widgets_mut().get_dyn_mut(id);` with the widget thrown away: five
|
||
with a `let _ =` in front, one with a comment explaining what the line was for
|
||
("taking mutable access is the ordinary content-change signal"), and one inside
|
||
a local function already called `mark`. The framework's own word for it is in
|
||
its comments -- "marked for redraw" -- and `Widgets::mark_for_redraw` is now
|
||
the method. `revision_cost.rs` keeps the long spelling and says why in place,
|
||
since it is deliberately in the API subset an old worktree also has.
|
||
|
||
**`assert_same_regions` could not see the defect the eighth sweep had just
|
||
fixed.** It zips the warm and cold id lists, so a list naming one widget twice
|
||
compares fewer boxes than it lists and reports nothing. It now rejects a
|
||
repeated id and two lists of different lengths, which is the same check applied
|
||
to the whole class rather than to the four fixtures that had it wrong -- and it
|
||
verifies that round's claim about the other nine: all eighteen cases pass.
|
||
|
||
**Bare pairs where the framework has named ones.** `random.rs`'s `Lens` and
|
||
`Aligns` were `[Option<LayoutLen>; 2]` and `[Option<AxisAlign>; 2]`, read as
|
||
`[0]`/`[1]` and zipped against a hand-written `[Axis::X, Axis::Y]` in two rigs.
|
||
They are `SizeRules` and `Align`, which is what the framework calls those
|
||
pairs; `Align` took the `Index<Axis>` every other per-axis pair on this branch
|
||
has, and `RegionAlign::from` does the "an axis left out is centred" step both
|
||
rigs were spelling per axis. The three sites that wrote the axis pair out say
|
||
`Axis::BOTH`, which the layout core already says ten times. The plan's
|
||
`Debug` is why `Align` gained one.
|
||
|
||
**Forty-five lines nothing references.** `BothAxis<T>`, `AxisT`, `XAxis` and
|
||
`YAxis` -- a const trait, two marker types and three accessors -- have no user
|
||
anywhere in the workspace, and are the mechanism `impl_axis_index!` replaced,
|
||
in the very file this branch took `Vec2::axis`/`axis_mut` out of. Deleted as a
|
||
drive-by in a block the branch was already rewriting, the way `MASK_NONE` was;
|
||
drop it if the scope matters more. They pre-date the PR, so a plain "is this
|
||
name used" scan over the diff does not surface them.
|
||
|
||
**One word for two things.** `Wrapper` (the widget) arrived on this branch
|
||
beside core's `WidgetWrapper` (a dynamic borrow guard), both in the prelude.
|
||
The alias had two uses in one file and `DynBorrower<dyn Widget>` is what they
|
||
are, so it is gone rather than renamed. `Wrapper::new`, `Wrapper::empty` and
|
||
its hand-written `Default` were three names for one value, two unused.
|
||
|
||
**Smaller.** `Arena::get_mut` was the only `pub(crate)` among `pub` siblings on
|
||
a public type. `Selector` rounded the pointer onto the pixel grid in order to
|
||
add two values already on it, losing the precision the platform gave it for
|
||
nothing -- the step between the two regions is taken on the grid instead, which
|
||
also makes it agree with `Selectable`, the other caller of `select`. And the
|
||
two `debug` profile settings now carry their reason where the next reader
|
||
looks: `profile.dev`'s was added in a commit about renaming `rest` and
|
||
explained nowhere, and `profile.test`'s only in the message of the commit that
|
||
made the tests one target.
|
||
|
||
### Tripped a rule and left as it stands
|
||
|
||
- `examples/random.rs` is a viewer for the fuzz generator rather than a
|
||
demonstration of a feature, which is what an example is for here. Left:
|
||
`scripts/run-headless.sh` opens an example by name, so this is how a
|
||
generated tree is put on a screen at all.
|
||
- It also carries a fifth copy of the six-line `env` helper, on top of the four
|
||
under `tests/`. Same answer as the eighth sweep gave: sharing it means a new
|
||
file for six lines of `std`.
|
||
- `IRIS_SEED`/`IRIS_DEPTH`, `IRIS_GENERATED_SEED(S)`/`IRIS_GENERATED_DEPTH` and
|
||
`IRIS_DUMP_SEEDS`/`IRIS_DUMP_DEPTH` are three names for two knobs. Left: the
|
||
prefixes are what lets one shell set a rig's seed without changing another's,
|
||
and the two rigs that grow the same tree from the same number do share the
|
||
unprefixed pair.
|
||
- `set_size_rule` takes a `SizeRule` while `set_size_rules` takes
|
||
`impl Into<SizeRule>` per axis, so seven callers write `SizeRule::Exact(len)`
|
||
where two write `Some(len)`. Left: widening the single-axis one makes
|
||
`harness.rs`'s `len.into()` ambiguous between two conversions. The doc that
|
||
called it "for a caller holding a pair" -- which it is not, since it takes
|
||
two values -- now just says both axes at once.
|
||
- `Wrapper::set` is `replace` with the result dropped, and both have a caller
|
||
in `examples/tabs`. Left: it is `Option::insert` beside `Option::replace`,
|
||
and the tab bar wants each.
|
||
- `Masked`'s size comment credits `Scroll` with the same reasoning rather than
|
||
restating it. Left: the reason it gives is its own ("it clips what is inside
|
||
to that box"), and the cross-reference is to a design parallel, not an API.
|
||
- The nine `RegionAlign`/`CardinalAlign` constants include four nothing names
|
||
(`TOP_CENTER`, `CENTER_RIGHT`, `BOT_CENTER`, `V_CENTER`). Left: they are one
|
||
vocabulary of nine positions and six cardinals, and deleting the members
|
||
nobody has needed yet is the rule written on one member of a set.
|
||
|
||
## Eighth sweep: the tests, and the seventh sweep's fix (2026-09-20)
|
||
|
||
Over the part no earlier round named -- the 6,300 lines under `tests/`, which
|
||
is more than half of what #19 adds -- and once more over `f8aa0c5`, which was
|
||
the seventh sweep's own fix and so unreviewed. Seven findings, all in
|
||
`77ed7a2`. No library code changed: the cold dump over 400 depth-5 trees is
|
||
byte-identical to `f8aa0c5` across all **34,488** boxes, and the seed scans
|
||
have nothing to find, since nothing that decides a layout moved.
|
||
|
||
That count is not the 34,492 the four rounds before this one recorded, nor
|
||
the 34,490 `77ed7a2`'s own message gives. The dump prints eight lines that
|
||
are not a box -- cargo's two, libtest's four, and two blanks -- so a `wc -l`
|
||
of the run comes to 34,496 and any partial filter lands somewhere between.
|
||
The boxes are the lines matching `^[0-9]+ [0-9]+ `, and there are 34,488 of
|
||
them at both `f8aa0c5` and `77ed7a2`. What every round actually established
|
||
is still true, since each diffed two dumps rather than trusting a count.
|
||
|
||
**A shrunk fixture that names one widget three times.** `width`, `sized` and
|
||
`align` set a rule on the widget they are handed and give its own id back;
|
||
only `pad` and `wrapper` make a new widget. So in
|
||
|
||
let wrapped = wtext("Wrapping shapes").size(16).wrap(true).add(&mut h.rsc);
|
||
let sized = wrapped.width(76).add(&mut h.rsc);
|
||
let aligned = sized;
|
||
|
||
all three names are one text, and all three went into the vector of ids the
|
||
case compares warm against cold. Measured: `plant` and `plant_fixed` list six
|
||
ids and hold four widgets, `plant_pair` lists four and holds three,
|
||
`plant_scrolled` lists eight and holds seven; the other nine fixtures are
|
||
honest. So four cases check fewer boxes than they say, and the doc comment of
|
||
each quotes the inflated count as the size of the tree the shrinker reduced
|
||
to -- which is the number a reader uses to judge whether a case is still the
|
||
minimal one. The names are gone and the counts are what the fixtures build.
|
||
`trace_unsettled.rs` carries a copy of two of these fixtures and had the same
|
||
aliases.
|
||
|
||
The check that mattered here is that a rebuilt fixture is the same tree: a
|
||
regression test whose fixture quietly changed still passes and no longer
|
||
covers its defect. Each was diffed against its old self -- same widget slots,
|
||
same regions, for both settings of `swapped`.
|
||
|
||
**A helper at the top of the file, and seven copies of its body below it.**
|
||
`assert_same_regions` collects every widget whose box moved and reports them
|
||
together. Six tests call it; seven more spell the eight lines out instead,
|
||
byte for byte. They call it now, and it took `#[track_caller]` so the panic
|
||
names the case rather than the helper.
|
||
|
||
**The GPU rigs' adapter probe, written twice.** `draw_cost` and `chain_cost`
|
||
each held a `config` (identical) and an adapter probe (identical but for the
|
||
feature it asks for and what it returns). The probe leaks its `Instance` on
|
||
purpose -- a Vulkan loader may unload the driver as a test thread exits --
|
||
and only `draw_cost` said so, with `chain_cost` referring the reader to it.
|
||
Both come from `tests/gpu/mod.rs` now, shared through `#[path]` the way
|
||
`scenario/mod.rs` already is, with the justification on the thing it is
|
||
about.
|
||
|
||
**The mask a widget is clipped by, resolved three times**, two of them a
|
||
byte-identical closure defined inside a loop. `mask_bounds` takes the
|
||
`MaskIdx` rather than the widget, because the third site reads the slot it
|
||
saved before the frame: that a redraw keeps the same slot is exactly what it
|
||
is checking, and a helper that looked the slot up again would have made that
|
||
assertion pass for the wrong reason.
|
||
|
||
**A field nothing reads.** `Layered::_revision` existed to be incremented, to
|
||
mark its widget dirty. Two tests in the same file already do that with
|
||
`widgets_mut().get_dyn_mut(id)`, which is the idiom `tests/scenario/mod.rs`
|
||
uses as well. The underscore was hiding the dead-code warning that would have
|
||
said so.
|
||
|
||
**A claim the test below it does not make.** `plan.rs` said "Every
|
||
simplification is strictly smaller, so taking them in turn reaches a fixed
|
||
point instead of circling", and then asserted `small.size() <= node.size()`.
|
||
Measured: 53 of one tree's 101 simplifications keep the widget count, because
|
||
`Plan::size` counts widgets and dropping an alignment or stepping `Wrapped ->
|
||
OneLine` does not change it. The assertion is the right one and the claim was
|
||
not. What actually rules out circling is that those steps are one-way too --
|
||
a `Some` becomes a `None`, a kind steps down a ladder with no way back up --
|
||
and the comment now says that. The test is `no_simplification_of_a_plan_is_larger_than_it`.
|
||
|
||
**Numbers and lines that had gone stale.** `generated.rs` says "Eight that
|
||
have never failed" and "the ten the others check"; both said one fewer,
|
||
having been written when `SEEDS` had nine entries and not updated when
|
||
`d8ae9c3` added seed 20. The `should_panic` scroll test ended in an
|
||
`h.frame()` that cannot run, because `Harness::set_root` lays the tree out
|
||
and is where the panic comes from -- verified by deleting it. Two
|
||
`drop(tree)` calls sat at the end of their own scope.
|
||
|
||
### Tripped a rule and left as it stands
|
||
|
||
- `env` is written four times under `tests/` (`layout_dump`,
|
||
`layout_diagnostics`, `revision_cost`, `scenario`). `revision_cost`'s is
|
||
deliberate and documented: that file is kept in the API subset an old
|
||
worktree also has, so it can be dropped in and measured there, and a
|
||
`#[path]` module would break that. Sharing the other three means either a
|
||
new file for six lines of `std` or pulling `scenario`'s 494 lines into two
|
||
more binaries.
|
||
- `determinism.rs`'s `BranchesOnMeasurement` is `iris::random::Branch` with
|
||
the same four fields and nearly the same body, and `unsettled.rs` beside it
|
||
uses the real `Branch`. The difference is load-bearing: the copy states no
|
||
contract, so the framework must re-ask it at every width, which is the
|
||
whole point of a test about whether a re-measure branches the same way.
|
||
`Branch` also has a `size_hint` that changes how a span treats it.
|
||
- Three tests call `h.frame()` immediately after `h.set_root`, which already
|
||
frames. Unlike the `should_panic` one, these are reachable and assert that
|
||
a settled second frame adds no draws. Left; the redundancy reads as noise
|
||
but removing it removes a check.
|
||
- `primitive_bounds` and `primitive_masks` share the walk from a widget's
|
||
primitives to their instances and differ only in the field they take.
|
||
Left: naming the intermediate means exporting the instance type into the
|
||
test, which is more coupling than the four shared lines are worth.
|
||
- `Harness::replay` ignores each sample's `t_ms`, and the parser rejects
|
||
time running backwards with a comment about the wait between samples --
|
||
which is `replay-touch`'s behaviour, not the harness's. Left: the parser
|
||
is shared by both replays and the rule is the real one, but a harness
|
||
gesture has no timing, so nothing in-process can measure a fling velocity.
|
||
- `layout_diagnostics.rs`'s `report` takes `_harness` and reads it, because
|
||
it is only used under `layout-diagnostics`. An underscore on a parameter
|
||
the body uses is backwards, but the alternative is a `cfg_attr`ed allow.
|
||
|
||
## Seventh sweep: the rigs, `Fixed`, and the sixth sweep's fix (2026-09-20)
|
||
|
||
Over what no earlier round named -- `src/random.rs` and `tests/scenario/`,
|
||
`core/src/fixed.rs`, `scripts/run-headless.sh` -- and once more over
|
||
`b7b8d09`, which was itself unreviewed because it was the sixth sweep's own
|
||
fix. Six findings, all in `f8aa0c5`. The cold dump over 400 depth-5 trees is
|
||
byte-identical to `b7b8d09` across all 34,492 boxes, and all three seed scans
|
||
pass (400 at depth 5 in 62.75s, 1,000 at depth 6 in 162.37s, 2,000 at depth 4
|
||
in 305.25s).
|
||
|
||
**A scroll still asking a question it has already answered.** The sixth
|
||
sweep found `anchor != Px::ZERO` in `Scroll`'s content test and removed it;
|
||
the operand beside it is the same defect and survived. `self.content_len` is
|
||
`answer_px.max(container_len)`, so content that fits has nothing to scroll
|
||
through, and `update_amt` on the line above clamps `amt` to
|
||
`content_len - container_len`, which is then zero. So in
|
||
|
||
let content = match self.amt != Px::ZERO || self.content_len != self.container_len {
|
||
|
||
the first disjunct can never decide the match: `amt != ZERO` implies
|
||
`content_len != container_len`, which the second already tests. It is now
|
||
`match self.content_len > self.container_len`, the exact complement of the
|
||
`content_len <= container_len` that guards the contract fifteen lines above.
|
||
Verified by asserting the implication in place and running the whole suite,
|
||
the scrolling tests included; it held. This is the lesson of "the fixes a
|
||
review produces are themselves unreviewed code" arriving on schedule -- one
|
||
round's fix left the same mistake in the expression it was editing.
|
||
|
||
**Things nothing reads, in the new arithmetic.** `Fixed::to_scale` converts a
|
||
value between two fixed-point grids, and `shift_round` exists only to serve
|
||
it. Both arrived on this branch; the only caller either has ever had is
|
||
`a_coarser_grid_rounds_and_a_finer_one_does_not`, the test written for them.
|
||
Every other `Fixed` method has real callers (checked one by one), and nothing
|
||
needs a grid conversion: `ratio`, `mul` and `div` already cross grids where
|
||
layout has to. All three deleted. `to_scale` was also the one operation in
|
||
the file that could overflow silently without documenting it -- going to a
|
||
finer grid is `self.0 << (TO - SHIFT)` and the test only ever went coarse,
|
||
fine, coarse with a value small enough to survive the trip.
|
||
|
||
**`Len` arithmetic written a component at a time.** `Len::align` built a
|
||
`Len` whose `px` is always `Px::ZERO`, then added to and subtracted from both
|
||
of its components by hand:
|
||
|
||
start: Len::from_parts(at.rel.sub(self.rel.mul(rel)), at.px.sub(self.px.mul(rel))),
|
||
end: Len::from_parts(at.rel.add(self.rel.mul(rest)), at.px.add(self.px.mul(rest))),
|
||
|
||
`Len::scale` is "both parts by the same fraction" and `Len` has `Add` and
|
||
`Sub`, so the whole rule is `at - self.scale(rel)` and
|
||
`at + self.scale(Rel::ONE.sub(rel))` -- the point the alignment names, less
|
||
the part of the length before it. Identical arithmetic, which the dump
|
||
confirms. It is the only place in the codebase that expanded a `Len`
|
||
operation like this; `LayoutLen::apply_leftover` touches `rel` alone, which
|
||
is genuinely one component.
|
||
|
||
**A question asked through a value, one line from its sibling.** `445287c`
|
||
moved every method that answers a question about a value to `&self`.
|
||
`LayoutLen::without_leftover` was missed, and its own doc comment calls
|
||
`apply_leftover` -- which takes `&self` -- "the opposite reading of the same
|
||
value". Now `&self` too. Every other by-value method in the workspace that
|
||
does not return `Self` was checked: the rest are conversions on numbers
|
||
(`Fixed::raw`, `to_f32`) or builders.
|
||
|
||
**A rig that scales a gesture against a mode the output no longer has.**
|
||
`run-headless.sh --mode` sets `out_w`/`out_h` beside the `swaymsg output ...
|
||
mode`, because those two numbers are the extent `replay-touch` passes to
|
||
`zwlr_virtual_pointer_v1::motion_absolute` -- the recording's coordinates are
|
||
a fraction of them. `--resize`, added in `b7b8d09`'s round, changed the mode
|
||
and left the extent alone, so `--resize 800x600@60Hz --replay flick.touch`
|
||
replayed every sample at `x * 800 / 1920` and finished looking like a run
|
||
that worked. Both are one `set_mode` function now, so a third mode change
|
||
cannot get it wrong. Found by reading rather than by running: this VM has no
|
||
recorded gesture that also resizes, which is exactly why it went unnoticed.
|
||
|
||
**A comment the plan/build split stranded.** `src/random.rs`'s "a row takes
|
||
the height it is given rather than its tallest child" sat above
|
||
`let gap = self.rng.below(3) as i32 * 4`, telling a reader that the line
|
||
consumes no randomness. It describes the `set_size_rules` that `98d4e98`
|
||
moved into `Build::kind`, and the line it was left above is one of the two
|
||
draws in the function. Moved to the rule it is about, and the
|
||
seed-stability half dropped: building a plan consumes no randomness at all
|
||
now, so there is nothing left for that clause to say.
|
||
|
||
### Tripped a rule and left as it stands
|
||
|
||
- `Align::tuple`, `UiVec2::partial_align` and `Vec2::partial_align` have no
|
||
callers anywhere. All three pre-date this PR and are outside its diff, so
|
||
they belong to the pre-review-gate sweep rather than to this branch.
|
||
- `Vec2::align`/`partial_align` are `UiVec2`'s two functions again with
|
||
`UiVec2::from(*self)` in front. Also pre-existing, and merging them means
|
||
deciding whether `Vec2` should have them at all.
|
||
- `impl_op!` has four grammars for one macro, and `core/src/util/vec2.rs`
|
||
spells two of them one line apart -- `impl_op!(impl Add for Vec2: add x y)`
|
||
beside `impl_op!(Vec2 Sub sub; x y)`. The `impl ... for ...:` arm has that
|
||
one caller. Pre-dates this PR; the two arms this PR added (`same ...`) are
|
||
a real distinction, since a type mixing a fraction and an offset has no
|
||
meaning for a bare `f32`.
|
||
- `AxisAlign`, `RegionAlign` and `Align` lost `Eq` when `AxisAlign` became a
|
||
`Rel` wrapper, which `Rel` derives. Nothing needs it, so it was left rather
|
||
than adding a derive with no reader.
|
||
- `Holds::through` special-cases a fully unbounded range before inverting a
|
||
fraction, which the general path would also answer correctly through
|
||
`narrow`. Left: it is the one place a `Px::MIN`-to-`Px::MAX` interval is
|
||
shifted and divided, and the early return says that no fraction can narrow
|
||
"every length".
|
||
- `Scroll`'s `content_len <= container_len` can only be equality, given the
|
||
`max` that built it. Left: `<=` reads as "the content fits", which is what
|
||
the branch means, and the mirror `>` now reads as "it overflows".
|
||
- `scenario::Case::name`, `window` and `Shuffle::of` take `self` on `Copy`
|
||
enums. They are test-rig code the `&self` pass did not cover, and an enum
|
||
with no fields is the one place taking a value costs a caller nothing.
|
||
|
||
## Sixth sweep: the shader boundary and the position widgets (2026-09-20)
|
||
|
||
Over what the five earlier rounds did not name -- the WGSL prelude and how
|
||
it is assembled, the position widgets, `orientation/`, and the sensor walk.
|
||
**Scoped against `upstream/main` at `ca2b4b2`, which is PR #19's real base**;
|
||
the first half of this sweep used the local `main` and had to be redone, for
|
||
which see "The branch layout" in `docs/HANDOFF.md`. Two findings.
|
||
The cold dump is byte-identical to `1096c31` and all three seed scans pass
|
||
(400 at depth 5 in 69.07s, 1,000 at depth 6 in 169.29s, 2,000 at depth 4 in
|
||
300.75s).
|
||
|
||
**A number both sides count in, written twice.** `module_source` already
|
||
builds each shader's preamble from `iris_core`'s own constants, with a
|
||
comment saying why: "a grid the two disagree about puts every coordinate
|
||
somewhere else". The move-chain work then added, to the very file it
|
||
prepends, a second copy of two of its own numbers -- `const MOVE_NONE` and
|
||
`const CHAIN_LIMIT`, under "Keep in step with `iris_core::CHAIN_LIMIT`" --
|
||
asking a reader by hand for what the mechanism beside it exists to do, and
|
||
duplicating the reasoning `CHAIN_LIMIT`'s Rust declaration already carries.
|
||
Both are injected now and the shader declares neither.
|
||
`every_shader_validates` composes the real preamble so it covers the change;
|
||
what it could never have caught is the two numbers drifting apart, which is
|
||
now unrepresentable.
|
||
|
||
`MASK_NONE` went in beside them, replacing a bare `4294967295u` in `masked`
|
||
-- where the CPU deliberately keeps `MaskIdx` and `MoveIdx` as separate
|
||
types so the two cannot be swapped. **That literal pre-dates this PR**; it
|
||
is a one-line drive-by in a block the PR was already rewriting, taken
|
||
because leaving it means a named sentinel for one index and a magic number
|
||
for its sibling four lines apart. Drop it if the scope matters more.
|
||
|
||
**A scroll positioning content it does not position.** Two halves, one
|
||
mistake, both new in this PR -- the base's `Scroll` has none of this
|
||
machinery. The author believed `Scroll` places its own content.
|
||
|
||
`content_len` is `answer_px.max(container_len)`, so it is never less than
|
||
the box. Two lines on, `slack` was `(container_len - content_len).max(ZERO)`
|
||
and `anchor` was `slack * align.rel()` -- provably always zero, whatever the
|
||
alignment, with `moved` then testing `anchor != ZERO` for nothing.
|
||
Instrumented at `1096c31`, a centred scroll over 50 px of content in a
|
||
200 px box prints `slack=0 align=AxisAlign(0.5) anchor=0` and centres the
|
||
content anyway: the framework does it, by placing the inner's answer in the
|
||
whole box, which comes out as `rel 0.5 - px 25` and resolves at any length.
|
||
The comment credited the arithmetic for behaviour it could not produce.
|
||
|
||
The same belief cost a redraw. The contract for content that fits was
|
||
guarded by `align == AxisAlign::NEG`, on the reasoning that at any other
|
||
alignment "it moves with every length the box takes and the drawing holds
|
||
for that length alone". It does not move -- the framework's placement is a
|
||
fraction of the box -- and the default alignment is the middle, so the
|
||
common case was the guarded one. `Painter::px_len` holds a drawing to the
|
||
length it read unless the widget says otherwise, so with no `holds` stated
|
||
the scroll redrew on every box change. Measured with `distinct_widgets`: 1
|
||
at `CENTER`, 0 at `TOP_LEFT`. Dropping the alignment test gives 0 at all
|
||
three, which is what `a_fitting_scroll_holds_for_every_box_its_content_fits_in`
|
||
asserts; it fails at `1096c31` on the `CENTER` case. `align` had no other
|
||
reader, so the widget no longer asks its own alignment at all -- which is
|
||
the tell that both halves were one mistake.
|
||
|
||
**`UiSpan::translated` and `UiRegion::translated`** arrived on this branch
|
||
and are reachable only from each other, which is why a plain "is this name
|
||
used anywhere else" scan does not see them: neither looks unused on its own.
|
||
The region one carried a performance argument for a function nobody calls.
|
||
Both deleted.
|
||
|
||
### Kept although they pre-date this PR
|
||
|
||
`GlyphAtlas::glyph_count`, `TextBuffer::new_empty` and the let-chain in
|
||
`TextEdit::apply_event` that #10 had expanded into a nested `if`. All three
|
||
were found under the wrong base and are outside #19's diff; Bryan said to
|
||
keep them anyway (2026-09-20), since the two are dead either way and the
|
||
third is a straight restoration.
|
||
|
||
### Tripped a rule and left as it stands
|
||
|
||
- `diag::untrace_widget` is new here with no caller, which is the test the
|
||
`translated` pair was deleted by. Left: it is the "removed" half of what
|
||
`trace_widget`'s own doc promises ("until explicitly removed or cleared"),
|
||
and `clear_traced_widgets` is the "cleared" half and does have one. A rig
|
||
drives this API from outside.
|
||
- `UiSpan::flip` swaps `start.rel` with `end.rel` and `start.px` with
|
||
`end.px`, where `Len` has exactly those two fields and one swap of the
|
||
structs would do. Pre-dates this PR.
|
||
- `UiVec2` translates under three names -- `shift`, `offset`, and
|
||
`UiRegion::offset` -- beside `Len::offset`, which adds pixels to one end
|
||
and is a different operation. Pre-existing, and one vocabulary is Bryan's
|
||
call rather than a sweep's.
|
||
- The nine `#[allow(unused_variables)]` are all on trait methods with empty
|
||
default bodies, where the parameter names are the signature's
|
||
documentation and underscoring them would hide it from implementors.
|
||
- `GpuPages::update` grows before it drains and both go through the one
|
||
queue, so the copy is ordered before the writes. Correct as it stands.
|
||
|
||
## Fifth sweep: the retained path, the renderer and the diagnostics (2026-09-20)
|
||
|
||
Over the parts the four earlier rounds did not read -- the renderer, the
|
||
text store, the input default, the harness -- and once more over `redraw`.
|
||
Six findings, `d8d5122` through `1096c31`, the last two from Bryan's reading
|
||
of the first four. The cold dump is byte-identical to `781199a` and all three
|
||
seed scans pass (400 at depth 5 in 69.45s, 1,000 at depth 6 in 214.69s, 2,000
|
||
at depth 4 in 419.50s).
|
||
|
||
**A contract was kept where it no longer held** (`d8d5122`). The sibling of
|
||
`713e3e7`, in the same function. `redraw` keeps the narrower of the old and
|
||
the fresh contract so widening and narrowing back do not churn the parent.
|
||
The drawing's half asks `was_holds.contains(window, rel_base, region)` first;
|
||
the answer's half did not. A widget whose answer contract widened in a frame
|
||
that also resized the window therefore kept a range the new window is
|
||
outside, and the parent's next ask refused it and redrew the whole subtree --
|
||
throwing away the drawing that widget had just made. Cost, not geometry: the
|
||
size kept is the size just reported. It needs both a resize the root does not
|
||
absorb and a mark on a deeper widget in the same frame, which is what
|
||
`a_contract_this_window_is_outside_is_not_kept` builds; it draws the leaf
|
||
twice at `781199a` and once with the guard in.
|
||
|
||
**Configuring a surface under its own texture** (`02048ea`). wgpu 30 says at
|
||
both `Surface::configure` and `Surface::get_current_texture` that configuring
|
||
while a texture the surface handed out is still alive panics. The
|
||
`Suboptimal` arm of `UiRenderer::draw` configured with the texture it was
|
||
about to draw with in hand, so the first suboptimal frame -- a resize or a
|
||
display change on some drivers -- takes the app down rather than rebuilding
|
||
the swapchain. The texture is good for that frame, so it is drawn with and
|
||
presented and the rebuild happens after `present` consumes it. Not
|
||
reproducible on demand here; the claim rests on wgpu's own documented panic.
|
||
|
||
This one is the reason for the sweep recorded under "The code written before
|
||
the review gate" below: nothing about the arm was hard, and it was written
|
||
that way anyway.
|
||
|
||
**A counter named the wrong contract** (`9b4cc32`). `AxisHolds` is four
|
||
contracts and `diag::outside` counted three: a refusal because this window
|
||
is outside the range the drawing was made for bumped "reuse outside: a rel
|
||
base". A window range is pixels and a rel base pin is a window-unit length an
|
||
unchanged window can still change, so the rig answered "why did that
|
||
redraw?" with the wrong one for every resize. Same class as `8088a1f`.
|
||
|
||
**Things nothing reads** (`7502176`, `1096c31`). `Axis::pair`,
|
||
`RegionAlign::NEAR` and `Painter::text_data` arrived on this branch with no
|
||
caller and never got one. The comment beside a span's cross-axis accumulator
|
||
said a scalable child "makes Children scalable too" -- `Children` names
|
||
nothing in this repository, and what it makes scalable is the span.
|
||
|
||
`text_data` was held back a round on the grounds that it is the only way a
|
||
widget inside `draw` can reach `TextData`, and the app's integration might
|
||
want it. That reasoning is wrong: nothing in iris is kept for the app's sake,
|
||
because the app is to be largely rewritten against this API rather than
|
||
ported call by call (Bryan, 2026-09-20).
|
||
|
||
**A question asked through a value** (`445287c`). `7502176` moved
|
||
`Holds::contains` from `&self` to `self` to match its five siblings, which
|
||
was the wrong way to reconcile them. A method taking `self` can only be
|
||
called on a value, so a caller holding a reference has to dereference to ask
|
||
-- `Copy` or not (Bryan, 2026-09-20). Every method that answers a question
|
||
about a value now takes `&self`: `Holds`, `AxisHolds` and `LayoutHolds`
|
||
throughout, `LayoutLen::{is_px, is_only_leftover, declared, fills}` and
|
||
`Size::within_box`. Builders that return a changed copy still take `self`.
|
||
|
||
### Tripped a rule and left as it stands
|
||
|
||
- `TextData`'s spare store clones the whole string into `Placed` on every
|
||
re-break. Bounded at 128 entries, but the clone is per re-break and
|
||
proportional to the text; a transcript-sized text would pay it on every
|
||
width change. Left because a cheaper key changes what "two texts of the
|
||
same words share an answer" means, which is a design question.
|
||
- `TextEdit`'s undo history pushes a whole copy of the text per changed
|
||
keystroke and is never bounded, and `apply_event` clones the text on every
|
||
event including the arrow keys. **`apply_event` is not in this PR's diff at
|
||
all**: it was last touched by #10 and #16, both already on `upstream/main`.
|
||
It keeps resurfacing in sweeps because they diffed against the local `main`
|
||
-- see "The branch layout" in `docs/HANDOFF.md`. Bryan wants the
|
||
unbounded push and the clone dealt with as a change of their own
|
||
(2026-09-20).
|
||
- `ActivationState::update` writes four arms where the `Start`/`On` and
|
||
`End`/`Off` pairs are identical, and `is_off` is `!is_on`. Also verbatim
|
||
from `main`.
|
||
- `TextView::draw`'s empty-with-hint branch matches on `self.hint` again
|
||
after `is_some()` guarded it, so its `None` arm is unreachable. The guard
|
||
cannot become an `if let` because `self.render(painter)` needs `&mut self`
|
||
between the two. Left rather than cloning the handle to satisfy the shape.
|
||
- `CurrentSurfaceTexture::Lost` is answered by reconfiguring, where wgpu says
|
||
to recreate the surface. It will not panic, and recreating needs the
|
||
window; worth doing with the next renderer change rather than this one.
|
||
|
||
## Quality sweep of the whole branch (2026-09-20)
|
||
|
||
A fourth sweep, over the layout core, the arithmetic, the atlas, the sensor
|
||
walk and the fuzz rig rather than over naming. Four findings, all on
|
||
`layout/one-ask` past `1ebd4d3`; the cold dump is byte-identical to it and
|
||
all three seed scans pass (400 at depth 5 in 65.09s, 1,000 at depth 6 in
|
||
161.27s, 2,000 at depth 4 in 301.90s).
|
||
|
||
**A kept contract was judged against the wrong box** (`713e3e7`). `redraw`
|
||
keeps the narrower guarantee a parent holds when the fresh drawing covers
|
||
it, so widening and narrowing back do not churn the parent. It asked
|
||
`was_holds.contains(.., active.placement)` -- where the answer put the
|
||
drawing -- when `holds` is about `active.region`, the box the drawing was
|
||
made in. The two differ on every axis a widget reported less than it was
|
||
offered, so such a widget marked its parent every time its contract
|
||
widened. Cost, not geometry: accepting is always safe, since `region` is
|
||
always inside the old range, so refusing only escalates. `resize` and
|
||
`try_reuse` both already ask about `region`.
|
||
`widening_what_a_drawing_holds_for_does_not_relay_out_the_parent` fails at
|
||
`1ebd4d3` and passes with the line changed; the existing
|
||
`widening_and_restoring_a_contract_does_not_invalidate_its_reader` cannot
|
||
see it, because its leaf reports `LEFTOVER`, which fills its box.
|
||
|
||
**Two things nothing read** (`aea0387`). `ActiveData::size_deps` was written
|
||
on every draw and cleared on every undraw, and read nowhere -- a `Vec` per
|
||
active widget. The `Painter`'s own copy is the live one, used in `draw_at`
|
||
to record whoever asked about a child it did not draw. `SizeRule::apply`
|
||
had no caller and would have been wrong with one: it answers the rule's own
|
||
length where `draw_at` resolves a fraction against the rel base first.
|
||
|
||
**Three reuse rejections said nothing** (`8088a1f`). Of the eight
|
||
rejections in `try_reuse`, a changed inherited mask counted and traced
|
||
nothing, an undrawn record traced without counting, and a changed
|
||
region-node choice counted without tracing. The mask one is what this
|
||
branch's repair was about, so the rig could not answer "why did that
|
||
redraw?" for it. Adding a counter meant editing a variant list and a name
|
||
list at the same index; they are one declaration now.
|
||
|
||
**A fuzz case ran only in the long scan** (`69ba915`). `Case::SizeResize`
|
||
was in `ALL` and in none of `generated.rs`'s `case!` invocations, so the
|
||
size-then-resize order -- which the enum's own comment argues is not the
|
||
same test as the other order -- was never checked by `cargo test`. The
|
||
tests and the list of which cases have one come from one macro invocation,
|
||
and a case missing from it now fails a test.
|
||
|
||
### Tripped a rule and left as it stands
|
||
|
||
- `Span` reads every child's cross length through `place_at(..).len(!axis)`
|
||
even where `has_exact_size(!axis)` makes it moot. The read looks like an
|
||
unwanted dependency, but `depend_on` only matters for a child that is not
|
||
in `children`, which is how `undraw` keeps a measured-then-dropped child
|
||
reachable. For a placed child it does nothing.
|
||
- `PixelRegion::contains` is inclusive at both ends, so two adjacent
|
||
widgets both claim the boundary step. Senses on one layer never block
|
||
each other, so both receiving it is what the design says.
|
||
- `CursorData::sense` is meaningless until `should_run` fills it, which the
|
||
code says in place and proposes a prepare stage for. A real
|
||
unrepresentable-state finding, but it is the event API's shape rather
|
||
than this branch's.
|
||
- `Wrapper` with no child answers `Size::default()`, which is `LEFTOVER`.
|
||
It reads as "nothing" but matches `impl Widget for ()`, whose comment
|
||
says a gap takes the default length so a span gives it a share.
|
||
- `ALL` in `tests/scenario/mod.rs` is still a hand-kept list of every
|
||
`Case`; `Case::name`'s match is the compiler-checked one. A variant left
|
||
out of `ALL` is invisible to the shrinker's `--case` selection too.
|
||
|
||
## Naming and logic sweep (2026-09-19)
|
||
|
||
Settled with Bryan across one session, on the branch past `58ce74d`. Nothing
|
||
here changed what layout computes: the cold dump is byte-identical to
|
||
`58ce74d` at every commit.
|
||
|
||
**How a description is said.** A `PlaceDescAxis` is built by chaining off the
|
||
value that says it -- `UiSpan::within_desc`/`shifted_desc`, `Len::as_desc` --
|
||
never by a constructor naming the type, because a constructor sends the
|
||
reader back to the start of the line. The `_desc` suffix is what says which
|
||
type comes out. `PlaceDescAxis::on_axis(axis)` lifts one axis into a pair
|
||
with the whole box across it; `on` alone was rejected as contentless and
|
||
reserved for events. `from_axes` is the constructor taking a function, beside
|
||
the `from_axis` taking one axis and two values.
|
||
|
||
**Arithmetic that needed a comment became a name.** `UiSpan::place` was the
|
||
aligned-placement rule written out three times; `LayoutLen::without_leftover`
|
||
was the sibling `apply_leftover` never had, at six sites; `is_px`,
|
||
`is_only_leftover` and `declared` name field comparisons the surrounding
|
||
comments had to translate; `Holds::covers` was interval containment by hand.
|
||
Seven module-level functions became methods on the value each took first.
|
||
|
||
**Every pair is a struct of two per-axis values, read with `[axis]`.**
|
||
`LayoutHolds` was four two-element arrays, so none of its own operations
|
||
could be written once; it is `AxisHolds` on `x` and `y`, and `and`, `covers`
|
||
and `contains` lost their loops. `impl_axis_index!` gives every pair
|
||
`Index<Axis>`/`IndexMut<Axis>`, replacing eighteen `axis`/`axis_mut`
|
||
methods -- `const_index` keeps them usable in const context. The bare
|
||
`[Option<LayoutLen>; 2]` became `Declared` of `Option<Len>`, which makes
|
||
"a share is never a declaration" structural rather than two filters and a
|
||
comment.
|
||
|
||
**Two findings in the logic, both one mistake.** A value computed from other
|
||
state was being stored as if it were state, and in both cases the visible
|
||
symptom was something that looked like an off-by-one:
|
||
|
||
- A `Span` carried `start` as a third accumulator beside `fixed` and `taken`,
|
||
assigned at three points, when every assignment was `reached(fixed,
|
||
taken)`. Both ends of a slot are now read where they are used; the variable
|
||
and two of the three calls per child go, and the gap added after the last
|
||
child derives nothing rather than needing to be subtracted.
|
||
- The measuring loop's `cursor` added `px` and `rel` by hand where the
|
||
placing loop below said `fixed += len.without_leftover()` -- the same sum,
|
||
one of them named.
|
||
|
||
**One property that held but nothing guarded.** A `Scroll`'s draw writes
|
||
`amt` and `snap_end`, so a second draw at another viewport reads what the
|
||
first wrote. Warm matches cold only because re-clamping is idempotent and
|
||
monotone. The seed scans build `Scroll`s and never scroll one, so this was
|
||
untested; `a_scrolled_view_resized_lands_where_a_cold_layout_puts_it` scrolls
|
||
four distances, one past the end, then widens. It passes.
|
||
|
||
## Follow-up implementation review (2026-09-19)
|
||
|
||
The ask/place split, window-unit frames, exact validity preimages, and
|
||
bottom-up dirty settling implement the settled design. Keep this approach.
|
||
It does not guarantee one body call per widget: an unhinted descendant that
|
||
reports leftover weight still needs a room ask and a slot ask. The explicit
|
||
measurement redesign remains deferred until an app screen justifies it.
|
||
|
||
The fixes below are on `layout/one-ask` in `/home/bob/repos/iris`:
|
||
|
||
- A collapsed share advances both span cursors. The regression covers one
|
||
and two collapsed children in all four directions.
|
||
- A masking widget owns a mask reference and reclaims its existing slot on
|
||
redraw. Primitives retain their own references. Removing the mask,
|
||
undrawing its owner, freeing the widget, and replacing the root release
|
||
ownership; a changed inherited mask rejects drawing reuse. Tests check
|
||
actual primitive mask indices, movement with and without a region node,
|
||
child draw counts, clip removal/addition, and empty-mask slot reuse.
|
||
- **A further handover defect:** `draw_inner` saved the old parent only
|
||
after a redraw replaced `ActiveData`. The old parent therefore kept the
|
||
child in its list and could undraw the subtree after its new parent drew
|
||
it. Capture the old parent before replacing the record. A branch-switch
|
||
regression reproduces disappearing content when its new parent owns a
|
||
region node, and also checks the ordinary reuse path.
|
||
|
||
The mask and handover tests fail on the reviewed code and pass with the
|
||
fixes. The mask fix preserves child reuse rather than redrawing descendants
|
||
on every mask repaint. No naming sweep or rounding-policy change is included.
|
||
|
||
Validation: workspace tests with and without diagnostics, the release fast
|
||
oracle, and all three prescribed seed scans pass. Comparing 34,488 cold
|
||
boxes against `cadfba0` finds 650 changes; withholding just the collapsed-slot
|
||
fix reproduces the baseline exactly. This is an expected geometry correction,
|
||
not a cost-only change whose dump should remain identical. The tabs example
|
||
and an exact 400 px collapsed-share fixture were rendered and inspected.
|
||
|
||
Two test-harness savings leave the random stream and coverage unchanged:
|
||
`generated` constructs one plan per seed for its sixteen scenarios, and the
|
||
warm/cold comparison constructs its diagnostic ancestry lookup only after
|
||
finding a mismatch. No overall speedup is claimed; no deep profile was run.
|
||
|
||
**Integration is larger than an API rename.** The app's pinned `32f6ad8`
|
||
has 45 commits not reachable from this review branch. In particular, the
|
||
app's nested/shape masks and shared `Ui` ownership are absent here: #19's
|
||
mask is still a single rectangle and `set_mask` rejects nested masks. Keep
|
||
the app pin until those existing capabilities have been integrated. The
|
||
"Masks" and "UI ownership" sections of `LAYOUT.md` describe the app-side
|
||
implementation, not everything already present on the upstream review branch.
|
||
|
||
## Original review of #19 at `cadfba0` (2026-09-19)
|
||
|
||
The original review read `cadfba0`, then the tip of `layout/one-ask`.
|
||
`cargo fmt --all --check`, clippy `-D warnings` and the 123-test suite were
|
||
clean there. These are the original failures, fixed by the follow-up above.
|
||
|
||
### A span misplaces the slot after a collapsed `leftover` child
|
||
|
||
`src/widget/position/span.rs:101` keeps two cursors while it places: `fixed`,
|
||
everything taken so far, and `start`, where the next slot begins. The branch
|
||
that drops a share child with no room to divide advances `fixed` by the gap
|
||
and not `start`:
|
||
|
||
```rust
|
||
if len.leftover > Weight::ZERO && len.px == Px::ZERO && len.rel == Rel::ZERO && !shares {
|
||
painter.undraw(child);
|
||
fixed.px += self.gap;
|
||
continue; // `start` still excludes this gap
|
||
}
|
||
let from = start;
|
||
```
|
||
|
||
In a 400 px row with `gap(10)` over children of 200 px, `leftover(1)` and
|
||
180 px -- exactly full, so nothing is left over and the share collapses --
|
||
the tail is placed at `(215, 0)..(395, 100)`. Its slot was 210..400, a gap
|
||
too long and a gap too early, and the declared 180 was then centred in it.
|
||
The row reports a total that ends at 400. A child drawn with `rel` lengths
|
||
is stretched into the extra gap instead of being centred in it, because the
|
||
slot fills.
|
||
|
||
Recomputing the cursor in that branch fixes it, and the tail lands at
|
||
`(220, 0)..(400, 100)`:
|
||
|
||
```rust
|
||
start = shared(fixed, taken, total.leftover, room);
|
||
```
|
||
|
||
The 123-test suite passes with the line in. Nothing in it or in the
|
||
generated oracle catches the defect: warm and cold layouts are wrong
|
||
identically, so an oracle comparing the two cannot see it. This is the
|
||
sharp form of the handoff's older "a vanished child leaves a double gap"
|
||
item, which described the accounting and not the misplacement.
|
||
|
||
### A reused child keeps the mask its parent replaced
|
||
|
||
`ActiveData::parent_mask` is recorded and documented as the inherited mask
|
||
"the one a redraw of it must not be handed back", but `try_reuse`
|
||
(`core/src/ui/render_state.rs:565`) never compares it with `info.mask`: it
|
||
gates on dirtiness, layer, move parent and region-node status only.
|
||
`Painter::set_mask` pushes a fresh `MaskIdx` on every draw, so a masking
|
||
widget that redraws while its child is reused leaves that child's
|
||
primitives naming a mask nobody updates again:
|
||
|
||
```
|
||
frame 1: masked mask=Id(0) inner mask=Id(0)
|
||
frame 2 (the masked widget alone marked): masked mask=Id(1) inner mask=Id(0)
|
||
frame 3 (the subtree moves up to y=10):
|
||
mask 0: y starts at 50 <- what the inner's primitives are clipped by
|
||
mask 1: y starts at 10 <- the live one, clipping nothing
|
||
```
|
||
|
||
The child's drawing is then clipped 40 px too high and its top is cut off.
|
||
`main` guarded this with `active.mask == mask` in its reuse gate and
|
||
re-marked the owners of a rebuilt mask in `remask_shape_users`; the rewrite
|
||
dropped both.
|
||
|
||
Returning `None` from `try_reuse` where `active.parent_mask != info.mask`
|
||
fixes the repro and keeps the suite green, but it redraws the whole subtree
|
||
whenever a masking ancestor redraws. The better fix is to give `set_mask` a
|
||
per-widget mask slot kept across redraws, the way `UiRenderState::move_slot`
|
||
already keeps a move entry, so the index is stable and `reposition` goes on
|
||
updating the one the descendants name.
|
||
|
||
### Regression coverage
|
||
|
||
Both paths now have regressions in `tests/cases/layout.rs` and
|
||
`tests/cases/retained.rs`; the follow-up above records the additional cases.
|
||
The descriptions above preserve the original failure at `cadfba0`.
|
||
|
||
### Clarity, in the order worth doing
|
||
|
||
Everything about naming is done: the unswept `extent`, the two same-typed
|
||
boxes on `ActiveData` and `Painter`'s four holds accumulators in `5642f20`,
|
||
`Part::All` in `aeb60e5`, and `Place`/`Part` themselves in `58ce74d`. The
|
||
settled vocabulary and the ask API are in `docs/LAYOUT.md`.
|
||
|
||
The clarity sweep of `3da1c71` and `7e2b4cd` closed the first three items
|
||
that stood here. Both are cold-dump identical to `6c84b6f`, so none of it
|
||
moved a box, and all three seed scans passed on `7e2b4cd` (400 at depth 5 in
|
||
66.25s, 1,000 at depth 6 in 188.34s, 2,000 at depth 4 in 300.80s) because
|
||
`reposition` and `redepth` were restructured on the retained path:
|
||
|
||
- `Answer` {size, holds} and `Drawn` {answer, drawing_holds} replace
|
||
`(Size, LayoutHolds)` and the three-tuple with two `LayoutHolds` in it.
|
||
`try_reuse` answers `bool` rather than `Option<()>`.
|
||
- `Span::along` is `Span::slot`; `far` is `row`, `shares` is `has_room`
|
||
beside a named `any_leftover`, and `reached` guards on the weight it
|
||
divides by rather than on the numerator.
|
||
- `DrawInfo::px` was the rel base in pixels, and all three readers printed
|
||
it as the box the widget drew in. Removed; each reads
|
||
`region.to_px(window)`. `Placing::window` existed only to feed it.
|
||
- `diag::outside` holds the 23 counter lines that were inside `try_reuse`.
|
||
- `ActiveData::is_region_node` replaces four copies of
|
||
`move_idx != parent_move`; `Axis::BOTH` replaces `AXES` in three modules;
|
||
`Len::rel_min`, `rel_max` and the unused `select_len` are gone.
|
||
|
||
`1ebd4d3` then closed the `PlaceSpan` item. `PlaceSpan` and `RelBase` are
|
||
`pub` and `ui/mod.rs` re-exports `place` by name rather than by glob, the way
|
||
it already did for `painter`, so the six `pub(crate)` accessors
|
||
(`stated_rel_base`, `narrows_rel_base`, `within_span`, `is_sized`,
|
||
`does_fill`, `with_rel_base`) are gone and `in_parent` matches
|
||
`(at.span, declared)`. `!at.is_sized()` was dead: `PlaceSpan::Sized` is built
|
||
only by `Len::as_desc`, which sets `RelBase::Len(self)` in the same literal,
|
||
and deleting `with_rel_base` removes the only writer that could have
|
||
separated them. Visibility here is plain `pub` plus a named re-export
|
||
wherever the path can be hidden (Bryan, 2026-09-19); `pub(super)` is for
|
||
inherent methods on types the crate exports, where it cannot.
|
||
|
||
What is left, none of it urgent:
|
||
|
||
- `widget_at` does three linear scans per child (`children.contains`,
|
||
`under.iter_mut().find`, `depend_on`), so a span of *n* children is
|
||
O(n^2) per draw. Not a problem at today's sizes; it is worth knowing
|
||
before a long transcript list lands on it.
|
||
- `DrawInfo` and `ActiveData` both carry `placed` and `asked`, two
|
||
`PlaceDesc` fields distinguished only by position in every literal.
|
||
They are genuinely different and documented, but the names are past
|
||
participles with no operand; a rename is Bryan's vocabulary call.
|