From c9dd173201c2a3d4290bf9e5aa46fee0eeeb363c Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Sun, 6 Sep 2026 13:21:07 -0400 Subject: [PATCH] [Fix] (Espryt): close four review minors on the twin table - a walk that can outlive its vector, a null call that is not arm-equivalent, and two comments that claimed more than the code does - ForEachLive walked with a range-for and handed fn a reference INTO m_slots, so a callee that reached GetOrCreate on the same table would resize the vector under both. Index loop and a copied twin, the shape ReclaimDeadSlots already uses. The one caller today happens not to insert; that is not a property the walk should depend on. - GetOrCreate(nullptr) reset the parking twin on EVERY call, so a second null call destroyed what the first was handed. The map arm kept its null-keyed entry until a sweep, so this was an arm difference in the one path (SyncTextureObjectToBackend) that documents relying on the tolerance. It now keeps the parked twin, and the case makes a second call. - The one-entry memo's comment claimed the three per-draw resolution paths ask for the same object every draw. Two of them do not: BindCurrentFBO resolves both targets in a frame and ResolveUnitSamplerBackend asks per texture unit, so both thrash a single-entry memo and pay a probe P1 did not. The comment now says so and names the fix (per-unit / per-target) and the gate that would price it (G11, device-side, owed). - HandleOf caches a NULL answer too - deliberate, because a bound-but-never-synced object would otherwise re-probe every draw - and what makes it safe is that GetOrCreate refreshes the memo. Nothing pinned that; RepeatedLookupsOfALiveObjectKeepOneHandle now does. - Removed the dead #if MOBILEGL_PIPE_PUSH nested inside #if MOBILEGL_PIPE_PUSH in ScopedDetachedTextureFramebufferAttachments. --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 4 +- MobileGL/MG_Backend/DirectGLES/SlotTables.h | 44 +++++++++++++------ MobileGL/MG_Test/SanityTest.cpp | 26 ++++++++++- 3 files changed, 57 insertions(+), 17 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 3b72d841..1946bd41 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -6706,7 +6706,8 @@ namespace MobileGL::MG_Backend::DirectGLES { } }; -#if MOBILEGL_PIPE_PUSH + // Already inside #if MOBILEGL_PIPE_PUSH, so no second guard here: the arm choice + // below is the RUNTIME one. if (EsprytSlotTablesEnabled()) { FramebufferImpl::g_backendFramebufferObjects.ForEachLive( [&](const SharedPtr& stateFBO, @@ -6715,7 +6716,6 @@ namespace MobileGL::MG_Backend::DirectGLES { }); return; } -#endif #if MOBILEGL_PIPE_LEGACY_MEMOS for (auto it = FramebufferImpl::g_backendFramebufferObjects.begin(); it != FramebufferImpl::g_backendFramebufferObjects.end(); ++it) { diff --git a/MobileGL/MG_Backend/DirectGLES/SlotTables.h b/MobileGL/MG_Backend/DirectGLES/SlotTables.h index b2c9751e..078e2466 100644 --- a/MobileGL/MG_Backend/DirectGLES/SlotTables.h +++ b/MobileGL/MG_Backend/DirectGLES/SlotTables.h @@ -137,12 +137,15 @@ namespace MobileGL::MG_Backend::DirectGLES { // No assert on null here, unlike the map arm: null is TOLERATED, so a DEBUG build // must not trap where the release build quietly does the documented thing. if (stateObj == nullptr) { - // The registry this replaces inserted a null key and handed back ITS twin slot - // (DirectGLES.cpp's SyncTextureObjectToBackend documents relying on exactly - // that tolerance), so a release build never dereferenced null here. Keep the - // shape: one per-table parking slot, never live, never swept, never handed a - // handle. A null object has no identity and therefore cannot have a twin. - m_nullTwin.reset(); + // The registry this replaces inserted a null KEY and handed back that entry's + // twin (DirectGLES.cpp's SyncTextureObjectToBackend documents relying on + // exactly that tolerance), so a release build never dereferenced null here. + // Keep the shape exactly, INCLUDING across calls: the map kept its null-keyed + // 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 + // therefore cannot have a {slot, gen}. return m_nullTwin; } @@ -285,11 +288,18 @@ namespace MobileGL::MG_Backend::DirectGLES { // a dangling key the way the old iteration could. template void ForEachLive(Fn&& fn) const { - for (const Entry& entry : m_slots) { + // 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. + for (SizeT slot = 0; slot < m_slots.size(); ++slot) { + const Entry& entry = m_slots[slot]; if (!entry.Live || !entry.backend) continue; const StatePtr state = entry.stateRef.lock(); if (!state) continue; - fn(state, entry.backend); + const BackendPtr twin = entry.backend; + fn(state, twin); } } @@ -322,12 +332,18 @@ namespace MobileGL::MG_Backend::DirectGLES { // Handed back by GetOrCreate for a null state object. Never live, never swept. BackendPtr m_nullTwin; - // ONE-entry resolution memo, lifetimeId -> handle. The three per-draw resolution paths - // (ResolveVaoTwin, SyncCurrentProgram, BindCurrentFBO) ask the SAME table for the SAME - // object every draw, so this turns the steady state back into an integer compare plus - // one array index - which is what the deleted TwinLookupMemos bought and what D13 - // promises ("direct slot indexing - the memo existed only to avoid the hash probe"). - // Without it every resolution went through the allocator's ByLifetimeId hash. + // ONE-entry resolution memo, lifetimeId -> handle. It exists because without it every + // resolution goes through the allocator's ByLifetimeId hash, which the deleted + // TwinLookupMemos existed to avoid and which D13 promises to replace with "direct slot + // indexing". + // + // It is one entry and therefore only helps a caller that asks for the SAME object twice + // running - ResolveVaoTwin and SyncCurrentProgram do, once per draw each. Two callers + // it does NOT help, recorded rather than claimed away: BindCurrentFBO resolves BOTH + // targets in a frame, and ResolveUnitSamplerBackend asks for a different sampler per + // texture unit, so both thrash a single-entry memo and pay the probe P1 did not (P1 had + // 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: // * the key is a lifetime id, which MG_State never hands out twice, so a recycled diff --git a/MobileGL/MG_Test/SanityTest.cpp b/MobileGL/MG_Test/SanityTest.cpp index 93cc3154..c7e5adaf 100644 --- a/MobileGL/MG_Test/SanityTest.cpp +++ b/MobileGL/MG_Test/SanityTest.cpp @@ -3263,8 +3263,16 @@ 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. + 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"; for (int i = 0; i < 8; ++i) { auto* slot = table.GetOrCreate(object) ? table.Find(object.get()) : nullptr; @@ -3413,7 +3421,11 @@ TEST(DirectGLESSlotTable, AnnouncedDeathKeepsObjectChurnFromAccumulatingWithoutA EXPECT_EQ(table.LiveCount(), 0u); EXPECT_EQ(slots.LiveCount(MG_Pipe::MGPipeKind::Query), liveBefore) << "the churn leaked slots the announced deaths should have returned"; - EXPECT_LE(slots.HighWater(MG_Pipe::MGPipeKind::Query) - highWaterBefore, 1u) + // 2 and not 1: on a cold allocator the high-water mark counts the RESERVED slot 0 + // (kMGPipeFirstAllocatableSlot is 1) as well as the one slot this loop recycles, and ctest + // runs every case in its own process, so this case sees a cold allocator. What the bound + // rules out is the thing that matters - 256 churned objects growing the space by 256. + EXPECT_LE(slots.HighWater(MG_Pipe::MGPipeKind::Query) - highWaterBefore, 2u) << "the slot space grew with the churn instead of being recycled"; } @@ -3430,6 +3442,18 @@ TEST(DirectGLESSlotTable, GetOrCreateToleratesANullStateObject) { EXPECT_EQ(table.LiveCount(), 0u) << "a null object took a slot"; EXPECT_EQ(table.Find(nullptr), nullptr); EXPECT_TRUE(MG_Pipe::MGPipeHandleIsNull(table.HandleOf(nullptr))); + + // ...and a SECOND null call is handed the same parking slot rather than destroying what the + // first one was given. The map arm kept its null-keyed entry until a sweep, so a table that + // reset here would answer differently on the two arms in the one path that documents + // relying on this tolerance. + twin = MakeShared(); + twin->marker = 5; + auto& again = table.GetOrCreate(none); + ASSERT_NE(again, nullptr) << "the second null call destroyed the first one's parked twin"; + EXPECT_EQ(again->marker, 5); + EXPECT_EQ(&again, &twin); + EXPECT_EQ(table.LiveCount(), 0u); } // P2 step e2, the backend half. A sweep is a stand-in for a death notice; this is the notice. // Nothing below calls CollectGarbage*: the slot comes back, and the twin goes, at the moment