From 414ba3d158e4fefa8678810340ccb5c0521aadbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Sun, 10 May 2026 19:32:43 -0400 Subject: [PATCH] Land a red TDD guard for live-frame texture re-upload (T10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every `WebSurfaceFrame::from_live_frame` today calls `Arc::new(RenderImage::new([image::Frame::new(image_buffer)]))` unconditionally — even when the underlying RGBA bytes are byte-for-byte identical to the previous frame. At 60 fps on a 1080p canvas that is roughly 960 MB/s of host-side cloning + a fresh GPUI texture upload, and the roundtable agreed it is the next material bottleneck after the file-system pipe (a80d039). The contract this test pins is the cheapest invariant we can hold against today's `SoftwareRenderingContext`: two `ServoLiveFrame` inputs whose `rgba_bytes` are bit-identical must produce the same underlying `Arc` instance. Today they do not; the ignored run confirms two distinct pointer values for back-to-back identical inputs. The fix has two recognised shapes. The interim shape lives entirely in `WebSurfaceFrame::from_parts`: remember the previous frame's bytes (hash or pointer-eq) and reuse the existing `Arc` when they match. The endgame shape removes the host-side image step entirely — `OffscreenRenderingContext` + IOSurface — at which point the assertion becomes meaningless and is replaced by a frame-time budget. Whichever lands first, the fix commit MUST delete the `#[ignore]` attribute outright; toggling its reason is a broken contract. To call `WebSurfaceFrame::from_live_frame` from a unit test without spawning a real sidecar process, `ServoLiveFrame` gains a `#[cfg(test)] pub(crate) fn for_test(...)` constructor that wraps the existing private `from_parts` with realistic defaults. No production path uses it. cargo test --bin ely_app: 112 passed, 0 failed, 2 ignored. cargo test --bin ely_app -- --ignored: 2 failed (expected RED: T7 click pipeline + T10 texture re-upload). --- crates/ely_app/src/services/servo_live.rs | 18 +++++ .../ely_app/src/shell/gpui_harness_tests.rs | 66 +++++++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/crates/ely_app/src/services/servo_live.rs b/crates/ely_app/src/services/servo_live.rs index 4f5961d..0eea1a3 100644 --- a/crates/ely_app/src/services/servo_live.rs +++ b/crates/ely_app/src/services/servo_live.rs @@ -236,6 +236,24 @@ impl ServoLiveFrame { pub fn into_rgba_bytes(self) -> Vec { self.rgba_bytes } + + #[cfg(test)] + pub(crate) fn for_test(width: u32, height: u32, rgba_bytes: Vec) -> Self { + Self { + loaded_url: Some("https://example.com/".to_string()), + title: Some("Example".to_string()), + render_state: "complete".to_string(), + width, + height, + #[cfg(all(test, feature = "live-site-smoke"))] + non_white_pixel_count: 0, + #[cfg(all(test, feature = "live-site-smoke"))] + content_pixel_count: 0, + #[cfg(all(test, feature = "live-site-smoke"))] + sample_hash: 0, + rgba_bytes, + } + } } #[derive(Debug, Error)] diff --git a/crates/ely_app/src/shell/gpui_harness_tests.rs b/crates/ely_app/src/shell/gpui_harness_tests.rs index 5175a4a..d4bf81c 100644 --- a/crates/ely_app/src/shell/gpui_harness_tests.rs +++ b/crates/ely_app/src/shell/gpui_harness_tests.rs @@ -25,6 +25,7 @@ use std::cell::RefCell; use std::rc::Rc; +use std::sync::Arc; use ely_domain::{TabId, UrlText}; use gpui::{ @@ -35,6 +36,9 @@ use gpui::InteractiveElement; use super::ShellState; use super::web_surface::WebSurfaceStore; +use super::web_surface_frame::WebSurfaceFrame; +use super::web_surface_geometry::WebSurfaceScrollOffset; +use crate::services::servo_live::ServoLiveFrame; #[cfg(test)] impl super::ElyShell { @@ -252,6 +256,68 @@ async fn baseline_overlay_div_receives_simulated_click(cx: &mut TestAppContext) ); } +/// TDD red guard for T10: today every `WebSurfaceFrame::from_live_frame` +/// call allocates a fresh `Arc::new(RenderImage::new(...))` regardless +/// of whether the underlying pixels changed. At 60 fps on a 1080p +/// canvas that is `~8 MB / frame` of host-side RGBA cloning + a new +/// GPUI texture upload, the cost Linus + Karpathy + Jony all flagged +/// as the next material bottleneck after the file-system pipe. +/// +/// The contract this test pins is the cheapest invariant we can hold +/// against today's `SoftwareRenderingContext`: two frames carrying +/// **byte-identical RGBA payloads must produce the same underlying +/// `Arc`**. Today they do not — every `from_live_frame` +/// blindly reallocates. The fix path is either dedup the upload +/// against the last bytes or switch to `OffscreenRenderingContext` + +/// IOSurface so the GPU texture is the source of truth. +/// +/// `#[ignore]` so default `cargo test` stays green; remove the +/// attribute the moment T10 lands so the regression is permanent. +#[test] +#[ignore = "T10 red guard. Failure mode confirmed via `cargo test -- --ignored`: \ + two ServoLiveFrames carrying byte-identical RGBA payloads still \ + produce distinct Arc instances, because \ + WebSurfaceFrame::from_parts unconditionally calls \ + Arc::new(RenderImage::new(...)). At 60 fps 1080p that is \ + ~960 MB/s of host-side RGBA cloning + GPUI texture allocations \ + and is the next bottleneck after the file-system pipe. The \ + fix lives in WebSurfaceFrame (interim: dedup upload against \ + last frame bytes) or in the rendering pipeline (final: \ + OffscreenRenderingContext + IOSurface zero-copy). The fix \ + commit must remove this attribute outright — not toggle it."] +fn identical_live_frames_share_render_image_arc() { + let width = 16u32; + let height = 8u32; + let rgba_bytes = vec![0xAAu8; (width as usize) * (height as usize) * 4]; + + let first = WebSurfaceFrame::from_live_frame( + "https://example.com/".to_string(), + WebSurfaceScrollOffset::default(), + 100, + ServoLiveFrame::for_test(width, height, rgba_bytes.clone()), + ) + .expect("first frame builds from identical bytes"); + let second = WebSurfaceFrame::from_live_frame( + "https://example.com/".to_string(), + WebSurfaceScrollOffset::default(), + 100, + ServoLiveFrame::for_test(width, height, rgba_bytes), + ) + .expect("second frame builds from identical bytes"); + + assert!( + Arc::ptr_eq(&first.image, &second.image), + "TDD red: two ServoLiveFrames with byte-identical RGBA produced \ + distinct Arc instances (first={:p}, second={:p}). \ + WebSurfaceFrame::from_parts must dedup the upload against the \ + previous frame's bytes, or the rendering pipeline must switch \ + to a GPU-side source of truth (IOSurface) so per-frame host \ + allocations stop entirely.", + Arc::as_ptr(&first.image), + Arc::as_ptr(&second.image), + ); +} + fn active_tab_overlay_state( shell: &gpui::Entity, cx: &mut gpui::VisualTestContext,