[lawrencecchen]

Summary

IOSurfaceLayer.setSurfaceOffMain began and committed a Core Animation transaction before its checks. So every frame that it did not show still committed an empty transaction on the GPU completion thread:

• frames that go through main: a new size while the embedder presents host sizes, a frame the embedder has not committed, or another size than the shown one;
• frames that a newer frame superseded.

cmux FRAME OWNER REFACTOR S4 measured this with Time Profiler (cmux step 4b41c42c, held keys-stress, 14 terminals, motion.frameOwner.terminals on): 405 ms of CA::Context::commit_transaction under the GPU completion queue in one round. Presents per round went from 84 (switch off) to about 850 (switch on). The new IOSurface per size cost only about +100 ms in the same round, and WindowServer's create_surface was the same with the switch off and on.

The checks now run first, read-only, under the Core Animation lock (offMainDecisionLocked). Only a frame that it shows begins a transaction. The transaction begins under the lock (a begin commits nothing), takes the contents, and commits after the unlock. So the invariant "no commit while holding the lock" still holds. Renderer mode (the default) gets the same change: a superseded frame no longer commits.

No C API change. No change to which frame is shown, or when.

Testing

• Red first: 694f7ffb7 adds host present: an off-main present that does not show its frame commits no transaction. It counts the presenting thread's commits with the existing CommitOrder swizzle, for each refusal (no host size, another size than the host's, a layer that shows another size, a present the embedder has not committed, an older frame), and expects one commit, outside the lock, for a frame that it shows.
• Fork CI, host present: filter only (scratch branches):
  • Red at 694f7ffb7, run 41953548703: 110/111 tests pass. Only the new test fails, at IOSurfaceLayer.zig:862, expected 0, found 1 (a layer that shows another size: the off-main attempt committed an empty transaction).
  • Green at 7f2dcbf36, run 41953551466: 111/111 tests pass, and lib-vt 97/97.
• After merge: release build, cmux pin PR, then the 8-row gate A/B and a profile on cmux24-perf, switch off and on.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 2d97ad66-e674-46d0-8ddc-a7203860f5ca

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[lawrencecchen]

Review (independent subagent): SHIP at 7f2dcbf

I read the diff of each commit myself and did not use the PR description. I did not build anything locally. The CI evidence comes from the fork runs. Line numbers refer to src/renderer/metal/IOSurfaceLayer.zig at the head.

1. Is the decision the same as before? Yes. The order and the return values match on every path.

• The host size check still runs first, before the lock and the transaction (:250-252). It returns false as before.
• offMainDecisionLocked (:289-299) runs the old checks in the old order:
  • nothing shown → .through_main → false
  • shown size differs → .through_main → false
  • seq <= presentSeq → .superseded → true
  • embedderPresents and mainPending → .through_main → false
  • otherwise .show, which sets presentSeq, sets contents and returns true (:272-276)
• The only change is that .through_main and .superseded no longer begin or commit a transaction.
• The caller in Metal.zig:473 reacts only to the return value, and that value does not change. embedderShowsKeptFrame (Metal.zig:505) still runs only after a false.

2. Lock, begin and commit order.

• Every switch branch unlocks before it returns (:259-268). The show path runs lock → begin → set → unlock → commit (:272-276).
• No code between the lock and the unlock can return an error, so the lock is never left held. Each begin has exactly one commit.
• Nothing commits under the lock. The new test checks this (seen_commits_under_lock == 0), and so do the two existing commit-order tests.
• The lock is recursive. A begin only pushes a transaction level and commits nothing. The old code already set contents under the lock, so the only new thing under the lock is begin. I see no deadlock risk.

3. Could the contents change land in an implicit transaction? No. begin (:272) runs before setProperty("contents") (:274), as in the old code. The decision now reads contents and the IOSurface size before begin. A getter does not open an implicit transaction, and this thread already reads layer properties with no explicit transaction (embedderShowsKeptFrame → embedderDrops reads contents, bounds and contentsScale).

• Residual gap, not blocking: the swizzle counts only +[CATransaction commit] calls. The test could not catch a nested or implicit transaction that delays the flush.

4. Does the test prove the fix?

• Red run 41953548703 (scratch 274689c = 694f7ff plus a workflow-only change) fails at IOSurfaceLayer.zig:862 with expected 0, found 1. That is case 3: the host has the new size but the layer shows the old size.
• Green run 41953551466 (scratch be25ec6 = 7f2dcbf plus a workflow-only change) has 111/111 "host present:" tests passing. That includes the new test and the 20 Metal.zig host-present tests.
• Cases 1 (no host size) and 2 (frame not at host size) return before the old begin (:250-252). They were already green on the base, so they guard against regressions and are not red evidence.
• The red run stops at case 3, so it never reached cases 4 (main pending) and 5 (superseded). In the old code begin ran before all of these checks, so both would also fail without the fix.
• Case 6 (shown frame, exactly 1 commit, none under the lock, contents == next) is green before and after the fix. It is a guard, not red evidence.
• No case is vacuous. Each case checks both presented and the commit count.
• The harness is safe across repeated calls:
  • Each testOffMainCommits call spawns a new thread, so the threadlocal depth, commits and commits_under_lock start at 0.
  • presentAt writes seen_* on every call, and join orders that write before the read.
  • install and remove restore the original implementations each time, so the next install gets the real originals again.
  • The main thread makes no CATransaction calls while the swizzle is installed.

5. Other findings. None that block. The sequence numbers in the test are correct: presentSeq is 4 after setSurfacePresented, so seq 3 is superseded and seq 6 shows. The doc comments still match the code.

• Cosmetic: case 3 calls setEmbedderPresents(true) and setEmbedderCommitted() before a check that does not need either. This is harmless.

Review (independent subagent): SHIP at 7f2dcbf