Skip to content

Follow up the Metal seams review: pin the handler lifetime and tidy the minors - #43

Merged
Xget7 merged 7 commits into
mainfrom
followup-ios
Oct 20, 2026
Merged

Xget7 merged 7 commits into
mainfrom
followup-ios

Conversation

@Xget7

@Xget7 Xget7 commented Oct 20, 2026 •

Copy link
Copy Markdown
Owner

Summary

Follow-ups from the review of #41. The destroy-in-flight guard was never seen red and could not fail in CI; it is now pinned at the MetalFrameCompute level, with the minor review items alongside.

MetalFrameComputeTest (runs on the hosted runners through test::Gpu)
  encode selection + sort, leave both uncommitted
  keep a copy of the loop's failure latch
  destroy the compute, then the loop
  assert use_count >= 2        (this test + the report, which the handlers keep alive and which owns the latch)
  commit, wait, assert both buffers completed and the latch is unset
 tests/rendering/metal/
 ├── MetalSplatRendererTest.mm      # destroy-in-flight test renamed: it is a smoke test
 ├── cull/MetalVisibilityPassTest.mm
+│   # MetalPrepareDrawArgumentsTest: exactly kDrawBatches records, sentinel guard after them
 ├── frame/MetalFrameComputeTest.mm
+│   # handlers keep the state they write alive after the compute is gone
 └── presentation/MetalFrameLoopTest.mm
-    # kSlots = 2
+    # sized from kFramesInFlight

Also in this PR:

  • markFailed() is removed; its two test callers store through failureLatch().
  • fallback.tileSize, fallback.lodErrorPixels and the LOD threshold default are named; toCppPolicy uses the SortKeyBits values instead of 16 and 32.
  • The MetalSurface.h and MetalFrameSlots.h comments keep only what the code cannot show; the render handler's capture states that the semaphore signal is its last action.
  • ACameraRequestRoundTripsThroughTheStateMirror is removed: it copied the fields by hand and exercised no production code.

Behaviour does not change.

Evidence

  • Before: with the handlers' report capture reverted locally to a raw pointer (the pre-fix shape), never committed, the new test fails without AddressSanitizer:

    MetalFrameComputeTest.SelectionAndSortHandlersKeepTheStateTheyWriteAliveAfterTheComputeIsGone
    MetalFrameComputeTest.mm:265: Failure
    Expected: (failed.use_count()) >= (2), actual: 1 vs 2
    [  FAILED  ] 1 test
    

    After: with the fix restored it passes, and the full suite passes:

    [       OK ] MetalFrameComputeTest.SelectionAndSortHandlersKeepTheStateTheyWriteAliveAfterTheComputeIsGone
    100% tests passed, 0 tests failed out of 386
    

    GpuReport owns the frame loop's failure latch and the handlers capture only the report, so the latch count reaches 2 only while a handler owns the report.
    The assertion is on ownership, checked before any handler runs, so it is deterministic, and >= 2 does not assume how many copies of a handler block Metal keeps.
    It does not observe a use-after-free itself; it fails when no handler owns the report, which is the property the fix adds.

  • Where it ran: Apple GPU on a Mac with the Metal test executable. No physical device and no simulator; no device is attached and none was run.

  • Checks:

    • ctest: 386 of 386 pass, up from 385 (two tests added, one removed). The same 5 fixture tests skip as before.
    • Shader text hash unchanged: 61ea372e20f1cd4f65eeb83e1a8da7f6327e9261.
    • scripts/lint-cpp.sh: clean.
    • scripts/build-ios.sh and scripts/package-ios.sh: exit 0.
    • Not run: sdk_harness.py, npm run check (nothing they cover changed).
  • M8: no existing test pinned that the prepare shader stops at kDrawBatches records; the visibility tests only read the first kDrawBatches. The new test fills extra records with a sentinel and asserts they are untouched.

Merge Danger

Door: two-way

Tests, comments, named constants and one removed unused method. Every change reverts with a plain revert.

Blast Radius: tests

The only non-test code changes name existing values (toCppPolicy accepts the same 16 and 32) and delete MetalFrameLoop::markFailed(), which had no callers outside tests.

@Xget7
Xget7 merged commit 85473b4 into main Oct 20, 2026
22 checks passed
@Xget7
Xget7 deleted the followup-ios branch October 20, 2026 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant