Repository navigation
perf(gpui): avoid allocations during freehand hit testing - #3488
himanshu748 wants to merge 1 commit into
Conversation
|
@himanshu748 is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| match state % 8 { | ||
| 0 => 0., | ||
| 1 => -0., | ||
| 2 => f64::INFINITY, | ||
| 3 => f64::NEG_INFINITY, | ||
| 4 => f64::NAN, | ||
| 5 => f64::from_bits(state), | ||
| _ => ((state >> 32) as i32) as f64 / 65536., |
There was a problem hiding this comment.
In freehand_hits_preserve_float_edge_cases, state % 8 repeats a fixed eight-value cycle. Consecutive coordinates cannot vary independently: positive infinity is always followed by negative zero, and NaN by negative infinity. This limits what the 10,000 comparisons cover and could hide failures on other combinations. Choose categories from higher bits and add a separate set of all-finite generated paths.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/screenshot_annotations.rs
Line: 4454-4461
Comment:
**Generated categories repeat fixed pairings**
In `freehand_hits_preserve_float_edge_cases`, `state % 8` repeats a fixed eight-value cycle. Consecutive coordinates cannot vary independently: positive infinity is always followed by negative zero, and NaN by negative infinity. This limits what the 10,000 comparisons cover and could hide failures on other combinations. Choose categories from higher bits and add a separate set of all-finite generated paths.
---
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!
Freehand annotation hover and selection currently allocate a transformed point vector for every stroke examined, even when the first segment is hit. Pointer movement pays that cost before deciding whether the hover state changed.
Stream transformed points through stroke-distance and nonzero-fill checks in one pass. This removes the temporary vector and lets early hits stop transforming samples. The implicit closing edge still contributes only to fill; distance arithmetic, zero-size transforms and topmost selection are preserved. Painting and export continue to receive the same collected points. No cache, new dependencies or persistent state.
Measured tradeoff
These are isolated pointer hit-test measurements, not frame time, painting, exported pixels or end-to-end UI latency. Synthetic fixtures contain circular strokes with 8, 32, 256, 2,048 or 8,192 samples, with 1, 8 or 32 annotations. Strokes occupy 2,000 × 2,000 units and are separated by 10,000-unit x offsets to isolate scan depth; they are not estimates of typical on-screen projects. Early/late targets hit the first or roughly three-quarter segment in the topmost/bottommost annotation; interior and miss cases scan further.
Linux x86-64, AMD EPYC 7763 VM, four-CPU quota, Rust 1.99.1, opt-level 3, one codegen unit, no LTO. Three sequential processes; each case uses 10 warmup pairs followed by 31 alternating before/after pairs. Batch size is
(32768 / (points * annotations)).clamp(16, 256). Fixture construction is excluded. Values are medians of the three process medians. The timing binary has no allocation counters.Late hits can be slower: the fused loop performs winding work before discovering a late stroke hit, whereas the old path skipped winding entirely after any stroke hit. The worst median regression in this 90-case matrix is about 6.7%; a 256-sample late hit is about 5.1% slower. This is a workload-dependent improvement, not a universal hover speedup. For the 32-stroke miss case, process medians ranged 1.876–1.966 ms before and 1.753–1.805 ms after; the three paired speedup ratios were 1.061–1.089.
A separate counting binary measured 1 allocation / 131,072 requested bytes → 0 for an examined 8,192-sample stroke, and 32 allocations / 4,194,304 requested bytes → 0 for the 32-stroke miss case. Requested bytes are not RSS or retained memory.
Correctness and reproduction
0a35a8840a4924cd6bbbd5e82900018221e158cf, using real project types whose source inputs were verified unchanged. It excludes GPUI/font shaping and does not build the application.Repeat the benchmark in three processes. It emits each process's per-case median; individual paired intervals are not emitted. Native release uses thin LTO, so its timings can differ from the isolated measurements above.
Formatting, diff checks and extracted-source Clippy pass. Local full-app checking reached the missing
alsa.pcprerequisite and was stopped while other dependency jobs were still running; a focused offlinealsa-syscheck exits 101 for that prerequisite. Native app tests, manual interaction, pixel comparisons and end-to-end latency measurements were not run locally. Native CI subsequently passed on this exact head (cc40ac575f13b8058ebb4755efbe354ecc1128be), tested as merge2db7af63406a075f7b594066f62475d32dcedfeeinto base0a35a8840a4924cd6bbbd5e82900018221e158cf. All four new regressions passed on Linux (1,603 app tests), Windows (1,482), Apple Silicon (1,525) and Intel macOS (1,520), with zero failures. The benchmark remains ignored in CI; these runs do not extend the isolated timing claims.This is separate from #3341's stroke ingestion and #3347's painting cache.
The PR appears safe to merge, with one non-blocking improvement to the generated test.
Findings
Fix with agent prompt
Summary
Freehand hit testing now transforms samples as needed instead of collecting a temporary vector.
Review checks were source-based; application tests and benchmarks were not run during this review.
himanshu748explicitly acknowledges that late hits can be slower because the fused loop performs extra winding work, and accepts this workload-dependent tradeoff.Reviews (1) · Last reviewed commit: "perf(gpui): avoid allocations during fre..." · Reviewed by Greptile