[Fix] Preserve disk-cached image scale - #79
Merged
Conversation
NachoSoto
marked this pull request as ready for review
July 11, 2026 18:58
This was referenced Jul 11, 2026
NachoSoto
added a commit
that referenced
this pull request
Jul 12, 2026
## Summary This branch combines the independently reviewed changes from: - #77 — skip unchanged UIKit image sizes - #78 — streamline SwiftUI geometry observation - #79 — preserve `UIImage.scale` in the disk cache - #80 — remove the redundant image-scheduler hop - #81 — lazily initialize SwiftUI image state - #82 — reduce multicast contention while preserving subscription semantics ## Component benchmarks | Change | WatchChess workload | Result | |---|---|---:| | UIKit unchanged-size guard (#77) | 128,000 origin-only mutations | **-32.24% mean**, scheduler calls 128,000 → 0 | | SwiftUI geometry (#78) | 100 real cells × 80 relayouts | **-2.86% mean**, 95% CI for saving 9.053–45.532 ms | | Scheduler hop (#80) | 96 cold + 96 repeated requests | scheduler operations **-50%**; wall time neutral | | Lazy SwiftUI state (#81) | 20,000 body reconstructions | **-4.44% mean**, 12/12 paired wins | | Multicast completed hits (#82) | 50,000 real `PieceView` hits | **-89.3% / 9.33× faster** | | Multicast distinct keys (#82) | 24 concurrent keys | neutral: -1.4% mean, 95% CI spans zero | ## Combined WatchChess benchmark Release WatchChess, iPhone 17 Pro Max / iOS 26.5 simulators, 10 samples per variant and flow. Negative CPU/hitch changes are improvements. ### CPU | Flow | CPU time, baseline → combined | Change (paired-bootstrap 95% CI) | Mean one-core-equivalent CPU | |---|---:|---:|---:| | Tournament-list scrolling | 2.230 → 2.264 s | +1.54% (-2.79% to +6.09%): neutral | 12.01% → 12.07% | | Rounds scrolling | 1.059 → 1.024 s | **-3.25% (-5.55% to -1.27%)** | 13.03% → 12.79% | | Open game | 0.452 → 0.439 s | **-2.89% (-5.36% to -0.12%)** | **13.47% → 13.11%** (-2.63%; CI -4.37% to -0.87%) | ### FPS and hitches `FPS` below is a main-run-loop `CADisplayLink` callback proxy over the exact UI-action window, not compositor presentation FPS. | Flow | FPS, baseline → combined | FPS change (95% CI) | Hitches/min, baseline → combined | Hitch change (95% CI) | |---|---:|---:|---:|---:| | Tournament-list scrolling | 58.93 → 59.00 | +0.12% (-0.20% to +0.48%): neutral | 40.67 → 37.39 | -8.06% (-29.75% to +14.24%): neutral | | Rounds scrolling | 58.53 → 58.98 | **+0.77% (+0.13% to +1.39%)** | 63.24 → 51.68 | -18.28% (-35.36% to +5.33%): trend, CI spans zero | | Open game | 54.14 → 54.07 | -0.14% (-1.20% to +0.91%): neutral | 131.04 → 145.44 | +10.99% (-5.26% to +29.68%): neutral | Missed-refresh rates were also neutral for tournaments (-3.62%, CI -21.79% to +14.17%) and game (+2.99%, CI -5.93% to +13.06%). Rounds trended lower (-26.69%, CI -47.25% to +4.29%). ## Methodology and controls - two-pass, pass-outer crossover: pass 1 baseline on simulator A then candidate on B; pass 2 candidate on A then baseline on B - five measurements per pass, variant, and flow; 10 total per variant/flow - engine analysis forced off (`currentEngineStrength = 1`) and validated inside every app launch/report - exact 24-entry real API fixture in cache-only mode; any cache miss failed the run - identical 147-path warm AsyncImageView cache coverage per variant - exact AsyncImageView path/hash/mtime validation before sample 1 and after every five-sample block - API path/size/hash validation before and after every block - arithmetic-mean CPU headline; duration-weighted ratio-of-sums FPS/hitch rates - deterministic 100,000-resample, pass-stratified paired bootstrap confidence intervals - symmetric full cache hashing before each measured block; these are deliberately warm-cache measurements - hitches: clipped display-link interval at least 1.5× its target refresh period All earlier measurements with uncontrolled engine state or unequal cache warmness were quarantined and excluded. ## Rendering and cache regression checks - cold game and rounds screenshots were pixel-identical; cold tournament app content was pixel-identical outside dynamic system chrome - all four pass-1 seed/verify tournament screenshots were pixel-identical across the 1320×2590 app region; the only difference was 5,708 pixels in the translucent system search-field blur - six gated 8 FPS recordings covered tournament scrolling, rounds scrolling, and opening a game for both variants - manual contact-sheet review found no blank flashes, stale substitutions, missing portraits, or persistent placeholders; both game recordings showed the same normal transition placeholders and matching fully rendered end states - all six recordings passed pre/post AsyncImageView and API cache audits - baseline and candidate kept identical 147 relative cache paths; each variant's hashes and mtimes remained stable The different cache bytes between variants are intentional: #79 stores image scale in a versioned envelope. Legacy raw-PNG entries become one normal miss and regenerate in the corrected format. Existing remote images may therefore re-download once; offline users can temporarily miss legacy cached images until connectivity returns. ## #79 and #82 semantics #79 and the [#82 synchronous-subscriber review finding](#82 (comment)) are different fixes, and both are included: - #79 fixes PNG disk-cache scale metadata. - #82 attaches the first subscriber before starting synchronous upstream work, preserving first-request miss / repeat-request hit semantics. #82's `lifetime.observeEnded` intentionally holds the wrapper and in-flight entry only until producer completion or cancellation. Weak-reference tests verify release after both paths, while cancellation still disposes upstream work; no retain cycle or leak was observed. ## Validation - `swiftlint --config .swiftlint.yml --strict`: 0 violations - full iOS simulator suite: 69 XCTest/Quick + 13 Swift Testing tests = **82 passed** - focused multicast concurrency, synchronous, retry, memory-warning, cancellation, and weak-lifetime coverage - WatchChess Release cache preflights, 60 CPU samples, 60 frame reports, and six visual recordings passed their evidence checks - CI updated to macOS 26 / Xcode 26.5
NachoSoto
added a commit
that referenced
this pull request
Jul 12, 2026
## Summary - replace the globally locked property cache with explicit in-flight and completed states - release the cache lock before renderer work while retaining one shared upstream render per key - return completed hits directly and preserve the latest value from multi-value renderers - start rendering atomically only after the first subscriber attaches, preserving the initial cache miss - keep active async state alive through completion/cancellation and dispose in-flight work when its entry is released - evict empty, interrupted, and non-cacheable attempts so later requests can recover - add focused concurrency, synchronous, lifetime, memory-warning, cancellation, multi-value, and retry coverage ## Performance ### Isolated AsyncImageView benchmarks | Workload | Baseline | This PR | Change | |---|---:|---:|---:| | 8 distinct synchronous 20 ms renders | 207–218 ms | 28–30 ms | about -86% | | 10,000 completed-cache hits | 63–66 ms | 19–20 ms | about -69% | ### WatchChess integration benchmark Release-hosted WatchChess benchmark with real `PieceView` render data and 10 strictly alternating samples per variant: | Workload | Baseline mean | This PR mean | Mean change | Baseline median | This PR median | |---|---:|---:|---:|---:|---:| | 50,000 completed-cache hits | 265.321 ms | 28.429 ms | -89.3% / 9.33× | 264.016 ms | 28.318 ms | | 24 concurrent distinct keys | 47.965 ms | 47.314 ms | -1.4% (noise) | 47.200 ms | 47.367 ms | Hot-hit paired mean saving was 236.892 ms (95% CI 234.031–239.753 ms). Updated distinct-key paired saving was 0.650 ms (95% CI -1.692 to 2.993 ms), so that workload has no measurable wall-time change after the safe subscriber-lifetime handling. ## Rendering and cache parity Every measured sample preserved output signatures and upstream counts. Final WatchChess correctness also verified: - synchronous first request is a miss and the repeat is a hit - real async rendering remains cold miss → warm hit - a temporary wrapper delivers delayed output after leaving scope - an in-flight memory warning preserves the original observer while starting replacement work - wrapper release cancels in-flight work - 32 same-key waiters share exactly one upstream render - cold, warm, persistent, and memory-warning-invalidated pixels match - multi-value first/latest output behavior is preserved - disk persistence and cache invalidation remain correct - empty upstream completion retries and caches instead of poisoning the key Weak-reference tests prove the renderer is released after both active-producer completion and cancellation; ReactiveSwift drops the intentional lifetime hold when that lifetime ends. A pre-existing PNG disk-reload scale-metadata loss remains identical here: the same 88×88 pixels reload as 88 pt @1× instead of 44 pt @2×. PR #79 fixes that separately. ## Validation - `swiftlint --config .swiftlint.yml --strict` - full iOS simulator suite: 62 XCTest/Quick + 10 Swift Testing tests passed - final WatchChess Release correctness suite passed against `1416481` - 20/20 hot-hit and 20/20 updated distinct-key timing samples passed signature/count assertions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NSDataConvertible, with defaults that preserve existing conformersUIImagecache values in a versioned binary envelope containing the PNG pixels and UIKit scaleImageResultuse the same scale-preserving cache formatUIImage.databehavior as raw PNGRoot cause
UIImage.pngData()preserves pixel dimensions but not the UIKitscalevalue. Reloading an 88×88-pixel image that was 44×44 points at 2× throughUIImage(data:)therefore produced an 88×88-point image at 1×.That could look identical when constrained by a view while still reporting the wrong point size and scale to layout or renderer code.
Behavior after this change
NSDataConvertiblevalues continue using their existingdatarepresentation through default implementationsValidation
swiftlint --config .swiftlint.yml --strict: 0 violationsUIImagescale/dimensions round-tripCacheRenderermiss → corrected disk hit with preserved scale