Harden Metal presentation lifetimes - #148
Conversation
Harden Metal presentation lifetimes
📝 WalkthroughWalkthroughMetal presentation now carries a generation token through frame submission, completion, and IOSurface assignment. IOSurfaceLayer validates lifetime, generation, update activity, and surface dimensions before applying synchronous or queued updates. ChangesMetal presentation generation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Metal
participant Frame
participant IOSurfaceLayer
Metal->>IOSurfaceLayer: Observe clamped presentation size
IOSurfaceLayer-->>Metal: Return presentation generation
Metal->>Frame: Begin frame with generation
Frame->>Metal: Present tokened frame with generation
Metal->>IOSurfaceLayer: Assign surface synchronously
IOSurfaceLayer-->>Metal: Return assignment result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryHarden Metal frame presentation against stale and late GPU completions.
Confidence Score: 5/5The PR appears safe to merge, with presentation ownership, invalidation, and stale-frame rejection consistently applied across the changed Metal paths. Synchronous frames have one explicit completion path, asynchronous frames retain the state needed by queued callbacks, and both paths reject invalid or mismatched presentation generations before touching layer state. Important Files Changed
Sequence DiagramsequenceDiagram
participant Renderer
participant Frame
participant GPU
participant Lifetime as PresentationLifetime
participant Layer as IOSurfaceLayer
participant Callback
Renderer->>Lifetime: observe drawable size
Renderer->>Frame: begin(generation)
Frame->>GPU: commit command buffer
GPU-->>Frame: completion
Frame->>Lifetime: acquire(generation)
alt lifetime and generation valid
Frame->>Layer: assign validated IOSurface
Frame-->>Renderer: return synchronous presentation
Renderer->>Renderer: unlock draw mutex
Renderer->>Callback: deliver acknowledgement
else invalidated or stale
Lifetime-->>Frame: reject acquisition
Frame-->>Layer: discard presentation
end
Reviews (1): Last reviewed commit: "Merge pull request #1 from x90skysn3k/fi..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/metal/IOSurfaceLayer.zig (1)
328-367: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPresentation tokens are dropped on every generation/size rejection path. Both the queued and synchronous presentation paths abandon the
FramePresentationwhen the new generation, activity, or size guard fails, so the embedder never receives the acknowledgement for that token.
src/renderer/metal/IOSurfaceLayer.zig#L328-L367: let the guard failures fall through to the delivery block at Lines 362-367 instead of returning fromsetSurfaceCallback, or document that rejected updates intentionally drop the token.src/renderer/metal/Frame.zig#L175-L186: whenpresentWithPresentationreturnsnull, still returnvalueso the generic caller delivers it afterdraw_mutexunlocks (or document the drop).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/metal/IOSurfaceLayer.zig` around lines 328 - 367, Ensure rejected presentations still deliver their presentation tokens: in src/renderer/metal/IOSurfaceLayer.zig lines 328-367, change the generation, activity, and size guard flow so it reaches the existing presentation delivery block instead of returning from setSurfaceCallback; in src/renderer/metal/Frame.zig lines 175-186, make the presentWithPresentation null result return value so the generic caller delivers it after draw_mutex is unlocked.
🧹 Nitpick comments (3)
src/renderer/Metal.zig (1)
399-409: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent drop when
setSurfaceSyncrejects.The
boolis discarded here, andsetSurfaceSynconly logs on the size-mismatch path — lease/generation rejection (Line 276 ofIOSurfaceLayer.zig) and the inactive-updates path returnfalsewith no trace. During a resize storm this presents as frozen output with nothing in the log. Alog.debugon the false branch would make it diagnosable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/Metal.zig` around lines 399 - 409, Add handling to the sync branch around setSurfaceSync so its returned bool is checked and a log.debug is emitted when it returns false, covering rejected leases/generations and inactive updates. Preserve the existing setSurface error propagation and successful sync behavior.src/renderer/metal/IOSurfaceLayer.zig (2)
418-422: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
create()hard-codesstd.heap.c_allocator.
CompletionLifetime.create(alloc)takes an allocator; this one does not, so allocation is untracked in tests and cannot use the renderer allocator. Consider threading the allocator throughIOSurfaceLayer.init()for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/metal/IOSurfaceLayer.zig` around lines 418 - 422, Update IOSurfaceLayer.create and the surrounding IOSurfaceLayer.init construction path to accept and use the caller-provided allocator instead of hard-coding std.heap.c_allocator. Thread that allocator through all relevant call sites, preserving existing initialization behavior while enabling renderer and test allocators to track the allocation.
410-448: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
acquirereturns a mutex-owning lease — document the discard hazard.
Liveholdsowner.mutexuntildeinit(). Any caller that discards the result (_ = lifetime.acquire(g);) or forgets thedeferleaves the lifetime mutex permanently locked, deadlocking every laterobserveSize/invalidate/acquire. A doc comment onacquire/Live(and ideallydefer live.deinit()immediately at each call site, which the current callers do) makes the contract explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/metal/IOSurfaceLayer.zig` around lines 410 - 448, Add documentation to the Live lease and acquire method stating that a successful acquire retains owner.mutex and must be paired with live.deinit(), including when the returned lease is otherwise unused. Verify each current acquire call site immediately defers deinit on the acquired lease, while preserving the existing invalid-generation null return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/renderer/metal/IOSurfaceLayer.zig`:
- Around line 328-367: Ensure rejected presentations still deliver their
presentation tokens: in src/renderer/metal/IOSurfaceLayer.zig lines 328-367,
change the generation, activity, and size guard flow so it reaches the existing
presentation delivery block instead of returning from setSurfaceCallback; in
src/renderer/metal/Frame.zig lines 175-186, make the presentWithPresentation
null result return value so the generic caller delivers it after draw_mutex is
unlocked.
---
Nitpick comments:
In `@src/renderer/Metal.zig`:
- Around line 399-409: Add handling to the sync branch around setSurfaceSync so
its returned bool is checked and a log.debug is emitted when it returns false,
covering rejected leases/generations and inactive updates. Preserve the existing
setSurface error propagation and successful sync behavior.
In `@src/renderer/metal/IOSurfaceLayer.zig`:
- Around line 418-422: Update IOSurfaceLayer.create and the surrounding
IOSurfaceLayer.init construction path to accept and use the caller-provided
allocator instead of hard-coding std.heap.c_allocator. Thread that allocator
through all relevant call sites, preserving existing initialization behavior
while enabling renderer and test allocators to track the allocation.
- Around line 410-448: Add documentation to the Live lease and acquire method
stating that a successful acquire retains owner.mutex and must be paired with
live.deinit(), including when the returned lease is otherwise unused. Verify
each current acquire call site immediately defers deinit on the acquired lease,
while preserving the existing invalid-generation null return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 364521f0-b79b-4c40-ae0b-142f2cf53258
📒 Files selected for processing (3)
src/renderer/Metal.zigsrc/renderer/metal/Frame.zigsrc/renderer/metal/IOSurfaceLayer.zig
1 participant
Fixes verified replay completion safety. Adds renderer lifetime invalidation, generation-based stale-frame rejection, and moves synchronous callback delivery outside the draw mutex while preserving current swap-chain drain and iOS queued behavior.\n\nVerified:
zig build test -Drenderer=metalpasses.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Hardens Metal presentation lifetimes to prevent stale or late GPU completions from mutating UI or renderer state after resize or teardown. Adds generation-based validation and moves sync presentation callback delivery outside the draw mutex while preserving swap-chain drain and iOS’s queued behavior.
IOSurfaceLayerlifetime rejects stale frames and cancels after teardown.presentation_generationis passed through frame/present paths.setSurfaceSyncvalidates generation and size; returns false instead of touching invalid state.draw_mutexunlocks (async and iOS unchanged).Written for commit d7df42d. Summary will update on new commits.
Summary by CodeRabbit