From ac09e5b37aaeb95c80849f5df38bf80bd35b815b Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 8 Sep 2026 15:33:42 -0400 Subject: [PATCH] [Fix] (Pipe): key the composite resolver's memo on the context as well as the pipeline's GL name - the resolver is a process singleton while GL names are per context, so a make-current released the other context's live composite --- MobileGL/MG_Impl/Pipe/CompositeResolver.h | 154 ++++++++++++------ MobileGL/MG_Impl/Pipe/ProgramEmit.h | 9 +- .../MG_Test/Pipe/CompositeResolverTest.cpp | 119 +++++++++++++- 3 files changed, 231 insertions(+), 51 deletions(-) diff --git a/MobileGL/MG_Impl/Pipe/CompositeResolver.h b/MobileGL/MG_Impl/Pipe/CompositeResolver.h index 99e0593a..ccaaf91d 100644 --- a/MobileGL/MG_Impl/Pipe/CompositeResolver.h +++ b/MobileGL/MG_Impl/Pipe/CompositeResolver.h @@ -39,10 +39,39 @@ // no-op: MGPipeSlotAllocator::Free refuses a slot that is not live at that generation and // bumps no generation of its own, so a double release cannot skip a generation either. // -// THE KEY IS ComputeDrawProgramSignature(), the per-graphics-stage {lifetimeId, GetLinkVersion()} -// array - and DELIBERATELY NOT GetBackendStateVersion(), which is what made the SSO -// conformance loop rebuild the composite (glslang + SPIR-V + spirv-opt) on every draw, because -// a glUniform1i to a sampler moves it. +// THE MEMO's KEY IS (CONTEXT ID, PIPELINE GL NAME) AND THE CONTEXT HALF IS NOT OPTIONAL. +// This resolver is a PROCESS singleton while a pipeline's GL name is per context: GLContext +// owns m_programPipelines AND its own name generator m_programPipelineNames (Core.h), so name +// N names two different ProgramPipelineObjects in two contexts, each with its own composite +// and its own handle. Keyed on the name alone, the first emission after a make-current found +// the OTHER context's entry, matched nothing - two composites are two ProgramObjects with two +// lifetime ids, so the handles differ even when the stage set and the signature are identical +// - and released it: a delete_shader_state and a cleared publication latch for a composite +// whose frontend ProgramObject is alive, its band slot handed back and re-issued at gen + 1, +// and the server rebuilding that program (glslang + SPIR-V + spirv-opt, the very cost the +// signature below exists to avoid) once per context switch. +// +// THE CONTEXT ID IS GLContext::GetTextureContextId() AND NOTHING ELSE - the tree's existing +// never-reused per-context id (TextureState::AllocateContextId; PipeInputs carries it as +// m_textureContextId at seven fill points and the backends' own per-context memos key on it). +// Deliberately NOT the GLContext ADDRESS that MGB_CTX_IDENTITY and MGPipeTracker::m_context +// compare, because Core.h states the reason that id exists at all: a context freed and remade +// lands on the old heap address, which would put this same defect back one context recreation +// later. +// +// WHAT RELEASES A DESTROYED CONTEXT's ENTRIES: nothing in this file, and that is the correct +// answer rather than an omission. Destroying a context drops m_programPipelines, which drops +// each ProgramPipelineObject, which drops the composite it cached; ~ProgramObject then runs +// MGPipeEmitShaderCsoDestroyAndFree - the composite's OWN release path, the second of the two +// above - and the slot goes back exactly once. The entries those composites leave behind can +// never be found again (no future Observe can carry a dead context id) and could not release +// anything if they were (the allocator erases the lifetime-id mapping on Free), so Reset() +// DROPS them instead of releasing them. That is also what bounds the vector; see Reset(). +// +// THE SIGNATURE IS ComputeDrawProgramSignature(), the per-graphics-stage {lifetimeId, +// GetLinkVersion()} array - and DELIBERATELY NOT GetBackendStateVersion(), which is what made +// the SSO conformance loop rebuild the composite (glslang + SPIR-V + spirv-opt) on every draw, +// because a glUniform1i to a sampler moves it. // // HEADER-ONLY, for the ownership reason Tracker.h states: a new .cpp would need the root // CMakeLists.txt, which is the contract package's. @@ -84,6 +113,11 @@ namespace MobileGL::MG_Pipe { Uint64 Mints = 0; // signatures this resolver has seen minted Uint64 Reuses = 0; // a signature that had not moved Uint64 Releases = 0; // signature-move releases, i.e. the pipeline-cache path + // Entries dropped by Reset() because the composite's slot was already gone - the + // shape every entry of a DESTROYED CONTEXT ends in. A dropped entry is not a + // release: nothing is emitted and nothing is freed, the obligation having been + // discharged by the composite's own ~ProgramObject. + Uint64 Sweeps = 0; }; // Told, at every emission, which composite the frontend handed out for which pipeline. @@ -95,63 +129,81 @@ namespace MobileGL::MG_Pipe { // one death helper and in its fixed order. That is the pipeline-cache release path; the // composite's own destructor is the other one and the second of the two is the proven // no-op. - MGPipeHandle Observe(const ProgramPipelineObject& pipeline, const ProgramObject& composite, - MGPipeHandle handle) { + MGPipeHandle Observe(Uint64 contextId, const ProgramPipelineObject& pipeline, + const ProgramObject& composite, MGPipeHandle handle) { const DrawProgramSignature signature = pipeline.ComputeDrawProgramSignature(); - const Uint key = pipeline.GetExternalIndex(); - Entry* entry = Find(key); + const Uint pipelineName = pipeline.GetExternalIndex(); + Entry* entry = Find(contextId, pipelineName); if (entry != nullptr) { if (entry->Signature == signature && entry->Handle == handle) { // THE SAME COMPOSITE. Not merely "the same signature": the handle is minted // off the composite ProgramObject's own lifetime id, so an identical handle - // IS an identical object, and there is nothing to release whatever the - // memo's freshness says. Re-arming Fresh here is what a Reset() costs - one - // re-observation - and Live is deliberately NOT touched, because it is the - // release obligation and it is still owed for exactly this handle. - entry->Fresh = true; + // IS an identical object and there is nothing to release. Live is + // deliberately NOT touched - it is the release obligation and it is still + // owed for exactly this handle. ++m_counters.Reuses; return handle; } + // A MOVED SIGNATURE ON THIS CONTEXT's OWN ENTRY, which is the only thing that + // can reach here now: another context's pipeline of the same name is not found + // above and therefore not released, its obligation staying owed to the context + // that took it. ReleaseEntry(*entry); } else { m_entries.push_back(Entry{}); entry = &m_entries.back(); - entry->PipelineName = key; + entry->ContextId = contextId; + entry->PipelineName = pipelineName; } entry->Signature = signature; entry->Handle = handle; entry->CompositeLifetimeId = composite.GetLifetimeId(); entry->Live = true; - entry->Fresh = true; ++m_counters.Mints; return handle; } - // A make-current. The entries name composites that belong to the frontend objects of - // the context being left, and those objects outlive the switch, so the RECORDS are not - // released here - releasing them would emit a delete for a live program. + // A make-current, and it RELEASES NOTHING. The entries name composites that belong to + // the frontend objects of the context being left, those objects outlive the switch, and + // releasing them would emit a delete for a live program. // - // ONLY THE MEMO's FRESHNESS IS DROPPED, AND `Fresh` IS A SEPARATE FLAG FROM `Live` FOR - // EXACTLY THAT REASON. The two were one flag and the conflation was a real defect: the - // reuse branch above returns before anything could restore it, so after the first - // make-current every entry stayed at false for the life of the process and ReleaseEntry - // early-returned for ever - no delete_shader_state, no Free, no counter, and the old - // composite's handle simply overwritten out of the resolver. `Live` is a RELEASE - // OBLIGATION and nothing but ReleaseEntry may clear it; `Fresh` is the memo's own "does - // this entry describe the current context's pipeline of this name", which costs at most - // one re-observation when it is wrong. + // NOR IS ANY MEMO INVALIDATED, and that is what the context key bought. This used to + // clear a per-entry `Fresh` flag beside `Live`, because with a name-only key an entry + // could not say whether it described "my own pipeline before the switch" or "another + // context's pipeline of the same name" - and exactly one of those two properties could + // hold at a time. The key answers the question directly now, so the freshness flag and + // its one reader (a HandleFor() accessor that had no caller anywhere in the tree) are + // both gone rather than left as scaffolding: `Live`, the release obligation, is the + // entry's only state and nothing but ReleaseEntry may clear it. + // + // WHAT IS LEFT TO DO HERE IS RECLAMATION, and this is the one moment the client is told + // that a context boundary was crossed. An entry whose composite slot is no longer live + // has had its obligation discharged elsewhere - by that composite's own ~ProgramObject, + // which is precisely what happened to EVERY entry of a context that has just been + // destroyed - so it is DROPPED rather than released: a release would resolve nothing + // anyway (the allocator erases the lifetime-id mapping on Free) and no reader is left. + // Without this the vector would grow by one per (context, pipeline name) pair the + // process ever used, where the name-only key bounded it by the highest pipeline name; + // with it, it is bounded by the pairs whose composite slot is actually live. void Reset() { - for (Entry& entry : m_entries) entry.Fresh = false; + SizeT kept = 0; + for (SizeT i = 0; i < m_entries.size(); ++i) { + if (!m_entries[i].Live || !MGPipeSlots().IsLive(MGPipeKind::ShaderCso, m_entries[i].Handle)) { + ++m_counters.Sweeps; + continue; + } + if (kept != i) m_entries[kept] = m_entries[i]; + ++kept; + } + m_entries.resize(kept); } void ResetCounters() { m_counters = Counters{}; } - MGPipeHandle HandleFor(Uint pipelineName) const { - for (const Entry& entry : m_entries) { - if (entry.PipelineName == pipelineName && entry.Live && entry.Fresh) return entry.Handle; - } - return kMGPipeNullHandle; - } + // Diagnostics and unit cases only; nothing on the emission path asks. There is no + // HandleFor(name) accessor and there must not be one: the emitter takes the handle from + // the composite ProgramObject it already holds, so a lookup by name would be a second + // authority on an identity the allocator already owns. SizeT Size() const { return m_entries.size(); } const Counters& GetCounters() const { return m_counters; } @@ -161,17 +213,21 @@ namespace MobileGL::MG_Pipe { // choice: a static that held one would put a frontend destructor on an exit // handler's path into a torn-down pipe. A GL name, a signature of plain integers, // a handle and a lifetime id are all this needs. - // KEYED ON THE GL NAME, because a ProgramPipelineObject has no lifetime id - - // ComputeDrawProgramSignature reads the STAGE programs' ids and the pipeline itself - // carries none. glGenProgramPipelines recycles names, so a deleted-and-recreated - // pipeline can inherit its predecessor's entry; that is bounded and self-correcting - // rather than a hazard. The first Observe on the new object finds a signature and a - // handle that do not match and releases the old entry, and that release resolves - // NOTHING - the allocator erases the lifetime-id mapping on Free, so a stale - // CompositeLifetimeId emits no delete and frees no slot; all it costs is one - // redundant, idempotent death notice, which is the same shape the composite's own - // second release path already has. The vector is keyed by name, so it cannot grow - // past the highest pipeline name the process ever used. + // KEYED ON (CONTEXT ID, GL NAME), and the name half is the GL name because a + // ProgramPipelineObject has no lifetime id - ComputeDrawProgramSignature reads the + // STAGE programs' ids and the pipeline itself carries none. The context half is + // GLContext::GetTextureContextId(); see the file header for why the name alone was + // wrong and why the context ADDRESS would be too. + // + // WITHIN ONE CONTEXT glGenProgramPipelines recycles names, so a deleted-and- + // recreated pipeline can still inherit its predecessor's entry; that is bounded and + // self-correcting rather than a hazard. The first Observe on the new object finds a + // signature and a handle that do not match and releases the old entry, and that + // release resolves NOTHING - the allocator erases the lifetime-id mapping on Free, + // so a stale CompositeLifetimeId emits no delete and frees no slot; all it costs is + // one redundant, idempotent death notice, which is the same shape the composite's + // own second release path already has. + Uint64 ContextId = 0; Uint PipelineName = 0; DrawProgramSignature Signature{}; MGPipeHandle Handle = kMGPipeNullHandle; @@ -179,13 +235,13 @@ namespace MobileGL::MG_Pipe { // THE RELEASE OBLIGATION. Set when this entry takes responsibility for a composite's // slot, cleared ONLY by ReleaseEntry when that responsibility is discharged. Bool Live = false; - // THE MEMO's FRESHNESS, and deliberately not the same flag as Live - see Reset(). - Bool Fresh = false; }; - Entry* Find(Uint pipelineName) { + // BOTH HALVES OF THE KEY, always. An entry of another context is not this pipeline's + // entry: not found, not matched, not released. + Entry* Find(Uint64 contextId, Uint pipelineName) { for (Entry& entry : m_entries) { - if (entry.PipelineName == pipelineName) return &entry; + if (entry.ContextId == contextId && entry.PipelineName == pipelineName) return &entry; } return nullptr; } diff --git a/MobileGL/MG_Impl/Pipe/ProgramEmit.h b/MobileGL/MG_Impl/Pipe/ProgramEmit.h index cab9302e..cebd2a7b 100644 --- a/MobileGL/MG_Impl/Pipe/ProgramEmit.h +++ b/MobileGL/MG_Impl/Pipe/ProgramEmit.h @@ -115,7 +115,14 @@ namespace MobileGL::MG_Pipe { // allocator refuses a slot that is not live at that generation. if (drawProgram && MGPipeProgramIsPipelineComposite(*drawProgram)) { if (const auto& pipeline = ctx.GetBoundProgramPipeline()) { - MGPipeCompositeResolverInstance().Observe(*pipeline, *drawProgram, drawCso); + // THE CONTEXT IS PART OF THE RESOLVER's KEY and this is the only place that + // supplies it: the resolver is a process singleton and a pipeline's GL name + // is per context, so without it a make-current between two contexts holding + // one pipeline name released the other context's LIVE composite. + // GetTextureContextId() is the tree's never-reused per-context id, the same + // one PipeInputs carries and the backends' per-context memos key on. + MGPipeCompositeResolverInstance().Observe(ctx.GetTextureContextId(), *pipeline, + *drawProgram, drawCso); } } const MGPipeHandle dispatchCso = diff --git a/MobileGL/MG_Test/Pipe/CompositeResolverTest.cpp b/MobileGL/MG_Test/Pipe/CompositeResolverTest.cpp index ae8ab015..0ae906e3 100644 --- a/MobileGL/MG_Test/Pipe/CompositeResolverTest.cpp +++ b/MobileGL/MG_Test/Pipe/CompositeResolverTest.cpp @@ -361,7 +361,9 @@ TEST(CompositeResolver, ASlotAtTheShaderCsoLimitIsRefusedWhileTheLastBandSlotIsN X(CompositeResolver, TwoPipelinesWithTheSameSignatureKeepTheirOwnComposite) \ X(CompositeResolver, EvictionThenDestructionFreesTheSlotExactlyOnce) \ X(CompositeResolver, DestructionThenEvictionFreesTheSlotExactlyOnce) \ - X(CompositeResolver, ASignatureMoveAfterAMakeCurrentStillReleasesThroughTheResolver) + X(CompositeResolver, ASignatureMoveAfterAMakeCurrentStillReleasesThroughTheResolver) \ + X(CompositeResolver, TwoContextsHoldingOnePipelineNameKeepTheirOwnComposites) \ + X(CompositeResolver, ADestroyedContextsEntryIsDroppedRatherThanReleasedASecondTime) #define MGL_DECLARE_PULL_SKIP(Suite, Name) \ TEST(Suite, Name) { GTEST_SKIP() << "compiled only under MOBILEGL_PIPE_PUSH"; } @@ -373,6 +375,7 @@ namespace { namespace GL = MobileGL::MG_Impl::GLImpl; using GLContext = MG_State::GLState::GLContext; using MG_State::GLState::ProgramObject; + using MG_State::GLState::ProgramPipelineObject; struct ResolverScope { ResolverScope() { Clear(); } @@ -620,6 +623,120 @@ void main() { o_color = vec4(0.5); } << "and the first composite's slot really went back, exactly once"; EXPECT_TRUE(MGPipeSlots().IsLive(MGPipeKind::ShaderCso, second)); } + + // THE RESOLVER IS A PROCESS SINGLETON AND A PIPELINE's GL NAME IS PER CONTEXT (C2-M1). + // GLContext owns m_programPipelines AND its own name generator m_programPipelineNames, so + // name N names two different ProgramPipelineObjects in two contexts, each with its own + // composite and its own handle. Keyed on the name alone, the first Observe after a + // make-current found the OTHER context's entry: the signature matched - two composites of + // the same stage set have the same signature, and here two default pipelines have the same + // all-zero one - while the handle could not, because two composites are two ProgramObjects + // with two lifetime ids. So it fell into ReleaseEntry and emitted delete_shader_state for a + // LIVE composite, cleared its publication latch and handed its band slot back while the + // frontend ProgramObject was still alive. + // + // DRIVEN AT THE RESOLVER RATHER THAN THROUGH GL, because one test process has one + // GLContext. Everything the case supplies is what the single production call site supplies: + // the context id is GLContext::GetTextureContextId()'s value, the two same-named pipeline + // objects are what two contexts hold, and the composites are ordinary ProgramObjects at + // external index 0 out of the reserved band, exactly as Core.cpp builds them. + TEST(CompositeResolver, TwoContextsHoldingOnePipelineNameKeepTheirOwnComposites) { + ResolverScope scope; + auto& resolver = MGPipeCompositeResolverInstance(); + const LinkArtifacts link; + const SpirvArtifacts spirv; + + constexpr Uint kSharedPipelineName = 9u; + constexpr Uint64 kContextA = 0x51A00001ull; + constexpr Uint64 kContextB = 0x51A00002ull; + const ProgramPipelineObject pipelineA{kSharedPipelineName}; + const ProgramPipelineObject pipelineB{kSharedPipelineName}; + const SharedPtr compositeA = MakeShared(0u); + const SharedPtr compositeB = MakeShared(0u); + ASSERT_TRUE(MGPipeProgramIsPipelineComposite(*compositeA)); + ASSERT_EQ(pipelineA.GetExternalIndex(), pipelineB.GetExternalIndex()); + + const MGPipeHandle handleA = MGPipeSlots().AllocateComposite(compositeA->GetLifetimeId()); + const MGPipeHandle handleB = MGPipeSlots().AllocateComposite(compositeB->GetLifetimeId()); + ASSERT_NE(handleA, handleB); + for (const MGPipeHandle handle : {handleA, handleB}) { + MGPipeApplyCreateShaderState(CompositeDesc(handle, 0x3u), &link, &spirv); + MGPipeNoteHandlePublished(MGPipeKind::ShaderCso, handle); + } + const Uint64 releasesBefore = resolver.GetCounters().Releases; + + EXPECT_EQ(resolver.Observe(kContextA, pipelineA, *compositeA, handleA), handleA); + // THE MAKE-CURRENT, which is what PipeFill's FreshlyPrimed arm reaches through the + // program emitter's own Reset()... + MGPipeProgramEmitterInstance().Reset(); + // ... and then context B draws with ITS pipeline of the same name. + EXPECT_EQ(resolver.Observe(kContextB, pipelineB, *compositeB, handleB), handleB); + + EXPECT_EQ(resolver.GetCounters().Releases, releasesBefore) + << "the other context's entry was released - it is not this pipeline's entry"; + EXPECT_TRUE(MGPipeSlots().IsLive(MGPipeKind::ShaderCso, handleA)) + << "A's band slot went back while A's composite ProgramObject was still alive"; + EXPECT_TRUE(MGPipeHandleIsPublished(MGPipeKind::ShaderCso, handleA)) + << "delete_shader_state went out for a live composite and cleared its latch"; + EXPECT_TRUE(MGPipeSlots().IsLive(MGPipeKind::ShaderCso, handleB)); + + // AND BACK TO A. Its obligation stayed owed to its own context, so the unmoved + // signature is a reuse of the same handle and still nothing is released. + const Uint64 reusesBefore = resolver.GetCounters().Reuses; + MGPipeProgramEmitterInstance().Reset(); + EXPECT_EQ(resolver.Observe(kContextA, pipelineA, *compositeA, handleA), handleA); + EXPECT_EQ(resolver.GetCounters().Reuses, reusesBefore + 1u); + EXPECT_EQ(resolver.GetCounters().Releases, releasesBefore); + } + + // WHERE A DESTROYED CONTEXT's ENTRIES ARE RELEASED, and it is not in the resolver. The + // context's death drops m_programPipelines, which drops the ProgramPipelineObject, which + // drops the composite it cached; ~ProgramObject then runs the one client-side death helper + // and the band slot goes back EXACTLY ONCE. The resolver speaks no second delete - the + // entry can never be found again, and the allocator has erased the lifetime-id mapping + // anyway - and the next make-current DROPS the stranded entry, which is what keeps the + // vector bounded now that its key carries the context. + TEST(CompositeResolver, ADestroyedContextsEntryIsDroppedRatherThanReleasedASecondTime) { + ResolverScope scope; + auto& resolver = MGPipeCompositeResolverInstance(); + const LinkArtifacts link; + const SpirvArtifacts spirv; + + constexpr Uint kPipelineName = 11u; + constexpr Uint64 kDoomedContext = 0x51A00003ull; + // ResolverScope's Clear() has already run one Reset(), so every entry standing here has + // a live composite slot and nothing but this case's own entry can be swept below. + const SizeT sizeBefore = resolver.Size(); + const Uint32 liveBefore = MGPipeSlots().LiveCount(MGPipeKind::ShaderCso); + + const ProgramPipelineObject pipeline{kPipelineName}; + SharedPtr composite = MakeShared(0u); + const MGPipeHandle handle = MGPipeSlots().AllocateComposite(composite->GetLifetimeId()); + MGPipeApplyCreateShaderState(CompositeDesc(handle, 0x3u), &link, &spirv); + MGPipeNoteHandlePublished(MGPipeKind::ShaderCso, handle); + ASSERT_EQ(resolver.Observe(kDoomedContext, pipeline, *composite, handle), handle); + ASSERT_EQ(resolver.Size(), sizeBefore + 1u); + const Uint64 releasesBefore = resolver.GetCounters().Releases; + const Uint64 sweepsBefore = resolver.GetCounters().Sweeps; + + // THE CONTEXT DIES: the last SharedPtr to its composite goes with its pipeline. + composite.reset(); + EXPECT_FALSE(MGPipeSlots().IsLive(MGPipeKind::ShaderCso, handle)) + << "~ProgramObject is the release path for this entry and it freed the slot"; + EXPECT_FALSE(MGPipeHandleIsPublished(MGPipeKind::ShaderCso, handle)) + << "and it took the publication latch with the delete"; + EXPECT_EQ(MGPipeSlots().LiveCount(MGPipeKind::ShaderCso), liveBefore) << "exactly once"; + EXPECT_EQ(resolver.GetCounters().Releases, releasesBefore) + << "the resolver spoke no release of its own for it"; + + // THE NEXT CONTEXT's FIRST VERB. The stranded entry is dropped, not released. + MGPipeProgramEmitterInstance().Reset(); + EXPECT_EQ(resolver.GetCounters().Sweeps, sweepsBefore + 1u); + EXPECT_EQ(resolver.GetCounters().Releases, releasesBefore) + << "a dropped entry emits nothing and frees nothing - the obligation was discharged"; + EXPECT_EQ(resolver.Size(), sizeBefore) + << "the vector is bounded by the pairs whose composite slot is actually live"; + } } // namespace #endif // MOBILEGL_PIPE_PUSH