Repository navigation
perf(gpui): SIMD vectorize PNG adaptive filters and eliminate buffer copies - #2674
nikolas-sapa wants to merge 1 commit into
Conversation
|
@nikolas-sapa is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for i in 0..len { | ||
| let diff = tail_c[i].wrapping_sub(tail_p[i]); | ||
| tail_o[i] = diff; | ||
| sum += u64::from((diff as i8).unsigned_abs()); |
There was a problem hiding this comment.
The new loops index slices with a range variable. The repository instructions require slice iteration or .iter().enumerate() instead. Use zipped iterators for these equal-length slices. The same pattern appears at lines 353–356 and 835–837, and elsewhere in the added filters. This repository requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/kernels/src/png.rs
Line: 282-285
Comment:
**Loops violate required Rust style**
The new loops index slices with a range variable. The repository instructions require slice iteration or `.iter().enumerate()` instead. Use zipped iterators for these equal-length slices. The same pattern appears at lines 353–356 and 835–837, and elsewhere in the added filters. This repository requirement must be satisfied before merging.
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| color, | ||
| } | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
The CLI branch now decodes every image into targets before benchmarking the first one. Previously, it decoded and released one image at a time. Large screenshot batches now need memory for all decoded images at once, which can cause swapping, distort timings, or stop the benchmark with an out-of-memory failure. Keep CLI targets lazy and decode each image just before benchmarking it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/kernels/benches/png_encode.rs
Line: 93
Comment:
**Image batches retain every buffer**
The CLI branch now decodes every image into `targets` before benchmarking the first one. Previously, it decoded and released one image at a time. Large screenshot batches now need memory for all decoded images at once, which can cause swapping, distort timings, or stop the benchmark with an out-of-memory failure. Keep CLI targets lazy and decode each image just before benchmarking it.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.|
|
||
| let mut ref_out = vec![0u8; len]; | ||
| apply_filter(2, bpp, &prev, &curr, &mut ref_out); | ||
| let ref_cost = sum_abs(&ref_out); |
There was a problem hiding this comment.
The expected cost uses sum_abs, which this PR also replaces with NEON on aarch64. Both sides therefore use the same vector absolute-value and sum operations, so a shared cost bug could pass every assertion. Compute the expected cost with a scalar iterator in this test, including explicit 0x80 values, to check the new cost code independently.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/kernels/src/png.rs
Line: 842
Comment:
**Cost tests share their implementation**
The expected cost uses `sum_abs`, which this PR also replaces with NEON on `aarch64`. Both sides therefore use the same vector absolute-value and sum operations, so a shared cost bug could pass every assertion. Compute the expected cost with a scalar iterator in this test, including explicit `0x80` values, to check the new cost code independently.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Thanks @nikolas-sapa! Greptile is at 4/5 here. We only merge once Greptile reaches 5/5, so please address its findings in the summary above (or reply to them if you think one is wrong), then comment |
This branch has not been deployed
2 participants
Summary
• Vectorizes all 4 PNG adaptive filter encoders (Up, Sub, Avg, Paeth) in
apps/desktop-gpui/kernels/src/png.rsusing ARM NEON on aarch64 with unrolled 64-byte block processing.• Zero-copy Up filter: Filter 2 (the initial filter evaluated by
adaptive_filter) filters directly into the output row buffer while accumulating the absolute sum in vector registers, eliminating the intermediate candidate buffer write and subsequent memcpy.• Vectorized Average filter (
vhaddq_u8): evaluates floor halving addition and subtraction in vector instructions with zero scalar division or bitwise overhead.• Vectorized Paeth filter: evaluates parallel 8-lane 16-bit absolute distance comparisons and bitselect (
vbslq_u16), eliminating all per-pixel scalar branches.• Fast SIMD
sum_abs: replaces the double-nested.chunks(32).map().sum()iterator loop with NEON vector absolute and horizontal pairwise addition (vabsq_s8+vaddlvq_u8).• Unit tests: adds
neon_filters_match_scalar_referenceverifying all 4 filters and their cost calculations match the scalar reference byte-for-byte across RGB and RGBA formats and varied row lengths.• Benchmark harness: adds default synthetic screen workloads (1080p, Retina, 4K) to
benches/png_encode.rswhen executed without CLI file arguments.Benchmark Results (Apple Silicon M-series)
Test Plan
• Run
cargo test --manifest-path apps/desktop-gpui/kernels/Cargo.toml• Run
cargo bench --manifest-path apps/desktop-gpui/kernels/Cargo.toml --bench png_encode• Run
cargo clippy --manifest-path apps/desktop-gpui/kernels/Cargo.tomlThe required Rust loop style must be satisfied before merging; the benchmark memory use and shared test reference are non-blocking improvements.
Findings
Fix with agent prompt
Summary
Adds ARM NEON implementations for PNG adaptive filters and writes the initial Up result directly into the output buffer.
Reviews (1) · Last reviewed commit: "perf(gpui): SIMD vectorize PNG adaptive ..." · Reviewed by Greptile