Skip to content

perf: the interactive path - #54

Open
ProdigyRahul wants to merge 8 commits into
Infrawrench:mainfrom
ProdigyRahul:combine/perf
Open

perf: the interactive path#54
ProdigyRahul wants to merge 8 commits into
Infrawrench:mainfrom
ProdigyRahul:combine/perf

Conversation

@ProdigyRahul

@ProdigyRahul ProdigyRahul commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

groups five branches plus the filter half of a sixth: work that ran on one core, on the ui thread, or with the wrong complexity.

gestures that froze the window

  • the freehand lasso was O(bbox area x 16 x point count): a leisurely trace round a 600x600 region with 2000 points is about 1.2e10 edge tests, on pointer-up. edges are now walked down the rows they span into a sorted crossing list per supersample row, so the cost goes from area to perimeter — and the lasso decimates on capture, which it never did.
  • content-aware move ran up to 160 full jacobi passes synchronously on pointer release; the pass is now a par_chunks_mut over rows, the same iteration spread over the cores.
  • quick selection wrote one history entry per pointer-move, each deep-cloning the whole selection twice — dozens to hundreds of entries per drag, enough to blow past the 200-entry limit and discard everything before it.

one drag with the quick selection tool, and what the history panel holds afterwards:

before after
before after

(the left panel is scrolled — there are more entries below it.)

wrong complexity

  • signed_distance scanned a (2r+1)² window per pixel, and photoshop's stroke and glow sizes go to 250. replaced with an exact euclidean distance transform — two 1-d passes, independent of radius.
  • disc morphology scanned the whole disc per pixel (~5000 taps at radius 40, on every slider tick of a live preview). a disc decomposes into one horizontal line per row, and a 1-d morphological pass is O(1) per pixel with a monotonic deque: 81 passes instead of 5000 taps.
  • seam carving rebuilt the entire energy map for every seam, and there is one seam per pixel of width change. a seam moves at most one column per row, so the field is carried across and only that band repaired.
  • the cpu box blur re-summed its window per pixel and never used rayon.

both are pinned by benches that time the old formulation against the new one, inlined the same way the equivalence tests carry it — cargo run --release -p schist-layer-fx --example strokebench and -p schist-fx --example cpubench. on this machine:

outside stroke 1000x1000 size=4       before     176.0 ms   after     25.5 ms
outside stroke 1000x1000 size=30      before    4879.1 ms   after     24.1 ms
outside stroke 1000x1000 size=250     before  287094.5 ms   after     27.2 ms
gaussian blur 1000x1000 r=50          before     525.9 ms   after     41.3 ms
gaussian blur 4000x3000 r=50          before    6953.8 ms   after    818.2 ms

memory with no ceiling

both tile caches were unbounded while the prefetcher deliberately warms the whole document — about 512 MB resident for an 8000² document and 2 GB for 16000². now a 256 MiB budget with lru eviction, display_tiles pruned to match, and the prefetcher stops at the budget instead of warming tiles that only evict each other.

and two correctness fixes that live here

filters clamped at the selection's bounding box instead of reading the surrounding image, leaving a visible band along the selection edge on any large-radius blur; FilterPlugin now advertises its reach and the shell hands over a grown buffer, with the write side still masked by the selection. region export swallowed both of its failure paths and reported however many had worked.

how the equivalence is checked, not asserted

every algorithm swap here is pinned against the formulation it replaced, sample for sample: the_scanline_fill_matches_the_brute_force_one (self-crossing star, triangle, concave L), the_distance_transform_matches_the_window_search (five limits), the_inpaint_matches_a_sequential_jacobi_solve, the_decomposed_disc_matches_a_full_disc_scan (five radii, both directions), incremental_energy_carves_the_same_seams (five targets, both directions).


cargo fmt --all --check, cargo clippy --workspace --all-targets -D warnings, cargo test --workspace — 635 passed, 0 failed.

how this relates to my other open prs

these seven are independent of each other — each branches off main and each is green on its own. they do share files with my five open prs (#36, #38, #42, #44, #46), mostly workspace.rs, so whichever lands first will leave the others needing a rebase. happy to rebase in whatever order suits you, or to split any of these further if one is too big to review in a sitting.

@ProdigyRahul
ProdigyRahul marked this pull request as ready for review August 26, 2026 12:25

@IAmJSD IAmJSD left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review was written by Claude (Fable 5), acting on Astrid's behalf.

The core algorithm work here is strong: the running-sum box blur, parallel lens blur/edges/warp/convolve/inpaint, disc-morphology decomposition, incremental seam-carve energy, scanline lasso fill, Felzenszwalb–Huttenlocher EDT, LRU tile caps and one-undo quick-select all check out as output-equivalent, and the equivalence tests inline the exact replaced implementations as references with real boundary cases. What blocks the merge is the app-shell glue around them:

  1. Selection + generator filters regress (workspace.rs, filter_region_with_context ~4711): with a selection active, the region is now intersected with content_bounds().inflated(pad) (pad = 0 on the dialog path). Render ▸ Clouds on a new empty layer creates content — content_bounds() is EMPTY, the intersection is empty, and begin_filter_preview refuses with "Nothing to filter" where main fills the selection. A selection extending past existing content similarly gets its clouds cropped.

  2. The headline "filters read through the selection edge" fix is dead plumbing (apply_filter ~4925 / begin_filter_preview ~4860): filter_region_with_context with context > 0 is unreachable — parameterless filters pass 0, and the dialog's Apply reuses preview.region, which begin_filter_preview computed ungrown via filter_region. So the whole FilterPlugin::context() surface (plugin-api, the blur impls, the simple_filter! heuristic) never grows a buffer on any interactive path, and the selection-edge band the PR body claims to fix is still there. The context_tests only assert that context() returns numbers. The growth needs to happen in begin_filter_preview.

  3. Prefetch permanently disabled on large documents (prefetch_tick ~5464): if self.cache.is_full() { self.prefetch_queue.clear(); return false; } — steady state after eviction is bytes >= budget, so once a 16k×16k document fills the 256 MiB budget, every rebuilt nearest-first queue for a new viewport is discarded each tick, including the mid-gesture warming main's own comments call out as what makes the settle frame land instantly. The comment's premise ("what is already in is what the viewport actually needs") is wrong the moment the viewport moves. Prefetch the near ring and let LRU evict distant tiles.

Smaller items:

  • fx_blur.wgsl:4-7 still claims the shader mirrors box_pass "tap for tap… no running total whose float error would drift" — now false; update the comment and ideally add a long-dimension parity case (drift grows with row length; the suite only tests 64×48).
  • horizontal_morph's "monotonic deque, O(1) per pixel" is a Vec with remove(0) — O(window) per removal; make it a VecDeque.
  • MAX_FILTER_CONTEXT was inserted between AUTOSAVE_SECS and its doc comment (workspace.rs ~87), and box_pass has a stale stacked doc comment — same splice pattern as sibling PRs.
  • simple_filter! treating any "amount" param as spatial reach conflates intensity with displacement (Add Noise grows by up to 256 px once item 2 is fixed).
  • cost_does_not_grow_with_the_radius asserts wall-clock ratios — a CI flake hazard that mostly tests the new code against itself.
  • The cited perf numbers (205 ms → …, 5.62 s, 4.43 s) have no harness behind them despite the repo's fxbench/afbench precedent — plausible given the complexity changes, but worth pinning.

Fix the three majors and this is a clear merge — the algorithmic core is the best-engineered part of this PR series.

@ProdigyRahul

Copy link
Copy Markdown
Contributor Author

fixed

@IAmJSD IAmJSD left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This follow-up review was written by Claude (Fable 5), acting on Astrid's behalf.

Most of the round landed well: the generator/selection regression is properly fixed (selection.bounds().inflated(pad) with no content intersection — Clouds into a selection on an empty layer works), the context plumbing is now live end-to-end on the dialog path with consistent preview/commit blit math, the wgsl comment is honest with a 4096-row parity case, horizontal_morph is a real VecDeque, the wall-clock test became a semantic one, and the benches exist. Good work.

One new major keeps this from merging — the prefetch fix trades "permanently dead" for "actively evicts the viewport" on the same large documents:

prefetch_tick no longer stops when full (good), but three facts combine badly (workspace.rs:65, crates/compositor/src/lib.rs:722,840):

  • PREFETCH_TILE_BUDGET = 2048 tiles ≈ 0.5–1 GiB, twice the cache's ~1024-tile capacity (256 MiB / 256 KiB per tile) — the queue's own comment says so.
  • TileCache::prewarm filters to missing tiles and never refreshes resident entries' touched stamps; paints are served from display_tiles first, which skips cache.get — so nothing re-touches visible tiles while a drain runs.
  • Each insert past capacity evicts the minimum-stamp entry, and display_tile's retain drops the display copies of whatever the composited cache evicts.

So on a 16k×16k document a full drain first evicts the viewport, then the near ring, then its own earliest (nearest) inserts; after ~8 s the resident set is the farthest ~1024 queued tiles, the next paint recomposites the whole viewport — exactly the stall prefetch exists to prevent — then rebuilds the queue and loops forever. The new comment's "LRU evicts the distant tiles instead" is inverted. Any one of these fixes it: cap the queue at (cache tile capacity − visible count); have the per-paint visible prewarm touch resident entries; or stop the drain when the next candidate is farther than the nearest would-be evictee.

Two smaller residuals, take or leave:

  • The Filter Gallery path (show_filter_gallery → plain begin_filter_preview(), workspace.rs:2493) still previews and commits with an ungrown region, so the selection-edge band this PR fixes for the dialog persists for the same filters applied via the gallery.
  • The dialog reach heuristic (workspace.rs:5276) includes "amount", which simple_filter!::context() deliberately dropped one commit earlier — Add Noise previews grow by up to 256 px for a filter that reads nothing. Perf-only, but the two lists should agree.

Also still open from last round: the unbounded failed.join(", ") export status string, and no end-to-end test asserting the region growth actually reaches a filter (context_tests still only check context() numbers). Fix the prefetch inversion and this merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants