diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index ca1ca669..448f9f92 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -178,9 +178,16 @@ namespace MobileGL::MG_Backend::DirectGLES { // for all six - which is why the notice carries the kind rather than there being six // ops tables. // + // Each arm below names the registry GLOBAL of its kind, but DestroyByLifetimeId is + // static: it is answered by every table of that kind that exists at the moment - the + // global's own and any by-value copy a fixture or a context reset is holding - and + // the slot goes back once, after all of them have let go (SlotTables.h, the holder + // list). Naming one instance here is a spelling, not a choice of holder. + // // A notice that arrives after exit() has begun is dropped: past that point the twin's // destructor must not call into the driver (see InProcessTeardown()), and the process - // is about to hand every GPU object back anyway. + // is about to hand every GPU object back anyway. That twin is a deliberate leak, not + // garbage for a later collection - there is none on this arm. void OnFrontendStateObjectDestroyed(MG_Pipe::MGPipeKind kind, Uint64 lifetimeId) { if (InProcessTeardown()) return; switch (kind) { diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.h b/MobileGL/MG_Backend/DirectGLES/Managers.h index 840ef309..a061774b 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.h +++ b/MobileGL/MG_Backend/DirectGLES/Managers.h @@ -315,20 +315,27 @@ namespace MobileGL::MG_Backend::DirectGLES { using BackendMap = UnorderedMap; using iterator = typename BackendMap::iterator; using const_iterator = typename BackendMap::const_iterator; +#if MOBILEGL_PIPE_PUSH + using SlotTable = BackendSlotTable; +#endif BackendPtr& GetOrCreate(const StatePtr& stateObj) { MOBILEGL_ASSERT(stateObj != nullptr, "State object must not be null"); - // Twin creation is the moment a driver-owned id starts needing a guarded - // destructor; cold path, so the once-guard costs nothing per draw. It is armed - // here, at the first insertion, on BOTH arms - a destructor hook on the table - // itself is wrong for the reason spelled out above InProcessTeardown(). - EnsureProcessTeardownSentinel(); #if MOBILEGL_PIPE_PUSH if (EsprytSlotTablesEnabled()) { + // The slot table arms the teardown sentinel itself, at its own first + // insertion (D13; SlotTables.h) - so a table used outside a registry arms + // it too, which is right: it is the twin, not the registry, that owns the + // driver id a guarded destructor exists for. return m_slotTable.GetOrCreate(stateObj); } #endif + // Twin creation is the moment a driver-owned id starts needing a guarded + // destructor; cold path, so the once-guard costs nothing per draw. It is armed + // here, at the first insertion - a destructor hook on the table itself is wrong + // for the reason spelled out above InProcessTeardown(). + EnsureProcessTeardownSentinel(); // Sweep BEFORE the entry reference below exists: the map is open-addressed and an // erase relocates the rest of the probe cluster, so collecting once that reference // is taken would invalidate it. The sweep is therefore owed from an earlier call @@ -417,15 +424,20 @@ namespace MobileGL::MG_Backend::DirectGLES { return nullptr; } - // P2 step e2. The legacy arm cannot answer this at all - its key is the frontend heap - // ADDRESS and the object is already gone by the time the notice arrives - so there it - // is a no-op and the garbage sweep stays its only death signal. That asymmetry is not - // an oversight: it is the A/B the compile-time arm exists to make measurable + // P2 step e2. STATIC, because a death notice is about an object and not about a + // registry instance: it is answered by EVERY table of this kind that exists - this + // registry's own, and any by-value copy of it a fixture or a context reset is holding + // (SlotTables.h explains the holder list and why one holder was a leak). + // + // The legacy arm cannot answer this at all - its key is the frontend heap ADDRESS and + // the object is already gone by the time the notice arrives - so there it is a no-op + // and the garbage sweep stays its only death signal. That asymmetry is not an + // oversight: it is the A/B the compile-time arm exists to make measurable // (ARCHITECTURE.md 9.6), and announced-versus-discovered death is one of the things // being measured. - Bool DestroyByLifetimeId(Uint64 lifetimeId) { + static Bool DestroyByLifetimeId(Uint64 lifetimeId) { if (EsprytSlotTablesEnabled()) { - return m_slotTable.DestroyByLifetimeId(lifetimeId); + return SlotTable::OnFrontendObjectDestroyed(lifetimeId); } return false; } @@ -454,13 +466,13 @@ namespace MobileGL::MG_Backend::DirectGLES { // The seven DirectGLES.cpp call sites drive the LEGACY arm and nothing else. On the // handle arm death is announced by the frontend object's destructor // (MG_State/GLState/StateObjectDeathNotice.h), so there is no garbage to collect on a - // tick and this is the predicted branch plus a return - which is how ROADMAP.md:18's - // "delete the GC" is delivered without deleting the legacy arm's own collector while - // that arm is still compiled beside it. + // tick, the slot table has no collector to forward to, and this is the predicted + // branch plus a return - which is how ROADMAP.md:18's "delete the GC" is delivered + // without deleting the legacy arm's own collector while that arm is still compiled + // beside it. void CollectGarbageIfNeeded() { #if MOBILEGL_PIPE_PUSH if (EsprytSlotTablesEnabled()) { - m_slotTable.CollectGarbageIfNeeded(); return; } #endif @@ -474,10 +486,12 @@ namespace MobileGL::MG_Backend::DirectGLES { #endif } + // Pre-P2 API, kept for the legacy arm. On the handle arm there is nothing it could + // collect: a twin leaves with its object's death notice, and a notice dropped during + // process teardown is a deliberate leak (SlotTables.h), not garbage awaiting a call. void CollectGarbageNow() { #if MOBILEGL_PIPE_PUSH if (EsprytSlotTablesEnabled()) { - m_slotTable.CollectGarbageNow(); return; } #endif diff --git a/MobileGL/MG_Backend/DirectGLES/SlotTables.h b/MobileGL/MG_Backend/DirectGLES/SlotTables.h index 078e2466..620d8385 100644 --- a/MobileGL/MG_Backend/DirectGLES/SlotTables.h +++ b/MobileGL/MG_Backend/DirectGLES/SlotTables.h @@ -28,8 +28,8 @@ // lose their reason to exist. // * The lookup stops being a hash probe into an open-addressed map and becomes one bounds // check plus one array index, so a returned BackendPtr* is NOT invalidated by the next Find -// or sweep on the table. That kills the hazard Managers.h documents at length, and with it -// the by-value copy plus second Find that SyncTextureObjectToBackend paid to survive it. +// on the table. That kills the hazard Managers.h documents at length, and with it the +// by-value copy plus second Find that SyncTextureObjectToBackend paid to survive it. // * Slots are dense per kind, which is what lets the server side (ARCHITECTURE.md 10.1, // MG_Remote/Server/PipeObjectTables) be an array rather than an object graph. // @@ -37,22 +37,35 @@ // deliverable ROADMAP.md:18 spells "GC" in and the one D13 makes a precondition of the switch- // over. All six re-keyed object classes raise MG_State::GLState::NotifyStateObjectDestroyed() // from their destructor (BufferBackendOps' shape, one entry point for six kinds), the backend -// consumes it in Managers.cpp, and DestroyByLifetimeId() below drops the twin and returns the -// slot at the moment the frontend object's last SharedPtr goes. So: -// * there is NO draw-path tick and NO creation tick on this arm. CollectGarbageIfNeeded() is -// an empty call, and the seven call sites in DirectGLES.cpp drive the LEGACY registry only; +// consumes it in Managers.cpp, and OnFrontendObjectDestroyed() below drops the twin in EVERY +// table of the kind and returns the slot, at the moment the frontend object's last SharedPtr +// goes. So: +// * there is NO draw-path tick, NO creation tick and NO sweep of any kind on this arm. The +// seven CollectGarbageIfNeeded call sites in DirectGLES.cpp drive the LEGACY registry only; // * a twin, and the driver storage it owns, is freed when the application lets go of the // object rather than up to 64 creations or 1024 draw ticks later. That is what // Managers.h's "dead gigabytes" note asked for. // -// The weak_ptr per entry survives, and only for what it is honest about: -// * ForEachLive() hands the callee a STRONG reference to the frontend object, which the one -// direct-iteration site (ScopedDetachedTextureFramebufferAttachments) needs; and -// * ReclaimDeadSlots() is kept as the body of the EXPLICIT CollectGarbageNow(), i.e. a -// collection someone asks for, never a periodic one. It is the backstop for the one case -// the notice cannot cover: a destructor that runs after exit() has begun, where -// InProcessTeardown() drops the notice because a twin destructor must not call the driver. -// It is never an identity test - that is what Gen is for. +// EVERY HOLDER OF THE KIND, not one. Two live tables of one kind is a real configuration - the +// ScopedDirectGLESTextureBindings fixture keeps a by-value copy of the Texture registry for the +// length of a test, and a context reset does the same in reverse - and the slot allocator +// erases its lifetimeId -> slot mapping on Free, so a notice delivered to one holder and +// resolved again by the next would find nothing to resolve. Every table therefore links itself +// into a per-table-type list at construction and out at destruction, and one notice resolves +// the handle ONCE, drops the twin in each holder BY HANDLE, and frees the slot once, last. No +// holder can be left naming a live entry for a dead object, and there is nothing a sweep could +// still find. (The list is per table TYPE; the kind is the type's template parameter, and each +// of the six kinds has exactly one table type in this backend. Magma's subsystem-4 table mints +// out of its own per-renderer allocator, not MGPipeSlots(), so it is not a holder here.) +// +// The weak_ptr per entry survives for exactly one reason: ForEachLive() hands the callee a +// STRONG reference to the frontend object, which the one direct-iteration site +// (ScopedDetachedTextureFramebufferAttachments) needs. It is never an identity test - that is +// what Gen is for - and it is never read to decide whether an entry is dead: a destructor that +// runs after exit() has begun has its notice dropped by InProcessTeardown(), and that twin is +// then a DELIBERATE leak (the process is exiting, the driver reclaims the object, and a twin +// destructor must not call into a driver that may already be unloaded), not something to be +// collected later. // // P3+ DEBT, recorded rather than hidden: this header is under MG_Backend/ and it MINTS // handles (MGPipeSlots().Acquire below) off a frontend SharedPtr's GetLifetimeId(). @@ -66,6 +79,11 @@ namespace MobileGL::MG_Backend::DirectGLES { #if MOBILEGL_PIPE_PUSH + // Declared in Managers.h as well; repeated here because this header is included from it + // before that declaration, and the table below is the arming site on this arm (D13: "the + // arming site moves to the slot table's first insertion"). + void EnsureProcessTeardownSentinel(); + // What the two knobs add up to. Split out as a PURE function of them so a test can drive // every combination without needing a process per combination. enum class EsprytSlotArmVerdict { @@ -90,6 +108,11 @@ namespace MobileGL::MG_Backend::DirectGLES { // D14/D18 A/B is driven with, which is what ROADMAP.md:7 forbids. So bring-up only // DIAGNOSES; the stop is raised by ResolveEsprytSlotTablesArm() at the first twin lookup, // which happens in the test body where the harness reports it as a failure. + // + // The CALL SITE (InitDisplayAndContext in DirectGLES.cpp) is pinned by + // DirectGLESSlotTable.EglBringUpUnderTheArmlessKnobPairReturnsInsteadOfStopping, which runs + // the real bring-up entry point under the pair in a forked child: edit that site back to + // ResolveEsprytSlotTablesArm() and the case fails naming both knobs. void DiagnoseEsprytSlotArm(); // Reads the config, logs, installs the death-notice consumer, and STOPS when the operator @@ -101,11 +124,11 @@ namespace MobileGL::MG_Backend::DirectGLES { // True when this process runs the {slot, gen} arm. Fixed for the life of the process: the // two arms hold their twins in different containers, so flipping mid-run would strand them. // - // INLINE on purpose. Every Find / GetOrCreate / HandleOf / ForEachLive / CollectGarbage* - // on the twin tables consults it, i.e. it is on the per-draw path several times per draw. - // As an out-of-line function in Managers.cpp (no LTO in any shipped configuration) that was - // a call through the PLT per lookup; here the caller sees a guard-variable load and a - // perfectly-predicted branch, and the arm dispatch folds into the caller. + // INLINE on purpose. Every Find / GetOrCreate / HandleOf / ForEachLive on the twin tables + // consults it, i.e. it is on the per-draw path several times per draw. As an out-of-line + // function in Managers.cpp (no LTO in any shipped configuration) that was a call through + // the PLT per lookup; here the caller sees a guard-variable load and a perfectly-predicted + // branch, and the arm dispatch folds into the caller. inline Bool EsprytSlotTablesEnabled() { static const Bool enabled = ResolveEsprytSlotTablesArm(); return enabled; @@ -120,9 +143,10 @@ namespace MobileGL::MG_Backend::DirectGLES { struct Entry { BackendPtr backend; - // LIVENESS ONLY. Never compared against another object to decide identity - that is - // what Gen is for - and never dereferenced for its address. Read by - // ReclaimDeadSlots(), and locked by ForEachLive() so the callee holds a strong ref. + // LIVENESS ONLY, and only for ForEachLive(), which locks it so the callee holds a + // strong ref. Never compared against another object to decide identity - that is + // what Gen is for - never dereferenced for its address, and never read to decide + // whether the slot is dead: death is announced, not discovered. StateWeakPtr stateRef; // The generation this entry's twin was built for. An entry whose Gen no longer // matches the allocator's is a twin of the slot's PREVIOUS owner. @@ -130,6 +154,49 @@ namespace MobileGL::MG_Backend::DirectGLES { Bool Live = false; }; + // Every constructor links the table into the per-type holder list and the destructor + // unlinks it, so a by-value copy (the ScopedDirectGLESTextureBindings fixture's saved + // registry) is a holder for exactly as long as it exists. Copy and move carry the + // ENTRIES and the memo; the links are the table's own and are never copied. + BackendSlotTable() { LinkHolder(); } + BackendSlotTable(const BackendSlotTable& other): + m_slots(other.m_slots), + m_nullTwin(other.m_nullTwin), + m_memoLifetimeId(other.m_memoLifetimeId), + m_memoHandle(other.m_memoHandle) { + LinkHolder(); + } + BackendSlotTable(BackendSlotTable&& other) noexcept: + m_slots(std::move(other.m_slots)), + m_nullTwin(std::move(other.m_nullTwin)), + m_memoLifetimeId(other.m_memoLifetimeId), + m_memoHandle(other.m_memoHandle) { + other.m_slots.clear(); + other.ForgetHandle(); + LinkHolder(); + } + BackendSlotTable& operator=(const BackendSlotTable& other) { + if (this != &other) { + m_slots = other.m_slots; + m_nullTwin = other.m_nullTwin; + m_memoLifetimeId = other.m_memoLifetimeId; + m_memoHandle = other.m_memoHandle; + } + return *this; + } + BackendSlotTable& operator=(BackendSlotTable&& other) noexcept { + if (this != &other) { + m_slots = std::move(other.m_slots); + m_nullTwin = std::move(other.m_nullTwin); + m_memoLifetimeId = other.m_memoLifetimeId; + m_memoHandle = other.m_memoHandle; + other.m_slots.clear(); + other.ForgetHandle(); + } + return *this; + } + ~BackendSlotTable() { UnlinkHolder(); } + // Resolve-or-create. The handle comes from the client allocator keyed on the frontend // object's lifetime id, so two calls for the same live object always land on the same // slot, and a successor object at the same heap address never does. @@ -144,11 +211,17 @@ namespace MobileGL::MG_Backend::DirectGLES { // entry, so a second null call was handed the same twin the first one got. // Resetting here instead would have destroyed it - an arm difference in the one // path that documents relying on this. One per-table parking slot, never live, - // never swept, never handed a handle, because a null object has no identity and + // never handed a handle, because a null object has no identity and // therefore cannot have a {slot, gen}. return m_nullTwin; } + // D13: the teardown sentinel is armed by the slot table's first insertion. Twin + // creation is the moment a driver-owned id starts needing a guarded destructor; + // this is the cold path, so the once-guard costs nothing per draw. On the legacy + // arm StateBackendObjectRegistry::GetOrCreate arms it itself. + EnsureProcessTeardownSentinel(); + const MG_Pipe::MGPipeHandle handle = MG_Pipe::MGPipeSlots().Acquire(kKind, stateObj->GetLifetimeId()); MOBILEGL_ASSERT(!MG_Pipe::MGPipeHandleIsNull(handle), @@ -173,9 +246,9 @@ namespace MobileGL::MG_Backend::DirectGLES { } // Null when no live twin of this object exists. Unlike the registry's Find this NEVER - // mutates the table, so the returned pointer survives any later Find or sweep on it; - // only a GetOrCreate that grows the vector can move it, and callers that hold one - // across a possible insertion still copy the BackendPtr out. + // mutates the table, so the returned pointer survives any later Find on it; only a + // GetOrCreate that grows the vector can move it, and callers that hold one across a + // possible insertion still copy the BackendPtr out. BackendPtr* Find(StateObject* stateObj) { if (stateObj == nullptr) return nullptr; return FindByHandle(HandleOf(stateObj)); @@ -195,91 +268,62 @@ namespace MobileGL::MG_Backend::DirectGLES { // The handle this object's twin is keyed on, or the null handle. This is what a backend // memo stores instead of a raw pointer, a GL name or a bare lifetime id. + // + // A NULL answer is never memoised. The memo is per table and the allocator is per + // kind, so with two holders of one kind the OTHER table can be the one that acquires; + // a cached "no handle" here would then outlive the twin's creation over there, and + // nothing on this table's own acquire path would ever refresh it. A miss costs the + // allocator probe it always cost; a hit is refreshed the moment anyone acquires. MG_Pipe::MGPipeHandle HandleOf(const StateObject* stateObj) const { if (stateObj == nullptr) return MG_Pipe::kMGPipeNullHandle; const Uint64 lifetimeId = stateObj->GetLifetimeId(); if (lifetimeId == m_memoLifetimeId) return m_memoHandle; const MG_Pipe::MGPipeHandle handle = MG_Pipe::MGPipeSlots().FindByLifetimeId(kKind, lifetimeId); - RememberHandle(lifetimeId, handle); + if (!MG_Pipe::MGPipeHandleIsNull(handle)) RememberHandle(lifetimeId, handle); return handle; } - // Drop the twin of every slot whose frontend object is gone and return the slot to the - // allocator. Freeing is what makes the NEXT handout of that slot bump Gen. - void ReclaimDeadSlots() { - if (m_isCollecting) return; - m_isCollecting = true; - for (SizeT slot = 0; slot < m_slots.size(); ++slot) { - Uint32 gen = 0; - // The twin's destructor is a driver call and could, in principle, re-enter - // GetOrCreate on this table and resize m_slots. So NOTHING that outlives the - // destructor may be a reference into m_slots: the twin is moved out into a - // local, the entry is finished with, and only then is the local released. - BackendPtr dead; - { - Entry& entry = m_slots[slot]; - if (!entry.Live || !entry.stateRef.expired()) continue; - gen = entry.Gen; - dead = std::move(entry.backend); - entry.backend.reset(); - entry.stateRef.reset(); - entry.Live = false; - } - MG_Pipe::MGPipeSlots().Free( - kKind, MG_Pipe::MGPipeHandle{static_cast(slot), gen}); - if (m_memoHandle.Slot == static_cast(slot)) ForgetHandle(); - dead.reset(); - } - m_isCollecting = false; - } - - // P2 step e2's backend half: the frontend object with this lifetime id has just been - // DESTROYED, so drop its twin and return its slot now rather than waiting for a sweep - // to notice the weak_ptr expired. Announced death is what the sweep is a stand-in for; - // it frees the driver storage the twin owns at the moment the application let go of - // the object, which is what Managers.h's "dead gigabytes" note is about. + // P2 step e2's backend half. The frontend object with this lifetime id has just been + // DESTROYED: resolve its handle ONCE, drop its twin in EVERY table of this type, and + // return the slot to the allocator - in that order, because the allocator forgets the + // lifetime id on Free and a holder told second could no longer resolve it. // - // Returns whether this table held the slot. The slot goes back to the allocator ONLY - // then, and this is not defensive: two holders of one kind already exist (the - // ScopedDirectGLESTextureBindings fixture's saved copy is a second live table of kind - // Texture; Magma's subsystem-4 table shares the VertexElementsCso kind), and a table - // that never twinned the object must not free a slot the other one still names. - Bool DestroyByLifetimeId(Uint64 lifetimeId) { + // The slot is returned whether or not any holder still had a twin at it: the lifetime + // id is dead and MG_State never hands one out twice, so nothing can acquire it again, + // and a slot minted for it that no table holds (a table reset with `= {}` drops its + // entries without freeing) would otherwise stay allocated for the life of the process. + // + // STATIC, and deliberately so: a notice is about an object, not about a table, and + // "which table holds it" is exactly the question that produced the two-holder leak. + // Returns whether the object had a slot of this kind, i.e. whether anything was freed; + // a second call for the same id answers false because the allocator no longer maps it. + static Bool OnFrontendObjectDestroyed(Uint64 lifetimeId) { const MG_Pipe::MGPipeHandle handle = MG_Pipe::MGPipeSlots().FindByLifetimeId(kKind, lifetimeId); if (MG_Pipe::MGPipeHandleIsNull(handle)) return false; - if (handle.Slot >= m_slots.size()) return false; - // Same ordering rule as ReclaimDeadSlots(): the twin's destructor is a driver call - // and could re-enter GetOrCreate and resize m_slots, so nothing that outlives it - // may be a reference into the vector. - BackendPtr dead; - { - Entry& entry = m_slots[handle.Slot]; - if (!entry.Live || entry.Gen != handle.Gen) return false; - dead = std::move(entry.backend); - entry.backend.reset(); - entry.stateRef.reset(); - entry.Live = false; + for (BackendSlotTable* holder = s_firstHolder; holder != nullptr;) { + // The successor is read BEFORE the release: ReleaseTwinAt runs the twin's + // destructor, which is a driver call, and nothing that outlives it may be a + // reference into this holder. + BackendSlotTable* const next = holder->m_nextHolder; + holder->ReleaseTwinAt(handle); + holder = next; } - if (m_memoHandle.Slot == handle.Slot) ForgetHandle(); MG_Pipe::MGPipeSlots().Free(kKind, handle); - dead.reset(); return true; } - // Deliberately EMPTY, and this is the P2 deliverable rather than an omission: on this - // arm death is announced, so there is nothing for a periodic sweep to discover. The - // seven DirectGLES.cpp call sites keep their spelling because they are the legacy - // registry's driver and that arm is still compiled beside this one; on this arm they - // cost the predicted branch in StateBackendObjectRegistry and return. - void CollectGarbageIfNeeded() {} - - // An EXPLICIT collection - someone asked, so it runs. Not a driver: nothing calls this - // on a tick. It is the backstop for a notice that could not be delivered (see the - // InProcessTeardown() note in the file header) and the tests' way of forcing the - // liveness sweep without waiting for one. - void CollectGarbageNow() { ReclaimDeadSlots(); } + // How many tables of this type exist right now. For the tests that pin the holder + // list; nothing on a shipping path asks. + static Uint32 HolderCount() { + Uint32 count = 0; + for (const BackendSlotTable* holder = s_firstHolder; holder != nullptr; + holder = holder->m_nextHolder) { + ++count; + } + return count; + } // fn(const StatePtr& state, const BackendPtr& twin) over every live, still-owned entry. // Replaces the registry's begin()/end(), whose iterator exposed the raw frontend @@ -291,8 +335,8 @@ namespace MobileGL::MG_Backend::DirectGLES { // Index loop and a COPIED twin, not a range-for over references: fn is arbitrary // backend code, and a nested GetOrCreate on this table would resize m_slots and // invalidate both the iterator and any reference into the vector that outlives the - // call. ReclaimDeadSlots walks by index for the same reason. The one caller today - // happens not to insert; that is not a property the walk should depend on. + // call. The one caller today happens not to insert; that is not a property the + // walk should depend on. for (SizeT slot = 0; slot < m_slots.size(); ++slot) { const Entry& entry = m_slots[slot]; if (!entry.Live || !entry.backend) continue; @@ -312,6 +356,32 @@ namespace MobileGL::MG_Backend::DirectGLES { } private: + // Drop the twin at `handle` if THIS table holds it. Frees nothing: the slot belongs to + // the kind, not to the table, and OnFrontendObjectDestroyed returns it once, after + // every holder has let go. + Bool ReleaseTwinAt(MG_Pipe::MGPipeHandle handle) { + // Forget the memo whenever it names this slot, even if this table has no entry + // there: a memo can be a handle learned from the allocator for an object another + // holder twinned, and it must not survive the slot's next handout. + if (m_memoHandle.Slot == handle.Slot) ForgetHandle(); + if (handle.Slot >= m_slots.size()) return false; + // The twin's destructor is a driver call and could, in principle, re-enter + // GetOrCreate on this table and resize m_slots. So NOTHING that outlives the + // destructor may be a reference into m_slots: the twin is moved out into a local, + // the entry is finished with, and only then is the local released. + BackendPtr dead; + { + Entry& entry = m_slots[handle.Slot]; + if (!entry.Live || entry.Gen != handle.Gen) return false; + dead = std::move(entry.backend); + entry.backend.reset(); + entry.stateRef.reset(); + entry.Live = false; + } + dead.reset(); + return true; + } + Entry& EntryAt(Uint32 slot) { if (slot >= m_slots.size()) m_slots.resize(static_cast(slot) + 1); return m_slots[slot]; @@ -326,10 +396,36 @@ namespace MobileGL::MG_Backend::DirectGLES { m_memoHandle = MG_Pipe::kMGPipeNullHandle; } + // The holder list: intrusive and doubly linked, so registering and unregistering are + // two pointer writes with no allocation, and its head is a constant-initialised + // static - which is what lets the process-lifetime registry globals in Managers.cpp + // link themselves in from their own constructors with no initialisation-order + // question to answer. Single-threaded, like every table it links (the tables live and + // die on the context thread, as the notice they answer does). + void LinkHolder() { + m_prevHolder = nullptr; + m_nextHolder = s_firstHolder; + if (s_firstHolder != nullptr) s_firstHolder->m_prevHolder = this; + s_firstHolder = this; + } + void UnlinkHolder() { + if (m_prevHolder != nullptr) { + m_prevHolder->m_nextHolder = m_nextHolder; + } else { + s_firstHolder = m_nextHolder; + } + if (m_nextHolder != nullptr) m_nextHolder->m_prevHolder = m_prevHolder; + m_prevHolder = nullptr; + m_nextHolder = nullptr; + } + + static inline BackendSlotTable* s_firstHolder = nullptr; + BackendSlotTable* m_prevHolder = nullptr; + BackendSlotTable* m_nextHolder = nullptr; + // Indexed by MGPipeHandle::Slot; [0] is the reserved slot and is never live. Vector m_slots; - Bool m_isCollecting = false; - // Handed back by GetOrCreate for a null state object. Never live, never swept. + // Handed back by GetOrCreate for a null state object. Never live, never handed a handle. BackendPtr m_nullTwin; // ONE-entry resolution memo, lifetimeId -> handle. It exists because without it every @@ -345,13 +441,15 @@ namespace MobileGL::MG_Backend::DirectGLES { // a per-unit memo and a direct-mapped 6-slot array there). Making the memo per-unit / // per-target is the fix, and G11 - the device-side gate that would price it - is owed. // - // It cannot serve a stale answer, by two independent arguments: + // It cannot serve a stale answer, by three independent arguments: // * the key is a lifetime id, which MG_State never hands out twice, so a recycled - // heap address cannot hit this memo the way it could hit an address-keyed one; and + // heap address cannot hit this memo the way it could hit an address-keyed one; + // * a null answer is never stored, so another holder's acquire cannot be hidden by + // a "no handle" this table remembered earlier; and // * even a hit for a slot that has since been freed and re-handed is caught, because // the caller resolves the handle through FindByHandle, which compares Gen. - // Cleared anyway when the sweep frees the memoised slot. 0 is never a live lifetime id - // (MG_State's counters start at 1), so a zeroed memo is a guaranteed miss. + // Cleared anyway when a death notice names the memoised slot. 0 is never a live + // lifetime id (MG_State's counters start at 1), so a zeroed memo is a guaranteed miss. mutable Uint64 m_memoLifetimeId = 0; mutable MG_Pipe::MGPipeHandle m_memoHandle = MG_Pipe::kMGPipeNullHandle; }; diff --git a/MobileGL/MG_Test/SanityTest.cpp b/MobileGL/MG_Test/SanityTest.cpp index c7e5adaf..a9a12686 100644 --- a/MobileGL/MG_Test/SanityTest.cpp +++ b/MobileGL/MG_Test/SanityTest.cpp @@ -36,6 +36,7 @@ #include #include #include +#include #include #include #include @@ -46,8 +47,14 @@ #include #include #include +#include #include #include +#if defined(_WIN32) +#include +#else +#include +#endif #if MOBILEGL_PIPE_PUSH namespace { @@ -3215,6 +3222,86 @@ namespace { BackendSlotTable; using FakeSharedKindSlotTable = MobileGL::MG_Backend::DirectGLES:: BackendSlotTable; + + // A log file path no other process and no other case can be writing to: the pid keeps two + // SanityTest processes on one host apart, the counter keeps two cases in one process apart. + std::filesystem::path UniqueScratchLogPath(const char* stem) { + static int counter = 0; +#if defined(_WIN32) + const long pid = static_cast(_getpid()); +#else + const long pid = static_cast(::getpid()); +#endif + const auto ticks = std::chrono::steady_clock::now().time_since_epoch().count(); + return std::filesystem::temp_directory_path() / + (std::string(stem) + "-" + std::to_string(pid) + "-" + std::to_string(++counter) + + "-" + std::to_string(ticks) + ".log"); + } + + // Points MobileGL's file log at `path` for the life of the guard and puts back whatever the + // operator had - the previous MOBILEGL_LOG_FILE_PATH, or none - on EVERY exit path, + // including a failed ASSERT. The log is closed on both sides of the switch, because Log.cpp + // reads the variable only when it opens the file. + struct ScopedLogFileRedirect { + explicit ScopedLogFileRedirect(const std::filesystem::path& path): m_path(path) { + if (const char* previous = std::getenv("MOBILEGL_LOG_FILE_PATH")) { + m_hadPrevious = true; + m_previous = previous; + } + std::filesystem::remove(m_path); + MobileGL::MG_Util::Debug::Close(); + SetEnvVar("MOBILEGL_LOG_FILE_PATH", m_path.string().c_str()); + } + ~ScopedLogFileRedirect() { + MobileGL::MG_Util::Debug::Close(); + if (m_hadPrevious) { + SetEnvVar("MOBILEGL_LOG_FILE_PATH", m_previous.c_str()); + } else { + UnsetEnvVar("MOBILEGL_LOG_FILE_PATH"); + } + std::error_code ignored; + std::filesystem::remove(m_path, ignored); + } + ScopedLogFileRedirect(const ScopedLogFileRedirect&) = delete; + ScopedLogFileRedirect& operator=(const ScopedLogFileRedirect&) = delete; + + // Everything written so far. Closes the log first so the last line is on disk. + std::string Contents() const { + MobileGL::MG_Util::Debug::Close(); + std::ifstream logFile(m_path); + if (!logFile.good()) return {}; + return std::string(std::istreambuf_iterator(logFile), std::istreambuf_iterator()); + } + + private: + std::filesystem::path m_path; + bool m_hadPrevious = false; + std::string m_previous; + }; + + // Sets the two knobs into the combination that leaves no twin-table arm at all - + // kMGPipeSubsystemEsprytSlots clear and PipeLegacyMemos false, which is what + // MOBILEGL_PIPE_PUSH=0 MOBILEGL_PIPE_LEGACY_MEMOS=0 in the environment produces - and + // restores MG_Config::Features on every exit path. + struct ScopedArmlessKnobPair { + ScopedArmlessKnobPair(): + m_savedPush(MobileGL::MG_Config::Features.PipePush), + m_savedLegacy(MobileGL::MG_Config::Features.PipeLegacyMemos) { + MobileGL::MG_Config::Features.PipePush = + m_savedPush & ~MobileGL::MG_Pipe::kMGPipeSubsystemEsprytSlots; + MobileGL::MG_Config::Features.PipeLegacyMemos = false; + } + ~ScopedArmlessKnobPair() { + MobileGL::MG_Config::Features.PipePush = m_savedPush; + MobileGL::MG_Config::Features.PipeLegacyMemos = m_savedLegacy; + } + ScopedArmlessKnobPair(const ScopedArmlessKnobPair&) = delete; + ScopedArmlessKnobPair& operator=(const ScopedArmlessKnobPair&) = delete; + + private: + MobileGL::Uint64 m_savedPush; + MobileGL::Bool m_savedLegacy; + }; } // namespace // The property the whole slice exists for. The pre-P2 registry keyed twins on the frontend heap @@ -3233,9 +3320,11 @@ TEST(DirectGLESSlotTable, ARecycledSlotIsANewHandleAndTheStaleOneResolvesToNothi ASSERT_FALSE(MG_Pipe::MGPipeHandleIsNull(firstHandle)); EXPECT_EQ(table.LiveCount(), 1u); - // The frontend object dies and the slot is reclaimed - which is the only moment Gen moves. + // The frontend object dies and announces it (FakeStateObject is not one of the six re-keyed + // classes, so the notice its destructor would raise is raised by hand), and the slot is + // reclaimed - which is the only moment Gen moves. first.reset(); - table.CollectGarbageNow(); + EXPECT_TRUE(FakeSlotTable::OnFrontendObjectDestroyed(0xA1u)); EXPECT_EQ(table.LiveCount(), 0u); EXPECT_EQ(table.FindByHandle(firstHandle), nullptr) << "a handle whose object is gone still resolved to a twin"; @@ -3254,6 +3343,9 @@ TEST(DirectGLESSlotTable, ARecycledSlotIsANewHandleAndTheStaleOneResolvesToNothi EXPECT_EQ(table.FindByHandle(firstHandle), nullptr); ASSERT_NE(table.FindByHandle(secondHandle), nullptr); EXPECT_EQ((*table.FindByHandle(secondHandle))->marker, 2); + + second.reset(); + EXPECT_TRUE(FakeSlotTable::OnFrontendObjectDestroyed(0xA2u)); } // Gen moves on reuse and ONLY on reuse: a live object that is looked up again, or respecified, @@ -3263,16 +3355,12 @@ TEST(DirectGLESSlotTable, RepeatedLookupsOfALiveObjectKeepOneHandle) { FakeSlotTable table; auto object = MakeShared(0xB1u); - // HandleOf caches whatever the allocator answered, INCLUDING the null handle - an object - // that is bound but never synced has no twin, and re-probing the hash for it every draw is - // exactly what the memo exists to avoid. What makes that safe is that GetOrCreate refreshes - // the memo, so a cached "no handle" can never outlive the twin's creation. Nothing pinned - // that; this does. + // An object that is bound but never synced has no twin and no handle; asking is a miss. EXPECT_TRUE(MG_Pipe::MGPipeHandleIsNull(table.HandleOf(object.get()))); table.GetOrCreate(object) = MakeShared(); const MG_Pipe::MGPipeHandle handle = table.HandleOf(object.get()); ASSERT_FALSE(MG_Pipe::MGPipeHandleIsNull(handle)) - << "the memo went on answering the null handle it cached before the twin existed"; + << "the memo went on answering a null handle after the twin was created"; for (int i = 0; i < 8; ++i) { auto* slot = table.GetOrCreate(object) ? table.Find(object.get()) : nullptr; @@ -3280,11 +3368,46 @@ TEST(DirectGLESSlotTable, RepeatedLookupsOfALiveObjectKeepOneHandle) { EXPECT_TRUE(table.HandleOf(object.get()) == handle) << "handle moved on lookup " << i; } EXPECT_EQ(table.LiveCount(), 1u); + + object.reset(); + EXPECT_TRUE(FakeSlotTable::OnFrontendObjectDestroyed(0xB1u)); +} + +// The round-4 review's minor 8: HandleOf used to memoise a NULL answer, on the argument that +// GetOrCreate refreshes the memo. That holds for ONE table. The allocator is per kind and the +// memo is per table, so once a second holder of the kind can be the one that acquires, the first +// table's cached "no handle" outlives the twin's creation and nothing on its own acquire path +// ever corrects it - every Find through it is a miss for a twin that exists. This is that +// configuration, and it must resolve. +TEST(DirectGLESSlotTable, ANegativeLookupIsNotCachedAcrossAnotherHoldersAcquire) { + using namespace MobileGL; + + FakeSharedKindSlotTable first; + FakeSharedKindSlotTable second; + auto object = MakeShared(0xB2u); + + // `first` asks before anyone has acquired: a miss, which must NOT be remembered. + EXPECT_TRUE(MG_Pipe::MGPipeHandleIsNull(first.HandleOf(object.get()))); + + // `second` is the holder that acquires. + second.GetOrCreate(object) = MakeShared(); + const MG_Pipe::MGPipeHandle handle = second.HandleOf(object.get()); + ASSERT_FALSE(MG_Pipe::MGPipeHandleIsNull(handle)); + + // `first` never acquired, so nothing on its own path refreshed its memo; it must still + // answer the handle the kind now has for this object. + EXPECT_TRUE(first.HandleOf(object.get()) == handle) + << "the first holder kept answering the null handle it cached before the second holder " + "acquired, so every lookup through it misses a twin that exists"; + + object.reset(); + EXPECT_TRUE(FakeSharedKindSlotTable::OnFrontendObjectDestroyed(0xB2u)); } // The lookup does not mutate the table, which is what lets SyncTextureObjectToBackend stop paying -// a by-value copy plus a second Find to survive the registry's erase-inside-Find. A dead entry -// stays put until the sweep, and a live entry's pointer is unaffected by looking up anything else. +// a by-value copy plus a second Find to survive the registry's erase-inside-Find. An entry whose +// object has gone stays put until the death notice arrives, and a live entry's pointer is +// unaffected by looking up anything else. TEST(DirectGLESSlotTable, FindNeverMutatesTheTable) { using namespace MobileGL; @@ -3298,35 +3421,58 @@ TEST(DirectGLESSlotTable, FindNeverMutatesTheTable) { ASSERT_NE(keptSlot, nullptr); const FakeBackendObject* keptTwin = keptSlot->get(); + // The object goes but its notice is deliberately withheld for a moment, so that whatever + // changes between here and the notice is Find's doing. The registry's Find would have + // erased the expired entry here and relocated the rest of the probe cluster, invalidating + // keptSlot. This one answers what it answers and touches nothing. doomed.reset(); - // The registry's Find would have erased the expired entry here and relocated the rest of the - // probe cluster, invalidating keptSlot. This one answers null and touches nothing. EXPECT_EQ(table.Find(kept.get()), keptSlot); - EXPECT_EQ(table.LiveCount(), 2u) << "Find reclaimed a slot; only the sweep may do that"; + EXPECT_EQ(table.LiveCount(), 2u) << "Find reclaimed a slot; only a death notice may do that"; EXPECT_EQ(keptSlot->get(), keptTwin); - table.CollectGarbageNow(); + EXPECT_TRUE(FakeSlotTable::OnFrontendObjectDestroyed(0xC2u)); EXPECT_EQ(table.LiveCount(), 1u); EXPECT_EQ(table.Find(kept.get())->get(), keptTwin); + + kept.reset(); + EXPECT_TRUE(FakeSlotTable::OnFrontendObjectDestroyed(0xC1u)); } // ScopedDirectGLESTextureBindings saves a whole twin table by value, resets it with `= {}` and -// restores it. The slot table has to keep that shape or the fixture stops isolating anything. +// restores it. The slot table has to keep that shape or the fixture stops isolating anything - +// and the saved copy is a HOLDER for as long as it exists, so a death announced while it is +// held reaches it too (the round-4 review's minor 2, in the fixture's own shape). TEST(DirectGLESSlotTable, AWholeTableSavesResetsAndRestores) { using namespace MobileGL; + const Uint32 holdersBefore = FakeSlotTable::HolderCount(); FakeSlotTable table; auto object = MakeShared(0xD1u); table.GetOrCreate(object) = MakeShared(); (*table.Find(object.get()))->marker = 7; + const MG_Pipe::MGPipeHandle handle = table.HandleOf(object.get()); const FakeSlotTable saved = table; + EXPECT_EQ(FakeSlotTable::HolderCount(), holdersBefore + 2u) + << "the by-value copy did not register as a holder"; table = {}; + EXPECT_EQ(FakeSlotTable::HolderCount(), holdersBefore + 2u) + << "the reset changed the holder count - a temporary's registration leaked or the " + "table's own was lost"; EXPECT_EQ(table.Find(object.get()), nullptr) << "the reset left the twin reachable"; table = saved; ASSERT_NE(table.Find(object.get()), nullptr); EXPECT_EQ((*table.Find(object.get()))->marker, 7); + + // The object dies while BOTH the working table and the saved copy hold its twin. One + // notice, and neither may keep a live entry. + object.reset(); + EXPECT_TRUE(FakeSlotTable::OnFrontendObjectDestroyed(0xD1u)); + EXPECT_EQ(table.FindByHandle(handle), nullptr); + EXPECT_EQ(table.LiveCount(), 0u); + EXPECT_EQ(saved.LiveCount(), 0u) + << "the saved copy kept the dead object's twin - the notice reached one holder only"; } // The whole point of routing every twin through the client allocator: a table that keeps its own @@ -3365,27 +3511,76 @@ TEST(DirectGLESSlotTable, TwoTablesOfTheSameKindShareOneSlotAndKeepTheirOwnTwin) EXPECT_EQ((*a.FindByHandle(handle))->marker, 1); EXPECT_EQ((*b.FindByHandle(handle))->marker, 2); - // TWO HOLDERS OF ONE SLOT is a real configuration, not a test artefact, and this is where - // the sharp edge is: whichever holder sweeps (or is told of the death) first returns the - // slot to the allocator while the other still names it. The allocator makes that SAFE - - // Free is generation-guarded and idempotent, and FindByHandle's Gen compare turns the - // other holder's now-stale handle into a miss - but not free: if the object is still alive - // the next resolution re-Acquires it onto a NEW slot, orphaning the first table's entry - // until its own sweep. Two such configurations exist or are landing: the + // TWO HOLDERS OF ONE SLOT is a real configuration, not a test artefact (the // ScopedDirectGLESTextureBindings fixture above holds a second live table of kind Texture - // for the length of a test, and package D's subsystem 4 re-keys VaoDrawMemo out of this - // same per-kind allocator. Flagged for the integrator rather than defended here, because - // the fix (a refcounted slot-ownership token) belongs with whoever owns both holders. - // - // The cleanup below is what keeps that out of the OTHER cases: object first, then both - // tables, so the slot is back on the free list and this kind's LiveCount is where it was. + // for the length of a test), and the death of the object is what makes it sharp: the + // allocator forgets the lifetime id on Free, so a notice delivered to ONE holder leaves + // the other with a live entry - and its twin's driver storage - that nothing can resolve + // and nothing sweeps. OneDeathNoticeDropsTheTwinInEveryHolderOfTheKind below is the case + // for that; here the cleanup only has to leave the kind's LiveCount where it was. object.reset(); - a.CollectGarbageNow(); - b.CollectGarbageNow(); + EXPECT_TRUE(FakeSharedKindSlotTable::OnFrontendObjectDestroyed(0xE1u)); EXPECT_EQ(slots.LiveCount(kKind), liveBefore) << "the shared slot outlived both holders and the object"; } +// The round-4 review's minor 2, closed: one notice, EVERY holder. Before this the dispatcher +// told one table per kind and DestroyByLifetimeId freed only a slot THIS table held, so with +// the sweep retired the second holder kept a live entry, and the twin, for the life of the +// process. Three holders here - two independent tables and a by-value copy, which is exactly +// what the fixture makes - and a fourth that never twinned the object and must be untouched. +TEST(DirectGLESSlotTable, OneDeathNoticeDropsTheTwinInEveryHolderOfTheKind) { + using namespace MobileGL; + + constexpr MG_Pipe::MGPipeKind kKind = MG_Pipe::MGPipeKind::Fence; + auto& slots = MG_Pipe::MGPipeSlots(); + const Uint32 liveBefore = slots.LiveCount(kKind); + const Uint32 holdersBefore = FakeSharedKindSlotTable::HolderCount(); + + FakeSharedKindSlotTable a; + FakeSharedKindSlotTable b; + FakeSharedKindSlotTable bystander; + auto object = MakeShared(0xE2u); + auto uninvolved = MakeShared(0xE3u); + + a.GetOrCreate(object) = MakeShared(); + b.GetOrCreate(object) = MakeShared(); + bystander.GetOrCreate(uninvolved) = MakeShared(); + const FakeSharedKindSlotTable copyOfA = a; // the fixture's saved registry + EXPECT_EQ(FakeSharedKindSlotTable::HolderCount(), holdersBefore + 4u); + + const MG_Pipe::MGPipeHandle handle = a.HandleOf(object.get()); + ASSERT_FALSE(MG_Pipe::MGPipeHandleIsNull(handle)); + // Observers on the twins, so "dropped" means destroyed and not merely unreachable. + const std::weak_ptr twinA = *a.FindByHandle(handle); + const std::weak_ptr twinB = *b.FindByHandle(handle); + EXPECT_EQ(slots.LiveCount(kKind), liveBefore + 2u); + + // ONE notice. The object is still alive so that nothing but the notice can be at work. + EXPECT_TRUE(FakeSharedKindSlotTable::OnFrontendObjectDestroyed(object->GetLifetimeId())); + + EXPECT_EQ(a.FindByHandle(handle), nullptr) << "holder a kept the twin"; + EXPECT_EQ(b.FindByHandle(handle), nullptr) << "holder b kept the twin"; + EXPECT_EQ(copyOfA.LiveCount(), 0u) << "the by-value copy kept the twin"; + EXPECT_EQ(a.LiveCount(), 0u); + EXPECT_EQ(b.LiveCount(), 0u); + EXPECT_TRUE(twinA.expired()) << "a's twin is unreachable but still allocated"; + EXPECT_TRUE(twinB.expired()) << "b's twin is unreachable but still allocated"; + EXPECT_EQ(slots.LiveCount(kKind), liveBefore + 1u) << "the slot was not returned exactly once"; + EXPECT_FALSE(FakeSharedKindSlotTable::OnFrontendObjectDestroyed(object->GetLifetimeId())) + << "a second notice for the same object found a slot to free"; + + // The holder that never twinned the object is exactly as it was. + EXPECT_EQ(bystander.LiveCount(), 1u); + ASSERT_NE(bystander.Find(uninvolved.get()), nullptr); + + object.reset(); + uninvolved.reset(); + EXPECT_TRUE(FakeSharedKindSlotTable::OnFrontendObjectDestroyed(0xE3u)); + EXPECT_EQ(bystander.LiveCount(), 0u); + EXPECT_EQ(slots.LiveCount(kKind), liveBefore); +} + // The sweep and both of its drivers are RETIRED on this arm (ROADMAP.md:18's "delete the GC"), // and this is the property that replaces them. The registry this table replaces learned of a // death only by finding an expired weak_ptr, so it needed a 1024-call draw tick AND a @@ -3412,7 +3607,7 @@ TEST(DirectGLESSlotTable, AnnouncedDeathKeepsObjectChurnFromAccumulatingWithoutA // classes really do fire it is EveryReKeyedObjectClassAnnouncesItsOwnDeath below, and // that the registries answer it per kind is // EverySwitchedOverKindResolvesItsTwinThroughTheHandleArm. - EXPECT_TRUE(table.DestroyByLifetimeId(object->GetLifetimeId())); + EXPECT_TRUE(FakeSlotTable::OnFrontendObjectDestroyed(object->GetLifetimeId())); } EXPECT_EQ(peakLive, 1u) @@ -3472,20 +3667,34 @@ TEST(DirectGLESSlotTable, AnAnnouncedDeathReturnsTheSlotWithoutASweep) { ASSERT_FALSE(MG_Pipe::MGPipeHandleIsNull(handle)); EXPECT_EQ(slots.LiveCount(MG_Pipe::MGPipeKind::Query), liveBefore + 1u); - // The object is STILL ALIVE here, which is the point: the sweep's weak_ptr test cannot - // fire, so anything that changes is the notice's doing and nothing else's. - EXPECT_TRUE(table.DestroyByLifetimeId(object->GetLifetimeId())); + // The object is STILL ALIVE here, which is the point: there is no weak_ptr test anywhere + // that could fire, so anything that changes is the notice's doing and nothing else's. + EXPECT_TRUE(FakeSlotTable::OnFrontendObjectDestroyed(object->GetLifetimeId())); EXPECT_EQ(table.LiveCount(), 0u) << "the twin survived its own destroy notice"; EXPECT_EQ(table.FindByHandle(handle), nullptr); EXPECT_EQ(table.Find(object.get()), nullptr); EXPECT_EQ(slots.LiveCount(MG_Pipe::MGPipeKind::Query), liveBefore) << "the slot was not returned to the allocator"; - // Idempotent, and a table that does not hold the slot says so rather than freeing it out - // from under whoever does. Both matter once two holders of one kind exist. - EXPECT_FALSE(table.DestroyByLifetimeId(object->GetLifetimeId())); - FakeSlotTable other; - EXPECT_FALSE(other.DestroyByLifetimeId(object->GetLifetimeId())); + // Idempotent: the allocator no longer maps the id, so a repeated notice frees nothing and + // says so - and it says so for every holder, since the notice is about the object and not + // about a table. + EXPECT_FALSE(FakeSlotTable::OnFrontendObjectDestroyed(object->GetLifetimeId())); + + // A slot minted for an object that NO table holds any more - the fixture's `table = {}` + // drops entries without freeing - still goes back when the object dies: the id is dead and + // cannot be acquired again, so keeping the slot would be the process-lifetime leak the + // review named. + auto orphaned = MakeShared(0x1E2A0002ull); + { + FakeSlotTable transient; + transient.GetOrCreate(orphaned) = MakeShared(); + } + EXPECT_EQ(slots.LiveCount(MG_Pipe::MGPipeKind::Query), liveBefore + 1u); + orphaned.reset(); + EXPECT_TRUE(FakeSlotTable::OnFrontendObjectDestroyed(0x1E2A0002ull)) + << "a slot no holder had an entry for was left allocated"; + EXPECT_EQ(slots.LiveCount(MG_Pipe::MGPipeKind::Query), liveBefore); } // The firing side of e2, on ALL SIX re-keyed object classes - the round-3 review's MAJOR 3. @@ -3670,6 +3879,39 @@ TEST(DirectGLESSlotTable, EverySwitchedOverKindResolvesItsTwinThroughTheHandleAr [] { return MakeShared(0u); }); } +// The two-holder fix, end to end through the REAL Texture registry and the REAL destructor: +// a by-value copy of TextureImpl::g_backendTextureObjects - which is precisely what +// ScopedDirectGLESTextureBindings keeps in `previousRegistry` for the length of a test - must +// drop the twin on the same notice the registry global does, with no sweep and no second call. +TEST(DirectGLESSlotTable, ASavedCopyOfARealRegistryDropsTheTwinOnTheSameNotice) { + using namespace MobileGL; + using namespace MobileGL::MG_Backend::DirectGLES; + using namespace MobileGL::MG_State::GLState; + + if (!EsprytSlotTablesEnabled()) { + GTEST_SKIP() << "the legacy arm keys twins on the frontend heap address and cannot " + "answer a death notice"; + } + + auto& registry = TextureImpl::g_backendTextureObjects; + SharedPtr texture = MakeShared(0u); + auto& twin = registry.GetOrCreate(texture); + (void)twin; + const MG_Pipe::MGPipeHandle handle = registry.HandleOf(texture.get()); + ASSERT_FALSE(MG_Pipe::MGPipeHandleIsNull(handle)); + + // The fixture's shape: copy the registry while the twin is live. + auto saved = registry; + ASSERT_NE(saved.FindByHandle(handle), nullptr) << "the copy did not carry the live entry"; + + // The real destructor raises the real notice; nothing else runs. + texture.reset(); + EXPECT_EQ(registry.FindByHandle(handle), nullptr) << "the registry global kept the twin"; + EXPECT_EQ(saved.FindByHandle(handle), nullptr) + << "the saved copy kept the dead texture's twin - the dispatcher told one holder only"; + EXPECT_FALSE(registry.DestroyByLifetimeId(0)) << "sanity: a null id frees nothing"; +} + // The gate on MAJOR 1 of the round-3 review. Commit d89fb684 raised // Fatal{PipeLegacyMemosDisabled} from inside InitDisplayAndContext(), i.e. from inside EGL // bring-up - and the integration harness pre-flights EGL bring-up in a FORKED CHILD, converting @@ -3697,20 +3939,18 @@ TEST(DirectGLESSlotTable, AnArmlessKnobCombinationStopsInsteadOfSkippingTheLane) EXPECT_EQ(MG_Backend::DirectGLES::ClassifyEsprytSlotArm(false, true), EsprytSlotArmVerdict::Legacy); EXPECT_EQ(MG_Backend::DirectGLES::ClassifyEsprytSlotArm(false, false), EsprytSlotArmVerdict::NoArm); - const Uint64 savedPush = MG_Config::Features.PipePush; - const Bool savedLegacy = MG_Config::Features.PipeLegacyMemos; - MG_Config::Features.PipePush = savedPush & ~MG_Pipe::kMGPipeSubsystemEsprytSlots; - MG_Config::Features.PipeLegacyMemos = false; + // Both guards restore on every exit path - a failed ASSERT included - so no later case in + // this binary runs on a mutated config or without the operator's file log, and the log + // path is unique per process and per case (the round-4 review's minor 5). + const ScopedArmlessKnobPair knobs; ASSERT_EQ(MG_Backend::DirectGLES::CurrentEsprytSlotArmVerdict(), EsprytSlotArmVerdict::NoArm); - - const fs::path logPath = fs::temp_directory_path() / "mobilegl-espryt-armless-knobs.log"; - fs::remove(logPath); - MG_Util::Debug::Close(); - SetEnvVar("MOBILEGL_LOG_FILE_PATH", logPath.string().c_str()); + const ScopedLogFileRedirect log(UniqueScratchLogPath("mobilegl-espryt-armless-knobs")); // Bring-up's half of the split. It must NAME the knobs and it must RETURN: this call is the // one InitDisplayAndContext() makes, and it runs inside the harness's forked pre-flight // child. If it ever stops again, this line takes the whole binary down and the case is red. + // (That the call SITE still makes this call and not the stopping one is + // EglBringUpUnderTheArmlessKnobPairReturnsInsteadOfStopping below.) MG_Backend::DirectGLES::DiagnoseEsprytSlotArm(); #if !defined(_WIN32) @@ -3721,19 +3961,9 @@ TEST(DirectGLESSlotTable, AnArmlessKnobCombinationStopsInsteadOfSkippingTheLane) ::testing::KilledBySignal(SIGABRT), ""); #endif - MG_Util::Debug::Close(); - UnsetEnvVar("MOBILEGL_LOG_FILE_PATH"); - MG_Config::Features.PipePush = savedPush; - MG_Config::Features.PipeLegacyMemos = savedLegacy; - - std::string contents; - { - std::ifstream logFile(logPath); - ASSERT_TRUE(logFile.good()) << "neither the diagnosis nor the fatal wrote a line an " - "operator could read"; - contents.assign(std::istreambuf_iterator(logFile), std::istreambuf_iterator()); - } - fs::remove(logPath); + const std::string contents = log.Contents(); + ASSERT_FALSE(contents.empty()) << "neither the diagnosis nor the fatal wrote a line an " + "operator could read"; EXPECT_NE(contents.find("PipeLegacyMemosDisabled"), std::string::npos) << contents; EXPECT_NE(contents.find("MOBILEGL_PIPE_PUSH"), std::string::npos) << contents; @@ -3746,6 +3976,64 @@ TEST(DirectGLESSlotTable, AnArmlessKnobCombinationStopsInsteadOfSkippingTheLane) #endif // MOBILEGL_PIPE_LEGACY_MEMOS } +// The round-4 review's minor 4: the case above pins the two FUNCTIONS, and nothing failed if +// InitDisplayAndContext() (DirectGLES.cpp) was edited back to call the stopping one - which is +// exactly the regression that produced the round-3 major. This pins the CALL SITE, by running +// the real bring-up entry point under the armless pair. +// +// No display is needed: InitDisplayAndContext's twin-arm call is its first statement after +// the context teardown, ahead of eglGetDisplay, so an EGL table whose eglGetDisplay answers +// EGL_NO_DISPLAY takes bring-up through that call and straight back out with `false`. The +// child must then EXIT with the code below. If the site stops again it dies of SIGABRT +// instead, and the death test fails - naming both knobs - rather than skipping: in the +// integration harness that same abort is what turned into a green lane that ran nothing. +// +// Not skipped in a build without the legacy arm either: there the verdict is Handles and +// bring-up has nothing to diagnose, but it must still return, and this says so. +TEST(DirectGLESSlotTable, EglBringUpUnderTheArmlessKnobPairReturnsInsteadOfStopping) { +#if defined(_WIN32) + GTEST_SKIP() << "needs a forked death test"; +#else + using namespace MobileGL; + + constexpr int kBringUpReturnedFalse = 0x51; + constexpr int kBringUpReturnedTrue = 0x52; + + // The redirect is set up in the PARENT: the child inherits the environment and writes the + // file, the parent reads it once the child has gone, and the guard restores the operator's + // log path either way. + const ScopedLogFileRedirect log(UniqueScratchLogPath("mobilegl-espryt-armless-bringup")); + + EXPECT_EXIT( + { + const ScopedArmlessKnobPair knobs; + MG_External::EGLFunctionsTable egl{}; + egl.eglGetDisplay = +[](EGLNativeDisplayType) -> EGLDisplay { return EGL_NO_DISPLAY; }; + MG_Backend::DirectGLES::SetEGLFuncsTable(egl); + const Bool ok = MG_Backend::DirectGLES::InitPbufferSurface(1, 1); + MG_Util::Debug::Close(); + std::exit(ok ? kBringUpReturnedTrue : kBringUpReturnedFalse); + }, + ::testing::ExitedWithCode(kBringUpReturnedFalse), "") + << "EGL bring-up under MOBILEGL_PIPE_PUSH with kMGPipeSubsystemEsprytSlots clear and " + "MOBILEGL_PIPE_LEGACY_MEMOS=0 did not RETURN: InitDisplayAndContext() is stopping " + "on the armless knob pair again instead of diagnosing it, and the integration " + "harness's forked pre-flight turns that stop into a lane that skips every scenario"; + +#if MOBILEGL_PIPE_LEGACY_MEMOS + // And it diagnosed, by name, on the way through - the operator is told which two knobs + // they set before the first draw - without a Fatal{} anywhere in bring-up. + const std::string contents = log.Contents(); + EXPECT_NE(contents.find("PipeLegacyMemosDisabled"), std::string::npos) + << "bring-up returned but did not diagnose the armless pair: " << contents; + EXPECT_NE(contents.find("MOBILEGL_PIPE_PUSH"), std::string::npos) << contents; + EXPECT_NE(contents.find("MOBILEGL_PIPE_LEGACY_MEMOS=0"), std::string::npos) << contents; + EXPECT_EQ(contents.find("Fatal{"), std::string::npos) + << "bring-up wrote a Fatal{} - the stop is back inside EGL bring-up: " << contents; +#endif +#endif +} + #else // G2 wants the pull and the push build to list the SAME ctest entries. The twin table only // exists under MOBILEGL_PIPE_PUSH, so in the pull build each case above keeps its name and @@ -3801,4 +4089,20 @@ TEST(DirectGLESSlotTable, AnArmlessKnobCombinationStopsInsteadOfSkippingTheLane) TEST(DirectGLESSlotTable, EverySwitchedOverKindResolvesItsTwinThroughTheHandleArm) { GTEST_SKIP() << "the {slot, gen} twin table is compiled only under MOBILEGL_PIPE_PUSH"; } + +TEST(DirectGLESSlotTable, ANegativeLookupIsNotCachedAcrossAnotherHoldersAcquire) { + GTEST_SKIP() << "the {slot, gen} twin table is compiled only under MOBILEGL_PIPE_PUSH"; +} + +TEST(DirectGLESSlotTable, OneDeathNoticeDropsTheTwinInEveryHolderOfTheKind) { + GTEST_SKIP() << "the {slot, gen} twin table is compiled only under MOBILEGL_PIPE_PUSH"; +} + +TEST(DirectGLESSlotTable, ASavedCopyOfARealRegistryDropsTheTwinOnTheSameNotice) { + GTEST_SKIP() << "the {slot, gen} twin table is compiled only under MOBILEGL_PIPE_PUSH"; +} + +TEST(DirectGLESSlotTable, EglBringUpUnderTheArmlessKnobPairReturnsInsteadOfStopping) { + GTEST_SKIP() << "the {slot, gen} twin table is compiled only under MOBILEGL_PIPE_PUSH"; +} #endif // MOBILEGL_PIPE_PUSH