From 6cabc6b070fba12b77b7035d99daccf8e1a996c1 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Wed, 16 Sep 2026 06:03:52 -0400 Subject: [PATCH] [Fix, Test] (MG_IntegrationTest, MG_Test): make the persistent-map membership expectation a function of the arm the map landed in, and pin the arm-independent half on IsLivePersistentMap where both answers can be produced --- .../PersistentCoherentMapScenario.cpp | 76 +++++++++++- MobileGL/MG_Test/Buffer/SplitBufferTest.cpp | 109 +++++++++++++++++- 2 files changed, 178 insertions(+), 7 deletions(-) diff --git a/MobileGL/MG_IntegrationTest/Scenarios/PersistentCoherentMapScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/PersistentCoherentMapScenario.cpp index c6e08c73..7ee9be12 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/PersistentCoherentMapScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/PersistentCoherentMapScenario.cpp @@ -43,6 +43,16 @@ // address really is valid in this process, which is precisely why E3(d) is worth gating at all - // and a direct count of declines would need a counter from packages b1/v1. // +// AND MEMBERSHIP FOLLOWS THE ARM (ID-42). The client's live-persistent-map set is the set of maps +// the client still has to PUSH, so an ADOPTED store is deliberately not in it - +// PersistentMapTracker::IsLivePersistentMap's second row is IsBackendPersistentMapped(), and an +// adopted store's bytes are already in coherent GPU memory. AssertMembership therefore expects +// `live == (arm == emulated)`, from the same peek AssertOrRecordArm reads. The arm-INDEPENDENT +// claim - "the predicate reads the chain, not the adopt tier" - belongs to a unit case that can +// drive both answers on demand (SplitBufferSet. +// TheAdoptedArmIsNotAMemberAndItIsTheChainRowThatSaysSo) and not to an integration entry, which +// only ever sees whichever arm its driver and transport happen to give it. +// // `mpr` (map-persistent-roundtrips) is counted per ACQUISITION ATTEMPT, mint or decline // (ARCHITECTURE.md:492), so it is the SAME NUMBER on both arms and in both transports: that is // what makes exit gate E3(c)'s "mpr equal to the monolith arm's" checkable by one process. Both @@ -243,6 +253,11 @@ void main() { oColor = vec4(vColor, 1.0); } ASSERT_TRUE(PeekBufferIsAdoptedPersistentMap(vbo, &adopted)) << "no frontend BufferObject behind GL buffer " << vbo << " " << when; const char* arm = adopted ? "adopted" : "emulated"; + // REMEMBERED, because AssertMembership's expectation is a function of it (ID-42) + // and the two have to be talking about ONE observation of ONE buffer. A second + // peek would be a second question, and a scenario that asked it twice could + // straddle a change and then compare two different moments. + m_observedArm = adopted ? ObservedArm::Adopted : ObservedArm::Emulated; RecordProperty("persistent_map_arm", arm); if (declared.empty()) return; ASSERT_EQ(declared, std::string(arm)) @@ -259,21 +274,63 @@ void main() { oColor = vec4(vColor, 1.0); } // b1-v1.md 4.1 item 2: the client's live-persistent-map set is meant to be exactly // SyncPersistentMappedRange's early-out chain. A drift between the two stops the push // silently; asking here makes it a named failure instead. + // + // MEMBERSHIP IS A PROPERTY OF THE ARM, NOT OF THE FLAGS - INTEGRATOR DECISION ID-42, + // and the first cut of this function got it wrong in a way no lane could see. It + // asserted `live` UNCONDITIONALLY, on the grounds that "a PERSISTENT|WRITE|COHERENT map + // that is not FLUSH_EXPLICIT is a member by construction". That sentence is true only + // on the EMULATED arm. PersistentMapTracker::IsLivePersistentMap's second row is + // `if (buffer.IsBackendPersistentMapped()) return false;` - deliberately, because the + // predicate must read the CHAIN and not the tier: an adopted store's bytes are already + // in coherent GPU memory and there is nothing for the push to ship, so it is not a + // member and must not be one. On the adopted arm `live` is false BY DESIGN. + // + // It survived the first wave because the two builds that ran it could not contradict + // it: the push build compiles no MG_Remote, so MGITEST_PERSISTENT_MAP_TRACKER is + // undefined and this function returned at the line above before asserting anything; + // the split build's monolith lane, where the tracker IS compiled, adopts + // (MGPipeApplyMapPersistent's R-6 decline is ANDed with `Transport != Monolith`, + // PipeApply.cpp:2005), and seven entries went red the first time the assertion ran at + // all. + // + // So the expectation is `live == (arm == emulated)`, taken from the SAME peek + // AssertOrRecordArm read. The arm-independent half - "the predicate reads the chain, + // not the tier" - is not an integration claim and is pinned where it can be driven + // directly, by MG_Test/Buffer/SplitBufferTest.cpp's + // SplitBufferSet.TheAdoptedArmIsNotAMemberAndItIsTheChainRowThatSaysSo. void AssertMembership(unsigned int vbo, const char* when) { if (!PersistentMapTrackerAvailable()) { RecordProperty("persistent_map_membership", "unavailable"); return; } + if (m_observedArm == ObservedArm::NotLookedAt) { + // AssertOrRecordArm could not look, so there is no arm to condition on and + // "could not look" is not "it was emulated" (PersistentMapPeek.h). A lane that + // DECLARES an arm has already been skipped by AssertOrRecordArm in this case. + RecordProperty("persistent_map_membership", "arm unknown"); + return; + } + const bool emulated = (m_observedArm == ObservedArm::Emulated); bool live = false; ASSERT_TRUE(PeekBufferIsLivePersistentMap(vbo, &live)) << "the tracker is compiled in but could not answer for GL buffer " << vbo << " " << when; - EXPECT_TRUE(live) - << "a PERSISTENT|WRITE|COHERENT map that is not FLUSH_EXPLICIT is a member of " - "the client's live-persistent-map set by construction, and it is not one " - << when + RecordProperty("persistent_map_membership", live ? "member" : "not a member"); + EXPECT_EQ(live, emulated) + << "the client's live-persistent-map set is the set of maps the client still has " + "to PUSH, so membership follows the arm: on the emulated arm this " + "PERSISTENT|WRITE|COHERENT non-FLUSH_EXPLICIT map must be a member, and on " + "the adopted arm it must not be - IsLivePersistentMap's " + "IsBackendPersistentMapped() row takes it out, because an adopted store's " + "bytes are already in coherent GPU memory and there is nothing to ship. This " + "map landed in the " + << (emulated ? "emulated" : "adopted") << " arm " << when << " and the predicate " + << (live ? "made it a member" : "did not make it a member") << ". The set is supposed to BE SyncPersistentMappedRange's early-out chain; if " - "they have drifted, the push stops shipping this buffer's blocks and nothing " - "else says so."; + "they have drifted, the push either stops shipping this buffer's blocks or " + "starts shipping an adopted store's, and nothing else says so. (ID-42. The " + "arm-independent statement - that the predicate reads the chain and not the " + "adopt tier - is pinned by SplitBufferSet." + "TheAdoptedArmIsNotAMemberAndItIsTheChainRowThatSaysSo, not here.)"; } void TearDown() override { @@ -320,6 +377,13 @@ void main() { oColor = vec4(vColor, 1.0); } return RegionIsMostly(image, 8, image.Width() - 9, 8, image.Height() - 9, color, 0.0, when); } + // The arm this scenario OBSERVED, as opposed to the one its lane declared. A lane may + // declare none (the monolith counting lane does not), and the peek may be unable to + // look at all, so the third state is not "assume emulated" - it is "there is no arm to + // condition on", and AssertMembership records rather than asserts under it. + enum class ObservedArm { NotLookedAt, Adopted, Emulated }; + ObservedArm m_observedArm = ObservedArm::NotLookedAt; + unsigned int m_program = 0; unsigned int m_vao = 0; unsigned int m_vbo = 0; diff --git a/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp b/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp index a1370ddb..d60f7aa1 100644 --- a/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp +++ b/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp @@ -41,6 +41,21 @@ namespace { using MG_Remote::Client::GpuWriteProducer; using MG_Remote::Client::PersistentMapTracker; + // A backend that MINTS a persistent mapping, for the one case that needs the adopted arm + // (TheAdoptedArmIsNotAMemberAndItIsTheChainRowThatSaysSo). Only AcquirePersistentMap is + // filled in: the frontend null-checks every op individually, and a table with one live + // member is the smallest thing that makes AcquireMemoryRange's legacy adoption arm fire. + // The storage is the CASE's, not this table's - AdoptPersistentMap keeps the pointer and + // never owns it - so the base travels through a file-scope variable the case sets and + // clears around the one acquisition it wants minted. + void* g_mintedPersistentBase = nullptr; + void* MintPersistentMap(BufferObject&) { return g_mintedPersistentBase; } + const MG_State::GLState::BufferBackendOps g_mintingBufferOps = [] { + MG_State::GLState::BufferBackendOps ops{}; + ops.AcquirePersistentMap = &MintPersistentMap; + return ops; + }(); + // Everything in this package is gated on `Transport != Monolith`, so every case has to // put the process into a split configuration and put it back. A fixture rather than a // lambda because the tracker is a process-wide singleton and a case that left an entry in @@ -64,6 +79,11 @@ namespace { MG_Remote::Client::ResetProducerMarkCountsForTest(); } void TearDown() override { + // Belt and braces for the one case that installs a minting backend: a table left + // behind would make the NEXT case's acquisition land in the adopted arm, and every + // membership assertion in this file would then be about a different code path. + MG_State::GLState::SetBufferBackendOps(nullptr); + g_mintedPersistentBase = nullptr; PersistentMapTracker::Instance().ClearForTest(); MG_State::pGLContext = Move(m_context); MG_Config::Transport = m_transport; @@ -258,6 +278,92 @@ TEST_F(SplitBufferSet, MembershipIsSyncPersistentMappedRangesOwnEarlyOutChain) { buffer->ReleaseMemory(false); } +// THE ADOPTED ARM IS NOT A MEMBER, AND IT IS THE CHAIN THAT SAYS SO - NOT THE TIER (ID-42). +// +// IsLivePersistentMap's second row is `if (buffer.IsBackendPersistentMapped()) return false;`, +// and its comment promises that the row answers for ITSELF: at tier T2 the arm is unreachable +// because MapPersistent declines, but a build that reaches T0/T1 later must get the same answer +// out of the same row. Nothing drove that promise. The case above only ever sees the declined +// arm, and PersistentCoherentMapScenario's membership assertion cannot pin it either - an +// integration entry sees whichever arm its driver and transport hand it, which is exactly how +// that assertion came to state the emulated arm's property as if it were universal and go red on +// seven split-monolith entries the first time it ran. +// +// So the statement is pinned HERE, where both answers can be produced on demand. The adoption is +// made by the PRODUCTION path - AcquireMemoryRange dispatching to the backend's +// AcquirePersistentMap and calling PipeResource::AdoptPersistentMap - and not by the case +// reaching into the buffer, because a hand-set flag would still be "true" with the production +// adoption deleted (R-16). The transport is Monolith for exactly that call, because R-6's decline +// is ANDed with `Transport != Monolith` and there is no other way to reach a mint in this build; +// the PREDICATE is then asked with the tier back at T2/InProcess, which is the whole point: the +// tier says "emulated, always" and the chain still says "not a member". +TEST_F(SplitBufferSet, TheAdoptedArmIsNotAMemberAndItIsTheChainRowThatSaysSo) { + constexpr SizeT kSize = 4096; + // The storage the backend "mints". Declared first so it outlives the buffer: AdoptPersistentMap + // stores the pointer and releases the shadow, and it never owns what it was handed. + Vector minted(kSize, static_cast(0)); + g_mintedPersistentBase = minted.data(); + + auto adopted = MakeBuffer(27u, kSize); + auto declined = MakeBuffer(28u, kSize); + + // The declined twin first, with no backend ops at all: same flags, same size, same call. + declined->AcquireMemoryRange(Range1D{0, kSize}, + BufferMappingAccessBit::Write | BufferMappingAccessBit::Persistent); + ASSERT_FALSE(declined->IsBackendPersistentMapped()); + ASSERT_TRUE(PersistentMapTracker::IsLivePersistentMap(*declined)) + << "the emulated arm IS a member - if this fails the two halves are not comparable and the " + "assertion below proves nothing"; + + { + MG_State::GLState::SetBufferBackendOps(&g_mintingBufferOps); + MG_Config::Transport = MG_Config::TransportMode::Monolith; + adopted->AcquireMemoryRange(Range1D{0, kSize}, + BufferMappingAccessBit::Write | BufferMappingAccessBit::Persistent); + MG_Config::Transport = MG_Config::TransportMode::InProcess; + MG_State::GLState::SetBufferBackendOps(nullptr); + } + ASSERT_TRUE(adopted->IsBackendPersistentMapped()) + << "the backend declined the mint, so there is no adopted arm here to ask about"; + + // THE TIER SAYS EMULATED. The chain must still say "not a member". + ASSERT_TRUE(MG_Remote::Client::AdoptTierIsEmulate()); + ASSERT_NE(MG_Config::Transport, MG_Config::TransportMode::Monolith); + EXPECT_FALSE(PersistentMapTracker::IsLivePersistentMap(*adopted)) + << "an adopted store's bytes are already in host-visible coherent GPU memory and there is " + "nothing for the push to ship, so IsBackendPersistentMapped() takes it out of the set. " + "This answer must come from that ROW and not from the adopt tier: the tier is T2 and the " + "transport is InProcess right now, which is the configuration in which R-6 says every " + "acquisition declines - and the store in front of the predicate is adopted anyway, " + "because a later phase's T0/T1 will mint one. A predicate that read the tier would call " + "it a member and the push would read an adopted store's Bytes() as if it were the " + "shadow."; + + // The SET agrees with the predicate, asked through the production entry point rather than by + // reading a member: NoteMapStateChanged is what every one of the five maintenance events + // calls, and it must refuse to enrol an adopted store. + PersistentMapTracker::Instance().NoteMapStateChanged(*adopted); + EXPECT_EQ(PersistentMapTracker::Instance().MemberCount(), 1u) + << "only the declined twin: enrolling an adopted store would make the push read its " + "Bytes() - which is the GPU map, not the shadow - as if it were bytes to ship"; + + // ...and the consequence, which is the one that would actually corrupt something. + const Uint64 before = MG_Util::PipeStats::TotalBytes(MG_Util::PipeStats::ByteClass::PersistentMapPush); + MG_Remote::Client::PushPersistentMapsBeforeVerb(); + EXPECT_EQ(MG_Util::PipeStats::TotalBytes(MG_Util::PipeStats::ByteClass::PersistentMapPush) - before, + static_cast(kSize)) + << "the declined twin's 4096 bytes and nothing else: the adopted buffer must contribute no " + "pushed bytes at all"; + + declined->ReleaseMemory(false); + // The adopted one is NOT released through ReleaseMemory: ReleasePersistentMap is for a store + // being redefined, and a persistent map the application holds outlives every unmap by + // definition (PipeResource.h:121-127). It is dropped here with the mapping still adopted, + // which is also the shape ~BufferObject has to survive. + adopted.reset(); + g_mintedPersistentBase = nullptr; +} + // pmap is non-zero, and it is non-zero in BLOCKS. TEST_F(SplitBufferSet, ThePushCutsTheMappedSpanIntoBlocksAndMovesPmap) { constexpr SizeT kSize = 4u * 64u * 1024u; // exactly four 64 KiB blocks @@ -402,7 +508,7 @@ TEST_F(SplitBufferSet, OnlyAdoptTierTwoIsImplemented) { #else -// THE SAME FIFTEEN NAMES, SO THE ctest NAME SET DOES NOT MOVE BETWEEN LANES. G2 compares the +// THE SAME SIXTEEN NAMES, SO THE ctest NAME SET DOES NOT MOVE BETWEEN LANES. G2 compares the // pull and push name lists line for line and G14 allows build-split to ADD names but never to // remove one, so a case that exists only where it can run would break both gates for a reason // that has nothing to do with what it tests. It skips instead, and says why. @@ -417,6 +523,7 @@ TEST(SplitBufferSet, Row4AReadPixelsIntoAPackPboMarksThePbo) { MGL_SPLIT_ONLY_OR TEST(SplitBufferSet, Row5EndTransformFeedbackMarksTheCaptureTargets) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, TheWholeSetIsInertOnTheMonolithPath) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, MembershipIsSyncPersistentMappedRangesOwnEarlyOutChain) { MGL_SPLIT_ONLY_OR_SKIP(); } +TEST(SplitBufferSet, TheAdoptedArmIsNotAMemberAndItIsTheChainRowThatSaysSo) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, ThePushCutsTheMappedSpanIntoBlocksAndMovesPmap) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, TheLastBlockIsTheRemainderAndNotAWholeBlock) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, BothEdgesOfAWriteMapPublishOneStateRecord) { MGL_SPLIT_ONLY_OR_SKIP(); }