diff --git a/MobileGL/MG_Impl/Pipe/CompositeResolver.h b/MobileGL/MG_Impl/Pipe/CompositeResolver.h index 98b941b2..99e0593a 100644 --- a/MobileGL/MG_Impl/Pipe/CompositeResolver.h +++ b/MobileGL/MG_Impl/Pipe/CompositeResolver.h @@ -102,6 +102,13 @@ namespace MobileGL::MG_Pipe { Entry* entry = Find(key); 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; ++m_counters.Reuses; return handle; } @@ -115,35 +122,33 @@ namespace MobileGL::MG_Pipe { entry->Handle = handle; entry->CompositeLifetimeId = composite.GetLifetimeId(); entry->Live = true; + entry->Fresh = true; ++m_counters.Mints; return handle; } - // The pipeline object itself is going away, or a test is tearing down. Speaks the same - // release for whatever it still holds. - void Forget(Uint pipelineName) { - for (SizeT i = 0; i < m_entries.size(); ++i) { - if (m_entries[i].PipelineName != pipelineName) continue; - ReleaseEntry(m_entries[i]); - m_entries[i] = m_entries.back(); - m_entries.pop_back(); - return; - } - } - // 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. Only the - // memo's freshness is dropped, which costs at most one re-observation. + // released here - 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. void Reset() { - for (Entry& entry : m_entries) entry.Live = false; + for (Entry& entry : m_entries) entry.Fresh = false; } void ResetCounters() { m_counters = Counters{}; } MGPipeHandle HandleFor(Uint pipelineName) const { for (const Entry& entry : m_entries) { - if (entry.PipelineName == pipelineName && entry.Live) return entry.Handle; + if (entry.PipelineName == pipelineName && entry.Live && entry.Fresh) return entry.Handle; } return kMGPipeNullHandle; } @@ -156,11 +161,26 @@ 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. Uint PipelineName = 0; DrawProgramSignature Signature{}; MGPipeHandle Handle = kMGPipeNullHandle; Uint64 CompositeLifetimeId = 0; + // 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) { diff --git a/MobileGL/MG_Test/Pipe/CompositeResolverTest.cpp b/MobileGL/MG_Test/Pipe/CompositeResolverTest.cpp index 44cde3bd..ae8ab015 100644 --- a/MobileGL/MG_Test/Pipe/CompositeResolverTest.cpp +++ b/MobileGL/MG_Test/Pipe/CompositeResolverTest.cpp @@ -360,7 +360,8 @@ TEST(CompositeResolver, ASlotAtTheShaderCsoLimitIsRefusedWhileTheLastBandSlotIsN X(CompositeResolver, ASignatureThatHasNotMovedReusesOneComposite) \ X(CompositeResolver, TwoPipelinesWithTheSameSignatureKeepTheirOwnComposite) \ X(CompositeResolver, EvictionThenDestructionFreesTheSlotExactlyOnce) \ - X(CompositeResolver, DestructionThenEvictionFreesTheSlotExactlyOnce) + X(CompositeResolver, DestructionThenEvictionFreesTheSlotExactlyOnce) \ + X(CompositeResolver, ASignatureMoveAfterAMakeCurrentStillReleasesThroughTheResolver) #define MGL_DECLARE_PULL_SKIP(Suite, Name) \ TEST(Suite, Name) { GTEST_SKIP() << "compiled only under MOBILEGL_PIPE_PUSH"; } @@ -399,6 +400,13 @@ void main() { gl_Position = vec4(0.0, 0.0, 0.0, 1.0); } const char* kFs = R"(#version 430 core out vec4 o_color; void main() { o_color = vec4(1.0); } +)"; + // A SECOND fragment stage, so a pipeline's draw-program signature can be made to move for + // real: ComputeDrawProgramSignature is the per-stage {lifetimeId, GetLinkVersion()} array, + // and a different ProgramObject is a different lifetime id. + const char* kFs2 = R"(#version 430 core +out vec4 o_color; +void main() { o_color = vec4(0.5); } )"; // Built by hand rather than through glCreateShaderProgramv, for ProgramPipelineCompositeTest's @@ -555,6 +563,63 @@ void main() { o_color = vec4(1.0); } EXPECT_NE(recycled.Gen, cso.Gen); MGPipeSlots().Free(MGPipeKind::ShaderCso, recycled); } + + // THE RELEASE PATH THE TWO CASES ABOVE DO NOT TOUCH. Both of them call the death helper + // directly, so the resolver is not in the picture at all and its own release - the + // pipeline-cache path, the one the header says exists precisely because "usually" is not a + // contract - had no case of its own. This drives it, and it drives it AFTER A + // MAKE-CURRENT, which is where it used to be permanently disarmed: + // + // Reset() cleared the entry's one flag; the next Observe took the reuse branch and + // returned before anything could restore it; from then on ReleaseEntry saw !Live and + // returned immediately - no delete_shader_state, no Free, no counter movement - and the + // old composite's handle was simply overwritten out of the resolver. Nothing leaked + // today only because the frontend's one-slot pipeline cache drops the last SharedPtr on + // the overwrite, which is exactly the "a client that only reacted to destructors" case + // the design refuses to rely on. + // + // The release obligation now lives in its own flag and a make-current does not touch it. + TEST(CompositeResolver, ASignatureMoveAfterAMakeCurrentStillReleasesThroughTheResolver) { + ResolverScope scope; + const GLuint vs = MakeSeparableProgram(GL_VERTEX_SHADER, kVs); + const GLuint fs = MakeSeparableProgram(GL_FRAGMENT_SHADER, kFs); + const GLuint pipeline = MakeBoundPipeline(vs, fs); + + ASSERT_GT(MGPipeProgramEmitterInstance().EmitShaderState(Ctx()), 0u); + const MGPipeHandle first = MGPipeProgramEmitterInstance().DrawCso(); + ASSERT_FALSE(MGPipeHandleIsNull(first)); + ASSERT_TRUE(MGPipeIsCompositeShaderSlot(first.Slot)); + ASSERT_TRUE(MGPipeSlots().IsLive(MGPipeKind::ShaderCso, first)); + ASSERT_EQ(MGPipeCompositeResolverInstance().GetCounters().Releases, 0u); + + // THE MAKE-CURRENT. This is exactly what PipeFill's FreshlyPrimed arm does, and it + // reaches MGPipeCompositeResolver::Reset() through the program emitter's own Reset(). + MGPipeProgramEmitterInstance().Reset(); + // ... followed by a draw whose stage set has NOT moved, which is the reuse branch. + MGPipeProgramEmitterInstance().EmitShaderState(Ctx()); + EXPECT_EQ(MGPipeProgramEmitterInstance().DrawCso(), first) + << "the same stage set is the same composite and the same handle"; + EXPECT_GE(MGPipeCompositeResolverInstance().GetCounters().Reuses, 1u); + EXPECT_EQ(MGPipeCompositeResolverInstance().GetCounters().Releases, 0u) + << "a reuse releases nothing"; + + // NOW THE STAGE SET REALLY MOVES: a different fragment stage program is a different + // lifetime id, so ComputeDrawProgramSignature moves and the frontend builds a second + // composite. The resolver has to speak the release for the first one. + const GLuint fs2 = MakeSeparableProgram(GL_FRAGMENT_SHADER, kFs2); + GL::UseProgramStages(pipeline, GL_FRAGMENT_SHADER_BIT, fs2); + GL::UseProgram(0); + + MGPipeProgramEmitterInstance().EmitShaderState(Ctx()); + const MGPipeHandle second = MGPipeProgramEmitterInstance().DrawCso(); + ASSERT_FALSE(MGPipeHandleIsNull(second)); + EXPECT_NE(second, first) << "a moved signature is a second composite with its own handle"; + EXPECT_EQ(MGPipeCompositeResolverInstance().GetCounters().Releases, 1u) + << "the resolver's own release path, spoken after a make-current"; + EXPECT_FALSE(MGPipeSlots().IsLive(MGPipeKind::ShaderCso, first)) + << "and the first composite's slot really went back, exactly once"; + EXPECT_TRUE(MGPipeSlots().IsLive(MGPipeKind::ShaderCso, second)); + } } // namespace #endif // MOBILEGL_PIPE_PUSH