perf: Select > Modify > Feather/Smooth on 24 MP within budget (#2873) (#2882)

The blur itself was already parallel (9-15 ms on 24 MP); most of the
command was serial glue around it. Modify copied the mask into a padded
buffer and back out sample by sample even with no padding, and combine
built a full-canvas byte buffer, scanned it, then copied it into tiles
on one thread.

- modify: use the mask as is when the selection is clear of the canvas
  edge (pad 0); pad and crop row by row otherwise.
- combine / mask_to_surface: fill selection tiles in parallel straight
  from the mask, a row at a time; fully selected tiles share one
  copy-on-write tile.

Bit-identical: the per-sample oracles (combine, now with whole tiles,
and the old pad/crop) are compared in tests.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

crates/algo/src/selection.rs

@@ -3,12 +3,14 @@
 //! modify (expand, contract, border, smooth, feather), anti-aliased lasso,
 //! and combining with an existing selection.
 
+use std::sync::Arc;
+
 use photocraft_color::PixelFormat;
 use photocraft_geom::{Rect, TILE_SIZE, TileCoord};
 use photocraft_raster::Surface;
 use serde::{Deserialize, Serialize};
 
-use crate::photo_util::par_rows;
+use crate::photo_util::{par_map, par_rows};
 
 /// How a new selection combines with the current one.
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize, Default)]
@@ -79,49 +81,57 @@ fn coverage8(v: f32) -> u8 {
     (v.clamp(0.0, 1.0) * 255.0 + 0.5) as u8
 }
 
-/// Builds a GRAY8 selection surface from 8-bit coverage over `area`, allocating only the tiles
-/// that hold something (as [`Surface::prune`] would leave them).
-fn surface_from_coverage8(bytes: &[u8], area: Rect) -> Surface {
+/// Builds a GRAY8 selection surface over `area` (masks row-major over it) whose coverage at
+/// each sample is `f(prev, new)`, `prev` reading 0 without a `prev` mask and missing samples
+/// reading 0. Tiles are filled in parallel straight from the masks, a row at a time (no
+/// full-canvas byte buffer); only the tiles that hold something are kept (as [`Surface::prune`]
+/// would leave them), and fully selected tiles share one tile (copy-on-write), so the inside of a
+/// large selection costs no memory per tile (#211).
+fn surface_from_masks(area: Rect, new: &[f32], prev: Option<&[f32]>, f: impl Fn(f32, f32) -> f32 + Sync + Send) -> Surface {
     let mut s = Surface::new(PixelFormat::GRAY8);
+    if area.is_empty() {
+        return s;
+    }
     let w = area.width() as usize;
     let ts = TILE_SIZE as usize;
-    for tc in area.tiles() {
-        let tr = tc.rect().intersect(&area);
-        let (lx, span) = ((tr.x0 - area.x0) as usize, tr.width() as usize);
-        let row = |y: i32| bytes.get((y - area.y0) as usize * w + lx..(y - area.y0) as usize * w + lx + span);
-        if !(tr.y0..tr.y1).any(|y| row(y).is_some_and(|r| r.iter().any(|b| *b != 0))) {
-            continue;
-        }
-        let data = s.tile_mut(tc).bytes_mut();
-        for y in tr.y0..tr.y1 {
-            let o = (y - tc.ty * TILE_SIZE) as usize * ts + (tr.x0 - tc.tx * TILE_SIZE) as usize;
-            if let (Some(src), Some(dst)) = (row(y), data.get_mut(o..o + span)) {
-                dst.copy_from_slice(src);
+    let full = s.solid_tile(&[1.0]);
+    let row = |i0: usize, dst: &mut [u8]| {
+        let n = dst.len();
+        match (new.get(i0..i0 + n), prev.map(|p| p.get(i0..i0 + n))) {
+            (Some(sn), None) => dst.iter_mut().zip(sn).for_each(|(d, v)| *d = coverage8(f(0.0, *v))),
+            (Some(sn), Some(Some(sp))) => dst.iter_mut().zip(sn.iter().zip(sp)).for_each(|(d, (v, p))| *d = coverage8(f(*p, *v))),
+            _ => {
+                let at = |m: &[f32], i: usize| m.get(i).copied().unwrap_or(0.0);
+                for (i, d) in (i0..).zip(dst.iter_mut()) {
+                    *d = coverage8(f(prev.map_or(0.0, |p| at(p, i)), at(new, i)));
+                }
             }
         }
-    }
+    };
+    // Every tile starts as one shared blank tile; `Arc::make_mut` gives each task its own copy.
+    let blanks = s.take_tiles(area);
+    let built = par_map(blanks.len(), |k| {
+        let (tc, blank) = blanks.get(k)?;
+        let tr = tc.rect().intersect(&area);
+        let (lx, span) = ((tr.x0 - area.x0) as usize, tr.width() as usize);
+        let mut tile = Arc::clone(blank);
+        let data = Arc::make_mut(&mut tile).bytes_mut();
+        let (mut lo, mut hi) = (if tr == tc.rect() { u8::MAX } else { 0 }, 0u8);
+        for y in tr.y0..tr.y1 {
+            let o = (y - tc.ty * TILE_SIZE) as usize * ts + (tr.x0 - tc.tx * TILE_SIZE) as usize;
+            let Some(dst) = data.get_mut(o..o + span) else { continue };
+            row((y - area.y0) as usize * w + lx, dst);
+            (lo, hi) = dst.iter().fold((lo, hi), |(lo, hi), b| (lo.min(*b), hi.max(*b)));
+        }
+        (hi > 0).then(|| (*tc, if lo == u8::MAX { Arc::clone(&full) } else { tile }))
+    });
+    s.put_tiles(built.into_iter().flatten());
     s
 }
 
-/// 8-bit coverage of `m` (missing samples are 0), computed in parallel.
-fn to_coverage8(m: &[f32], area: Rect, f: impl Fn(usize, f32) -> f32 + Sync + Send) -> Vec<u8> {
-    let w = area.width() as usize;
-    let mut bytes = vec![0u8; w * area.height() as usize];
-    par_rows(&mut bytes, w, 1, |y, row| {
-        for (x, b) in row.iter_mut().enumerate() {
-            let i = y * w + x;
-            *b = coverage8(f(i, m.get(i).copied().unwrap_or(0.0)));
-        }
-    });
-    bytes
-}
-
-/// Builds a GRAY8 selection surface from a mask over `area`.
+/// Builds a GRAY8 selection surface from a mask over `area` (missing samples are 0).
 pub fn mask_to_surface(m: &[f32], area: Rect) -> Surface {
-    if area.is_empty() {
-        return Surface::new(PixelFormat::GRAY8);
-    }
-    surface_from_coverage8(&to_coverage8(m, area, |_, v| v), area)
+    surface_from_masks(area, m, None, |_, v| v)
 }
 
 /// Combines `new` with `old` by `mode`. Returns `None` when nothing is selected.
@@ -132,22 +142,18 @@ pub fn combine(old: Option<&Surface>, new: &[f32], area: Rect, mode: SelectionMo
     // Quantize to the 8-bit grid selections are stored on, so combining a mask with itself is
     // exact. Below half an 8-bit step is nothing.
     let q = |v: f32| f32::from(coverage8(v)) / 255.0;
-    let bytes = match mode {
-        SelectionMode::Replace => to_coverage8(new, area, |_, v| v),
+    let s = match mode {
+        SelectionMode::Replace => surface_from_masks(area, new, None, |_, v| v),
         _ => {
             let prev = mask_from_surface(old, area);
-            let at = |i: usize| prev.get(i).copied().unwrap_or(0.0);
             match mode {
-                SelectionMode::Add => to_coverage8(new, area, |i, v| at(i).max(q(v))),
-                SelectionMode::Subtract => to_coverage8(new, area, |i, v| (at(i) - q(v)).max(0.0)),
-                _ => to_coverage8(new, area, |i, v| at(i).min(q(v))),
+                SelectionMode::Add => surface_from_masks(area, new, Some(&prev), |p, v| p.max(q(v))),
+                SelectionMode::Subtract => surface_from_masks(area, new, Some(&prev), |p, v| (p - q(v)).max(0.0)),
+                _ => surface_from_masks(area, new, Some(&prev), |p, v| p.min(q(v))),
             }
         }
     };
-    if bytes.iter().all(|b| *b == 0) {
-        return None;
-    }
-    Some(surface_from_coverage8(&bytes, area))
+    (s.tile_count() > 0).then_some(s)
 }
 
 fn close(a: [f32; 4], b: [f32; 4], tol: f32) -> bool {
@@ -1202,7 +1208,10 @@ mod tests {
         let mut rng = crate::photo_util::Rng(7);
         let mut u = move || (rng.next() >> 11) as f32 / (1u64 << 53) as f32;
         // Off-grid areas spanning several tiles, including negative origins.
-        for area in [Rect::new(0, 0, 300, 20), Rect::new(-37, 250, 530, 263), Rect::new(255, -3, 258, 600), Rect::new(5, 5, 6, 6)] {
+        // The last one holds whole tiles, which fully selected masks fill (shared tiles).
+        for area in
+            [Rect::new(0, 0, 300, 20), Rect::new(-37, 250, 530, 263), Rect::new(255, -3, 258, 600), Rect::new(5, 5, 6, 6), Rect::new(-37, -300, 530, 263)]
+        {
             let n = area.width() as usize * area.height() as usize;
             let old_m: Vec<f32> = (0..n).map(|i| if i % 97 < 40 { u() } else { 0.0 }).collect();
             let old = old_mask_to_surface(&old_m, area);
@@ -1211,7 +1220,7 @@ mod tests {
             let mut slow_read = Vec::new();
             old.read_region_into(area, &mut slow_read);
             assert_eq!(mask_from_surface(Some(&old), area), slow_read);
-            for kind in 0..3 {
+            for kind in 0..4 {
                 let new: Vec<f32> = (0..n)
                     .map(|i| match kind {
                         0 => u() * 1.2 - 0.1,
@@ -1222,7 +1231,8 @@ mod tests {
                                 0.0
                             }
                         }
-                        _ => 0.001,
+                        2 => 0.001,
+                        _ => f32::from(u8::from(i >= 1000)),
                     })
                     .collect();
                 for mode in [SelectionMode::Replace, SelectionMode::Add, SelectionMode::Subtract, SelectionMode::Intersect] {

crates/engine/src/selection_cmds.rs

@@ -398,7 +398,9 @@ fn modify(s: &mut Session, p: &Value, op: &str) -> Result<Value> {
     if pw.checked_mul(ph).is_none_or(|n| n > MAX_MODIFY_PIXELS) {
         return Err(EngineError::BadParams { cmd: format!("select.modify.{op}"), msg: "the selection is too large to modify".into() });
     }
-    let padded = pad_mask(&m, w, h, pad, at_bounds);
+    // Unpadded (the usual case) the mask is used as it is: two serial full-canvas copies were
+    // most of a 24 MP Feather or Smooth (#211).
+    let padded = if pad == 0 { m } else { pad_mask(&m, w, h, pad, at_bounds) };
     let out = match op {
         "expand" => sel::expand(&padded, pw, ph, r),
         "contract" => sel::contract(&padded, pw, ph, r),
@@ -406,7 +408,7 @@ fn modify(s: &mut Session, p: &Value, op: &str) -> Result<Value> {
         "smooth" => sel::smooth(&padded, pw, ph, r),
         _ => sel::feather(&padded, pw, ph, r),
     };
-    let out = crop_mask(&out, pw, pad, w, h);
+    let out = if pad == 0 { out } else { crop_mask(&out, pw, pad, w, h) };
     let label = match op {
         "expand" => "Expand Selection",
         "contract" => "Contract Selection",
@@ -435,19 +437,18 @@ fn pad_mask(m: &[f32], w: usize, h: usize, pad: usize, outside_empty: bool) -> V
     if w == 0 || h == 0 {
         return out;
     }
+    // Row by row (a copy and two fills), not sample by sample (#211).
     for (py, row) in out.chunks_exact_mut(pw).enumerate() {
-        let inside_y = (pad..pad + h).contains(&py);
-        if outside_empty && !inside_y {
+        if outside_empty && !(pad..pad + h).contains(&py) {
             continue;
         }
         let y = py.saturating_sub(pad).min(h - 1);
-        for (px, v) in row.iter_mut().enumerate() {
-            let inside_x = (pad..pad + w).contains(&px);
-            if outside_empty && !inside_x {
-                continue;
-            }
-            let x = px.saturating_sub(pad).min(w - 1);
-            *v = m.get(y * w + x).copied().unwrap_or(0.0);
+        let (Some(src), Some((left, rest))) = (m.get(y * w..(y + 1) * w), row.split_at_mut_checked(pad)) else { continue };
+        let Some((mid, right)) = rest.split_at_mut_checked(w) else { continue };
+        mid.copy_from_slice(src);
+        if !outside_empty {
+            left.fill(src.first().copied().unwrap_or(0.0));
+            right.fill(src.last().copied().unwrap_or(0.0));
         }
     }
     out
@@ -455,7 +456,11 @@ fn pad_mask(m: &[f32], w: usize, h: usize, pad: usize, outside_empty: bool) -> V
 
 /// The `w`×`h` centre of a mask padded by `pad_mask`.
 fn crop_mask(m: &[f32], pw: usize, pad: usize, w: usize, h: usize) -> Vec<f32> {
-    m.chunks_exact(pw).skip(pad).take(h).flat_map(|row| row.iter().skip(pad).take(w).copied()).collect()
+    let mut out = Vec::with_capacity(w.saturating_mul(h));
+    for row in m.chunks_exact(pw.max(1)).skip(pad).take(h) {
+        out.extend_from_slice(row.get(pad..pad + w).unwrap_or_default());
+    }
+    out
 }
 
 /// Grow (contiguous) / Similar (anywhere): pixels whose colour lies within
@@ -1007,4 +1012,45 @@ mod tests {
         s.execute("edit.undo", json!({})).unwrap();
         assert_eq!(coverage(&s, 2, 2), 1.0);
     }
+
+    /// The previous per-sample padding and cropping, kept as the oracle for the row-wise ones.
+    fn old_pad_mask(m: &[f32], w: usize, h: usize, pad: usize, outside_empty: bool) -> Vec<f32> {
+        let pw = w + 2 * pad;
+        let mut out = vec![0.0f32; pw * (h + 2 * pad)];
+        for (py, row) in out.chunks_exact_mut(pw).enumerate() {
+            let inside_y = (pad..pad + h).contains(&py);
+            if outside_empty && !inside_y {
+                continue;
+            }
+            let y = py.saturating_sub(pad).min(h - 1);
+            for (px, v) in row.iter_mut().enumerate() {
+                if outside_empty && !(pad..pad + w).contains(&px) {
+                    continue;
+                }
+                *v = m.get(y * w + px.saturating_sub(pad).min(w - 1)).copied().unwrap_or(0.0);
+            }
+        }
+        out
+    }
+
+    #[test]
+    fn row_wise_pad_and_crop_match_per_sample_ones() {
+        let mut seed = 0x2545_f491_4f6c_dd1du64;
+        let (w, h) = (37, 23);
+        let m: Vec<f32> = (0..w * h)
+            .map(|_| {
+                seed ^= seed << 13;
+                seed ^= seed >> 7;
+                seed ^= seed << 17;
+                (seed % 1000) as f32 / 999.0
+            })
+            .collect();
+        for pad in [0, 1, 5, 40] {
+            for outside_empty in [false, true] {
+                let padded = pad_mask(&m, w, h, pad, outside_empty);
+                assert_eq!(padded, old_pad_mask(&m, w, h, pad, outside_empty), "pad {pad} {outside_empty}");
+                assert_eq!(crop_mask(&padded, w + 2 * pad, pad, w, h), m, "pad {pad}");
+            }
+        }
+    }
 }

perf/budgets.toml

@@ -324,7 +324,7 @@ issue = 211
 bench = "perf_scenarios"
 row = "Select > Modify > Smooth r 50 on 24 MP"
 budget_ms = 100
-enforce = false  # #211: fastest samples 30-33 ms; p95 still over budget at load avg > 100
+enforce = false  # #2873: p95 20-45 ms on 16 cores (load up to ~15), over 100 ms only at load avg > 20; CI cores unmeasured
 
 [[scenario]]
 id = "P27"
@@ -333,7 +333,7 @@ issue = 211
 bench = "perf_scenarios"
 row = "Select > Modify > Feather r 50 on 24 MP"
 budget_ms = 100
-enforce = false  # #211: fastest samples 30-33 ms; p95 still over budget at load avg > 100
+enforce = false  # #2873: p95 20-45 ms on 16 cores (load up to ~15), over 100 ms only at load avg > 20; CI cores unmeasured
 
 [[scenario]]
 id = "P28"