diff --git a/IRIS_TODO.md b/IRIS_TODO.md index 32ec671..9a6b6e0 100644 --- a/IRIS_TODO.md +++ b/IRIS_TODO.md @@ -26,43 +26,61 @@ order and what "done" looks like. Tick and date them in place. still reaches the button — confirmed to fail on the pre-fix code and pass after. -- [ ] **Appending one image to an already-loaded list rebuilds every other - image's bind group (2026-09-05).** Found by the benchmark below, not - designed against: `GpuTextures::update` (`core/src/render/texture.rs`) - triggers `rebuild_image_bind_groups` — a loop over *every live +- [x] **Appending one image to an already-loaded list rebuilds every other + image's bind group (2026-09-05, fixed 2026-09-05).** Found by the + benchmark below: `GpuTextures::update` (`core/src/render/texture.rs`) + triggered `rebuild_image_bind_groups` — a loop over *every live standalone image*, rebuilding its `BindGroup` — whenever the shared - `masks` or `move_offsets` GPU buffer is resized (`masks_resized || + `masks` or `move_offsets` GPU buffer was resized (`masks_resized || moves_resized` in `UiRenderNode::update`, `core/src/render/mod.rs`), and a widget getting its *first* move-offset slot (LAYOUT.md section 2 — - every widget gets one on first draw) can be exactly what grows that + every widget gets one on first draw) could be exactly what grows that buffer. So one new message with one new image, appended to a transcript - that already has N images loaded, does not cost O(1): it costs one + that already has N images loaded, did not cost O(1): it cost one `create_image` for the new image plus one `make_image_bind_group` per *existing* image, because the new widget's own move slot pushed the arena past its capacity. Measured directly in `iris/examples/bench_images.rs`: appending a 1,001st image to 1,000 - already-settled ones reports **1,001** bind-group creates for that one + already-settled ones reported **1,001** bind-group creates for that one frame, not 1 (`./run-bench.sh images`, frame 5 in the transcript below). - This is the same class of cost LAYOUT.md's move chain exists to avoid - elsewhere in the codebase, just not yet closed off here — the fix is - presumably to size `masks`/`move_offsets` with headroom (the array - texture already grows by doubling, `grow_array`, for the same reason) so - an ordinary append does not cross a capacity boundary, or to stop tying - the *image* bind group's contents to a buffer that changes on every new - widget in the whole tree, image or not. Not designed further here per - the "do not redesign, record it" instruction this benchmark was built - under. -- [ ] **Bind-group creation takes two frames to reach the steady state, not - one (2026-09-05).** Same benchmark: loading 1,000 images cold reports - 1,000 creates on frame 1 (expected — this is `create_image`, one per - new image) *and again* 1,000 on frame 2, with nothing between the two - frames marked dirty, before settling to 0 from frame 3. The second - frame's 1,000 is `rebuild_image_bind_groups` again, for the same - masks/move-offsets buffer-growth reason as the item above — the arena - apparently does not finish growing to its steady size within the first - frame the tree is drawn. Not chased further; recorded so whoever fixes - the item above checks whether the fix also closes this one, since they - look like the same root cause measured two different ways. + + **Fix**: `masks`/`move_offsets` never belonged in a standalone image's own + bind group (group 2) in the first place — the group also holds that + image's own texture view, which is the only thing that is genuinely + per-image, so a buffer shared by *everything* forced a rebuild of + *every* group the moment it moved. Gave masks/move_offsets their own + bind group (group 3 in `shader.wgsl` and `UiRenderNode`: `masks_layout`/ + `masks_group`), bound once per frame in `UiRenderNode::draw` rather than + once per draw call, instead of duplicating them into every per-image + group. `GpuTextures` and its image bind groups now know nothing about + either buffer — `rebuild_image_bind_groups` is called only from + `grow_array` (the atlas array texture growing, which genuinely does + change what every image's own bind group must reference) — so a + masks/move_offsets resize now touches exactly one bind group, ever, + regardless of how many images are live. Numbers after the fix, same + benchmark and command: + + ./run-bench.sh images + frame=1 bind_group_creates=1000 (cold load, unchanged) + frame=2 bind_group_creates=0 (was 1000 -- see the item below) + frame=3 bind_group_creates=0 + frame=4 bind_group_creates=0 + (append one image here) + frame=5 bind_group_creates=1 (was 1001) + frame=6 bind_group_creates=0 + + `run-headless.sh tabs --shot` still 27266 bytes, byte-for-byte unchanged, + confirming the bind-group restructuring changed nothing about what is + drawn. +- [x] **Bind-group creation takes two frames to reach the steady state, not + one (2026-09-05, closed by the fix above, 2026-09-05).** Same benchmark: + loading 1,000 images cold used to report 1,000 creates on frame 1 + (expected — `create_image`, one per new image) *and again* 1,000 on + frame 2, before settling to 0 from frame 3. This was `rebuild_image_bind_groups` + firing a second time for the same masks/move-offsets buffer-growth + reason as the item above, confirming the guess recorded here — the two + were exactly the same root cause measured two different ways. Frame 2 + now reports 0 (see the numbers above); not a separate fix. ## Build @@ -123,7 +141,7 @@ order and what "done" looks like. Tick and date them in place. draws=320 rewrites=40 moves=160 (identical at every N) per-line average: 0.0012-0.0013ms (identical at every N) - cd iris && ./run-bench.sh images + cd iris && ./run-bench.sh images (2026-09-05, before the fix) frame=1 bind_group_creates=1000 (cold load) frame=2 bind_group_creates=1000 (see Fix item above) frame=3 bind_group_creates=0 @@ -132,6 +150,15 @@ order and what "done" looks like. Tick and date them in place. frame=5 bind_group_creates=1001 (see Fix item above) frame=6 bind_group_creates=0 + cd iris && ./run-bench.sh images (2026-09-05, after the fix) + frame=1 bind_group_creates=1000 (cold load, unchanged -- genuine work) + frame=2 bind_group_creates=0 + frame=3 bind_group_creates=0 + frame=4 bind_group_creates=0 + (append one image here) + frame=5 bind_group_creates=1 (one image's own create_image, O(1)) + frame=6 bind_group_creates=0 + **Reading it**: (a) is real, necessary work — shaping and laying out N never-before-seen text rows — and scales with N as it must, ~10x cost per 10x N. (b) and (c) are the pass conditions that matter: both are @@ -140,8 +167,9 @@ order and what "done" looks like. Tick and date them in place. the message list — draws/moves per tick or per line do not grow with list size, and the per-operation cost (a fraction of a microsecond) is nowhere near a frame budget. (d)'s cold-load and steady-state halves - behave as designed; its *append* half did not, which is the two Fix - items above. + behave as designed; its *append* half did not, until the fix above moved + masks/move_offsets out of the per-image bind group — now flat at O(1) + the same way (b) and (c) are. - [ ] **Masks defined relative to each other.** Wanted: mask A multiplies by something *and also* applies mask B — a mask can reference a parent mask, the way the move chain references a parent offset. Today masks diff --git a/iris/core/src/render/mod.rs b/iris/core/src/render/mod.rs index a056b72..5788254 100644 --- a/iris/core/src/render/mod.rs +++ b/iris/core/src/render/mod.rs @@ -35,6 +35,19 @@ pub struct UiRenderNode { textures: GpuTextures, masks: ArrBuf, move_offsets: ArrBuf, + /// Group 3: the masks and move-offsets storage buffers, on their own -- + /// see IRIS_TODO.md's "Appending one image ... rebuilds every other + /// image's bind group". These used to live in group 2 alongside each + /// standalone image's own texture view, so an image's bind group named + /// the masks/move_offsets buffer directly; the moment either buffer + /// resized (which a widget getting its *first* move slot can trigger, + /// unrelated to any image), `ArrBuf::update` handed back a new `Buffer` + /// identity and every image's bind group -- one per live image -- had + /// to be rebuilt to reference it. Pulling both buffers into their own + /// group, bound once per frame rather than once per draw call, means a + /// buffer resize now rebuilds exactly this one group instead of N. + masks_layout: BindGroupLayout, + masks_group: BindGroup, } struct RenderLayer { @@ -54,6 +67,13 @@ impl UiRenderNode { pub fn draw<'a>(&'a self, pass: &mut RenderPass<'a>) { pass.set_pipeline(&self.pipeline); pass.set_bind_group(0, &self.uniform_group, &[]); + // Set once, not per layer or per image: masks/move_offsets are read + // by every primitive and every standalone image alike, and living + // in their own group (rather than folded into group 2 alongside the + // per-image texture view) is what keeps an image's own bind group + // from naming a buffer that changes size on an unrelated widget's + // first draw -- see the comment on `masks_group` below. + pass.set_bind_group(3, &self.masks_group, &[]); for i in &self.active { let layer = &self.layers[i]; if layer.instance.len() == 0 && layer.image_instance.len() == 0 { @@ -164,21 +184,13 @@ impl UiRenderNode { } else { false }; - let rebuild_main = self.textures.update( - &mut ui.textures, - &self.rsc_layout, - &self.masks, - &self.move_offsets, - masks_resized || moves_resized, - ); + if masks_resized || moves_resized { + self.masks_group = + Self::masks_group(device, &self.masks_layout, &self.masks, &self.move_offsets); + } + let rebuild_main = self.textures.update(&mut ui.textures, &self.rsc_layout); if rebuild_main { - self.rsc_group = Self::rsc_group( - device, - &self.rsc_layout, - &self.textures, - &self.masks, - &self.move_offsets, - ); + self.rsc_group = Self::rsc_group(device, &self.rsc_layout, &self.textures); } } @@ -265,11 +277,18 @@ impl UiRenderNode { ); let rsc_layout = Self::rsc_layout(device); - let rsc_group = Self::rsc_group(device, &rsc_layout, &tex_manager, &masks, &move_offsets); + let rsc_group = Self::rsc_group(device, &rsc_layout, &tex_manager); + let masks_layout = Self::masks_layout(device); + let masks_group = Self::masks_group(device, &masks_layout, &masks, &move_offsets); let pipeline_layout = device.create_pipeline_layout(&PipelineLayoutDescriptor { label: Some("UI Shape Pipeline Layout"), - bind_group_layouts: &[&uniform_layout, &primitive_layout, &rsc_layout], + bind_group_layouts: &[ + &uniform_layout, + &primitive_layout, + &rsc_layout, + &masks_layout, + ], immediate_size: 0, }); let pipeline = device.create_render_pipeline(&RenderPipelineDescriptor { @@ -322,6 +341,8 @@ impl UiRenderNode { textures: tex_manager, masks, move_offsets, + masks_layout, + masks_group, } } @@ -355,12 +376,13 @@ impl UiRenderNode { }) } - /// Group 2: the shared atlas array, one standalone-image slot (a null + /// Group 2: the shared atlas array and one standalone-image slot (a null /// view for the main draw, a real one for each image's own bind group -- - /// see `GpuTextures`), one sampler and the masks buffer. No `count` on - /// any entry: this needs nothing beyond plain Vulkan 1.0 / GLES - /// sampling, unlike the `binding_array` layout it replaced (see - /// TEXTURES.md's "Recommended shape"). + /// see `GpuTextures`), plus one sampler. No `count` on any entry: this + /// needs nothing beyond plain Vulkan 1.0 / GLES sampling, unlike the + /// `binding_array` layout it replaced (see TEXTURES.md's "Recommended + /// shape"). Masks and move_offsets are deliberately *not* here -- see + /// `masks_layout` below for why they get their own group. fn rsc_layout(device: &Device) -> BindGroupLayout { device.create_bind_group_layout(&BindGroupLayoutDescriptor { entries: &[ @@ -390,26 +412,6 @@ impl UiRenderNode { ty: BindingType::Sampler(SamplerBindingType::NonFiltering), count: None, }, - BindGroupLayoutEntry { - binding: 3, - visibility: ShaderStages::FRAGMENT, - ty: BindingType::Buffer { - ty: BufferBindingType::Storage { read_only: true }, - has_dynamic_offset: false, - min_binding_size: None, - }, - count: None, - }, - BindGroupLayoutEntry { - binding: 4, - visibility: ShaderStages::VERTEX | ShaderStages::FRAGMENT, - ty: BindingType::Buffer { - ty: BufferBindingType::Storage { read_only: true }, - has_dynamic_offset: false, - min_binding_size: None, - }, - count: None, - }, ], label: Some("ui rsc"), }) @@ -421,8 +423,6 @@ impl UiRenderNode { device: &Device, layout: &BindGroupLayout, tex_manager: &GpuTextures, - masks: &ArrBuf, - move_offsets: &ArrBuf, ) -> BindGroup { device.create_bind_group(&BindGroupDescriptor { layout, @@ -439,16 +439,64 @@ impl UiRenderNode { binding: 2, resource: BindingResource::Sampler(tex_manager.sampler()), }, + ], + label: Some("ui rsc"), + }) + } + + /// Group 3: the masks and move_offsets storage buffers, shared by the + /// main draw and every standalone image alike (see the field comment on + /// `masks_group`). Bound once per frame in `draw()` rather than folded + /// into group 2, so a resize of either buffer -- which an unrelated + /// widget's first move slot can trigger -- rebuilds this one group + /// instead of every image's. + fn masks_layout(device: &Device) -> BindGroupLayout { + device.create_bind_group_layout(&BindGroupLayoutDescriptor { + entries: &[ + BindGroupLayoutEntry { + binding: 0, + visibility: ShaderStages::FRAGMENT, + ty: BindingType::Buffer { + ty: BufferBindingType::Storage { read_only: true }, + has_dynamic_offset: false, + min_binding_size: None, + }, + count: None, + }, + BindGroupLayoutEntry { + binding: 1, + visibility: ShaderStages::VERTEX | ShaderStages::FRAGMENT, + ty: BindingType::Buffer { + ty: BufferBindingType::Storage { read_only: true }, + has_dynamic_offset: false, + min_binding_size: None, + }, + count: None, + }, + ], + label: Some("ui masks"), + }) + } + + fn masks_group( + device: &Device, + layout: &BindGroupLayout, + masks: &ArrBuf, + move_offsets: &ArrBuf, + ) -> BindGroup { + device.create_bind_group(&BindGroupDescriptor { + layout, + entries: &[ BindGroupEntry { - binding: 3, + binding: 0, resource: masks.buffer.as_entire_binding(), }, BindGroupEntry { - binding: 4, + binding: 1, resource: move_offsets.buffer.as_entire_binding(), }, ], - label: Some("ui rsc"), + label: Some("ui masks"), }) } diff --git a/iris/core/src/render/shader.wgsl b/iris/core/src/render/shader.wgsl index 0e15d78..8abe499 100644 --- a/iris/core/src/render/shader.wgsl +++ b/iris/core/src/render/shader.wgsl @@ -72,9 +72,12 @@ var atlas: texture_2d_array; var image_texture: texture_2d; @group(2) @binding(2) var samp: sampler; -@group(2) @binding(3) +// Their own group, bound once per frame rather than folded into group 2: see +// UiRenderNode::masks_layout for why an image's own bind group must not name +// either buffer. +@group(3) @binding(0) var masks: array; -@group(2) @binding(4) +@group(3) @binding(1) var move_offsets: array; // A move chain more than this deep means something else is wrong (an diff --git a/iris/core/src/render/texture.rs b/iris/core/src/render/texture.rs index f420a09..ef67ba6 100644 --- a/iris/core/src/render/texture.rs +++ b/iris/core/src/render/texture.rs @@ -1,9 +1,7 @@ use image::{DynamicImage, EncodableLayout, GenericImageView}; use wgpu::{util::DeviceExt, *}; -use crate::{ - Mask, MoveOffset, PatchRect, TextureKind, TextureUpdate, Textures, render::util::ArrBuf, -}; +use crate::{PatchRect, TextureKind, TextureUpdate, Textures}; use super::atlas::PAGE; @@ -74,32 +72,21 @@ pub struct GpuTextures { impl GpuTextures { /// Applies queued `Textures` updates, then reports whether the *main* /// bind group (the one rects and glyphs draw with) needs rebuilding -- - /// true when the atlas array was recreated (its view identity changed) - /// or the masks buffer was, since both are bound there. Pushing or - /// freeing a standalone image never touches that group: it built or drops - /// its own. - pub fn update( - &mut self, - textures: &mut Textures, - rsc_layout: &BindGroupLayout, - masks: &ArrBuf, - move_offsets: &ArrBuf, - masks_resized: bool, - ) -> bool { - let mut rebuild_main = masks_resized; - if masks_resized { - // The masks or move-offsets buffer just moved, so every bind - // group holding a reference to either -- one per live - // standalone image -- is stale. - self.rebuild_image_bind_groups(rsc_layout, masks, move_offsets); - } + /// true exactly when the atlas array was recreated (its view identity + /// changed). Pushing or freeing a standalone image never touches that + /// group: it built or drops its own. Masks/move_offsets resizing is + /// `UiRenderNode`'s own concern now (its `masks_group`, group 3) -- + /// see that struct's field comment for why standalone images no longer + /// hear about either buffer at all. + pub fn update(&mut self, textures: &mut Textures, rsc_layout: &BindGroupLayout) -> bool { + let mut rebuild_main = false; for update in textures.updates() { match update { TextureUpdate::Push(kind, image) => { - rebuild_main |= self.push(kind, image, rsc_layout, masks, move_offsets); + rebuild_main |= self.push(kind, image, rsc_layout); } TextureUpdate::Set(kind, i, image) => { - rebuild_main |= self.set(kind, i, image, rsc_layout, masks, move_offsets); + rebuild_main |= self.set(kind, i, image, rsc_layout); } // A patch changes texture contents, not which layer or bind // group exists, so it never asks for a rebuild -- rebuilding @@ -118,10 +105,8 @@ impl GpuTextures { kind: TextureKind, image: &DynamicImage, rsc_layout: &BindGroupLayout, - masks: &ArrBuf, - move_offsets: &ArrBuf, ) -> bool { - let (slot, rebuilt) = self.make_slot(kind, image, rsc_layout, masks, move_offsets); + let (slot, rebuilt) = self.make_slot(kind, image, rsc_layout); self.slots.push(slot); rebuilt } @@ -132,10 +117,8 @@ impl GpuTextures { i: u32, image: &DynamicImage, rsc_layout: &BindGroupLayout, - masks: &ArrBuf, - move_offsets: &ArrBuf, ) -> bool { - let (slot, rebuilt) = self.make_slot(kind, image, rsc_layout, masks, move_offsets); + let (slot, rebuilt) = self.make_slot(kind, image, rsc_layout); self.slots[i as usize] = slot; rebuilt } @@ -145,18 +128,16 @@ impl GpuTextures { kind: TextureKind, image: &DynamicImage, rsc_layout: &BindGroupLayout, - masks: &ArrBuf, - move_offsets: &ArrBuf, ) -> (Slot, bool) { match kind { TextureKind::Image => { - let gpu = self.create_image(image, rsc_layout, masks, move_offsets); + let gpu = self.create_image(image, rsc_layout); (Slot::Image(gpu), false) } TextureKind::Page { layer } => { let mut rebuilt = false; if layer >= self.array_capacity { - self.grow_array(rsc_layout, masks, move_offsets); + self.grow_array(rsc_layout); rebuilt = true; } self.write_full_layer(layer, image); @@ -244,12 +225,7 @@ impl GpuTextures { /// copies the old layers across GPU-side -- no readback. Recreates the /// array's view, which invalidates every bind group that referenced it, /// so this also rebuilds all of them before returning. - fn grow_array( - &mut self, - rsc_layout: &BindGroupLayout, - masks: &ArrBuf, - move_offsets: &ArrBuf, - ) { + fn grow_array(&mut self, rsc_layout: &BindGroupLayout) { let new_capacity = self.array_capacity * 2; let new_texture = Self::create_array_texture(&self.device, new_capacity); if self.page_count > 0 { @@ -285,15 +261,14 @@ impl GpuTextures { ..Default::default() }); self.array_capacity = new_capacity; - self.rebuild_image_bind_groups(rsc_layout, masks, move_offsets); + self.rebuild_image_bind_groups(rsc_layout); } - fn rebuild_image_bind_groups( - &mut self, - rsc_layout: &BindGroupLayout, - masks: &ArrBuf, - move_offsets: &ArrBuf, - ) { + /// Called only from `grow_array`: the atlas array's view identity is the + /// one thing an image's bind group (group 2) still names that can + /// change out from under it. Masks/move_offsets resizing no longer + /// reaches here at all -- see `UiRenderNode::masks_group`. + fn rebuild_image_bind_groups(&mut self, rsc_layout: &BindGroupLayout) { for slot in &mut self.slots { if let Slot::Image(gpu) = slot { gpu.bind_group = Self::make_image_bind_group( @@ -302,21 +277,13 @@ impl GpuTextures { &self.array_view, &gpu.view, &self.sampler, - masks, - move_offsets, ); self.bind_group_creates += 1; } } } - fn create_image( - &mut self, - image: &DynamicImage, - rsc_layout: &BindGroupLayout, - masks: &ArrBuf, - move_offsets: &ArrBuf, - ) -> ImageGpu { + fn create_image(&mut self, image: &DynamicImage, rsc_layout: &BindGroupLayout) -> ImageGpu { let rgba = image.to_rgba8(); let (width, height) = rgba.dimensions(); let texture = self.device.create_texture_with_data( @@ -345,8 +312,6 @@ impl GpuTextures { &self.array_view, &view, &self.sampler, - masks, - move_offsets, ); self.bind_group_creates += 1; ImageGpu { @@ -357,16 +322,16 @@ impl GpuTextures { } /// Builds group 2 for one standalone image: the shared atlas array, this - /// image's own view, the shared sampler, and the shared masks buffer -- - /// the same layout the main draw uses with a null view in the image slot. + /// image's own view and the shared sampler -- the same layout the main + /// draw uses with a null view in the image slot. Deliberately does not + /// touch masks/move_offsets (group 3, `UiRenderNode::masks_group`): see + /// that field's comment for why folding them in here was the bug. fn make_image_bind_group( device: &Device, rsc_layout: &BindGroupLayout, array_view: &TextureView, image_view: &TextureView, sampler: &Sampler, - masks: &ArrBuf, - move_offsets: &ArrBuf, ) -> BindGroup { device.create_bind_group(&BindGroupDescriptor { layout: rsc_layout, @@ -383,14 +348,6 @@ impl GpuTextures { binding: 2, resource: BindingResource::Sampler(sampler), }, - BindGroupEntry { - binding: 3, - resource: masks.buffer.as_entire_binding(), - }, - BindGroupEntry { - binding: 4, - resource: move_offsets.buffer.as_entire_binding(), - }, ], label: Some("ui rsc image"), })