Record the tenth sweep, over the deferred request system
This commit is contained in:
1 parent
e6a035d06e
commit
992a4a2e97
2 files changed
+114
-8
No files matched your search
+21
-8
@@ -4,10 +4,22 @@ Where the work in flight stands. The settled layout design, the vocabulary
|
|||||||
and the measurement method are in `docs/LAYOUT.md`; what the review of #19
|
and the measurement method are in `docs/LAYOUT.md`; what the review of #19
|
||||||
found is in `docs/LAYOUT_LOG.md`.
|
found is in `docs/LAYOUT_LOG.md`.
|
||||||
|
|
||||||
|
## A quality sweep of the deferred system is submitted on PR #19
|
||||||
|
|
||||||
|
`layout/one-ask` is at `4cb6f68` in `/home/bob/repos/iris-deferred`. The tenth
|
||||||
|
sweep is the first review of `76aaf06` through `0e838e9` -- the bound rules,
|
||||||
|
the deferred request path and the invalidation fix. Ten findings, the largest
|
||||||
|
two being a bound's held length looked up a second time through a value that
|
||||||
|
could not promise the end it named (`Bound::at` `expect`ed it), and a bound in
|
||||||
|
pixels pinning the rel base it is not read against. `docs/LAYOUT_LOG.md` has
|
||||||
|
them all, with the two rules it tripped and left and two corpus questions for
|
||||||
|
Bryan. Verified: format, clippy both ways, 197 and 201 tests, all three seed
|
||||||
|
scans (69.02s, 174.87s, 330.39s), and the cold dump byte-identical to
|
||||||
|
`0e838e9` across all 34,986 boxes.
|
||||||
|
|
||||||
## Performance sweep is submitted on PR #19
|
## Performance sweep is submitted on PR #19
|
||||||
|
|
||||||
`layout/one-ask` is at `0e838e9` in `/home/bob/repos/iris-deferred`. The fix
|
The performance fix is `0e838e9`. It keeps dependencies only for size requests the allocator uses, and borrows
|
||||||
keeps dependencies only for size requests the allocator uses, and borrows
|
|
||||||
`Widgets::size_rules` rather than cloning both axes at every lookup. Used
|
`Widgets::size_rules` rather than cloning both axes at every lookup. Used
|
||||||
hints retain their reader dependency; adding a cap after layout is checked for
|
hints retain their reader dependency; adding a cap after layout is checked for
|
||||||
both hinted and measured shares. Ordinary text measurement still tracks its
|
both hinted and measured shares. Ordinary text measurement still tracks its
|
||||||
@@ -95,6 +107,8 @@ comparison work above, its review rounds past `cadfba0` are described in
|
|||||||
overflow its pixels asked for, said as the place the parent gives and
|
overflow its pixels asked for, said as the place the parent gives and
|
||||||
sharing one comparison with the span; and `Image` grown in the generated
|
sharing one comparison with the span; and `Image` grown in the generated
|
||||||
trees, which is the only widget here whose hint is a length in pixels.
|
trees, which is the only widget here whose hint is a length in pixels.
|
||||||
|
- **The deferred request system's own sweep**, `4cb6f68` -- described at the
|
||||||
|
top of this file and in full in `docs/LAYOUT_LOG.md`.
|
||||||
- **One ask, the root's included**, `0d03267` -- the root had a layout path
|
- **One ask, the root's included**, `0d03267` -- the root had a layout path
|
||||||
of its own, so a rule that reads the box it is offered reached every widget
|
of its own, so a rule that reads the box it is offered reached every widget
|
||||||
but that one. `Placing::WINDOW` is the box nobody drew and `Placing::ask`
|
but that one. `Placing::WINDOW` is the box nobody drew and `Placing::ask`
|
||||||
@@ -155,12 +169,11 @@ the name it is reachable by.
|
|||||||
|
|
||||||
**Always**, because they cost nothing: format, workspace clippy under
|
**Always**, because they cost nothing: format, workspace clippy under
|
||||||
`-D warnings` with and without `layout-diagnostics`, the workspace tests, and
|
`-D warnings` with and without `layout-diagnostics`, the workspace tests, and
|
||||||
the **cold dump**. `layout_dump` over 400 depth-5 trees is **34,571** boxes
|
the **cold dump**. `layout_dump` over 400 depth-5 trees is **34,986** boxes
|
||||||
since `2dba90b` grew images in the trees, and `0d03267` left every one of them
|
since `76aaf06` grew bounds in the trees. It was 34,571 from `2dba90b`, which
|
||||||
byte-identical. It was 34,488 before the images; the fourth through eighth
|
grew the images, and 34,488 before those; the fourth through eighth sweeps all
|
||||||
sweeps all repeated 34,492, which is a `wc -l` of the whole run rather than of
|
repeated 34,492, which is a `wc -l` of the whole run rather than of its box
|
||||||
its box lines, so count the lines that are a box
|
lines, so count the lines that are a box (`grep -cE '^[0-9]+ [0-9]+ '`). It is the only thing that catches two
|
||||||
(`grep -cE '^[0-9]+ [0-9]+ '`). It is the only thing that catches two
|
|
||||||
same-typed values being swapped, which is the failure mode of a rename or a
|
same-typed values being swapped, which is the failure mode of a rename or a
|
||||||
move. The repair moved 650 of those boxes, all from the collapsed-share
|
move. The repair moved 650 of those boxes, all from the collapsed-share
|
||||||
correction; every commit since has been byte-identical to `84dad21`.
|
correction; every commit since has been byte-identical to `84dad21`.
|
||||||
|
|||||||
@@ -6,6 +6,99 @@ nothing here is rediscovered. Each entry says who found it and when.
|
|||||||
it (settled design, the measurement method) belongs in `docs/LAYOUT.md`, and
|
it (settled design, the measurement method) belongs in `docs/LAYOUT.md`, and
|
||||||
the current plan is in `docs/HANDOFF.md`.
|
the current plan is in `docs/HANDOFF.md`.
|
||||||
|
|
||||||
|
## 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)
|
## Performance sweep of #19 (2026-09-20)
|
||||||
|
|
||||||
`0e838e9`, by Codex.
|
`0e838e9`, by Codex.
|
||||||
|
|||||||
Reference in new issue
Block a user