diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index a19fa96a..0d8d315f 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -307,8 +307,9 @@ namespace MobileGL::MG_Backend::DirectGLES { // whole death itself (MGPipeEmitVertexElementsDestroyAndFree: delete the // applier record, raise this notice, free the slot), because the slot is minted // client-side on every backend and a backend that installs no death ops - which - // Magma deliberately does not - otherwise leaked the slot and the record per - // VAO for the life of the process. What is left here is the one thing only this + // Magma did not until P7 wave 2 package C, and whose table even now only EMITS + // the wire record - otherwise leaked the slot and the record per VAO for the + // life of the process. What is left here is the one thing only this // side can do: drop the driver VAO the twin owns. It is raised while the handle // still resolves, so OnFrontendObjectDestroyed's shared free (which the other // five kinds still depend on) is simply the one that gets there first; the diff --git a/MobileGL/MG_Backend/DirectVulkan/BackendObject_DirectVulkan.cpp b/MobileGL/MG_Backend/DirectVulkan/BackendObject_DirectVulkan.cpp index 65093f57..64c8de26 100644 --- a/MobileGL/MG_Backend/DirectVulkan/BackendObject_DirectVulkan.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/BackendObject_DirectVulkan.cpp @@ -367,6 +367,15 @@ namespace MobileGL::MG_Backend::DirectVulkan { void BackendObject_DirectVulkan::Initialize() { #if MOBILEGL_BUILD_DISAGGREGATED VkBufferManager::RegisterWireResourceOps(); +#if MOBILEGL_PIPE_PUSH + // P7 wave 2 package C (CONTRACT-P7 §5.5). BESIDE the resource ops and for the same + // reason: this is the one place both Magma server roles pass through - the inproc + // role and the spawn/TCP session child - and ServerLoop::CreateBackend calls it at + // step 1 of InitServerRoleCommon, before the client session exists and therefore + // before any frontend object can die unheard. A per-transport install is how one of + // the two roles ends up without it. + InstallStateObjectDeathOps(); +#endif #endif m_initialized = true; } diff --git a/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp b/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp index 870cdc71..c1979280 100644 --- a/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp @@ -21,6 +21,13 @@ #include #include #include +#if MOBILEGL_PIPE_PUSH +// P7 wave 2 package C (CONTRACT-P7 §5.5): Magma's own death-notice table. Both headers are +// push-only and reached here for the same reason DirectGLES/Managers.cpp reaches them - the +// notice is declared by the frontend and answered by whichever backend is running. +#include +#include +#endif #endif #include #include @@ -55,6 +62,66 @@ namespace MobileGL::MG_Backend::DirectVulkan { std::atomic g_rendererGeneration{1}; } // namespace +#if MOBILEGL_BUILD_DISAGGREGATED && MOBILEGL_PIPE_PUSH + namespace { + // P7 wave 2 package C, CONTRACT-P7 §5.5: MAGMA'S MIRROR OF g_glesStateObjectDeathOps + // (DirectGLES/Managers.cpp:335), and the one arm of that table Magma has any work in. + // + // WHAT THE NOTICE IS FOR HERE, and it is not what it is for on Espryt. Espryt's arm + // destroys a BACKEND TWIN keyed by lifetime id; Magma keeps no such twin - its two + // client-minted kinds (VertexElementsCso, Buffer) are keyed on {slot, gen} by + // MagmaPipeIdentityTables and every other kind is still reached from its frontend + // object. So the switch Espryt runs under `#else` has nothing to do on this backend + // and this table is exactly the EMIT arm: with a transport up and off the apply + // thread, the death crosses as the `object_death` record + // (MG_Remote::Client::EmitObjectDeathRecord, CONTRACT-P5C §5.2). + // + // WHY THAT MATTERS, measured rather than argued (package L's probe, + // notes/p7/magma-two-process-first-run.md §6): under SPAWN the client process has no + // DirectVulkan backend at all, so InstallClientWireTables' own emitter already + // answered the notice and CtWireScenario's two death cases PASSED. Under INPROC the + // server role's backend is a thread of this process, Transport is Split rather than + // Spawn, and WireTables' install condition deliberately stands aside for "the + // backend's own dispatcher" - which on Magma did not exist. Both cases therefore + // FAILED on inproc with `deaths` stuck at 0. This table is that dispatcher. + // + // FRAMEBUFFER IS WHY THE GAP WAS NOT MERELY COSMETIC. The client-side SLOT is freed + // backend-neutrally by P4a's per-kind helpers (MG_Impl/Pipe/PipeFill.cpp's + // NotifyAndFree frees whatever the notice does), so PipeSlotPeek never saw a leak. + // What leaked was the SERVER's twin: a framebuffer has NO wire delete opcode at all + // (BRIEF-P4A D-I2), so `object_death` is its only death delivery, and without a + // consumer here the server's record stayed Live for the life of the session. + void OnMagmaFrontendStateObjectDestroyed(MG_Pipe::MGPipeKind kind, Uint64 lifetimeId) { + // Monolith has no record to emit and no server twin to tell. + if (MG_Config::Transport == MG_Config::TransportMode::Monolith) return; + // Raised ON the apply thread: the server role destroying a frontend object it + // created itself (Magma's hidden blit / depth-mipmap resources). The client's + // allocator is a client-thread surface (CONTRACT-P5C §3.1) and EmitObjectDeathRecord + // probes it, so this arm stays silent exactly as Espryt's does. + if (MG_Remote::Server::ServerLoop::OnApplyThread()) return; + (void)MG_Remote::Client::EmitObjectDeathRecord(kind, lifetimeId); + } + + const MG_State::GLState::StateObjectDeathOps g_magmaStateObjectDeathOps = { + .OnDestroyed = OnMagmaFrontendStateObjectDestroyed, + }; + } // namespace + + void InstallStateObjectDeathOps() { + // Installed from BackendObject_DirectVulkan::Initialize(), i.e. step 1 of + // InitServerRoleCommon (ServerLoop::CreateBackend calls Initialize), which is before + // the client session starts and therefore before any frontend object can die. + // Unconditional, exactly as Espryt's ResolveEsprytSlotTablesArm is: only one backend + // is live at a time, and InstallClientWireTables' own "whoever installed first keeps + // the notice" guard is what keeps the remote client from stomping this. + MG_State::GLState::SetStateObjectDeathOps(&g_magmaStateObjectDeathOps); + } + + Bool StateObjectDeathOpsInstalled() { + return MG_State::GLState::GetStateObjectDeathOps() == &g_magmaStateObjectDeathOps; + } +#endif // MOBILEGL_BUILD_DISAGGREGATED && MOBILEGL_PIPE_PUSH + Uint64 GetRendererGeneration() { return g_rendererGeneration.load(std::memory_order_acquire); } diff --git a/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.h b/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.h index cdf36141..03437024 100644 --- a/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.h +++ b/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.h @@ -23,6 +23,18 @@ namespace MobileGL::MG_Backend::DirectVulkan { Uint64 GetRendererGeneration(); void BumpRendererGeneration(); +#if MOBILEGL_BUILD_DISAGGREGATED && MOBILEGL_PIPE_PUSH + // P7 wave 2 package C (CONTRACT-P7 §5.5). Magma's StateObjectDeathOps table, installed + // from BackendObject_DirectVulkan::Initialize() so that GetStateObjectDeathOps() is + // non-null on every Magma role that owns a backend. See the definition for what the one + // arm does and for the inproc gap it closes. Push-only and disaggregated-only: a pull + // build declares no notice at all (StateObjectDeathNotice.h) and has nothing to emit. + void InstallStateObjectDeathOps(); + // True when the notice currently installed is this backend's. Read by MG_Test so the + // install can be asserted without a death having to happen first. + Bool StateObjectDeathOpsInstalled(); +#endif + // Drops every cached program-resource reflection entry (CPU-side strings/vectors // only, no Vulkan handles). Called at EGL teardown next to the renderer reset; // safe because GL calls are serialized in this codebase, and any still-live diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaPipeArms.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaPipeArms.h index 37c76d25..962148f8 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaPipeArms.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/MagmaPipeArms.h @@ -166,9 +166,17 @@ namespace MobileGL::MG_Backend::DirectVulkan { // WHAT IT DOES NOT COVER, AND WHY NO REPRODUCER OF THIS SHAPE CAN [fix-aba review v1, // MAJOR 1]. It does NOT exercise the GENERATION half of {slot, gen}: // - // * this mint has no death notification - nothing in MG_Backend/DirectVulkan consumes - // NotifyStateObjectDestroyed - so a slot returns to the free list only through - // OnFrameBoundary's age sweep (kSweepInterval 256, kRetireAgeBoundaries 1024, below); + // * THIS MINT still has no death notification, and that is now a narrower statement than + // it was. P7 wave 2 package C gives DirectVulkan a StateObjectDeathOps table + // (DirectVulkan.cpp's g_magmaStateObjectDeathOps, CONTRACT-P7 §5.5), so + // NotifyStateObjectDestroyed DOES have a consumer on this backend - but that consumer's + // one job is to emit the `object_death` RECORD so the server drops its twin, and it + // never touches these per-renderer identity tables: they are the SERVER-side renderer's + // own, the notice is raised on the CLIENT thread, and an Acquire/retire from the wrong + // side is precisely the cross-role read CONTRACT-P5C §3.1 forbids. So a slot here still + // returns to the free list only through OnFrameBoundary's age sweep (kSweepInterval + // 256, kRetireAgeBoundaries 1024, below), and everything the two bullets below derive + // from that is unchanged; // * HandleRecycleScenario issues five frame boundaries, so the free list is empty when // the replacement VAO acquires and it gets a BRAND-NEW slot at Gen 1 (measured: // redVao slot=2 gen=1, greenVao slot=3 gen=1). The knob-off FRESH verdict there is diff --git a/MobileGL/MG_IntegrationTest/Harness/PipeSlotPeek.h b/MobileGL/MG_IntegrationTest/Harness/PipeSlotPeek.h index 530d36a3..8bb8c530 100644 --- a/MobileGL/MG_IntegrationTest/Harness/PipeSlotPeek.h +++ b/MobileGL/MG_IntegrationTest/Harness/PipeSlotPeek.h @@ -13,8 +13,19 @@ // the leak it rules out is entirely inside the library, and it is invisible in pixels, in GL // names and in `glGetError` - so the only honest observable is the allocator's own live count // and high-water mark. Reading them is what makes the case fail on the backend it actually -// failed on (DirectVulkan, which installs no StateObjectDeathOps) rather than only on the one -// where a backend-owned free happened to exist. +// failed on (DirectVulkan, which at P3a installed no StateObjectDeathOps) rather than only on +// the one where a backend-owned free happened to exist. +// +// P7 wave 2 package C: DirectVulkan now installs a table of its own (CONTRACT-P7 §5.5), and +// the sentence above is history rather than the current state - but the reason this header +// exists is UNCHANGED and is worth spelling out, because the obvious reading of that news is +// wrong. Magma's table does not free a client slot and never could: the free is P4a's, it +// runs from the frontend object's own destructor on the client thread whatever backend is +// live (MG_Impl/Pipe/PipeFill.cpp's NotifyAndFree), and Magma's table is the EMIT arm that +// tells the SERVER to drop its twin. So the allocator is still the only observable of the +// client half, and a case that reads `deaths` alone would be blind to a slot that never came +// back. The two readings belong together, which is what CtWireScenario's framebuffer case now +// does. // // A separate translation unit for BackendCapsPeek.h's reason, verbatim: the scenario sources // include the GL headers with prototypes and MobileGL's umbrella header is not meant to meet diff --git a/MobileGL/MG_IntegrationTest/Scenarios/CtWireScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/CtWireScenario.cpp index 4bf1f13f..2295dd01 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/CtWireScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/CtWireScenario.cpp @@ -25,6 +25,7 @@ // half of that control is TheDirectApplierResetCallOnTheGLThreadIsRoleViolation below, and // the guard's two non-Fatal arms are pinned in PipeWireCodecTest's CtWireFatals suite. +#include "../Harness/PipeSlotPeek.h" #include "../Harness/PipeStatsWindow.h" #include "../Harness/ScenarioFixture.h" #include "../Harness/SplitRuntimePeek.h" @@ -147,13 +148,16 @@ namespace { TEST_F(CtWireScenario, TextureDeathCrossesAndTheRecycledSlotAnswersTheNewObject) { if (!Ready()) return; - // The object_death producer is Espryt-side (OnFrontendStateObjectDestroyed, - // CONTRACT-P5C.md §5.2); Magma installs no StateObjectDeathOps (P7), so under - // DirectVulkan there is no death record to watch and the case has nothing to prove. - if (HeadlessGL::Get().BackendName() != "DirectGLES") { - GTEST_SKIP() << "object_death is produced by the DirectGLES death-notice ops; " - "DirectVulkan has none until P7"; - } + // P7 wave 2 package C: THE BACKEND GUARD IS GONE, and both halves of why it was here + // are answered rather than waived. The producer used to be Espryt-side only + // (OnFrontendStateObjectDestroyed, CONTRACT-P5C.md §5.2), so a DirectVulkan run had + // no death record to watch; Magma now installs its own StateObjectDeathOps + // (DirectVulkan.cpp, CONTRACT-P7 §5.5) and emits the same `object_death`. Package L + // measured that the SPAWN arm already passed once the WireTables install condition + // went backend-agnostic and that the INPROC arm failed here with `deaths` 0 vs 0 + // (notes/p7/magma-two-process-first-run.md §6); the table is what closes the inproc + // half, and this case is the gate on it. It is also the RED-ONCE for that table: with + // the install site short-circuited, both cases red on the two EXPECT_GT below. const GLubyte red[4] = {255, 0, 0, 255}; const GLubyte green[4] = {0, 255, 0, 255}; @@ -204,14 +208,13 @@ namespace { TEST_F(CtWireScenario, FramebufferDeathCrossesAndTheRecycledSlotAnswersTheNewObject) { if (!Ready()) return; - // Same producer reason as the texture case above: object_death is emitted by the - // DirectGLES death-notice ops; DirectVulkan has none until P7. - if (HeadlessGL::Get().BackendName() != "DirectGLES") { - GTEST_SKIP() << "object_death is produced by the DirectGLES death-notice ops; " - "DirectVulkan has none until P7"; - } + // Backend-agnostic for the texture case's reason (P7 wave 2 package C): both + // backends now install a StateObjectDeathOps table and both emit `object_death`. // Framebuffer is the kind object_death EXISTS for: it has no other wire delete - // opcode (CONTRACT-P5C.md §5.2). The renderbuffer goes along so the FBO has storage. + // opcode (CONTRACT-P5C.md §5.2) - which is exactly why the missing Magma table was + // a SERVER-side twin leak and not merely a missing counter: with no consumer for + // the notice, nothing on the wire ever told the server this framebuffer had died. + // The renderbuffer goes along so the FBO has storage. GLuint fbo = 0, renderbuffer = 0; glGenFramebuffers(1, &fbo); glGenRenderbuffers(1, &renderbuffer); @@ -234,6 +237,21 @@ namespace { const auto beforeCounters = ServerCounters(); ASSERT_TRUE(beforeCounters.valid) << "server object-death telemetry missing"; const MobileGL::Uint64 deathsBefore = beforeCounters.deaths; + // THE CLIENT'S OWN ACCOUNTING, read beside the server's (P7 wave 2 package C, + // CONTRACT-P7 §5.5's PipeSlotPeek clause). The two numbers answer DIFFERENT halves of + // one death and neither implies the other, which is exactly why both are read here: + // * `deaths` below says the SERVER was told - the half the missing Magma table + // broke, and the only delivery a framebuffer has (no wire delete opcode at all); + // * this one says the CLIENT returned the slot. That half has been backend-neutral + // since P4a (PipeFill.cpp's NotifyAndFree frees whatever the notice does), so it + // is a CONTROL rather than the subject: if it ever regressed, a green `deaths` + // would be measuring an object that never let go of its handle. + // `false` means the allocator is out of reach (a pull build, or Android's hidden + // symbols), and the peek is then simply not asserted - "could not look" is not + // "did not leak", so nothing is concluded from it either way. + unsigned fboSlotsBefore = 0; + const bool slotsReadable = + PeekPipeSlotLiveCount(PipeSlotKind::Framebuffer, &fboSlotsBefore); glDeleteFramebuffers(1, &fbo); // THE FENCE (MOBILEGL_IPC_BATCH_WAITS, default on): object_death is a kCtxObject // value-class record - published WITHOUT waiting for its own apply (production-safe: @@ -245,7 +263,16 @@ namespace { ASSERT_TRUE(afterCounters.valid) << "server object-death telemetry missing"; EXPECT_GT(afterCounters.deaths, deathsBefore) << "the framebuffer's death produced no object_death record - and no other " - "opcode can carry it"; + "opcode can carry it. Backend " + << Gl().BackendName(); + if (slotsReadable) { + unsigned fboSlotsAfter = 0; + ASSERT_TRUE(PeekPipeSlotLiveCount(PipeSlotKind::Framebuffer, &fboSlotsAfter)); + EXPECT_LT(fboSlotsAfter, fboSlotsBefore) + << "the dead framebuffer kept its client slot: live count " << fboSlotsBefore + << " -> " << fboSlotsAfter << " across the delete. Backend " + << Gl().BackendName(); + } glDeleteRenderbuffers(1, &renderbuffer); // Recycle: a new FBO and a new backing store, cleared to a different colour. A stale diff --git a/MobileGL/MG_IntegrationTest/Scenarios/HandleRecycleScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/HandleRecycleScenario.cpp index e066b92d..f45eb450 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/HandleRecycleScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/HandleRecycleScenario.cpp @@ -1394,13 +1394,21 @@ void main() { oColor = texture(uTex, vUv); } // back? Until C-1 the answer under DirectVulkan was NO. The mint is the client's // (MGPipeVertexInputEmitter::EmitVertexElements acquires a VertexElementsCso slot at // every validate point with a VAO bound) and the only free in the tree was Espryt's - // StateObjectDeathOps consumer - so under Magma, which installs none deliberately + // StateObjectDeathOps consumer - so under Magma, which installed none // (MagmaPipeArms.h: "an allocator here would grow by one SlotState plus one map node // per object EVER created, for the life of the process, on a platform with an LMK"), // every VAO ever created held its slot and its ~1.3 KB applier record until the process // died, on the shipped 0x1ff mask, until create_vertex_elements began tripping // Fatal{ProtocolCorruption} permanently at kMGPipeMaxVertexElementsSlots. // + // P7 WAVE 2 PACKAGE C DOES NOT RETIRE THIS CASE, and the reason is the whole point of + // C-1. Magma now installs a StateObjectDeathOps table (CONTRACT-P7 §5.5), but that + // table's one arm EMITS the `object_death` record so the SERVER drops its twin - it + // frees no client slot and could not: the free is the client's own, from the frontend + // destructor, on the client thread. So this case still measures the only thing that + // ever protected the allocator, and it would still be the case that goes red if that + // free were ever moved back behind a backend table. + // // THE OBSERVABLE IS THE ALLOCATOR, not pixels: this leak produces correct pictures the // whole way to the fatal, which is exactly why the four cases above ran green over it. // It runs on BOTH backends' Handles lanes; DirectVulkan is where it was red. diff --git a/MobileGL/MG_Pipe/PipeMutation.h b/MobileGL/MG_Pipe/PipeMutation.h index b46b8a46..1e47a7b8 100644 --- a/MobileGL/MG_Pipe/PipeMutation.h +++ b/MobileGL/MG_Pipe/PipeMutation.h @@ -139,8 +139,9 @@ namespace MobileGL::MG_Pipe { // THE VERTEX-ELEMENTS CSO's DEATH, and it is BACKEND-NEUTRAL - which is the whole point. // Before this, the only thing that ever returned a VertexElementsCso slot was DirectGLES' - // StateObjectDeathOps table; under any backend that installs none - DirectVulkan/Magma, - // which keeps its own age-reclaimed identity table on purpose - every VAO ever created + // StateObjectDeathOps table; under a backend that installed none - DirectVulkan/Magma at + // P3a, which keeps its own age-reclaimed identity table on purpose and which P7 wave 2 + // package C gave an EMIT-only table that still frees no slot - every VAO ever created // held its slot and its ~1.3 KB applier record for the life of the process, on the shipped // 0x1ff mask, and past 65536 slots every create_vertex_elements became a permanent // Fatal{ProtocolCorruption}. The client mints the slot, so the client is where the death diff --git a/MobileGL/MG_Remote/Client/WireTables.cpp b/MobileGL/MG_Remote/Client/WireTables.cpp index 38d0ceba..d2a5b1b1 100644 --- a/MobileGL/MG_Remote/Client/WireTables.cpp +++ b/MobileGL/MG_Remote/Client/WireTables.cpp @@ -757,13 +757,21 @@ namespace MobileGL::MG_Remote::Client { // That is what "keep inproc's existing backend dispatcher" meant, and it is also // what keeps this from stomping a backend that installs ops of its own later. // - // WHAT THIS DOES *NOT* DO IS GIVE MAGMA A DEATH TABLE. Magma installs no - // StateObjectDeathOps at all (wave 2 package C mirrors g_glesStateObjectDeathOps for - // it); until it does, the client-side emitter installed here still asks - // MGPipeSlots().FindByLifetimeId for a handle, gets a null one for an object Magma - // never routed, and emits nothing. The count under Magma may therefore still be 0 - - // but it is now 0 because no object crossed, which the lane can see and attribute, - // instead of 0 because nobody was listening, which it could not. + // MAGMA NOW HAS A DEATH TABLE OF ITS OWN (P7 wave 2 package C, CONTRACT-P7 §5.5: + // DirectVulkan.cpp's g_magmaStateObjectDeathOps), and the two installs do NOT race, + // which is worth stating because the global is a bare last-writer-wins pointer with + // no stacking. They cannot meet: Magma's is installed from + // BackendObject_DirectVulkan::Initialize(), i.e. only in a process that OWNS a + // DirectVulkan backend, and the arm here runs only under `Transport == Spawn`, which + // is by construction the process that owns NO backend at all (InitSplitRoles builds a + // BackendObject_Remote there). So the spawn client keeps this emitter, the inproc and + // server-side roles keep Magma's, and `GetStateObjectDeathOps() == nullptr` below + // keeps meaning what P6.5 meant by it. + // + // MEASURED, both directions (package C's red-once): with Magma's install + // short-circuited, the two CtWireScenario death cases red on the INPROC arm alone + // (`deaths` 0 vs 0) and stay green on spawn and tcp - which is exactly the split of + // responsibility this comment claims. if (MG_Config::Transport == MG_Config::TransportMode::Spawn && MG_State::GLState::GetStateObjectDeathOps() == nullptr) { MG_State::GLState::SetStateObjectDeathOps(&kClientStateObjectDeathOps); diff --git a/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.cpp b/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.cpp index fe9222ff..d0f3e325 100644 --- a/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.cpp +++ b/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.cpp @@ -50,9 +50,12 @@ namespace MobileGL::MG_State::GLState { // ALL FOUR OF THOSE GO THROUGH ONE HELPER, and it is the client's rather than a // backend's: the handle is minted by MGPipeVertexInputEmitter on every backend, so a // death path that only exists inside a backend's death-ops table is no path at all - // under a backend that installs none - which is what DirectVulkan/Magma does on - // purpose, and what made every VAO leak a slot and a ~1.3 KB applier record for the - // life of the process on the shipped mask. The helper emits delete_vertex_elements, + // under a backend that installs none - which DirectVulkan/Magma did, and what made + // every VAO leak a slot and a ~1.3 KB applier record for the life of the process on + // the shipped mask. P7 wave 2 package C gave Magma a table (CONTRACT-P7 §5.5) and the + // argument is UNCHANGED, because that table emits the wire death record and frees no + // slot: the client's helper is still the only path any of the four take. The helper + // emits delete_vertex_elements, // raises the notice (a no-op unless a backend registered the ops) and frees the slot, // in that fixed order; MG_Pipe/PipeMutation.h and its definition say why each position // is where it is. The buffer's death has exactly this shape one file over diff --git a/docs/Disaggregated/notes/p7/magma-c.md b/docs/Disaggregated/notes/p7/magma-c.md new file mode 100644 index 00000000..fd652eb1 --- /dev/null +++ b/docs/Disaggregated/notes/p7/magma-c.md @@ -0,0 +1,80 @@ +# P7 wave 2 package C — Magma object death, resident sub-data, the reflection archive, the vertex-layout split + +Base: `p7/magma-c` off `feat/disaggregated @ a686131e` (pipe HEAD at branch time). +Build: `build-split` = `-DMOBILEGL_BUILD_DISAGGREGATED=ON -DMOBILEGL_BUILD_DISAGGREGATED_INPROC=ON +-DMOBILEGL_PIPE_PUSH=ON -DMOBILEGL_BUILD_INTEGRATION_TEST=ON`, ICD pinned to +`/usr/share/vulkan/icd.d/lvp_icd.json` (the same lavapipe ICD `~/w7/pipe/build-split` pins; the +worktree tool leaves `MOBILEGL_ITEST_VK_ICD` empty and CMake warns about it). + +Contract rows: CONTRACT-P7 §5.5 (`StateObjectDeathOps`), §5.4 (OQ-10), §5.3 (OQ-8), §3.2 +(`vertex-layout`, `vertex-format-conversion`). Discipline: rule I (no `std::abort()` on a Magma +wire arm), rule J / R-16 (every fix red once on a two-process arm first). + +--- + +## Slice 1 — Magma `StateObjectDeathOps` (§5.5) + +**What landed.** `MG_Backend/DirectVulkan/DirectVulkan.cpp` gains +`g_magmaStateObjectDeathOps` and `InstallStateObjectDeathOps()`, installed from +`BackendObject_DirectVulkan::Initialize()` — i.e. from `ServerLoop::CreateBackend`, step 1 of +`InitServerRoleCommon`, which both Magma server roles (inproc and the spawn/TCP child) run and +which is before any client session exists. Guarded `#if MOBILEGL_BUILD_DISAGGREGATED && +MOBILEGL_PIPE_PUSH`, so the pull build compiles nothing new (G1). + +**The one arm, and why it is only one.** Espryt's `g_glesStateObjectDeathOps` has two jobs: emit +the wire `object_death` record off the apply thread, and otherwise destroy a backend twin keyed +by lifetime id. Magma has no lifetime-id-keyed twin at all — its two client-minted kinds +(`VertexElementsCso`, `Buffer`) are keyed on `{slot, gen}` by `MagmaPipeIdentityTables` and the +other six are still reached from their frontend objects — so the Magma table is exactly the EMIT +arm and nothing else. + +**Correction to the plan's wording (worth the integrator's attention).** Plan §1.3 and the task +brief describe the gap as a client SLOT leak provable with `PipeSlotPeek`. It is not, and has not +been since P4a: `MG_Impl/Pipe/PipeFill.cpp`'s `NotifyAndFree` frees the client slot +unconditionally and backend-neutrally, so `PipeSlotPeek` never saw a Magma leak and does not see +a change now. What leaked was the **server's twin**. A framebuffer has no wire delete opcode at +all (BRIEF-P4A D-I2), so `object_death` is its only death delivery; with no consumer installed +under Magma inproc, the server's record stayed `Live` for the life of the session. The +`CtWireScenario` framebuffer case now reads BOTH numbers — the server's `deaths` tally and the +client's `PipeSlotKind::Framebuffer` live count — so a future regression in either half is +attributable. + +**Cleanup of the Magma special cases.** The two executable ones are the `CtWireScenario` skip +guards (`:153` / `:209`), both deleted. Everything else the audit counted is comment text, and it +is reworded rather than deleted because the underlying argument survives: `PipeSlotPeek.h`, +`MagmaPipeArms.h` (the age-sweep-instead-of-death-notice paragraph — still true, because the new +table never touches the per-renderer identity tables), `PipeMutation.h`, `PipeFill.cpp`, +`VertexArrayObject.cpp`, `DirectGLES/Managers.cpp`, `HandleRecycleScenario.cpp`, +`WireTables.cpp`. + +**The install-order question, answered rather than assumed.** `SetStateObjectDeathOps` is a bare +last-writer-wins pointer with no stacking, so two installers could in principle race. +They cannot meet: Magma's runs only in a process that OWNS a DirectVulkan backend, and +`InstallClientWireTables`' arm runs only under `Transport == Spawn`, which is by construction the +process that owns no backend (`InitSplitRoles` builds a `BackendObject_Remote` there). The +red-once below measures exactly that split of responsibility. + +**Proof (green).** `ctest -R "DirectVulkan\.(Split|Spawn|Tcp)\.Ct\..*Death"` — 6/6 pass +(`integration-magma-all-{split,spawn,tcp}`), plus the two `TcpServer` fixtures. DirectGLES +control `ctest -R "DirectGLES\.(Split|Spawn|Tcp)\.Ct\..*Death"` — 6/6 pass, unchanged. + +**RED-ONCE (executed).** `BackendObject_DirectVulkan::Initialize()`'s call changed to +`if (false) InstallStateObjectDeathOps();`, rebuilt, lane re-run: + +``` +1/8 DirectVulkan.Split.Ct.CtWireScenario.TextureDeath... ***Failed +2/8 DirectVulkan.Spawn.Ct.CtWireScenario.TextureDeath... Passed +4/8 DirectVulkan.Tcp.Ct.CtWireScenario.TextureDeath... Passed +5/8 DirectVulkan.Split.Ct.CtWireScenario.FramebufferDeath... ***Failed +6/8 DirectVulkan.Spawn.Ct.CtWireScenario.FramebufferDeath... Passed +7/8 DirectVulkan.Tcp.Ct.CtWireScenario.FramebufferDeath... Passed +75% tests passed, 2 tests failed out of 8 + +CtWireScenario.cpp:264: Failure +Expected: (afterCounters.deaths) > (deathsBefore), actual: 0 vs 0 +``` + +Restored and rebuilt: 8/8 green again. This is precisely the shape package L's temporary probe +predicted (`notes/p7/magma-two-process-first-run.md` §6): spawn and tcp were already carried by +`InstallClientWireTables`' emitter, and **inproc was the only hole** — which is the hole this +table closes.