diff --git a/MobileGL/MG_Backend/MGPipe/PipeInputs.cpp b/MobileGL/MG_Backend/MGPipe/PipeInputs.cpp index 23ede787..939c03b5 100644 --- a/MobileGL/MG_Backend/MGPipe/PipeInputs.cpp +++ b/MobileGL/MG_Backend/MGPipe/PipeInputs.cpp @@ -87,6 +87,58 @@ namespace MobileGL::MG_Pipe { const MGPipeVerbClass verbClass = kMGPipeVerbClass[verbIndex]; return MGPipeFieldMaskHas(kMGPipeClassFieldMask[static_cast(verbClass)], field); } + + // ---- the same verdict, per verb CLASS, as a constant (P5d round 3, package C) ---- + // + // THE STAMP's ANSWER IS A CONSTANT OF THE VERB CLASS AND OF NOTHING ELSE. Both halves of + // `answerable` below read constexpr tables only - kMGPipeFieldOwnership, kMGPipeVerbClass + // and kMGPipeClassFieldMask - so the 63-field loop was recomputing, at every verb on the + // apply thread, a table the compiler can build once. The 2026-09-17 inproc profile put + // MGPipeServerStampVerbBoundary at 1.1% self of the apply thread (Minecraft 26.3-rc-3, + // ~852 draws/frame), and every cycle taken there is a cycle the lockstep client waits for. + // + // IT IS A MASK, NOT A BOOL ARRAY, so the stamp loop's body stays a shift and a store with + // no branch: FilledGen[i] = serial & -bit, which is `serial` for an answerable field and + // the WITHDRAWAL 0 for every other. The verdict itself is unchanged, and it is still + // computed by the one expression argued at the stamp - it just runs at compile time. + constexpr MGPipeFieldMask AnswerableMaskForClass(MGPipeVerbClass verbClass) { + MGPipeFieldMask mask{}; + for (SizeT i = 0; i < kMGPipeInputFieldCount; ++i) { + const auto field = static_cast(i); + const MGPipeFieldOwnership ownership = kMGPipeFieldOwnership[i]; + const Bool answerable = (ownership == MGPipeFieldOwnership::kRecordSupplied || + ownership == MGPipeFieldOwnership::kApplierDerived) && + MGPipeFieldMaskHas( + kMGPipeClassFieldMask[static_cast(verbClass)], field); + if (answerable) mask.Words[i / 64] |= (Uint64{1} << (i % 64)); + } + return mask; + } + + struct AnswerableMaskTable { + MGPipeFieldMask ByClass[kMGPipeVerbClassCount]; + }; + + constexpr AnswerableMaskTable MakeAnswerableMaskTable() { + AnswerableMaskTable table{}; + for (SizeT i = 0; i < kMGPipeVerbClassCount; ++i) { + table.ByClass[i] = AnswerableMaskForClass(static_cast(i)); + } + return table; + } + + constexpr AnswerableMaskTable kAnswerableByClass = MakeAnswerableMaskTable(); + + // A verb id outside the table answers NOTHING, which is exactly what FieldIsInVerbClass + // said for it (`verbIndex >= kMGPipeVerbCount` -> false for every field) and therefore + // what the stamp said: all 63 withdrawn, every read the poison Fatal. + constexpr MGPipeFieldMask kNoFieldIsAnswerable{}; + + const MGPipeFieldMask& AnswerableMaskForVerb(MGPipeVerb verb) { + const SizeT verbIndex = static_cast(verb); + if (verbIndex >= kMGPipeVerbCount) return kNoFieldIsAnswerable; + return kAnswerableByClass.ByClass[static_cast(kMGPipeVerbClass[verbIndex])]; + } } // namespace // The third door into the storage (see PipeInputs.h). It exists because neither of the @@ -108,23 +160,25 @@ namespace MobileGL::MG_Pipe { // BOTH branches of MGPipeInputFieldIsFresh, so the zeroing below is a real withdrawal // rather than a stamp that happens to be old. ++filled.CurrentVerbSerial; + // STAMPED: in this verb's class AND answerable out of the records the applier has + // already applied. WITHDRAWN (0): everything else - which is BARRIER-PULLED, FATAL, + // and anything the verb's own may-read table says this verb does not read. + // + // The withdrawal is the load-bearing half of the rule: the client's residual fill + // stamped all 63 fields at its own verb boundary, so without it every field would + // read fresh on the server, `rsp` would be identically 0 and the exit gate would be + // decoration. It also cancels the sticky exemption for free - generated/ + // PipeFilled.inc tests "never filled" BEFORE it tests sticky, so 0 wins over + // kMGPipeInputFieldSticky without a line of the generated file changing. + // + // THE VERDICT IS THE SAME EXPRESSION; it is just precomputed per verb class + // (AnswerableMaskForClass above) instead of re-derived 63 times per verb, so what is + // left here is a table lookup and a branch-free fill. + const MGPipeFieldMask& answerable = AnswerableMaskForVerb(verb); + const Uint64 serial = filled.CurrentVerbSerial; for (SizeT i = 0; i < kMGPipeInputFieldCount; ++i) { - const auto field = static_cast(i); - const MGPipeFieldOwnership ownership = kMGPipeFieldOwnership[i]; - // STAMPED: in this verb's class AND answerable out of the records the applier has - // already applied. WITHDRAWN (0): everything else - which is BARRIER-PULLED, FATAL, - // and anything the verb's own may-read table says this verb does not read. - // - // The withdrawal is the load-bearing half of the rule: the client's residual fill - // stamped all 63 fields at its own verb boundary, so without it every field would - // read fresh on the server, `rsp` would be identically 0 and the exit gate would be - // decoration. It also cancels the sticky exemption for free - generated/ - // PipeFilled.inc tests "never filled" BEFORE it tests sticky, so 0 wins over - // kMGPipeInputFieldSticky without a line of the generated file changing. - const Bool answerable = (ownership == MGPipeFieldOwnership::kRecordSupplied || - ownership == MGPipeFieldOwnership::kApplierDerived) && - FieldIsInVerbClass(field, verb); - filled.FilledGen[i] = answerable ? filled.CurrentVerbSerial : 0; + const Uint64 bit = (answerable.Words[i / 64] >> (i % 64)) & Uint64{1}; + filled.FilledGen[i] = serial & (Uint64{0} - bit); } MGPipeStampAccess::SetServerStamped(inputs, true); } diff --git a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp index 43b10a02..9acd5a4e 100644 --- a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp +++ b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp @@ -6745,6 +6745,16 @@ namespace MobileGL::MG_Impl::GLImpl { MG_State::pGLContext->GetImageTextureBinding(static_cast(unit)) .Bind(textureObject, level, layered, layer, access, format); MG_State::pGLContext->NoteTextureUnitTouched(static_cast(unit)); +#if MOBILEGL_PIPE_PUSH + // AND THE IMAGE-UNIT MARK BESIDE IT. The line above moves the TEXTURE-unit high-water + // mark, which is a different array: a reader that needs "the highest image unit ever + // bound" cannot take it from there without either over-walking (a texture bind at unit + // 31 with no image bound anywhere) or, worse, under-walking if that line ever moves. The + // split client's per-draw writable-image sweep is that reader + // (MG_Remote/Client/GpuWritePending.cpp). Push builds only, so the pull build's bytes do + // not move (G1). + MG_State::pGLContext->NoteImageUnitTouched(static_cast(unit)); +#endif MGP_FILL(BindImageTexture); bindImageTexture(unit, texture, level, layered, layer, access, format); } diff --git a/MobileGL/MG_Impl/Pipe/ImageEmit.h b/MobileGL/MG_Impl/Pipe/ImageEmit.h index 811143e5..59ece2b0 100644 --- a/MobileGL/MG_Impl/Pipe/ImageEmit.h +++ b/MobileGL/MG_Impl/Pipe/ImageEmit.h @@ -64,12 +64,20 @@ namespace MobileGL::MG_Pipe { // set_shader_images. Start is 0 and Count is the image-unit window described below. // - // WHERE THE HIGH-WATER MARK COMES FROM, because the frontend has none and this is the - // one place a reader will look for it. DirectGLES keeps g_imageUnitHighWaterMark, but - // that is written from inside its own per-unit sync and lives on the far side of the - // boundary; TextureState::NoteUnitTouched is the TEXTURE-unit path and - // glBindImageTexture does not reach it. Adding a counter to TextureState would edit - // another package's file and resize the pull build's object, which G1 forbids outright. + // WHERE THE HIGH-WATER MARK COMES FROM, because this is the one place a reader will look + // for it. DirectGLES keeps g_imageUnitHighWaterMark, but that is written from inside its + // own per-unit sync and lives on the far side of the boundary. + // + // AND IT IS STILL NOT TextureState's MARK, EVEN THOUGH ONE NOW EXISTS. P5d round 3 + // (package C) added TextureState::NoteImageUnitTouched - a push-build-only image-unit + // high-water mark, fed by glBindImageTexture - for the split client's per-draw + // GPU-write sweep. That mark answers "which units could hold a binding"; the window + // below answers the strictly narrower "which units could a SHADER READ", which is what + // makes the zero early-out fire for an application that binds an image no shader names. + // Swapping one for the other would widen this emitter for no record's benefit, and the + // paragraph that used to stand here - "adding a counter to TextureState would resize the + // pull build's object, which G1 forbids outright" - is answered by that mark being + // compiled only into push builds, where the object is already a different size. // // So the window is derived instead, from the one thing that decides whether an image // unit can matter at all: the highest image unit the CURRENT PROGRAM names, memoised diff --git a/MobileGL/MG_Impl/Pipe/PipeFill.cpp b/MobileGL/MG_Impl/Pipe/PipeFill.cpp index 5a516af4..e5304d78 100644 --- a/MobileGL/MG_Impl/Pipe/PipeFill.cpp +++ b/MobileGL/MG_Impl/Pipe/PipeFill.cpp @@ -361,7 +361,16 @@ namespace MobileGL::MG_Pipe { // An empty value never clears an omission a test armed through MGPipeSetPoisonOmission. void ParsePoisonOmissionKnob() { const String& knob = MG_Config::Features.PipePoisonOmit; - if (g_omissionKnobParsed && knob == g_omissionKnobValue) return; + // THE UNSET KNOB IS TWO LENGTH LOADS AND A BRANCH, inline, and that is the whole + // change (P5d r3, package C): this runs at every verb - 852 draws a frame on the + // profiled workload - and `knob == g_omissionKnobValue` is an out-of-line String + // compare even when both sides are empty, which is every shipping configuration. + // Sizes first, contents only when a non-empty value is involved; the re-parse + // condition is unchanged (the value differing from what the last parse latched). + if (g_omissionKnobParsed && knob.size() == g_omissionKnobValue.size() && + (knob.empty() || knob == g_omissionKnobValue)) { + return; + } g_omissionKnobParsed = true; g_omissionKnobValue = knob; if (knob.empty()) return; @@ -2355,6 +2364,168 @@ namespace MobileGL::MG_Pipe { return answer; } + // ---- the residual fill's SUPPLIED SET, computed once per environment (P5d r3, C) ---- + // + // WHY THIS CACHE EXISTS, AND WHAT IT DOES NOT CHANGE. Step 4 of the validate point asks, + // for every one of the 63 fields and at EVERY verb, whether an emitted call already + // supplied it. The question is the seven-term conjunction spelled below - and it is still + // spelled exactly once, so there is ONE copy of it: five of its terms are constants of the + // FIELD, and the other two are facts about the PROCESS - which subsystems the operator's + // mask carries, and whether the backend consumes the P4a families - that move a handful of + // times in a process's life and never inside a verb. So the conjunction is evaluated per + // field once per distinct environment and read back as a bit. Nothing about WHICH fields + // are copied moves: same expression, same inputs, same answer. + // + // AND THE PROFILE IS WHY IT IS WORTH A CACHE AT ALL. The 2026-09-17 inproc profile + // (Minecraft 26.3-rc-3, view distance 12, ~852 draws/frame) put MGPipeValidateForVerb at + // 4.11% self / 10.9% inclusive of the GL thread, of which MGPipeTracker::Update is 1.46 + // and CopyField 2.54; most of the rest is this walk's per-field predicate. + // P4aFamilyHasItsConsumer alone is a CapsMirror read under split, and it was taken 63 + // times per verb for an answer that is the same 63 times. + // + // THE KEY IS THE WHOLE OF WHAT THE EXPRESSION READS BESIDE THE FIELD ID, which is what + // makes this a memo rather than a latch that goes stale: + // - the push mask (MG_Config::Features.PipePush), which the per-subsystem A/B lanes move; + // - ApplierDerivesRenderStateFields(), a one-shot probe - in the key anyway, so that a + // build where it ever stopped being one-shot cannot keep a stale answer silently; + // - contextValuesWireLive, which flips when a session starts, stops, or tears its tables + // down (P5c rv, CONTRACT-P5C.md 5.3) - the half that must never disagree with the + // emission's half at the validate point, which is why it is PASSED IN rather than + // re-read here; + // - P4aFamilyHasItsConsumer, which flips when the caps mirror adopts a snapshot (R-12: a + // second arrival IS the invalidation) or a backend registers its resource op table. + // ALL FOUR FAMILIES RIDE THE ONE SIGNAL - that is P4aFamilyHasItsConsumer's own rule, + // argued where it is defined - so asking it once for the whole family mask is asking + // it for every family at once, and the rebuild re-asks it per subsystem from the same + // unchanged mirror. It is also the one key input that CANNOT MOVE AN ANSWER TODAY, + // and the static_assert below is that fact's trip wire rather than a claim in prose. + // + // NOT THREAD-LOCAL, AND THAT IS THIS FILE'S EXISTING RULE RATHER THAN A NEW ONE: + // g_residualDue, g_omission and the verify latches beside it are file-scope too, and what + // keeps a single writer on them is the verb barrier (ROADMAP.md's G1 row states it for + // gPipeInputs itself). A validate that runs on the apply thread is inside that barrier by + // construction. + // + // BUT THE ANSWER IS HANDED OUT BY VALUE, WHICH IS NOT THE SAME ARGUMENT (P5d r3 package C + // review, minor 1). Those latches are single BITS: a reader cannot observe one + // half-written. A 2x64-bit mask can, so the rebuild below fills a LOCAL and publishes it + // into the memo in one assignment, and the caller gets a copy rather than a reference into + // storage the next rebuild would zero under it. Sixteen bytes is cheaper at the use site + // than the indirection was anyway - the walk reads the copy 63 times. + + // AND THE FOURTH KEY INPUT CANNOT MOVE AN ANSWER TODAY, WHICH IS A FACT WITH A TRIP WIRE + // RATHER THAN A COMMENT (P5d r3 package C review, the major). + // + // The consumer conjunct sits in the expression because the expression reads it, and the + // key carries it because a key that omits an input the expression reads is how a memo + // goes stale. But every field whose emitter belongs to a P4a family - the five of them: + // GetFramebufferBindingSlot, GetImageTextureBinding, GetTextureUnitObject, + // GetProgramForDraw, GetProgramForDispatch - is ALSO a field + // EmittedCallSuppliesTheWholeField() answers `false` for: their storage + // is a frontend heap reference (a BindingSlot, an ImageTextureBinding, a TextureUnit, + // two SharedPtr) that a payload cannot carry, so the fill pulls them + // whatever the consumer says. The consumer conjunct is therefore DOMINATED: no motion of + // the caps mirror or of MGPipeGetResourceOps() can change one bit of the mask, and no + // unit case can distinguish a key that carries it from one that does not. + // + // WHEN THIS FIRES, that stopped being true - P3b/P4b/P7/P8 gave one of those rows a twin + // the applier can write - and the consumer signal became observable through + // MGPipeResidualFillSuppliesField. At that point the memo's key needs the same + // move-it-and-read-it-back pair steps 1-3 of + // FieldOwnershipTest.TheResidualFillsSuppliedMemoReKeysOnEveryInputThatMovesAnAnswer + // give the other three inputs, and that case's step 4 (which today pins the domination + // instead) has to become it. Do that rather than deleting this line. + constexpr Bool NoP4aFamilyFieldIsWhollySupplied() { + for (SizeT i = 0; i < kMGPipeInputFieldCount; ++i) { + const auto field = static_cast(i); + const Uint64 subsystem = SubsystemForEmitter(kMGPipeFieldEmittedBy[i]); + if ((subsystem & kMGPipeP4aFamilySubsystems) != 0 && + EmittedCallSuppliesTheWholeField(field)) { + return false; + } + } + return true; + } + static_assert(NoP4aFamilyFieldIsWhollySupplied(), + "a P4a-family field became whole-supplied: the residual fill's supplied-set " + "memo can now answer differently for the consumer signal, so that key input " + "needs the unit case the paragraph above names"); + + struct ResidualFillPlan { + Bool Valid = false; + Uint64 PushMask = 0; + Bool ApplierDerives = false; + Bool ContextValuesWireLive = false; + Bool P4aConsumer = false; + // One bit per FIELD - not per verb class. The class mask is applied at the walk + // exactly as it always was, so "does this verb read this field" stays the walk's + // business and this stays a statement about EMISSION alone. + MGPipeFieldMask Supplied{}; + }; + ResidualFillPlan g_fillPlan; + + MGPipeFieldMask SuppliedFieldMask(Uint64 pushMask, Bool applierDerives, + Bool contextValuesWireLive) { + // THE CONSUMER SIGNAL IS READ ONLY WHERE THE WALK BELOW COULD REACH IT, and that + // guard is not a micro-optimisation - it is what keeps R-8's DIAGNOSTIC COUNTERS + // where they were (P5d r3 package C review, minor 2). In the conjunction below + // P4aFamilyHasItsConsumer sits AFTER `(pushMask & subsystem) != 0`, and it answers a + // constant `true` for every subsystem outside the P4a families - so at a mask that + // carries no P4a bit NO field reaches the caps mirror at all. Asking it here anyway + // would have moved CapsMirror::ServerConsumes' refusal count and its one-shot + // MGLOG_W at exactly the hand-picked A/B lanes (and the bring-up window, CallMask 0) + // where the baseline never touched the mirror, which is the opposite direction from + // the one this change is supposed to move them in. The key stays COMPLETE because + // pushMask is itself in the key: a mask that gains a P4a bit re-keys on the mask. + const Bool p4aConsumer = (pushMask & kMGPipeP4aFamilySubsystems) != 0 && + P4aFamilyHasItsConsumer(kMGPipeP4aFamilySubsystems); + if (g_fillPlan.Valid && g_fillPlan.PushMask == pushMask && + g_fillPlan.ApplierDerives == applierDerives && + g_fillPlan.ContextValuesWireLive == contextValuesWireLive && + g_fillPlan.P4aConsumer == p4aConsumer) { + return g_fillPlan.Supplied; + } + MGPipeFieldMask built{}; + for (SizeT i = 0; i < kMGPipeInputFieldCount; ++i) { + const auto field = static_cast(i); + // A field a P2 call now supplies is not pulled again - that second pull is exactly + // the cost P2 exists to remove. THE STAMP IS UNCHANGED either way: a stamp says + // "this verb published this field", which is as true of an emitted field as of a + // copied one, and withholding it would abort every backend read of the very fields + // the migration just took over. The stamp is still written at the walk; only the + // QUESTION moved up here. + const MGPipeFieldEmitter emitter = kMGPipeFieldEmittedBy[i]; + const Uint64 subsystem = SubsystemForEmitter(emitter); + // P4aFamilyHasItsConsumer and P4aFamilyDependenciesAreSet are in this conjunction + // for the reason they are in `wants()`: "supplied" means A CALL WENT OUT CARRYING + // THIS FIELD, and on a backend with no consumer - or at a mask that leaves one of + // the family's D-K2 dependency bits clear - no P4a call went out at all, so + // withholding the pull here would leave the field unfilled at the very verb that + // reads it. + // + // AND THE LAST CONJUNCT IS P5c rv's (CONTRACT-P5C.md 5.3): set_context_values has + // NO PRODUCER without a live wire (its emission is transport-gated at the validate + // point), so its eight fields keep being pulled under monolith - G1's byte-for-byte + // rule - and are skipped only when the record really crosses. + const Bool supplied = subsystem != 0 && (subsystem & kMGPipeWiredSubsystems) != 0 && + (pushMask & subsystem) != 0 && + P4aFamilyHasItsConsumer(subsystem) && + P4aFamilyDependenciesAreSet(subsystem, pushMask) && + EmittedCallSuppliesTheWholeField(field) && + (applierDerives || AppliedWithoutDerivation(field)) && + (emitter != MGPipeFieldEmitter::SetContextValues || + contextValuesWireLive); + if (supplied) built.Words[i / 64] |= (Uint64{1} << (i % 64)); + } + // PUBLISHED IN ONE ASSIGNMENT, after the walk - see the paragraph above the struct. + g_fillPlan.Valid = true; + g_fillPlan.PushMask = pushMask; + g_fillPlan.ApplierDerives = applierDerives; + g_fillPlan.ContextValuesWireLive = contextValuesWireLive; + g_fillPlan.P4aConsumer = p4aConsumer; + g_fillPlan.Supplied = built; + return built; + } // set_pixel_pack_state. PACK only, deliberately: nothing on the far side of the // boundary reads unpack state, and the staged-repack upload path does not even issue @@ -2777,6 +2948,16 @@ namespace MobileGL::MG_Pipe { Uint64 MGPipeVertexAttribDefaultRepairCount() { return g_attribDefaultRepairs; } MGPVertexAttribDefaults MGPipeVertexAttribDefaultsLastHeader() { return g_attribDefaultLastHeader; } + // The unit gate's door onto step 4's predicate (PipeFill.h says why it needs one). It + // returns the memo's answer rather than a second copy of the expression, so a case that + // moves one input and reads the answer back is a statement about the MEMO and not about a + // re-implementation of it. + Bool MGPipeResidualFillSuppliesField(MGPipeInputField field, Uint64 pushMask, Bool applierDerives, + Bool contextValuesWireLive) { + return MGPipeFieldMaskHas(SuppliedFieldMask(pushMask, applierDerives, contextValuesWireLive), + field); + } + // ---- the validate point (P2 brief D1) ---- void MGPipeValidateForVerb(MGPipeVerb verb) { PipeInputs& inputs = gPipeInputs; @@ -3057,7 +3238,17 @@ namespace MobileGL::MG_Pipe { tracker.ClearPendingBaseInstance(); // ---- step 4: the residual fill, for what an emitted call did NOT supply ---- + // THE SUPPLIED QUESTION IS ASKED ONCE PER ENVIRONMENT, NOT ONCE PER FIELD PER VERB + // (SuppliedFieldMask above, P5d r3 package C): the conjunction it used to spell inline + // here reads nothing about the verb, so it is a memo keyed on the four process facts it + // does read. The two halves of P5c rv's gate still read the ONE `contextValuesWireLive` + // computed at the top of this function - it is the memo's key AND its argument - so they + // cannot disagree about who supplies the eight value-class fields. const Bool applierDerives = ApplierDerivesRenderStateFields(); + // BY VALUE, NOT BY REFERENCE: the memo's storage is rebuilt in place when the key moves, + // and the walk below holds this across 63 iterations. + const MGPipeFieldMask supplied = + SuppliedFieldMask(pushMask, applierDerives, contextValuesWireLive); for (SizeT i = 0; i < kMGPipeInputFieldCount; ++i) { const auto field = static_cast(i); if (!MGPipeFieldMaskHas(mask, field)) continue; @@ -3071,33 +3262,7 @@ namespace MobileGL::MG_Pipe { #else if (kMGPipeInputFieldSticky[i]) continue; #endif - // A field a P2 call now supplies is not pulled again - that second pull is - // exactly the cost P2 exists to remove. THE STAMP IS UNCHANGED either way: a - // stamp says "this verb published this field", which is as true of an emitted - // field as of a copied one, and withholding it would abort every backend read of - // the very fields the migration just took over. - const MGPipeFieldEmitter emitter = kMGPipeFieldEmittedBy[i]; - const Uint64 subsystem = SubsystemForEmitter(emitter); - // P4aFamilyHasItsConsumer and P4aFamilyDependenciesAreSet are in this conjunction for - // the reason they are in `wants()`: "supplied" means A CALL WENT OUT CARRYING THIS - // FIELD, and on a backend with no consumer - or at a mask that leaves one of the - // family's D-K2 dependency bits clear - no P4a call went out at all, so withholding - // the pull here would leave the field unfilled at the very verb that reads it. - // - // AND THE LAST CONJUNCT IS P5c rv's (CONTRACT-P5C.md §5.3): set_context_values has - // NO PRODUCER without a live wire (its emission is transport-gated, above), so its - // eight fields keep being pulled under monolith - G1's byte-for-byte rule - and are - // skipped only when the record really crosses. The two sides read the ONE answer - // computed at the top of this function, so they cannot disagree. - const Bool supplied = subsystem != 0 && (subsystem & kMGPipeWiredSubsystems) != 0 && - (pushMask & subsystem) != 0 && - P4aFamilyHasItsConsumer(subsystem) && - P4aFamilyDependenciesAreSet(subsystem, pushMask) && - EmittedCallSuppliesTheWholeField(field) && - (applierDerives || AppliedWithoutDerivation(field)) && - (emitter != MGPipeFieldEmitter::SetContextValues || - contextValuesWireLive); - if (!supplied) MGPipeFillAccess::CopyField(inputs, *ctx, field); + if (!MGPipeFieldMaskHas(supplied, field)) MGPipeFillAccess::CopyField(inputs, *ctx, field); #if MOBILEGL_PIPE_POISON // The value is copied either way; only the stamp is withheld for the omitted pair. if (!IsOmitted(verb, field)) filled.FilledGen[i] = filled.CurrentVerbSerial; diff --git a/MobileGL/MG_Impl/Pipe/PipeFill.h b/MobileGL/MG_Impl/Pipe/PipeFill.h index 04b34fbf..311e3c48 100644 --- a/MobileGL/MG_Impl/Pipe/PipeFill.h +++ b/MobileGL/MG_Impl/Pipe/PipeFill.h @@ -70,6 +70,29 @@ namespace MobileGL::MG_Pipe { // through it, and this returns that same expression rather than a second copy of it. Bool MGPipeP4aFamilyEmits(Uint64 subsystem, Uint64 wired); + // PipeFill.cpp. DOES AN EMITTED CALL SUPPLY THIS FIELD at the environment named, i.e. would + // the validate point's residual fill SKIP it? This is step 4's own predicate, which P5d + // round 3 (package C) turned from a per-field-per-verb conjunction into a memo keyed on the + // three arguments plus the P4a consumer signal it reads for itself - the profile had it at + // 63 CapsMirror reads per verb for one answer. + // + // IT IS EXPORTED FOR THE UNIT GATE AND FOR NO OTHER CALLER, for MGPipeP4aFamilyEmits' + // reason and one of its own: which fields the fill copies has NO other observable, because + // in a push build every emission this predicate asks about is routed straight into the + // applier, which writes the same storage the fill would have written. So the only way to + // state "the memo's key is complete" - the one thing a memo can get wrong that the + // expression it replaced could not - is to ask it directly. A call re-keys the memo, which + // is exactly what the case is for. + // + // AND A STALE ANSWER IS SILENT, WHICH IS WHY THAT CASE IS THE ONLY GUARD. It is tempting to + // say a wrongly-skipped field aborts as Fatal{UnmigratedPipeInput}; it does not. The walk + // stamps FilledGen from the verb serial whether or not it copied (PipeFill.cpp, step 4), so + // a field the fill skips reads FRESH with the PREVIOUS verb's value - a stale binding slot + // rendered without a word, caught only by the verify lane's comparison. Poison catches an + // UNSTAMPED read, not a stamped-but-uncopied one. + Bool MGPipeResidualFillSuppliesField(MGPipeInputField field, Uint64 pushMask, Bool applierDerives, + Bool contextValuesWireLive); + // PipeFill.cpp. P3a D-H2.1: the DRAW's raw vertex-fetch base instance, which // set_vertex_buffers now carries as an explicit field. // diff --git a/MobileGL/MG_Impl/Pipe/SlotAllocator.cpp b/MobileGL/MG_Impl/Pipe/SlotAllocator.cpp index 397c75ea..5661263a 100644 --- a/MobileGL/MG_Impl/Pipe/SlotAllocator.cpp +++ b/MobileGL/MG_Impl/Pipe/SlotAllocator.cpp @@ -25,8 +25,8 @@ namespace MobileGL::MG_Pipe { // The named exemption of CONTRACT-P5C §3.1: the sites whose handle-carrying records // are not emitted yet (P4b/P7) probe read-only inside the scope. The G6 // frontend-keyed registry family (P3b/P4b) does the same inside its own scope. - if (MGPipeReverseAnnouncementScope::Active()) return; - if (MGPipeFrontendKeyedRegistryScope::Active()) return; + if (MGPipeReverseAnnouncementScope::ActiveOnApplyThread()) return; + if (MGPipeFrontendKeyedRegistryScope::ActiveOnApplyThread()) return; MGLOG_F("MGPipe: Fatal{RoleViolation, \"MGPipeSlots\"} - the apply thread called " "MGPipeSlots().%s. With an active transport the client slot allocator is " "client-only memory (CONTRACT-P5C §3.1, rule E): a handle arrives already " @@ -38,34 +38,67 @@ namespace MobileGL::MG_Pipe { } namespace { - thread_local Uint32 g_reverseAnnouncementScopeDepth = 0; + // NOT thread_local any more, and not atomic either (P5d round 3, package D). + // + // WHY THERE IS NO DATA RACE. Both depths have exactly ONE reader in the whole tree: + // MGPipeRefuseAllocatorFromApplyThread above, whose first two lines return unless the + // transport is active AND ServerLoop::OnApplyThread() is true. So the only thread whose + // depth could ever change an answer is the apply thread - and the four scope bodies + // below now increment and decrement ONLY when OnApplyThread() says so. That makes the + // apply thread the single writer and the single reader of both counters; no other + // thread touches them, and a monolith process never even reads them (the guard's first + // line returns on TransportMode::Monolith and there is no apply thread to make the + // second true). Single-writer-single-reader on one thread needs no lock, no atomic and + // no TLS. + // + // WHY IT WAS WORTH DOING. A shared-library thread_local costs an __emutls_get_address + // call per access. On the MONOLITH's GL thread that symbol is the TOP entry of the + // profile at 7.7%, and these two scopes' ctor/dtor are 32.7% of its samples + // (MGPipeFrontendKeyedRegistryScope 18.75 ctor + 13.95 dtor, MGPipeReverseAnnouncement + // 8.4); on the split's apply thread emutls is 7.4%. The scope is constructed at ~20 + // sites in DirectGLES.cpp, several inside StateBackendObjectRegistry::HandleOf, which + // is per-draw. The cost on the GL thread is now one inlined predicate per end. + // + // The query's meaning ON THE APPLY THREAD is unchanged, which is the only meaning + // the guard reads. Off it the answer is now always false, and the query is named + // ActiveOnApplyThread() rather than Active() so that a reader who needs the other + // meaning cannot ask for it by accident: whoever wants "is a scope open on THIS + // thread" has to add it, and move the counting back, rather than read a false that + // looks like an answer. + Uint32 g_reverseAnnouncementScopeDepth = 0; } - MGPipeReverseAnnouncementScope::MGPipeReverseAnnouncementScope() { - ++g_reverseAnnouncementScopeDepth; + MGPipeReverseAnnouncementScope::MGPipeReverseAnnouncementScope() + // Decided ONCE and remembered: the destructor must undo exactly what the constructor + // did, and re-asking the predicate would leak a count across a scope that straddled + // the apply thread's exit block. + : m_counted(MG_Remote::Server::ServerLoop::OnApplyThread()) { + if (m_counted) ++g_reverseAnnouncementScopeDepth; } MGPipeReverseAnnouncementScope::~MGPipeReverseAnnouncementScope() { - --g_reverseAnnouncementScopeDepth; + if (m_counted) --g_reverseAnnouncementScopeDepth; } - Bool MGPipeReverseAnnouncementScope::Active() { + Bool MGPipeReverseAnnouncementScope::ActiveOnApplyThread() { return g_reverseAnnouncementScopeDepth != 0; } namespace { - thread_local Uint32 g_frontendKeyedRegistryScopeDepth = 0; + // Same counter, same argument, same single reader - see the block above. + Uint32 g_frontendKeyedRegistryScopeDepth = 0; } - MGPipeFrontendKeyedRegistryScope::MGPipeFrontendKeyedRegistryScope() { - ++g_frontendKeyedRegistryScopeDepth; + MGPipeFrontendKeyedRegistryScope::MGPipeFrontendKeyedRegistryScope() + : m_counted(MG_Remote::Server::ServerLoop::OnApplyThread()) { + if (m_counted) ++g_frontendKeyedRegistryScopeDepth; } MGPipeFrontendKeyedRegistryScope::~MGPipeFrontendKeyedRegistryScope() { - --g_frontendKeyedRegistryScopeDepth; + if (m_counted) --g_frontendKeyedRegistryScopeDepth; } - Bool MGPipeFrontendKeyedRegistryScope::Active() { + Bool MGPipeFrontendKeyedRegistryScope::ActiveOnApplyThread() { return g_frontendKeyedRegistryScopeDepth != 0; } #endif diff --git a/MobileGL/MG_Impl/Pipe/SlotAllocator.h b/MobileGL/MG_Impl/Pipe/SlotAllocator.h index b30b9450..26918fab 100644 --- a/MobileGL/MG_Impl/Pipe/SlotAllocator.h +++ b/MobileGL/MG_Impl/Pipe/SlotAllocator.h @@ -181,13 +181,31 @@ namespace MobileGL::MG_Pipe { // ONE read-only lifetime-id probe stays legal; the scope is the debt's measurable, // greppable form, and it retires with P4b's emission (Espryt) and P7's server-side // binding table (Magma). Every other apply-thread allocator access stays Fatal. + // + // THE DEPTH IS COUNTED ONLY ON THE APPLY THREAD (P5d round 3, package D). The counter's + // only reader is MGPipeRefuseAllocatorFromApplyThread, which returns before it looks unless + // ServerLoop::OnApplyThread() is true, so a depth kept on any OTHER thread could never + // change an answer - it was pure cost. The query is called ActiveOnApplyThread() and not + // Active() BECAUSE OF THAT (review round 3): a name that promised "is a scope open" would + // now be quietly answering "is a scope open ON THE APPLY THREAD", and the next reader to + // come along - a P4b/P7 probe deciding on the GL thread whether to emit a record - would + // read a truthful-looking false and take the wrong branch with nothing to warn it. The name + // carries the precondition so a second reader has to notice it. m_counted remembers what the constructor decided so + // the destructor undoes exactly what the constructor did; asking the predicate twice would + // leak a count for a scope that outlived the apply thread. On the GL thread (and in every + // monolith process) both ends are now one inlined predicate and a branch instead of an + // emutls call, which is where 32.7% of the monolith GL thread's __emutls_get_address - its + // top symbol at 7.7% - was going. class MGPipeReverseAnnouncementScope { public: MGPipeReverseAnnouncementScope(); ~MGPipeReverseAnnouncementScope(); MGPipeReverseAnnouncementScope(const MGPipeReverseAnnouncementScope&) = delete; MGPipeReverseAnnouncementScope& operator=(const MGPipeReverseAnnouncementScope&) = delete; - static Bool Active(); + static Bool ActiveOnApplyThread(); + + private: + Bool m_counted; }; // The SECOND named exemption family (CONTRACT-P5C §5.4): the frontend-keyed twin @@ -197,13 +215,21 @@ namespace MobileGL::MG_Pipe { // P3b/P4b's, not P5c's. A probe inside this scope stays a read-only, barrier-held debt; // wrapping a NEW site in it is the greppable act of naming that debt, and an unwrapped // probe from the apply thread is still Fatal{RoleViolation, "MGPipeSlots"}. + // + // Apply-thread-only depth, and m_counted, for MGPipeReverseAnnouncementScope's reasons + // above. This is the scope that pays: it is constructed at ~20 sites in DirectGLES.cpp, + // several of them inside StateBackendObjectRegistry::HandleOf on the per-draw path, and + // its ctor/dtor alone were 18.75% + 13.95% of the monolith GL thread's emutls samples. class MGPipeFrontendKeyedRegistryScope { public: MGPipeFrontendKeyedRegistryScope(); ~MGPipeFrontendKeyedRegistryScope(); MGPipeFrontendKeyedRegistryScope(const MGPipeFrontendKeyedRegistryScope&) = delete; MGPipeFrontendKeyedRegistryScope& operator=(const MGPipeFrontendKeyedRegistryScope&) = delete; - static Bool Active(); + static Bool ActiveOnApplyThread(); + + private: + Bool m_counted; }; #endif } // namespace MobileGL::MG_Pipe diff --git a/MobileGL/MG_IntegrationTest/Scenarios/CtWireScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/CtWireScenario.cpp index 350026ba..d697f308 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/CtWireScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/CtWireScenario.cpp @@ -143,6 +143,13 @@ namespace { glBindTexture(GL_TEXTURE_2D, 0); const MobileGL::Uint64 deathsBefore = ServerVerbs().ObjectDeaths(); glDeleteTextures(1, &texture); + // THE FENCE, the framebuffer case's exactly (MOBILEGL_IPC_BATCH_WAITS, default on): + // object_death is a kCtxObject value-class record, published WITHOUT waiting for its + // own apply, and the server-side tally only moves at apply time. Without a wait + // boundary this read races the apply thread - it passed by timing until P5d round 3's + // clock-free spin changed that timing, then failed 1 in 5. A clear is kCtxVerb and + // still waits, and its wait covers the death. + glClear(GL_COLOR_BUFFER_BIT); EXPECT_GT(ServerVerbs().ObjectDeaths(), deathsBefore) << "the texture's death produced no object_death record; the server's twin was " "never told to let go"; diff --git a/MobileGL/MG_Remote/CONTRACT-P5.md b/MobileGL/MG_Remote/CONTRACT-P5.md index ddf3d319..4789818b 100644 --- a/MobileGL/MG_Remote/CONTRACT-P5.md +++ b/MobileGL/MG_Remote/CONTRACT-P5.md @@ -352,7 +352,7 @@ make-current and teardown do to it — the column `ARCHITECTURE.md` does not hav 1. client publishes and waits for the server to drain and acknowledge; 2. **`Doorbell::Kill()`** — *the only thing that can wake an apply thread parked on - `kWaitForever`* (`Doorbell.h:211-221`; the shape is already pinned by + `kWaitForever`* (`CondVarDoorbell::Kill` in `Doorbell.h`; the shape is already pinned by `InProcessTransportTest.cpp:344`); 3. **join**, bounded (that test uses 5 s) so a regression is a red test and not a hung CI job; 4. only then may the client free anything an emitter owns — a tail still referenced by an diff --git a/MobileGL/MG_Remote/Client/ClientSession.cpp b/MobileGL/MG_Remote/Client/ClientSession.cpp index b487509e..bd749387 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.cpp +++ b/MobileGL/MG_Remote/Client/ClientSession.cpp @@ -748,7 +748,7 @@ namespace MobileGL::MG_Remote::Client { } // 2. Doorbell::Kill(). THE ONLY thing that can wake an apply thread parked on - // kWaitForever (Doorbell.h:211-221): a Notify is consumed by one Park, after which + // kWaitForever (CondVarDoorbell::Kill): a Notify is consumed by one Park, after which // Doorbell::Wait re-tests a condition nothing published, finds the bell alive and // parks again, forever. InProcessChannel::Close kills both bells. if (m_transport != nullptr) { @@ -896,7 +896,24 @@ namespace MobileGL::MG_Remote::Client { // the number of round trips between them changes. Set / CSO / object records carry // their whole value in the payload and are consumed at the next (waited) sync, which // the in-order ring guarantees precedes that sync's apply. - if (!ownsReplySlot && MG_Config::Ipc.BatchWaits != 0) { + // + // WITH ONE NAMED EXCEPTION, FOUND BY ITS OWN FATAL (P5d round 3): generate_mipmap is + // catalogued kCtxObject, but its sink is the backend's GenerateMipmap, which reads + // MGB_CTX->GetActiveTextureUnit() and the unit's binding slot - residual inputs whose + // freshness is the CLIENT's verb serial. Published without a wait, the record sits in + // the ring while this thread runs the next verb's MGPipeValidateForVerb, which moves + // that serial and withdraws the server stamp; the apply thread then reads the unit + // stale and aborts Fatal{UnmigratedPipeInput, "GetActiveTextureUnit@"} + // (F1WireScenario.GenerateMipmap*Pixels, reproduced on the gate tree once the round-3 + // spin no longer read the clock). So it waits, exactly as the four stamping verbs + // do. The rule this encodes: a record may skip its wait only if its apply reads + // nothing the residual fill writes - the value carried in the payload is the whole + // input. No other kCtxObject row has a backend body behind it that reads MGB_CTX + // (set_texture_params, resource_subdata, resource_readback and get_texture_image + // own reply slots and wait anyway; the resource_* transfers and object_death resolve + // by handle). + if (!ownsReplySlot && MG_Config::Ipc.BatchWaits != 0 && + op != MG_Pipe::MGPWireOp::GenerateMipmap) { const MG_Pipe::MGPipeCallClass callClass = MG_Pipe::MGPipeCallClassFor(op); if (callClass == MG_Pipe::kCtxState || callClass == MG_Pipe::kCtxCso || callClass == MG_Pipe::kCtxObject) { @@ -996,13 +1013,21 @@ namespace MobileGL::MG_Remote::Client { } void ClientSession::RefusePipeInputsTouchWhileApplierOwnsIt(const char* surface) { - if (Server::ServerLoop::OnApplyThread()) return; // the applier owns the block inside a verb + // THE CHAIN IS A CONJUNCTION, SO ITS ORDER IS FREE - AND IT IS ORDERED CHEAPEST-FIRST + // (P5d round 3, package D). Every line below is a pure predicate whose only effect is + // "no violation, go home"; the Fatal fires on the AND of all of them, so reordering + // cannot change which touches abort. It changes only what a touch that does NOT abort + // pays, and this guard sits on the per-verb fill path (PipeFill.cpp:542, :1951, :2789 + // - MGPipeValidateForVerb is 4.11% self / 10.9% inclusive of the client thread), so + // "what it costs to answer no" is the whole of its cost in a healthy process. + // + // The two config reads come first because they are the two answers that are constant + // for the life of the process and true for nearly every process that runs this code: + // a monolith build never has a role split at all, and MOBILEGL_IPC_BATCH_WAITS + // defaults to 1, which retires this guard by design (see its note below). Only after + // both miss do we pay for a role probe, two singleton reads, a shared atomic and the + // thread_local at the end. if (MG_Config::Transport == MG_Config::TransportMode::Monolith) return; - if (!ClientSessionInstance().Started()) return; // the bring-up window pre-dates the roles - // MOBILEGL_IPC_VERB_BARRIER=0 is R-1's NEGATIVE CONTROL: the single-writer rule is off - // by the operator's own hand there, and EmitAndWait's Fatal{BarrierViolation} owns the - // red. Firing here instead would pre-empt the control's evidence line. - if (!ClientSessionInstance().BarrierArmed()) return; // MOBILEGL_IPC_BATCH_WAITS=1 makes "the fill runs while the apply thread applies an // earlier value record" the INTENDED shape: the fill writes only fields no record // supplies (the wire-live skips are the same answer the emitters use), the applier @@ -1010,7 +1035,16 @@ namespace MobileGL::MG_Remote::Client { // pulled reads fenced. This guard exists for the model where that is not true, so // it stands only while the batch is off. if (MG_Config::Ipc.BatchWaits != 0) return; + if (Server::ServerLoop::OnApplyThread()) return; // the applier owns the block inside a verb + if (!ClientSessionInstance().Started()) return; // the bring-up window pre-dates the roles + // MOBILEGL_IPC_VERB_BARRIER=0 is R-1's NEGATIVE CONTROL: the single-writer rule is off + // by the operator's own hand there, and EmitAndWait's Fatal{BarrierViolation} owns the + // red. Firing here instead would pre-empt the control's evidence line. + if (!ClientSessionInstance().BarrierArmed()) return; if (!ApplyThreadIsInsideApplier()) return; + // LAST, because it is the only thread_local left in the chain: an emutls call per + // access in a shared library, and it must stay thread-local (see InBarrierWait's note + // above - a second GL thread would otherwise read the first one's wait as its own). if (InBarrierWait()) return; MGLOG_F("MGPipe: Fatal{RoleViolation, \"gPipeInputs\"} - the GL thread touched gPipeInputs " "(%s) while the apply thread was inside the applier and this thread was not in a " @@ -1133,8 +1167,9 @@ namespace MobileGL::MG_Remote::Client { MGLOG_I("MG_Remote client: wire ledger: maxrec=%llu maxrecop=%s cap=%llu (%llu.%llu%% of " "RingProducer::MaxRecordBytes, half of a %llu byte SEG_CMD) cmdbytes=%llu " "ringwraps=%llu ringpads=%llu " - "ringwaits=%llu emitseq=%llu - R-10's proof obligation and R-9's producer " - "readings, published from the session that produced them", + "ringwaits=%llu emitseq=%llu cliwait=%llu clipark=%llu - R-10's proof obligation, " + "R-9's producer readings and P5d round 3's client wait pair, published from the " + "session that produced them", static_cast(maxRecord), m_encoder.MaxRecordOpName(), static_cast(cap), static_cast(permille / 10), @@ -1144,7 +1179,13 @@ namespace MobileGL::MG_Remote::Client { static_cast(m_encoder.CmdWraps()), static_cast(m_encoder.CmdWrapPads()), static_cast(m_encoder.StageReclaimWaits()), - static_cast(m_encoder.EmitSeq())); + static_cast(m_encoder.EmitSeq()), + // The same pair the summary line's wait[] carries, on the channel that exists + // in EVERY split private log rather than in the two counting lanes: the round-3 + // profile runs did not set MOBILEGL_PIPE_STATS either, and "did the barrier spin + // or did it park" is the question their logs have to be able to answer. + static_cast(m_producer.Waits()), + static_cast(m_producer.Parks())); } #undef MGP5_C0_STUB diff --git a/MobileGL/MG_Remote/Client/ClientSession.h b/MobileGL/MG_Remote/Client/ClientSession.h index 775ed025..b4ff76df 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.h +++ b/MobileGL/MG_Remote/Client/ClientSession.h @@ -84,7 +84,7 @@ namespace MobileGL::MG_Remote::Client { // Teardown order matters and is table 3's fourth column: publish and let the server // drain, Doorbell::Kill() (the ONLY thing that wakes an apply thread parked on - // kWaitForever, Doorbell.h:211-221), then join, and only then release anything an + // kWaitForever, CondVarDoorbell::Kill), then join, and only then release anything an // emitter owns - a tail still referenced by an unapplied record is a use-after-free // the join is what prevents. void Stop(); @@ -103,7 +103,7 @@ namespace MobileGL::MG_Remote::Client { // this and refuses `replySize != DstSize` by name rather than trust the copy. // // Waiting is spin(MOBILEGL_IPC_SPIN_US) then park, through Doorbell::Wait, with - // producerParked set before blocking - the shape Doorbell.h:121 already implements. + // producerParked set before blocking - the shape Doorbell::Wait already implements. Uint64 EmitAndWait(MG_Pipe::MGPWireOp op, const void* payload, Uint64 payloadBytes, const void* varTail, Uint64 varTailBytes, void* replyOut, Uint64 replyBytes, Int32* statusOut, Uint64* replySizeOut = nullptr); diff --git a/MobileGL/MG_Remote/Client/EmitTables.cpp b/MobileGL/MG_Remote/Client/EmitTables.cpp index 8e786848..ac5dc9e4 100644 --- a/MobileGL/MG_Remote/Client/EmitTables.cpp +++ b/MobileGL/MG_Remote/Client/EmitTables.cpp @@ -1003,6 +1003,17 @@ namespace MobileGL::MG_Remote::Client { MG_Util::PipeStats::PublishGauge(Gauge::RingWrapPads, encoder.CmdWrapPads()); MG_Util::PipeStats::PublishGauge(Gauge::RingWaits, encoder.StageReclaimWaits()); + // P5d round 3's wait ledger, CLIENT HALF, and it rides this frame boundary for + // the same two reasons the five above it do: the producer keeps both as run + // totals, so it is two relaxed stores per frame rather than two per wait, and + // publishing before the present record means the first summary line of a run + // carries real numbers instead of two zeroes that mean "not published yet". + // The SERVER half is published by the apply thread from its own loop - rule E + // does not let this thread read ServerLoop's counters. + const Transport::SessionProducer& producer = session.Producer(); + MG_Util::PipeStats::PublishGauge(Gauge::ClientWaits, producer.Waits()); + MG_Util::PipeStats::PublishGauge(Gauge::ClientParks, producer.Parks()); + // AND THE ROW, WHENEVER THE MAXIMUM MOVES. The summary line can carry the // number but not the name - MG_Util is below MG_Remote and has no WireOpName - // and the name is the actionable half: R-10 makes the integrator choose between diff --git a/MobileGL/MG_Remote/Client/GpuWritePending.cpp b/MobileGL/MG_Remote/Client/GpuWritePending.cpp index 9e0497b3..957b02eb 100644 --- a/MobileGL/MG_Remote/Client/GpuWritePending.cpp +++ b/MobileGL/MG_Remote/Client/GpuWritePending.cpp @@ -87,14 +87,29 @@ namespace MobileGL::MG_Remote::Client { if (!context) return; // The backend keeps a bitset of writable image-buffer units // (DirectGLES.cpp:2350-2352, maintained from its own SyncImageTextureBinding) and - // the client has no equivalent, so it sweeps. The sweep is bounded by the array, - // not by a device limit read: MaxImageUnits would be a backend read, and a stale - // or absent backend object would silently shorten the walk - which is the one - // direction this set may not fail in. The loop body is a null test on a - // contiguous array until a unit is actually bound. P5 records cost and does not - // gate on it (2026-09-08 rule); an image-unit high-water mark on GLContext is the - // obvious narrowing and belongs with P8's binding-walk migration. - for (Int unit = 0; unit < MG_State::GLState::TextureState::MAX_TEXTURE_IMAGE_UNITS; ++unit) { + // the client has no equivalent, so it sweeps. The sweep is NOT bounded by a device + // limit read: MaxImageUnits would be a backend read, and a stale or absent backend + // object would silently shorten the walk - which is the one direction this set may + // not fail in. + // + // IT IS BOUNDED BY THE FRONTEND's OWN IMAGE-UNIT HIGH-WATER MARK (P5d round 3, + // package C). The paragraph that stood here said the narrowing "belongs with P8's + // binding-walk migration"; the 2026-09-17 inproc profile moved it forward - + // MarkWritableImageBufferTextures was 2.15% self / 3.59% inclusive of the GL thread + // (GetImageTextureBinding 1.37 of it) at ~852 draws/frame, in a workload that never + // binds an image at all, purely because the walk was MAX_TEXTURE_IMAGE_UNITS (192) + // units wide unconditionally - 192 GetImageTextureBinding reads per draw for a + // context that has never bound one. + // + // AND THE MARK IS SAFE IN THE ONLY DIRECTION THAT MATTERS. It is written by + // glBindImageTexture itself (GL_Texture.cpp, TextureState::NoteImageUnitTouched) and + // it only ever grows - an unbind, a delete-unbind, a context that stops using an + // image unit all leave it where it was - so `unit <= mark` can only be WIDER than + // the set of units that hold a binding, never narrower. -1 means no image unit has + // ever been bound in this context, and then this is one compare. + const Int highest = context->GetMaxTouchedImageUnit(); + for (Int unit = 0; unit <= highest && unit < MG_State::GLState::TextureState::MAX_TEXTURE_IMAGE_UNITS; + ++unit) { const auto& binding = context->GetImageTextureBinding(unit); if (!ImageUnitIsAWritableBufferTexture(binding)) continue; auto* textureBuffer = static_cast(binding.Texture.get()); diff --git a/MobileGL/MG_Remote/Client/PersistentMapTracker.cpp b/MobileGL/MG_Remote/Client/PersistentMapTracker.cpp index 1b54326a..62d17fee 100644 --- a/MobileGL/MG_Remote/Client/PersistentMapTracker.cpp +++ b/MobileGL/MG_Remote/Client/PersistentMapTracker.cpp @@ -62,6 +62,15 @@ namespace MobileGL::MG_Remote::Client { constexpr SizeT kMaxTrackedMaps = 64; constexpr SizeT kPageShift = 12; constexpr SizeT kPageBytes = 1u << kPageShift; + // THE PAGE-GRANULAR SHADOW IS THE TRACKER'S PRECONDITION, NOT A COINCIDENCE. Every + // shadow a split build hands out is SHADOW_ALLOCATION_ALIGNMENT-aligned AND sized to + // a multiple of it (PipeResource.h, ShadowAllocationBytesFor), so a shadow owns every + // byte of every page it touches; the outward alignment in TrackWriteMap rests on + // that, and it is only true when the allocator's unit is a whole number of the + // tracker's pages. + static_assert(MG_State::GLState::SHADOW_ALLOCATION_ALIGNMENT % kPageBytes == 0, + "the shadow allocator's granule must be a whole number of tracker pages, " + "or a page-granular shadow could still end mid-page"); // The bitmap is FIXED-CAPACITY and never reallocated: a handler mid-flight on // another thread can hold the pointer it loaded before a retire, so a grow-and-free // would be a use-after-free no memory ordering can close. 64 K pages = 256 MB per @@ -76,10 +85,16 @@ namespace MobileGL::MG_Remote::Client { TrackedWriteMap g_trackedMaps[kMaxTrackedMaps]; struct sigaction g_prevSegvAction {}; std::atomic g_segvInstalled{false}; - // Bumped by the handler for every write fault it answers. PushDrawConsumers - // compares it against the GL thread's last-walk value: an unmoved epoch proves - // no interior byte of any tracked map changed, because interior dirtiness can - // only ever arrive through a fault. + // Bumped by the handler for every write fault it answers, and by TrackWriteMap + // for every registration (a fresh slot has EVERY page marked, which is a fault of + // every page as far as the push is concerned). PushDrawConsumers compares it + // against the GL thread's last-walk value: an unmoved epoch proves no page of any + // tracked map was marked since the last walk, because a mark can only ever arrive + // through one of those two events - and the walk drains every marked page of + // every tracked member, which is what makes the proof worth having. The bump is a + // RELEASE and the walk's load an ACQUIRE, so the bit the handler set before it + // bumped is visible to a walk that saw the bump; a chunk builder writing an arena + // off the GL thread is the ordinary case, not a corner. std::atomic g_faultEpoch{0}; // THE OWNERSHIP TESTS. "Ours" is a NARROW claim, and every fault that fails any @@ -200,11 +215,13 @@ namespace MobileGL::MG_Remote::Client { void PersistentWriteFaultHandler(int sig, siginfo_t* info, void* ucontext) { if (info != nullptr && info->si_code == SEGV_ACCERR) { const uintptr_t address = reinterpret_cast(info->si_addr); - // EVERY overlapping slot gets its bit, not just the first: two tracked - // shadows can share an edge page (separate heap allocations, one page - // granularity), and unprotecting after only the first mark would leave the - // second buffer's block permanently un-pushed - the page is writable now, - // so it never faults again. + // EVERY overlapping slot gets its bit, not just the first: unprotecting + // after only the first mark would leave a second owner's block permanently + // un-pushed - the page is writable now, so it never faults again. With + // page-granular shadows no two tracked ranges can share a page any more + // (each protected page is one shadow's own), so the loop finds at most one + // owner; it stays in this form because a fault is rare and the exhaustive + // scan is what makes the claim checkable rather than assumed. Bool matched = false; Bool bitWasSet = false; for (SizeT i = 0; i < kMaxTrackedMaps; ++i) { @@ -227,8 +244,10 @@ namespace MobileGL::MG_Remote::Client { if (matched && !RefaultOfANonWriteAccess(FaultPcFrom(ucontext), address, bitWasSet)) { // The epoch moves BEFORE the unprotect: a walk that starts the - // moment the page becomes writable must still see this fault. - g_faultEpoch.fetch_add(1, std::memory_order_relaxed); + // moment the page becomes writable must still see this fault. And it + // moves with RELEASE, after the bit: a walk that acquires the moved + // epoch is guaranteed to see the bit that goes with it. + g_faultEpoch.fetch_add(1, std::memory_order_release); // Un-arm is a raw syscall, deliberately not the libc wrapper's // bookkeeping: the handler's whole job is to get out of the way. mprotect(reinterpret_cast(address & ~(kPageBytes - 1)), @@ -239,7 +258,32 @@ namespace MobileGL::MG_Remote::Client { ChainToPreviousSegvHandler(sig, info, ucontext); } + // THE KERNEL'S PAGE MUST BE THE TRACKER'S PAGE. Every ownership argument here - + // above all "a protected page holds only this shadow's bytes" - is made at + // kPageBytes granularity, and mprotect protects at the KERNEL's. On a 16 KB-page + // kernel (Android 15 admits them) a kPageBytes mprotect would silently cover + // three neighbouring 4 KB quarters that may be foreign memory, which is exactly + // the process-killer the page-granular shadow exists to keep out. So on such a + // kernel the arm is not installed at all and every persistent write map takes + // the hash arm: slower, never unsafe. Said once, not per map - and decided BEFORE + // g_segvInstalled can read true to anyone: a function-static answers exactly once, + // on whichever thread asks first, and every other asker waits on that answer, so + // no caller can pass TrackWriteMap's installed gate while the page size is still + // being checked. + Bool KernelPageIsTheTrackerPage() { + static const Bool answer = [] { + const long kernelPage = sysconf(_SC_PAGESIZE); + if (kernelPage == static_cast(kPageBytes)) return true; + MGLOG_W("MGPipe: persistent-map mprotect tracking unavailable (kernel page %ld " + "is not the tracker's %zu); persistent write maps fall back to the hash scan", + kernelPage, kPageBytes); + return false; + }(); + return answer; + } + void InstallWriteFaultHandler() { + if (!KernelPageIsTheTrackerPage()) return; Bool expected = false; if (!g_segvInstalled.compare_exchange_strong(expected, true)) return; struct sigaction action {}; @@ -256,35 +300,81 @@ namespace MobileGL::MG_Remote::Client { void UntrackWriteMap(Uint64 lifetimeId); + // `shadowExtent` is the shadow allocation's own size in bytes as its allocator + // sized it (BufferObject::ShadowAllocationBytes), or 0 when the caller cannot vouch + // for it; it is what decides between the two alignments below. Bool TrackWriteMap(Uint64 lifetimeId, const Uint8* shadow, - SizeT rangeBegin, SizeT rangeEnd) { + SizeT rangeBegin, SizeT rangeEnd, SizeT shadowExtent) { if (shadow == nullptr || rangeEnd <= rangeBegin) return false; InstallWriteFaultHandler(); if (!g_segvInstalled.load()) return false; - // INWARD alignment, and the reason is a process-killer: with OUTWARD - // alignment the protected edge pages hold FOREIGN bytes (the shadow is a - // heap allocation; malloc metadata and other allocations share its first - // and last page). A foreign write to such a page is fine for a thread - // that runs our handler - it is marked and unprotected - but driver and - // runtime worker threads commonly block every signal, and a fault there - // with SIGSEGV blocked ends the process on the spot, undebuggable. - // Protecting only pages FULLY inside the mapped range means every byte of - // a protected page is the shadow's own, and no other allocation can ever - // be made to fault by us. The head/tail partial pages stay writable and - // their blocks are pushed unconditionally at every push (at most two). - const uintptr_t rawBegin = reinterpret_cast(shadow + rangeBegin); - const uintptr_t rawEnd = reinterpret_cast(shadow + rangeEnd); - uintptr_t base = (rawBegin + kPageBytes - 1) & - ~static_cast(kPageBytes - 1); - uintptr_t end = rawEnd & ~static_cast(kPageBytes - 1); - // A SUB-PAGE range is not refused; it is registered with an EMPTY interior - // (no pages are protected, no bitmap is needed) and hasEdges set, so the - // whole range is served by the two edge hashes - bounded at 8 KB per push - - // instead of falling to the hash arm, where as an "untracked" member it - // would veto the epoch skip for every tracked buffer in the process and - // force the full binding walk at every draw (measured on device). end is - // clamped up to base so the [base, end) arithmetic below never underflows. - if (end < base) end = base; + // WHICH WAY TO ALIGN IS A QUESTION OF OWNERSHIP, and the reason is a + // process-killer: a protected page that holds FOREIGN bytes can be written by + // a thread that does not run our handler - driver and runtime worker threads + // commonly block every signal - and a fault there with SIGSEGV blocked ends the + // process on the spot, undebuggable (the ctest-only SmallRing segfault of + // cb06538c). So a page may be protected only when every byte of it is this + // shadow's own, and the two alignments are the two ways of guaranteeing that: + // + // OUTWARD, when the shadow is PAGE-GRANULAR: its base is on a page boundary + // and its allocation is a whole number of pages (PipeResource.h's allocator + // promises both in a split build, and `shadowExtent` is what it promised). + // Then every page that intersects the mapped range lies inside + // [shadow, shadow + shadowExtent), and every byte of that span belongs to + // this allocation and to no other - the heap handed it out as ONE block of + // that size, and its own metadata lives before the block, never inside it. + // The mapped range's containing pages are protected whole, there are NO + // unprotected edge bytes, and a sub-page map is one protected page. The bytes + // of a head/tail page that lie OUTSIDE the mapped range are still the + // shadow's: only the client's own API paths write them (a SubData / + // FlushMappedRange memcpy into the shadow, a readback writeback landing on + // the client thread), on threads that run this handler, and such a fault is + // answered like any interior one - the push then clamps to [begin, end), so + // at worst one block ships redundantly, and the byte itself crossed by its own + // record. + // + // INWARD, for anything else (an unaligned base, an extent the caller could not + // vouch for, a range running past the extent it was told): only pages FULLY + // inside the mapped range are protected, which are the shadow's own by + // construction, and the head/tail partial pages stay writable. Nothing tells + // us they changed, so they are hashed per push (the edge hashes) - which was + // 8.5% of the client thread at VD12 on every draw for every live map, and + // the reason the outward arm exists. A sub-page range still registers here + // with an EMPTY interior and hasEdges set rather than falling to the hash + // arm, where as an "untracked" member it would veto the epoch skip for every + // tracked buffer in the process (measured on device); end is clamped up to + // base so the [base, end) arithmetic never underflows. + const uintptr_t shadowBase = reinterpret_cast(shadow); + const uintptr_t rawBegin = shadowBase + rangeBegin; + const uintptr_t rawEnd = shadowBase + rangeEnd; + constexpr uintptr_t kPageMask = ~static_cast(kPageBytes - 1); + const Bool pageGranular = (shadowBase & (kPageBytes - 1)) == 0 && shadowExtent != 0 && + (shadowExtent & (kPageBytes - 1)) == 0 && rangeEnd <= shadowExtent; + uintptr_t base = 0; + uintptr_t end = 0; + Bool outward = pageGranular; + if (outward) { + base = rawBegin & kPageMask; + end = (rawEnd + kPageBytes - 1) & kPageMask; + // Already implied by rangeEnd <= shadowExtent and the extent's own page + // granularity; the clamp is kept because it is the line the ownership + // argument rests on, and it must not depend on a caller's arithmetic. + if (end > shadowBase + shadowExtent) end = shadowBase + shadowExtent; + // WIDENING NEVER DEMOTES A MAP THE INWARD ARM COULD HOLD. Outward covers up + // to one page more than inward does for the same range (both ends rounded + // away instead of towards), so a map sitting on the bitmap's capacity that + // was tracked inward before this round would otherwise fall off the tracker + // altogether - onto the hash arm, where as an untracked member it vetoes the + // epoch skip for every buffer in the process. It keeps the inward arm and + // its two hashed edges instead: the old cost, on the one ~256 MB map that + // can pay it, and nothing else changes. + if (((end - base) >> kPageShift) > kMaxTrackedPages) outward = false; + } + if (!outward) { + base = (rawBegin + kPageBytes - 1) & kPageMask; + end = rawEnd & kPageMask; + if (end < base) end = base; + } const SizeT pageCount = (end - base) >> kPageShift; if (pageCount > kMaxTrackedPages) { MGLOG_W("MGPipe: persistent write map of %zu pages exceeds the mprotect " @@ -309,7 +399,10 @@ namespace MobileGL::MG_Remote::Client { g_trackedMaps[i].lifetimeId = lifetimeId; g_trackedMaps[i].edgeHashHead = 0; g_trackedMaps[i].edgeHashTail = 0; - g_trackedMaps[i].hasEdges = base != rawBegin || end != rawEnd; + // Edges are UNPROTECTED mapped bytes: the range starts before the first + // protected page or ends after the last. Outward alignment never leaves + // any (base <= rawBegin, end >= rawEnd); inward leaves one per unaligned end. + g_trackedMaps[i].hasEdges = base > rawBegin || end < rawEnd; // EVERY BIT SET, NOT ZEROED - the same "fresh state pushes everything // once" rule the hash arm keeps: the server has never seen these bytes, // so the first push must ship the whole interior, not just the pages the @@ -318,8 +411,9 @@ namespace MobileGL::MG_Remote::Client { // does on a pre-first-push fault is answered once and unprotected - // harmless, and the invariant (bit set <=> writable) holds from the // first push onward. The last word is masked: a set bit past pageCount - // would re-arm a page outside the interior - a foreign page, which is - // the process-killer the inward alignment exists to keep out. + // would re-arm a page outside the protected span - past the shadow's + // extent, a foreign page, which is the process-killer both alignments + // exist to keep out. const SizeT wordCount = (pageCount + 63) / 64; for (SizeT w = 0; w < wordCount; ++w) { const SizeT bitsInWord = @@ -337,6 +431,13 @@ namespace MobileGL::MG_Remote::Client { // it can observe is only ever one whose end is already visible. g_trackedMaps[i].end.store(end, std::memory_order_release); g_trackedMaps[i].base.store(base, std::memory_order_release); + // AND THE EPOCH MOVES, as it would for a fault of every page: the bits just + // set are exactly what a fault leaves behind, and the epoch is the only thing + // PushDrawConsumers reads before deciding it has nothing to drain. Without + // this a member registered between two draws sits fully marked and fully + // protected until something else faults - "first push ships everything + // once" would then be true only of the read-only verbs' whole push. + g_faultEpoch.fetch_add(1, std::memory_order_release); return true; } MGLOG_W("MGPipe: the mprotect tracker is full (%zu live persistent write maps); " @@ -395,6 +496,19 @@ namespace MobileGL::MG_Remote::Client { return nullptr; } + // Whether any page of a live slot is marked - faulted since its last push, or never + // pushed since registration. Read on the GL thread by the consumer walk only (a + // walk runs when the epoch moved, never per draw in steady state), and it costs + // one word load per 64 pages: 40 loads for a 10 MB arena. A slot with an empty + // interior (the inward fallback's sub-page case) has no words and no marks. + Bool AnyPageMarked(const TrackedWriteMap& slot) { + const SizeT words = (slot.pageCount + 63) / 64; + for (SizeT w = 0; w < words; ++w) { + if (slot.pageBits[w].load(std::memory_order_relaxed) != 0) return true; + } + return false; + } + } // namespace PersistentMapTracker& PersistentMapTracker::Instance() { @@ -413,11 +527,23 @@ namespace MobileGL::MG_Remote::Client { return MG_Config::Transport != MG_Config::TransportMode::Monolith; } + // P5d round 3 (package D): the body this forwards to is now one relaxed load of the apply + // thread's key and one thread-pointer compare (ServerLoop.h), instead of a function-static + // guard, two acquire loads and an out-of-line thread::id comparison. It stays out of line + // here for WireTables.cpp's reason beside RunsAsTheServerRole: inlining it would put a + // server header inside PersistentMapTracker.h, which MG_State's BufferObject.cpp includes. Bool PersistentMapTracker::OnServerRole() { return Server::ServerLoop::OnApplyThread(); } // SyncPersistentMappedRange's early-out chain (BufferObject.cpp:341-353), in its order, // read as a membership test. Every line here has a line there; if one of them moves, the // unit case that drives both against each other is what says so. + // + // THE ORDER IS THE CONTRACT, so package D did not touch it even though the first row is the + // only guarded accessor in the chain (IsMapped -> RefuseLegacyBufferArmFromApplyThread) and + // moving it last would have saved a guard on the members that fail rows 2-6. It would also + // have stopped being the same chain as the one in BufferObject, which is the property this + // predicate exists to have. Item 1 made that guard two loads instead, which is cheaper than + // the reorder would have been and costs nothing in meaning. Bool PersistentMapTracker::IsLivePersistentMap(const BufferObject& buffer) { if (!buffer.IsMapped()) return false; // GPU-resident: the application already wrote into coherent GPU memory and there is @@ -445,17 +571,25 @@ namespace MobileGL::MG_Remote::Client { // set must follow the arm the member actually runs on. Erase is a no-op for a // member that was never large-untracked. m_untrackedMembers.erase(key); + m_edgedMembers.erase(key); if (IsLivePersistentMap(buffer)) { const auto range = buffer.GetMappedRange(); // The mprotect arm registers beside the hash arm: tracked buffers are pushed // from the fault bitmap, everything else keeps the content scan. A failed // registration (handler unavailable, table full, unaligned zero range) quietly - // leaves the buffer on the hash path. + // leaves the buffer on the hash path. The shadow's own extent goes along so + // the tracker can align outward (TrackWriteMap): a live member's MappedData() + // IS the shadow vector's storage - the predicate's IsBackendPersistentMapped + // row keeps every adopted store out - so the extent describes that pointer. TrackWriteMap(key, buffer.MappedData(), static_cast(range.start), - static_cast(range.end)); + static_cast(range.end), buffer.ShadowAllocationBytes()); TrackedWriteMap* slot = TrackedMapFor(key); m_livePersistentMaps[key] = MemberEntry{&buffer, slot}; - if (slot == nullptr) m_untrackedMembers[key] = 1; + if (slot == nullptr) { + m_untrackedMembers[key] = 1; + } else if (slot->hasEdges) { + m_edgedMembers[key] = 1; + } return; } m_livePersistentMaps.erase(key); @@ -465,6 +599,7 @@ namespace MobileGL::MG_Remote::Client { void PersistentMapTracker::Forget(const BufferObject& buffer) { const Uint64 key = buffer.GetLifetimeId(); m_untrackedMembers.erase(key); + m_edgedMembers.erase(key); m_livePersistentMaps.erase(key); m_blockHashes.erase(key); UntrackWriteMap(key); @@ -474,11 +609,34 @@ namespace MobileGL::MG_Remote::Client { m_livePersistentMaps.clear(); m_blockHashes.clear(); m_untrackedMembers.clear(); + m_edgedMembers.clear(); UntrackAllWriteMaps(); m_lastFaultEpoch = 0; ResetCountersForTest(); } + Bool PersistentMapTracker::MprotectArmAvailableForTest() { + InstallWriteFaultHandler(); + return g_segvInstalled.load(); + } + + const TrackedWriteMap* PersistentMapTracker::TrackedSlotForTest(const BufferObject& buffer) { + return TrackedMapFor(buffer.GetLifetimeId()); + } + + const TrackedWriteMap* PersistentMapTracker::TrackForTest(Uint64 lifetimeId, const Uint8* shadow, + SizeT rangeBegin, SizeT rangeEnd, + SizeT shadowExtent) { + TrackWriteMap(lifetimeId, shadow, rangeBegin, rangeEnd, shadowExtent); + return TrackedMapFor(lifetimeId); + } + + void PersistentMapTracker::UntrackForTest(Uint64 lifetimeId) { UntrackWriteMap(lifetimeId); } + + Uint64 PersistentMapTracker::FaultEpochForTest() { + return g_faultEpoch.load(std::memory_order_acquire); + } + void PersistentMapTracker::PushBlocksFor(BufferObject& buffer) { if (!PushIsArmed()) return; if (OnServerRole()) { @@ -532,10 +690,12 @@ namespace MobileGL::MG_Remote::Client { // 64 KB block per block that contains at least one faulted page, shipped with // the whole machinery of every other arm (serial bump, HasDefinedContent, // wire record), then the page is cleared and re-armed read-only. No scan, no - // hash, no per-draw cost for buffers the application did not touch. The two - // partial-page EDGE ranges beside the page-aligned interior (see TrackWriteMap's - // inward alignment) are never protected, so nothing tells us they changed - - // their blocks ride along on every push, at most two. + // hash, no per-draw cost for buffers the application did not touch. A + // page-granular shadow's protected span is the mapped range's containing pages + // (TrackWriteMap's outward alignment), so it can start BEFORE `begin` and end + // AFTER `end`, and the block clamp below is what keeps every push inside + // [begin, end). Only the inward fallback has the two unprotected partial-page + // EDGE ranges that nothing faults for; those are hashed per push, at most two. TrackedWriteMap* tracked = nullptr; if (const auto it = m_livePersistentMaps.find(buffer.GetLifetimeId()); it != m_livePersistentMaps.end() && it->second.tracked != nullptr && @@ -547,10 +707,11 @@ namespace MobileGL::MG_Remote::Client { const uintptr_t trackedEnd = tracked->end.load(std::memory_order_acquire); const uintptr_t shadowBase = reinterpret_cast(buffer.MappedData()); if (trackedBase != 0 && trackedEnd > trackedBase && shadowBase != 0) { - const Uint64 interiorBegin = - static_cast(trackedBase - shadowBase); // >= begin - const Uint64 interiorEnd = - static_cast(trackedEnd - shadowBase); // <= end + // The protected span in buffer offsets: <= begin / >= end when aligned + // outward, >= begin / <= end when aligned inward. Both are read below only + // to find the inward fallback's edges; the page loop never needs them. + const Uint64 interiorBegin = static_cast(trackedBase - shadowBase); + const Uint64 interiorEnd = static_cast(trackedEnd - shadowBase); // Pushes are made in ASCENDING offset order, and `pushedUpTo` trims each // span against the one before it: edge spans, duplicate pages inside one // 64 KB block and faulted blocks overlapping an edge all collapse to one @@ -561,7 +722,12 @@ namespace MobileGL::MG_Remote::Client { // And an edge span ships ONLY when its bytes changed since the last push // (the struct comment says why unconditional was measured and rejected): // no faults happen on unprotected bytes, so the hash is the only change - // detector there is for them. + // detector there is for them. The two conditions below are self-gating: + // under outward alignment interiorBegin <= begin and interiorEnd >= end, + // so neither hash runs for a page-granular shadow - every shadow in steady + // state - and that, not a flag, is what took the per-draw XXH3 off the + // client thread. hasEdges is the same fact recorded at registration for + // the epoch skip's benefit, not a second gate here. Uint64 pushedUpTo = begin; const auto pushSpan = [&](Uint64 spanBegin, Uint64 spanEnd) { if (spanBegin < pushedUpTo) spanBegin = pushedUpTo; @@ -609,11 +775,15 @@ namespace MobileGL::MG_Remote::Client { mprotect(reinterpret_cast(trackedBase + (pageIndex << kPageShift)), (runEnd - pageIndex) << kPageShift, PROT_READ); for (SizeT runPage = pageIndex; runPage < runEnd; ++runPage) { - // The interior was page-aligned INSIDE the mapped range at - // registration, so a faulted page's offset in buffer space is - // never negative; its containing block can still start before - // `begin` (a block is 64 KB, a page is 4 KB), and the clamp is - // what keeps the push inside [begin, end). + // A faulted page's offset in buffer space is never negative + // (the span starts at or after the shadow base either way), + // but under outward alignment the head page can start before + // `begin` and the tail page runs past `end`; and a containing + // block starts before `begin` in any case (a block is 64 KB, a + // page is 4 KB). The clamp is what keeps the push inside + // [begin, end): a write to the shadow's own bytes outside the + // mapped range faults here too and ships, at worst, one block + // of the range redundantly. const Uint64 pageOffset = (trackedBase + (runPage << kPageShift)) - shadowBase; const Uint64 blockFloor = (pageOffset / blockBytes) * blockBytes; @@ -709,46 +879,56 @@ namespace MobileGL::MG_Remote::Client { std::abort(); } if (m_livePersistentMaps.empty()) return; - // THE EPOCH SKIP. Interior dirtiness can only arrive as a write fault, and the - // handler bumps the fault epoch for every one it answers before it unprotects; - // an unmoved epoch therefore proves no interior byte of any tracked map changed - // since the last walk. The skip still serves a tracked member's partial-page - // edge spans right here, per member, with no binding state read at all - they - // are never protected, so their two spans are hashed per push. An untracked - // (hash-arm) member vetoes the skip outright: only the binding walk knows - // whether this draw consumes it, and scanning every untracked member on faith - // measured strictly worse than the walk (VD12, on device). With none present - // and the epoch unmoved, nearly every draw in steady flight costs a handful of - // 4 KB edge hashes. + // THE EPOCH SKIP, AND WHAT IT RESTS ON. A page of a tracked map is marked by + // exactly two events - the handler answering a write fault, and TrackWriteMap + // setting every bit of a fresh slot - and both move the fault epoch after the + // mark. An unmoved epoch therefore proves no page of any tracked map was marked + // since the last walk. That is only the whole answer if the last walk DRAINED + // every marked page there was, and it does: the walk below pushes every tracked + // member with a marked page whether or not this draw consumes it (see there). + // Before this round the skip's per-member edge service did that drain by + // accident - it ran PushBlocksForChecked, bitmap walk included, on every member + // with edges, and nearly every member had edges; with page-granular shadows no + // member has, so the drain has to be where the argument says it is. With every + // byte of every tracked map protected (TrackWriteMap's outward alignment) the + // skip then costs one atomic load and two empties. The one exception is a + // member on the inward fallback, whose unprotected edge spans are served right + // here, per such member, with no binding state read at all - the m_edgedMembers + // set is exactly those and is empty in steady state. An untracked (hash-arm) + // member vetoes the skip outright: only the binding walk knows whether this draw + // consumes it, and scanning every untracked member on faith measured strictly + // worse than the walk (VD12, on device). const Uint64 epoch = g_faultEpoch.load(std::memory_order_acquire); if (m_untrackedMembers.empty() && epoch == m_lastFaultEpoch) { - for (const auto& entry : m_livePersistentMaps) { - BufferObject* member = entry.second.buffer; - if (member == nullptr) continue; - // The cached slot pointer, validated against the slot's own lifetimeId: - // a slot retired since the cache was written fails the test, as does one - // re-claimed by a different buffer. - const TrackedWriteMap* tracked = entry.second.tracked; - if (tracked != nullptr && tracked->lifetimeId == member->GetLifetimeId() && - tracked->hasEdges) { - PushBlocksForChecked(*member); + if (!m_edgedMembers.empty()) { + // Keys copied out first: PushBlocksForChecked can Forget() a member that + // stopped being one, and ska::flat_hash_map invalidates on erase. + Vector edged; + edged.reserve(m_edgedMembers.size()); + for (const auto& entry : m_edgedMembers) edged.push_back(entry.first); + for (const Uint64 key : edged) { + const auto it = m_livePersistentMaps.find(key); + if (it == m_livePersistentMaps.end() || it->second.buffer == nullptr) continue; + // The cached slot pointer, validated against the slot's own + // lifetimeId: a slot retired since the cache was written fails the + // test, as does one re-claimed by a different buffer. + const TrackedWriteMap* tracked = it->second.tracked; + if (tracked != nullptr && tracked->lifetimeId == key && tracked->hasEdges) { + PushBlocksForChecked(*it->second.buffer); + } } } return; } + // Taken BEFORE the drain, on purpose: a fault landing while the drain runs moves + // the epoch past this value, and the next draw walks again for it. m_lastFaultEpoch = epoch; - const auto& ctx = MG_State::pGLContext; - if (ctx == nullptr) return; - // The buffers the upcoming draw or dispatch can read, and only those. Every one is - // named by the frontend's own binding state, read on this (the GL) thread - VAO - // attribute and element buffers, the indexed binding points of every indexed - // target, and the indirect / parameter slots. Client-sourced attributes (no - // BufferObject) live on this side already and need no push. + // The buffers this walk pushes: every tracked member with a marked page, and the + // buffers the upcoming draw or dispatch can read. One entry per buffer, not per + // binding point: an arena bound to six attribute slots would otherwise get its + // predicate re-check, bitmap walk and edge hashes run six times per draw. Vector consumers; - // One entry per buffer, not per binding point: an arena bound to six attribute - // slots would otherwise get its predicate re-check, bitmap walk and edge hashes - // run six times per draw. const auto consume = [&consumers](BufferObject* buffer) { if (buffer == nullptr) return; for (const BufferObject* existing : consumers) { @@ -756,57 +936,91 @@ namespace MobileGL::MG_Remote::Client { } consumers.push_back(buffer); }; - if (const auto& vao = ctx->GetBoundVertexArray()) { - for (Uint i = 0; i < MG_Pipe::kMGPipeMaxVertexAttribs; ++i) { - const auto& attrib = vao->GetAttribute(i); - if (attrib.Enabled && attrib.Buffer != nullptr) { - consume(attrib.Buffer.get()); - } - } - if (const auto& element = vao->GetIndexBufferBindingSlot().GetBoundObject()) { - consume(element.get()); + + // THE DRAIN THE SKIP RESTS ON: every tracked member with a marked page, whether + // or not this draw consumes it. The epoch says "some page of some tracked map was + // marked" and nothing about which; pushing only the consumers would advance the + // watermark past a mark on a buffer the draw did not bind, and that buffer would + // then never move the epoch again (its faulted page is writable, its bit is set, + // nothing re-arms it) - the next draw that DOES consume it would skip, and the + // server would draw stale bytes. Such a push is bitmap-driven, so it ships only + // the faulted blocks, not the range: the cost the consumer filter exists to avoid + // is the whole-range rescan of the hash arm, which no tracked member pays. The + // cached slot pointer is validated against the slot's own lifetimeId: a slot + // retired since the cache was written fails the test, as does one re-claimed by + // a different buffer. + for (const auto& entry : m_livePersistentMaps) { + const TrackedWriteMap* tracked = entry.second.tracked; + if (tracked != nullptr && tracked->lifetimeId == entry.first && AnyPageMarked(*tracked)) { + consume(entry.second.buffer); } } - static constexpr BufferTarget kIndexedTargets[] = { - BufferTarget::Uniform, BufferTarget::ShaderStorage, BufferTarget::AtomicCounter, - BufferTarget::TransformFeedback, BufferTarget::CopyRead, BufferTarget::CopyWrite, - BufferTarget::Query, BufferTarget::Texture}; - for (const BufferTarget target : kIndexedTargets) { - const SizeT count = ctx->GetBufferBindingPointCount(target); - for (SizeT i = 0; i < count; ++i) { - if (const auto& bound = ctx->GetBufferBindingPoint(target, i).GetBoundObject()) { + + // The buffers the upcoming draw or dispatch can read, and only those. Every one is + // named by the frontend's own binding state, read on this (the GL) thread - VAO + // attribute and element buffers, the indexed binding points of every indexed + // target, and the indirect / parameter slots. Client-sourced attributes (no + // BufferObject) live on this side already and need no push. This is the half of + // the walk that serves the hash-arm (untracked) members and the inward fallback's + // edges: nothing but a binding can say whether this draw reads them. + if (const auto& ctx = MG_State::pGLContext) { + if (const auto& vao = ctx->GetBoundVertexArray()) { + for (Uint i = 0; i < MG_Pipe::kMGPipeMaxVertexAttribs; ++i) { + const auto& attrib = vao->GetAttribute(i); + if (attrib.Enabled && attrib.Buffer != nullptr) { + consume(attrib.Buffer.get()); + } + } + if (const auto& element = vao->GetIndexBufferBindingSlot().GetBoundObject()) { + consume(element.get()); + } + } + static constexpr BufferTarget kIndexedTargets[] = { + BufferTarget::Uniform, BufferTarget::ShaderStorage, BufferTarget::AtomicCounter, + BufferTarget::TransformFeedback, BufferTarget::CopyRead, BufferTarget::CopyWrite, + BufferTarget::Query, BufferTarget::Texture}; + for (const BufferTarget target : kIndexedTargets) { + const SizeT count = ctx->GetBufferBindingPointCount(target); + for (SizeT i = 0; i < count; ++i) { + if (const auto& bound = ctx->GetBufferBindingPoint(target, i).GetBoundObject()) { + consume(bound.get()); + } + } + } + static constexpr BufferTarget kSlotTargets[] = {BufferTarget::DrawIndirect, + BufferTarget::DispatchIndirect, + BufferTarget::Parameter}; + for (const BufferTarget target : kSlotTargets) { + if (const auto& bound = ctx->GetBufferBindingSlot(target).GetBoundObject()) { consume(bound.get()); } } - } - static constexpr BufferTarget kSlotTargets[] = {BufferTarget::DrawIndirect, - BufferTarget::DispatchIndirect, - BufferTarget::Parameter}; - for (const BufferTarget target : kSlotTargets) { - if (const auto& bound = ctx->GetBufferBindingSlot(target).GetBoundObject()) { - consume(bound.get()); - } - } - // BUFFER TEXTURES (the cloud renderer's shape, and the gap the binding-point walk - // cannot see): a texture attached to a buffer with glTexBuffer is read by the draw - // through a texture unit, never through a buffer binding point. Walking the touched - // units and following a buffer-backed texture to its backing buffer is the only way - // those bytes earn their push - without this arm a persistent-mapped TexBuffer is - // only refreshed by the read-only verbs' whole push (once per present), which is - // exactly the per-frame-cadence cloud twitch this comment is written against. - const Int maxUnit = ctx->GetMaxTouchedTextureUnit(); - for (Int unit = 0; unit <= maxUnit; ++unit) { - const auto& texture = - ctx->GetTextureUnitObject(unit).GetBindingSlot(TextureTarget::TextureBuffer).GetBoundObject(); - if (texture == nullptr || texture->GetStorageType() != TextureStorageType::Buffer) { - continue; - } - auto* bufferTexture = static_cast(texture.get()); - if (const auto& backing = bufferTexture->GetBufferBindingSlot().GetBoundObject()) { - consume(backing.get()); + // BUFFER TEXTURES (the cloud renderer's shape, and the gap the binding-point + // walk cannot see): a texture attached to a buffer with glTexBuffer is read by + // the draw through a texture unit, never through a buffer binding point. + // Walking the touched units and following a buffer-backed texture to its + // backing buffer is the only way those bytes earn their push - without this + // arm a persistent-mapped TexBuffer is only refreshed by the read-only verbs' + // whole push (once per present), which is exactly the per-frame-cadence cloud + // twitch this comment is written against. + const Int maxUnit = ctx->GetMaxTouchedTextureUnit(); + for (Int unit = 0; unit <= maxUnit; ++unit) { + const auto& texture = ctx->GetTextureUnitObject(unit) + .GetBindingSlot(TextureTarget::TextureBuffer) + .GetBoundObject(); + if (texture == nullptr || texture->GetStorageType() != TextureStorageType::Buffer) { + continue; + } + auto* bufferTexture = static_cast(texture.get()); + if (const auto& backing = bufferTexture->GetBufferBindingSlot().GetBoundObject()) { + consume(backing.get()); + } } } if (consumers.empty()) return; + // The membership test is what keeps a bound non-member (an ordinary buffer, an + // adopted store) out of PushBlocksForChecked; PushBlocksForChecked can Forget() + // a member that stopped being one, which is why the list was copied out first. for (BufferObject* buffer : consumers) { if (buffer != nullptr && m_livePersistentMaps.count(buffer->GetLifetimeId()) != 0) { PushBlocksForChecked(*buffer); diff --git a/MobileGL/MG_Remote/Client/PersistentMapTracker.h b/MobileGL/MG_Remote/Client/PersistentMapTracker.h index a7604f94..eb1d0e4c 100644 --- a/MobileGL/MG_Remote/Client/PersistentMapTracker.h +++ b/MobileGL/MG_Remote/Client/PersistentMapTracker.h @@ -74,16 +74,23 @@ namespace MobileGL::MG_Remote::Client { std::atomic* pageBits = nullptr; // allocated once, never freed or moved SizeT pageCount = 0; // GL thread only Uint64 lifetimeId = 0; // GL thread only - // GL thread only. The two partial-page edge spans are never protected (see - // TrackWriteMap's inward alignment), so nothing faults for them; pushing their - // blocks unconditionally measured ~2 x 64 KB x every draw for a hot unaligned - // buffer (112 MB/frame). They are hashed per push instead - 8 KB of XXH3 at - // worst - and their containing block ships only on a change. + // GL thread only, and only on the INWARD fallback (TrackWriteMap: a shadow that + // failed the page-granular test). There the two partial-page edge spans are + // never protected, so nothing faults for them; pushing their blocks + // unconditionally measured ~2 x 64 KB x every draw for a hot unaligned buffer + // (112 MB/frame). They are hashed per push instead - 8 KB of XXH3 at worst - and + // their containing block ships only on a change. A page-granular shadow (every + // shadow the allocator hands out in a split build) is aligned OUTWARD and has no + // edges, so these are never read for it - that per-draw XXH3 was 8.5% of the + // client thread at VD12 (P5d round 3), on nearly every live map. Uint64 edgeHashHead = 0; Uint64 edgeHashTail = 0; - // GL thread only: whether the mapped range has partial-page edge spans at all. - // Feeds the epoch skip's per-member edge service (PushDrawConsumers) - edge - // bytes are never protected, so their dirtiness is invisible to the fault epoch. + // GL thread only: whether the mapped range has UNPROTECTED partial-page edge spans + // at all - true only on the inward fallback. Feeds the epoch skip's per-member edge + // service (PushDrawConsumers, through m_edgedMembers) - edge bytes are never + // protected, so their dirtiness is invisible to the fault epoch. The push itself + // does not read it: its two edge hashes are gated by the span geometry, which + // says the same thing. Bool hasEdges = false; }; @@ -151,6 +158,22 @@ namespace MobileGL::MG_Remote::Client { // Unit tests only: clears the membership, the hash cache and the mprotect tracked // slots (restoring every tracked page writable first), and resets the counters. void ClearForTest(); + // Unit tests only: the mprotect arm's white-box surface, for the alignment cases + // (SplitBufferTest) that the public GL surface cannot see - which pages a map + // protected is not observable through any push count. MprotectArmAvailableForTest + // is the arm's precondition (handler installable, kernel page == tracker page); + // a case that needs the arm skips on it rather than going red on a host that has + // no arm. TrackedSlotForTest reads a member's live slot (nullptr on the hash arm). + // TrackForTest / UntrackForTest register an arbitrary range with an arbitrary + // declared extent, which is the only way to reach the inward fallback on a build + // whose allocator never hands out an unaligned shadow. FaultEpochForTest is the + // handler's answered-fault counter, the one observable a protected page has. + static Bool MprotectArmAvailableForTest(); + static const TrackedWriteMap* TrackedSlotForTest(const MG_State::GLState::BufferObject& buffer); + static const TrackedWriteMap* TrackForTest(Uint64 lifetimeId, const Uint8* shadow, SizeT rangeBegin, + SizeT rangeEnd, SizeT shadowExtent); + static void UntrackForTest(Uint64 lifetimeId); + static Uint64 FaultEpochForTest(); private: // One live member. tracked is the member's tracker slot when it is on the @@ -183,13 +206,22 @@ namespace MobileGL::MG_Remote::Client { Uint64 m_blocksPushed = 0; Uint64 m_bytesPushed = 0; // THE EPOCH SKIP's state (see PushDrawConsumers). m_lastFaultEpoch is the fault - // counter's value at the last consumer walk. m_untrackedMembers holds EVERY + // counter's value at the last walk - and the walk drains every tracked member + // with a marked page, consumer or not, which is the only reason an unmoved + // counter can stand for "nothing to push". m_untrackedMembers holds EVERY // live member that fell back to the hash arm, and any one of them vetoes the // skip: the walk is what tells an untracked member's consumption, and scanning // every untracked member unconditionally (the alternative, measured on device // at VD12) costs more than the walk it replaced. Uint64 m_lastFaultEpoch = 0; UnorderedMap m_untrackedMembers; + // The tracked members whose slot has UNPROTECTED edge spans (the inward fallback, + // TrackedWriteMap::hasEdges): the epoch skip serves exactly these per draw and + // nobody else, so it walks this set and not the membership. Empty in steady state + // on a split build - every shadow is page-granular and aligns outward - which is + // what makes the skip's common path a handful of loads with no per-member work. + // Maintained beside m_untrackedMembers at the same three events. + UnorderedMap m_edgedMembers; // Everything of PushBlocksFor after the two role guards, for callers that already // ran them once (the two Members hooks) - the guard's thread-local read was a // measurable per-buffer-per-draw cost on the emutls path. diff --git a/MobileGL/MG_Remote/Client/WireTables.cpp b/MobileGL/MG_Remote/Client/WireTables.cpp index ca9fe284..947fb771 100644 --- a/MobileGL/MG_Remote/Client/WireTables.cpp +++ b/MobileGL/MG_Remote/Client/WireTables.cpp @@ -72,6 +72,15 @@ namespace MobileGL::MG_Remote::Client { // NOT in the anonymous namespace, because M5 needs it from MG_Impl/Pipe/PipeFill.cpp too // (its split-only respecify/flush branches must not run on the apply thread). Declared in // WireTables.h. + // + // AND IT STAYS OUT OF LINE (P5d round 3, package D). OnApplyThread() became an inline + // relaxed load plus a thread-pointer compare in ServerLoop.h, so the body this forwards to + // is now smaller than the call that reaches it - but inlining THIS would mean including a + // server header from WireTables.h, and the declaration above exists precisely so PipeFill + // does not have to. One PLT hop per verb is the price of that boundary; the four-part + // predicate behind it, which was the measured cost (2.66% self on the client thread at + // head 56a77348), is gone either way. The "one atomic load" the paragraph above promised + // is now literally what it costs. Bool RunsAsTheServerRole() { return Server::ServerLoop::OnApplyThread(); } namespace { diff --git a/MobileGL/MG_Remote/Server/ServerLoop.cpp b/MobileGL/MG_Remote/Server/ServerLoop.cpp index 3ac42680..1d56898d 100644 --- a/MobileGL/MG_Remote/Server/ServerLoop.cpp +++ b/MobileGL/MG_Remote/Server/ServerLoop.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -211,6 +212,7 @@ namespace MobileGL::MG_Remote::Server { // tally - and a developer running the binary directly gets the count CI sees. m_drained.store(0, std::memory_order_release); m_parks.store(0, std::memory_order_release); + m_parkBlocks.store(0, std::memory_order_release); m_nativeBinds.store(0, std::memory_order_release); m_clientReleases.store(0, std::memory_order_release); m_makeCurrentRepublishes.store(0, std::memory_order_release); @@ -226,15 +228,17 @@ namespace MobileGL::MG_Remote::Server { Bool ServerLoop::Running() const { return m_running.load(std::memory_order_acquire); } - Bool ServerLoop::OnApplyThread() { - ServerLoop& loop = ServerLoopInstance(); - return loop.m_running.load(std::memory_order_acquire) && - loop.m_applyThreadId.load(std::memory_order_acquire) == std::this_thread::get_id(); - } + // OnApplyThread() is inline in the header now (P5d round 3, package D). Its whole body is + // one relaxed load of Detail::g_applyThreadKey and one comparison against the caller's own + // thread pointer; the two writes to that key are in ApplyThreadMain, below. Uint64 ServerLoop::ResolvedAffinityMask() const { return m_affinityMask; } Uint64 ServerLoop::DrainedRecords() const { return m_drained.load(std::memory_order_acquire); } Uint64 ServerLoop::ParkCount() const { return m_parks.load(std::memory_order_acquire); } + + Uint64 ServerLoop::ParkBlockCount() const { + return m_parkBlocks.load(std::memory_order_relaxed); + } Uint64 ServerLoop::NativeBindCount() const { return m_nativeBinds.load(std::memory_order_acquire); } Uint64 ServerLoop::ClientReleaseCount() const { return m_clientReleases.load(std::memory_order_acquire); @@ -325,7 +329,12 @@ namespace MobileGL::MG_Remote::Server { } void ServerLoop::ApplyThreadMain() { - m_applyThreadId.store(std::this_thread::get_id(), std::memory_order_release); + // THE IDENTITY IS PUBLISHED FIRST, BEFORE THE THREAD HAS A NAME OR AN AFFINITY. Every + // role guard in the tree reads it, and the very first record this thread applies must + // already answer "yes" to OnApplyThread() - otherwise the applier's own reads would + // look like client reads to the guards. Relaxed for the reason stated beside the key's + // declaration: it is an identity, not a handshake, and this thread is the only writer. + Detail::g_applyThreadKey.store(Detail::CurrentThreadKey(), std::memory_order_relaxed); NameThisThread("mgl-srv-apply"); Bool recognised = true; @@ -380,8 +389,31 @@ namespace MobileGL::MG_Remote::Server { if (m_stopRequested.load(std::memory_order_acquire)) break; if (ready()) continue; - m_parks.fetch_add(1, std::memory_order_acq_rel); - const bool woke = bell.Wait(control.consumerParked, ready, spinUs, Transport::kWaitForever); + const Uint64 waits = m_parks.fetch_add(1, std::memory_order_acq_rel) + 1; + // THE WAIT LEDGER'S SERVER HALF (P5d round 3, package T item 4). The client may not + // name this counter - CONTRACT-P5C rule E, layer 2: a GL-thread read of + // ServerLoop's memory is exactly the cross-role access P5c zeroed - so the server + // publishes its own reading and MG_Util::PipeStats is the meeting point. Two + // relaxed stores behind the stats latch, on the one path in this loop that is about + // to spin or block anyway; with MOBILEGL_PIPE_STATS unset it is a global load and a + // predicted branch. + // + // BOTH NUMBERS, because they answer different questions. `srv` is how often the + // loop reached Doorbell::Wait at all (ParkCount()'s long-standing meaning: an + // intention, counted on the way TOWARD a park - ServerLoopTest's C10 note); `srvpark` + // is how often it really blocked. The ratio is what says whether the spin budget is + // sized for this workload, and it is only readable as a ratio because the two + // numbers count the same events: m_parkBlocks is THIS loop's tally, handed to the + // Wait below, not the bell's own ParkEntries(). A bell counts every waiter on its + // endpoint, so reading it here would have made `srvpark` a superset of `srv` the + // moment anything else waited on the consumer bell. + if (MG_Util::PipeStats::Enabled()) { + using MG_Util::PipeStats::Gauge; + MG_Util::PipeStats::PublishGauge(Gauge::ServerWaits, waits); + MG_Util::PipeStats::PublishGauge(Gauge::ServerParks, ParkBlockCount()); + } + const bool woke = bell.Wait(control.consumerParked, ready, spinUs, + Transport::kWaitForever, &m_parkBlocks); if (!woke) { // Wait returns false only on a dead bell or an expired deadline, and this park // has no deadline. A dead bell IS the shutdown signal (table 3's teardown step @@ -393,7 +425,9 @@ namespace MobileGL::MG_Remote::Server { break; } MGLOG_E("MG_Remote server: Doorbell::Wait(kWaitForever) returned false on a live " - "bell - that is impossible by Doorbell.h:139-178 and means the bell's " + "bell - that is impossible by Doorbell::Wait's park loop (the only " + "false returns are the Dead() arm and an expired deadline) and means " + "the bell's " "Dead() answer moved under the waiter. Shutting the loop down rather " "than spinning"); break; @@ -435,6 +469,10 @@ namespace MobileGL::MG_Remote::Server { // NOT_INITIALIZED without publishing). There is no third order. { const std::lock_guard lock(m_controlMutex); + // The shadow dies with the mailbox it shadows, under the same lock and inside the + // same critical section as the m_running clear: nothing will ever pump again, so a + // `true` left here would be a claim no thread can retire. + m_controlPosted.store(false, std::memory_order_release); if (m_controlPending) { m_controlPending = false; m_controlFinished = true; @@ -443,6 +481,30 @@ namespace MobileGL::MG_Remote::Server { "exited; it is answered NOT_INITIALIZED rather than left blocking"); } m_running.store(false, std::memory_order_release); + // THE IDENTITY DIES WITH m_running, IN THE SAME CRITICAL SECTION, because it IS + // m_running as far as OnApplyThread() is concerned (P5d round 3, package D): the + // old predicate was `m_running && id == mine`, so it stopped answering true at + // exactly this store, and the key has to stop meaning "this thread" at exactly the + // same store or the window would have moved. Everything that must still run as the + // server role - the final PumpControlRequest/DrainRing, Applier().Detach(), + // ~BackendObject_DirectGLES and the ReleaseEGLResources it posts back to itself - + // is already above this block. + // + // DO NOT MOVE THIS STORE UP ON THE STRENGTH OF A GREEN ctest (review round 3). That + // this line EXISTS is pinned below (ServerLoopTest's + // AThreadCreatedAfterTheLoopStoppedIsNotMistakenForTheApplyThread: delete the store + // and the key survives the join, and the next thread that inherits this one's TLS + // block - hence its thread pointer - answers OnApplyThread() TRUE). That it sits + // HERE rather than fifteen lines earlier is NOT pinned by any unit case and cannot + // be: ServerLoopTest has no GL context, so CreateBackend is never called and the + // ~BackendObject_DirectGLES -> ReleaseEGLResources self-post named above is never + // built. Move the store above m_backend.reset() and that post misses + // RunOnApplyThread's re-entrancy arm (ServerLoop.cpp: `if (OnApplyThread()) return + // work(user);`) while m_running is still true, so it publishes into the one-slot + // mailbox and waits on m_controlDone for an answer only THIS thread could ever + // give. The integration lane - a real EGL teardown - is the only thing that would + // catch that, and it would catch it as a hang, not as a failed assertion. + Detail::g_applyThreadKey.store(0, std::memory_order_relaxed); } m_controlDone.notify_all(); @@ -450,18 +512,38 @@ namespace MobileGL::MG_Remote::Server { } Bool ServerLoop::ControlIsPending() const { - const std::lock_guard lock(const_cast(m_controlMutex)); - return m_controlPending; + // ONE ACQUIRE LOAD, NO LOCK - see the header block on m_controlPosted. This is the + // hottest call in the whole loop (the `ready` lambda runs it on every spin iteration of + // every wait) and it used to be a mutex round trip. + return m_controlPosted.load(std::memory_order_acquire); } Bool ServerLoop::PumpControlRequest() { + // THE IDLE POLL'S FAST NEGATIVE. The loop calls this once per iteration whether or not + // anything was posted, and the common answer is "nothing". Taking m_controlMutex to + // learn that was the other half of package T's finding; the shadow answers it with a + // load, and everything below this line still runs under the lock exactly as before. + if (!m_controlPosted.load(std::memory_order_acquire)) return false; + ControlWork work = nullptr; void* user = nullptr; { const std::lock_guard lock(m_controlMutex); - if (!m_controlPending) return false; + if (!m_controlPending) { + // The shadow outlived its request - it cannot happen through the poster's + // path, but a `true` that nobody clears would make `ready` permanently true + // and the loop would never park again, so it is cleared here rather than + // trusted. + m_controlPosted.store(false, std::memory_order_release); + return false; + } work = m_controlWork; user = m_controlUser; + // CLEARED WHEN THE REQUEST IS TAKEN, not when it finishes. m_controlPending stays + // true for the duration of `work` (the C2 exit block reads it), but the thread that + // would act on the shadow is THIS one and it is busy running the request; leaving + // the shadow set would only make the post-work iteration spin instead of park. + m_controlPosted.store(false, std::memory_order_release); } const MobileGLResult result = work == nullptr ? MOBILEGL_ERR_INVALID_ARGUMENT : work(user); { @@ -475,6 +557,14 @@ namespace MobileGL::MG_Remote::Server { } Uint64 ServerLoop::DrainRing() { + // THE EMPTY-RING ANSWER COSTS TWO LOADS AND NOTHING ELSE, and that was AUDITED rather + // than assumed (P5d round 3, package T item 2): the idle poll calls this on every + // iteration, so a lock or a clock read in here would be the same defect + // PumpControlRequest had. The path is SessionConsumer::ApplyOne -> RingConsumer::Pop, + // and Pop's first act on an empty ring is one acquire load of cmdHead compared against + // the consumer's own m_localTail (Ring.cpp:222-232) - no mutex, no steady_clock, no + // allocation; `applied == 0` then skips the retire block below entirely. Keep it that + // way: anything added here runs ~900 times a frame doing nothing. ServerSession& session = *m_session; Transport::SessionConsumer& consumer = session.Consumer(); PipeApplier& applier = session.Applier(); @@ -584,7 +674,14 @@ namespace MobileGL::MG_Remote::Server { m_controlUser = user; m_controlPending = true; m_controlFinished = false; - // Publish THEN ring, in that order and never the other (Doorbell.h:186-193). The bell + // THE SHADOW IS PUBLISHED LAST OF THE FOUR AND WITH A RELEASE, inside this same + // critical section: the apply thread's park predicate reads ONLY this word, so an + // acquire load that returns true must also see the three fields above it. Storing it + // first would let a pump that is already past its own fast-negative read a slot that + // does not name the work yet. + m_controlPosted.store(true, std::memory_order_release); + // Publish THEN ring, in that order and never the other (Doorbell.h, NotifyIfParked's + // PRECONDITION block: the fence only orders what precedes it). The bell // is rung unconditionally rather than through NotifyIfParked because this side does not // know whether the apply thread is parked or spinning, and CondVarDoorbell remembers a // wakeup that arrives while nobody is waiting. Held under m_controlMutex: wait() releases @@ -643,7 +740,8 @@ namespace MobileGL::MG_Remote::Server { MGLOG_F("MGPipe: Fatal{ApplyThreadJoinTimeout} - mgl-srv-apply did not exit within " "%u ms of Stop(). Stop() published m_stopRequested (in the park predicate) " "BEFORE it rang, so a plain Notify should already have un-parked the thread; " - "Doorbell::Kill() (Doorbell.h:211-221, table 3 step 2) is the belt to that " + "Doorbell::Kill() (Doorbell.h, CondVarDoorbell::Kill; table 3 step 2) is " + "the belt to that " "Notify's braces. If the thread is still parked after both, the wakeup was " "lost, not slow. Aborting rather than detaching: a detached apply thread still " "owns the context and would read rings the client is about to unmap", @@ -651,7 +749,10 @@ namespace MobileGL::MG_Remote::Server { std::abort(); } m_thread.join(); - m_applyThreadId.store(std::thread::id{}, std::memory_order_release); + // The identity is NOT cleared here any more: the thread clears it itself, under + // m_controlMutex, beside the m_running store it used to be paired with. Clearing it a + // second time after the join would be harmless but would also be the second place a + // reader has to check to know when the key stops meaning "the apply thread". m_running.store(false, std::memory_order_release); m_session = nullptr; } diff --git a/MobileGL/MG_Remote/Server/ServerLoop.h b/MobileGL/MG_Remote/Server/ServerLoop.h index 4681ec93..9aebf78e 100644 --- a/MobileGL/MG_Remote/Server/ServerLoop.h +++ b/MobileGL/MG_Remote/Server/ServerLoop.h @@ -26,8 +26,8 @@ // kWaitForever waiter, not one, and the difference is the whole of review m-4: for the STOP case a // plain Doorbell::Notify() is sufficient, because m_stopRequested is in the park predicate and // Stop() publishes it BEFORE it rings (Doorbell.h re-tests ready() after every Park return); -// Doorbell::Kill() (Doorbell.h:211-221) is load-bearing only for a predicate that has NOTHING to -// see, which is why the redcheck's park-predicate-loses-control entry - the one that drops the +// Doorbell::Kill() (Doorbell.h, CondVarDoorbell::Kill) is load-bearing only for a predicate +// that has NOTHING to see, which is why the redcheck's park-predicate-loses-control entry - the one that drops the // control flag FROM the predicate - is the case that actually times out. Kill BEFORE join; join // before the client frees any emitter-owned Vector; and the join must be bounded (that test uses // 5 s) so a regression is a red test and not a hung CI job. @@ -74,11 +74,118 @@ #include #include +#include +#include #include #include +#include namespace MobileGL::MG_Remote::Server { + // --------------------------------------------------------------------------------- + // THE APPLY-THREAD IDENTITY, AS ONE INTEGER (P5d round 3, package D) + // --------------------------------------------------------------------------------- + // + // OnApplyThread() is the predicate under EVERY role guard on both hot paths - + // RefuseLegacyBufferArmFromApplyThread in BufferObject's accessors, PersistentMapTracker:: + // OnServerRole, ClientSession::RefusePipeInputsTouchWhileApplierOwnsIt, RunsAsTheServerRole, + // MGPipeRefuseAllocatorFromApplyThread, RefuseLegacyTextureArmFromApplyThread. simpleperf at + // head 56a77348 (FCL + Minecraft, VD12, Adreno 830) measured it at 2.66% self / 3.64% + // inclusive ON THE CLIENT THREAD, for an answer that is "no" every single time. + // + // The old body paid five times over for that "no": ServerLoopInstance()'s function-static + // guard byte, an acquire load of m_running, an acquire load of an atomic, + // std::this_thread::get_id(), and a thread::id comparison that is an out-of-line + // pthread_equal through the PLT (@plt was 6.1% of the apply thread). None of it carries + // information the guard needs. A live thread's identity is already sitting in a + // register-cheap place - the thread pointer, TPIDR_EL0 on aarch64 - so the loop publishes + // ITS identity as a plain integer here and every caller compares one relaxed load against + // one register read, inline, with no call and no singleton. + // + // ZERO MEANS "NO APPLY THREAD IS RUNNING", which is exactly the answer m_running == false + // used to give: the key is published as the apply thread's FIRST act (ApplyThreadMain) and + // cleared in the same m_controlMutex critical section that clears m_running on the way out + // (ApplyThreadMain's C2 exit block), so "key != 0" and "m_running" move together and the + // window in which a live-but-exiting apply thread answers true is byte-for-byte the old + // one. The ServerLoop object still OWNS the key - nothing but ApplyThreadMain writes it - + // it simply no longer has to be reached through the singleton to be read. + namespace Detail { + // RELAXED ON BOTH SIDES, and that is a claim about recycling, not a shrug. The only + // dangerous outcome would be a NEW thread that reuses the exited apply thread's TLS + // block (hence its thread pointer) while still reading the pre-clear key. It cannot + // happen: the clear is sequenced before the apply thread's exit, the exit releases the + // TLS block under libc's own lock, and the thread that later receives that block takes + // the same lock - so the clear happens-before any load the new thread performs, and + // coherence forbids a relaxed load from reading a value that a happens-before store + // overwrote. A stale ZERO is harmless in the other direction: a thread that is not the + // apply thread wants "false" anyway, and the apply thread always reads its own store. + // + // No acquire is needed for a second reason: this is an IDENTITY, not a handshake. No + // caller publishes data through it; the real ordering lives in the ring, the doorbell + // and m_controlMutex. + inline std::atomic g_applyThreadKey{0}; + + // The calling thread's identity as the same integer. aarch64 - the split's only + // measured target - reads TPIDR_EL0 with a single instruction and no call. Everywhere + // else falls back to std::this_thread::get_id() memcpy'd into the key: still one libc + // call, but the atomic load and the out-of-line pthread_equal that + // dominated the old body are gone either way. x86_64 is deliberately NOT on the + // builtin path - __builtin_thread_pointer only grew x86 support in clang 17 / GCC 11 + // and older toolchains accept __has_builtin for it and then fail in codegen, which is + // a build break on the desktop ctest lane for a target whose frame rate nobody + // measures. + // + // A LIVE THREAD'S KEY IS NEVER 0 on either path (a thread pointer is a real TLS + // address; std::this_thread::get_id() on a running thread is never the + // default-constructed id, whose object representation is the all-zero one on every + // library in scope), which is what lets 0 mean "nobody". + // + // THE SELECTOR IS A BUILD KNOB, NOT A HEADER SECRET (review round 3). This header is + // pulled into MG_State, MG_Impl and MG_Test translation units now, so the name carries + // the MOBILEGL_DETAIL_ prefix to read as internal, it is #ifndef-guarded so a toolchain + // whose __has_builtin(__builtin_thread_pointer) answers 1 and then fails in codegen can + // be steered off the path with -DMOBILEGL_DETAIL_APPLY_THREAD_POINTER=0 instead of a + // header edit, and it is always DEFINED - to 0 when unavailable - and tested with #if + // rather than #ifdef, so that a -D...=0 really disables it instead of being ignored. +#ifndef MOBILEGL_DETAIL_APPLY_THREAD_POINTER +#if defined(__aarch64__) && defined(__has_builtin) +#if __has_builtin(__builtin_thread_pointer) +#define MOBILEGL_DETAIL_APPLY_THREAD_POINTER 1 +#endif +#endif +#endif +#ifndef MOBILEGL_DETAIL_APPLY_THREAD_POINTER +#define MOBILEGL_DETAIL_APPLY_THREAD_POINTER 0 +#endif + inline Uint64 CurrentThreadKey() { +#if MOBILEGL_DETAIL_APPLY_THREAD_POINTER + return static_cast(reinterpret_cast(__builtin_thread_pointer())); +#else + const std::thread::id id = std::this_thread::get_id(); + static_assert(sizeof(std::thread::id) <= sizeof(Uint64), + "std::thread::id does not fit the apply-thread key; this fallback has " + "to hash rather than copy on that platform"); + // AND IT MUST HAVE NO PADDING (review round 3). Copying the OBJECT REPRESENTATION of + // a type that has padding bytes would let two calls ON THE SAME THREAD return + // different keys, and OnApplyThread() would then answer FALSE ON THE APPLY THREAD - + // a role guard that has stopped guarding, which is the one direction CONTRACT-P5C + // rule E says a guard may never fail in (RunOnApplyThread would also lose its + // re-entrancy shortcut and the EGL teardown post would deadlock on m_controlDone). + // Every library this fallback is built against today wraps a single scalar + // (libstdc++ __gthread_t, libc++ __libcpp_thread_id, MSVC unsigned int) and + // satisfies this; one that did not is a BUILD ERROR here rather than a silently + // dead guard at run time. + static_assert(std::has_unique_object_representations_v, + "std::thread::id has padding on this platform: copying its object " + "representation would make the apply-thread key unstable within one " + "thread. Hash the id (std::hash) instead."); + Uint64 key = 0; + std::memcpy(&key, &id, sizeof(id)); + return key; +#endif + } + } // namespace Detail + class ServerLoop { public: // Creates the apply thread, names it mgl-srv-apply, applies @@ -131,7 +238,18 @@ namespace MobileGL::MG_Remote::Server { // True on the apply thread itself. RunOnApplyThread uses it to run inline rather than // deadlock when the apply thread posts to itself - which the EGL teardown path does, // because ~BackendObject_DirectGLES runs THERE and reaches ReleaseEGLResources. - static Bool OnApplyThread(); + // + // INLINE, AND THAT IS THE POINT (P5d round 3, package D): see the Detail block above. + // Every role guard in MG_State, MG_Impl, MG_Pipe and MG_Remote/Client asks this once + // or more per draw, so the call itself - a cross-library PLT hop - was a measurable + // share of the cost of the answer. + static Bool OnApplyThread() { + const Uint64 key = Detail::g_applyThreadKey.load(std::memory_order_relaxed); + // Short-circuit ON PURPOSE: with TransportMode::Monolith, and in every process + // before Start() and after Stop(), the key is 0 and the caller pays one relaxed + // load and one branch - it never even reads its own identity. + return key != 0 && key == Detail::CurrentThreadKey(); + } // The CPU mask MOBILEGL_IPC_SERVER_AFFINITY resolved to and sched_setaffinity accepted. // 0 means "no affinity was applied" - the honest answer for `off`, for a platform with @@ -146,6 +264,13 @@ namespace MobileGL::MG_Remote::Server { // fail for its own reason. Uint64 DrainedRecords() const; Uint64 ParkCount() const; + // How many of those waits ran out of spin budget and really blocked. ParkCount is an + // INTENTION (counted on the way toward a park, C10's note); this is the outcome, and + // the pair is what says whether MOBILEGL_IPC_SPIN_US is sized for the workload. It is + // this loop's OWN tally rather than the consumer bell's ParkEntries() because a bell + // belongs to an endpoint and counts every waiter on it; only a tally passed into the + // one Wait this loop makes is guaranteed to stay a subset of ParkCount(). + Uint64 ParkBlockCount() const; // Scheduling perturbation only: the hook runs after application, before retirement. // Integration tests use it to observe real producer back-pressure from GL uploads. void SetBeforeRetireHookForTesting(void (*hook)()) { @@ -192,11 +317,35 @@ namespace MobileGL::MG_Remote::Server { void ForgetCurrentTuple(); void ForgetCurrentTupleIfItNames(EGLSurface surface); - private: - void ApplyThreadMain(); // Part of the apply thread's park predicate: a posted control request must be able to // un-park a thread waiting on kWaitForever, which a Notify alone cannot do. + // + // IT READS ONE ATOMIC AND TAKES NO LOCK (P5d round 3, package T item 1). It used to + // take m_controlMutex on every call, and the idle poll calls it on every iteration - + // once inside the `ready` lambda and once more through PumpControlRequest - which + // simpleperf at head 56a77348 measured as ~49% of the whole apply thread's cycles + // (mutex::lock 12.4 + mutex::unlock 13.6 + the two pthread_* halves, 58-60% of them + // with ApplyThreadMain as the DIRECT caller). An idle poll on an empty ring must cost + // loads, not a futex-backed critical section. m_controlPosted is that load; the + // mailbox's own fields stay under the mutex and the handshake below is unchanged. + // + // PUBLIC because ServerLoopTest reads it: the clear side of the shadow has a failure + // mode - a shadow left set makes `ready` permanently true and the loop never parks + // again - that no other public surface can see. + // + // READ THE PREDICATE LITERALLY: "a request is POSTED AND NOT YET TAKEN", which is + // narrower than the name's "a control request is in flight". The shadow is cleared + // when the pump takes the work, not when the work returns, so for the whole duration + // of `work(user)` this answers false although m_controlPending is still true and the + // poster is still blocked in RunOnApplyThread. That is deliberate - it is a PARK + // PREDICATE, and the thread that would act on it is the one running the work - but it + // means this is not a liveness query and must not be used as one. The only in-tree + // reader outside the test is the apply thread's own `ready` lambda, which by + // construction cannot be inside that window. Bool ControlIsPending() const; + + private: + void ApplyThreadMain(); // Runs a posted control request, if there is one. Returns true if it ran one. Bool PumpControlRequest(); // Pops and applies every record currently in the ring; returns how many it applied. @@ -207,7 +356,9 @@ namespace MobileGL::MG_Remote::Server { std::atomic m_running{false}; std::atomic m_stopRequested{false}; std::thread m_thread; - std::atomic m_applyThreadId{}; + // The apply thread's identity used to live here as an atomic. It is + // Detail::g_applyThreadKey now - see the block at the top of this header for why the + // identity had to stop being a member reachable only through ServerLoopInstance(). // The private backend object. Destroyed ON the apply thread while it still owns the // context - see Stop(). @@ -217,7 +368,19 @@ namespace MobileGL::MG_Remote::Server { // client thread runnable at a time; m_callerMutex serialises anything that is not. std::mutex m_callerMutex; std::mutex m_controlMutex; - std::condition_variable m_controlPosted; + // THE MAILBOX'S ONE-BIT SHADOW, and the only field of it the idle poll may read. + // Written to `true` by the poster under m_controlMutex AFTER m_controlPending, and + // back to `false` under the same lock by whoever TAKES the request (PumpControlRequest, + // or ApplyThreadMain's exit block when it answers NOT_INITIALIZED). A reader that sees + // `true` through the release/acquire pair therefore also sees m_controlPending; a + // reader that sees `false` cannot be missing a request, because the store precedes the + // poster's Notify and Doorbell::Wait re-tests its predicate after every Park return. + // The mutex still owns every other field - this is a fast NEGATIVE answer, not a second + // copy of the mailbox. + // + // It replaces a std::condition_variable of the same name that had no waiter and no + // notifier anywhere in the tree (m_controlDone is the one that carries the handshake). + std::atomic m_controlPosted{false}; std::condition_variable m_controlDone; ControlWork m_controlWork = nullptr; void* m_controlUser = nullptr; @@ -235,6 +398,9 @@ namespace MobileGL::MG_Remote::Server { Uint64 m_affinityMask = 0; std::atomic m_drained{0}; std::atomic m_parks{0}; + // Incremented by Doorbell::Wait through its `parkTally` out-parameter, once per wait + // that reached the blocking Park. Relaxed everywhere: it is a gauge. + std::atomic m_parkBlocks{0}; std::atomic m_beforeRetireHook{nullptr}; // C7 / ID-54: the (dpy, draw, read, ctx) currently bound on the apply thread. Written and diff --git a/MobileGL/MG_Remote/Transport/Doorbell.cpp b/MobileGL/MG_Remote/Transport/Doorbell.cpp index 1851f38b..8ebb36dd 100644 --- a/MobileGL/MG_Remote/Transport/Doorbell.cpp +++ b/MobileGL/MG_Remote/Transport/Doorbell.cpp @@ -28,6 +28,112 @@ namespace MobileGL::MG_Remote::Transport { + // ----------------------------------------------------------------------- + // The spin calibration (Doorbell.h's SpinItersPerUs block has the argument) + // ----------------------------------------------------------------------- + + namespace Detail { + + std::atomic g_spinItersPerUs{0}; + + namespace { + + // The probe's length. Big enough that one steady_clock read at each end is + // a rounding error against it (a few microseconds of loop), small enough + // that a thread calibrating on its first wait does not visibly stall: at + // any plausible rate this is 2-30 us. + constexpr std::uint32_t kProbeIterations = 8192; + + // The clamps. Neither is a guess about a device - they are the bounds + // outside which the READING itself cannot be true. A real iteration is an + // acquire load plus a yield/pause: below ~0.5 ns each (2000/us) no core + // retires it, and above ~250 ns each (4/us) the probe was preempted or the + // clock is lying. Clamping is what keeps a bad reading from turning a 50 us + // hint into a millisecond spin or into no spin at all. + constexpr std::uint32_t kMinSpinItersPerUs = 4; + constexpr std::uint32_t kMaxSpinItersPerUs = 2000; + + // What a platform with a clock too coarse to measure the probe gets. 200 + // iterations per microsecond is the middle of the plausible range (5 ns an + // iteration); the fallback exists so that an unmeasurable clock produces a + // spin of roughly the right order rather than none. It is NOT what a + // measurably slow probe gets - see the clamp at the bottom of this + // function, which keeps the two readings apart. + constexpr std::uint32_t kFallbackSpinItersPerUs = 200; + + } // namespace + + std::uint32_t CalibrateSpinItersPerUs() { + // The probe body is the spin loop's body with the caller's `ready()` replaced + // by an atomic load the compiler may not hoist and may not fold: an atomic + // the optimiser cannot prove constant is the only portable way to keep the + // loop from vanishing, and it is also the closest thing to what a real + // `ready()` costs. + std::atomic probe{0}; + + // TWO PASSES, AND THE FASTER ONE WINS. The first pass pays the i-cache miss, + // the branch predictor's warm-up and - on a big.LITTLE phone - the tail of a + // frequency ramp; the second is the reading. Taking the MAXIMUM rather than + // the last value is the deliberate choice: a pass that was preempted reads + // absurdly slow, and under-spinning costs a park (a syscall and a context + // switch) while over-spinning costs cycles the thread had nothing else to do + // with. Both directions stay inside the clamps. + // + // "Did any pass produce a number at all?" - which is a DIFFERENT question + // from "was the number small?", and conflating them is how a starved machine + // ends up with the largest spin in the table. See the clamp below. + bool sawUsableClock = false; + std::uint32_t best = 0; + for (int pass = 0; pass < 2; ++pass) { + const auto start = std::chrono::steady_clock::now(); + for (std::uint32_t i = 0; i < kProbeIterations; ++i) { + if (probe.load(std::memory_order_acquire) != 0) { + break; + } + CpuRelax(); + } + const auto elapsedNs = std::chrono::duration_cast( + std::chrono::steady_clock::now() - start) + .count(); + if (elapsedNs <= 0) { + continue; // clock too coarse to see this probe at all + } + sawUsableClock = true; + const std::uint64_t perUs = (static_cast(kProbeIterations) * 1000ull) / + static_cast(elapsedNs); + const std::uint32_t clamped = + perUs > kMaxSpinItersPerUs ? kMaxSpinItersPerUs : static_cast(perUs); + if (clamped > best) { + best = clamped; + } + } + // THE TWO READINGS THAT BOTH ARRIVE AS best == 0 ARE OPPOSITE, and the first + // cut of this clamp answered them the same way. `perUs` is + // kProbeIterations*1000/elapsedNs, so it truncates to zero as soon as a pass + // takes longer than 8.192 ms - that is EXACTLY the heavily preempted probe on + // an oversubscribed runner or a thermally throttled device, and handing it + // kFallbackSpinItersPerUs would install 200 iters/us (50x the floor, ~10 ms of + // busy spin per 50 us wait) on the one machine that can least afford it. A + // probe that never saw a usable clock is the opposite case: nothing was + // measured, the machine may be perfectly fast, and a mid-range guess is the + // only sane answer. So: measured-but-slow clamps UP to the floor, unmeasurable + // takes the fallback. + if (best < kMinSpinItersPerUs) { + best = sawUsableClock ? kMinSpinItersPerUs : kFallbackSpinItersPerUs; + } + // Relaxed, and a race here is not a bug: two threads that calibrate at the + // same moment both store a legitimate budget, and the loser's is as usable as + // the winner's. A once_flag would put a guard-variable load on every Wait, + // which is the cost this whole change exists to remove. + g_spinItersPerUs.store(best, std::memory_order_relaxed); + MGLOG_D("MG_Remote doorbell: spin calibrated at %u iterations per microsecond " + "(clock-free spin budget = that times MOBILEGL_IPC_SPIN_US)", + static_cast(best)); + return best; + } + + } // namespace Detail + // ----------------------------------------------------------------------- // CondVarDoorbell // ----------------------------------------------------------------------- diff --git a/MobileGL/MG_Remote/Transport/Doorbell.h b/MobileGL/MG_Remote/Transport/Doorbell.h index 92b6d581..11992c07 100644 --- a/MobileGL/MG_Remote/Transport/Doorbell.h +++ b/MobileGL/MG_Remote/Transport/Doorbell.h @@ -83,6 +83,62 @@ namespace MobileGL::MG_Remote::Transport { #endif } + // ----------------------------------------------------------------------- + // THE SPIN BUDGET IS COUNTED IN ITERATIONS, NOT IN CLOCK READS. + // + // WHY IT CHANGED (P5d round 3, package T item 3). Wait() used to spin until + // steady_clock passed `start + spinUs`, reading the clock once per 64 spins. + // On the workload device a steady_clock read is a real syscall (no vDSO), and + // simpleperf at head 56a77348 measured the GL thread paying + // steady_clock::now() 2.34 self / 5.91 inclusive plus __kernel_clock_gettime + // 2.37 and clock_gettime 1.94 - about 6.6% of the thread spent asking what + // time it is, at 900+ waits per frame, for a duration that is a HINT. The + // common case (WaitForApplied, server not yet there) paid at least two of + // those reads per wait even when the spin succeeded on its second iteration. + // + // WHAT REPLACES IT. Once per process the loop measures itself: how many + // `load(acquire)` + CpuRelax() iterations fit in a microsecond. Wait() then + // spins `itersPerUs * spinUs` iterations and reads no clock at all; only the + // park phase - which is about to block, so a syscall there is noise - computes + // a deadline. + // + // THE OVERSHOOT BOUND, STATED HONESTLY. The old loop overshot by at most one + // batch of 64 yields. This one overshoots by whatever the calibration was + // wrong by, and the calibration is a single reading taken on one core at one + // frequency: + // - a later DVFS drop or a migration to a little core stretches the spin by + // the frequency ratio (on the workload device's big/little spread that is + // bounded by roughly 3-4x, so a 50 us budget can become ~200 us); + // - a preemption inside the spin adds one scheduler slice, exactly as before; + // - the calibration probe does not run the caller's `ready()`, which is one + // or two acquire loads, so a real iteration is dearer than a probed one and + // the spin ends EARLY rather than late by that factor; + // - the clamps below cap the pathological directions: a probe that was + // preempted cannot produce a budget smaller than kMinSpinItersPerUs, and a + // probe that saw a broken clock cannot produce one larger than + // kMaxSpinItersPerUs. + // None of it is a correctness bound: `ready()` is re-tested every iteration + // and the deadline, when there is one, is still measured against the real + // clock in the park phase. The cost of being wrong is CPU burnt before a park + // (too long) or a park that a few more spins would have avoided (too short). + // ----------------------------------------------------------------------- + namespace Detail { + // 0 until the first Wait calibrates it. Plain relaxed atomic rather than a + // function-local static: a static's guard variable is a load and a branch on + // EVERY Wait, and the whole point of this block is that the common path is a + // load and a predicted branch. + extern std::atomic g_spinItersPerUs; + // Measures, clamps, stores and returns. Two threads that race here both + // store a legitimate reading and neither is wrong; there is nothing to + // serialise, so nothing does. + std::uint32_t CalibrateSpinItersPerUs(); + } // namespace Detail + + inline std::uint32_t SpinItersPerUs() { + const std::uint32_t cached = Detail::g_spinItersPerUs.load(std::memory_order_relaxed); + return cached != 0 ? cached : Detail::CalibrateSpinItersPerUs(); + } + class Doorbell { public: virtual ~Doorbell() = default; @@ -115,34 +171,95 @@ namespace MobileGL::MG_Remote::Transport { // bell that cannot die. virtual bool Dead() const { return false; } + // How many Wait() calls on this bell exhausted their spin budget and + // really blocked. THE WAIT LEDGER'S RAW READING: the pair (waits, parks) + // is what says whether the spin budget is sized for the workload, and it + // is the one number a wait counter cannot give - a wait that spun and a + // wait that blocked cost three orders of magnitude apart. + // + // ONCE PER Wait, NOT ONCE PER Park: a wait that blocks, wakes on a + // remembered Notify, re-tests false and blocks again is ONE wait that + // paid the park, and a `chunkMs == 0` Park is a poll that does not block + // at all. Both are counted out at the site below, so parks <= waits holds + // for a single waiter. + // + // PER BELL, THOUGH, NOT PER WAIT PATH. A bell belongs to an ENDPOINT and + // every waiter on that endpoint shares it: the client's self bell carries + // SessionProducer's barrier waits AND the encoder's SEG_STAGE retirement + // wait (PipeWireCodec's m_stageRetirementBell, which is the same object). + // A ledger that prints parks as the subset of ITS OWN waits must pass a + // `parkTally` to Wait rather than read this; this counter answers "parks + // on this bell", which is what a test on a private bell wants. + // + // Read from any thread; the counter is monotone and relaxed, so a reader + // may see a value one behind, which is exactly what a per-frame gauge + // wants. + std::uint64_t ParkEntries() const { return m_parkEntries.load(std::memory_order_relaxed); } + // Spin `spinUs`, then park until `ready()` or the deadline. // `parked` is the RingControl flag the peer tests before ringing. + // + // `parkTally`, when given, is incremented alongside this bell's own park + // counter - once, the first time THIS call really blocks. It is how a + // caller gets parks that are a subset of its OWN waits off a bell it + // shares with another subsystem; see ParkEntries() above. template bool Wait(std::atomic& parked, Ready&& ready, std::uint32_t spinUs, - std::uint32_t timeoutMs) { + std::uint32_t timeoutMs, std::atomic* parkTally = nullptr) { if (ready()) { return true; } - const auto start = std::chrono::steady_clock::now(); - const auto deadline = timeoutMs == kWaitForever - ? std::chrono::steady_clock::time_point::max() - : start + std::chrono::milliseconds(timeoutMs); - const auto spinEnd = start + std::chrono::microseconds(spinUs); - // The clock is read once per 64 spins, not per spin: on the - // workload device a steady_clock read is a REAL syscall (no vDSO), - // and per-iteration reads made this loop the profile's top line - // (~20% of the whole process at ~1000 waits per frame). The spin - // duration is a hint, so the overshoot - one batch of yields at - // most - costs nothing. - std::uint32_t spinBatch = 0; - while ((++spinBatch & 63) != 0 || std::chrono::steady_clock::now() < spinEnd) { + // THE SPIN PHASE READS NO CLOCK AT ALL. It used to read one per 64 + // iterations plus one before the loop - see the SpinItersPerUs block + // above for the measurement that made even that too dear, and for the + // overshoot bound the iteration budget buys instead. The duration is + // still a hint; what changed is that asking for it is free. + // + // spinUs == 0 NOW MEANS ZERO ITERATIONS, AND THAT IS A DELIBERATE CHANGE + // rather than a side effect. The old batched loop tested + // `(++spinBatch & 63) != 0 || now < spinEnd`, so a caller that asked for + // no spin still ran 63 iterations before the first clock read could end + // the loop. That was an artifact of the batch size, never a promise, and + // multiplying a budget the caller deliberately set to zero would be the + // wrong way to keep it. Two callers pass 0: PipeWireCodec's SEG_STAGE + // retirement wait (rare, already slow, and it wants the park) and any + // lane that sets MOBILEGL_IPC_SPIN_US=0 as a no-spin control, which now + // really gets no spin - which is the point of such a control. The + // short-circuit also keeps a zero-spin caller from paying the one-time + // calibration probe for a reading it will never multiply by. + const std::uint64_t budget = + spinUs == 0 ? 0ull + : static_cast(SpinItersPerUs()) * + static_cast(spinUs); + for (std::uint64_t spun = 0; spun < budget; ++spun) { if (ready()) { return true; } CpuRelax(); } + // THE PARK PHASE, AND THE ONLY PLACE A DEADLINE IS COMPUTED. Reading + // the clock here is free in the sense that matters: this side is about + // to block on a condition variable or a poll(), which costs orders of + // magnitude more than the syscall that sizes it. kWaitForever reads no + // clock even here. + // + // THE DEADLINE STARTS NOW, NOT WHEN Wait WAS ENTERED. The spin the + // caller asked for is no longer charged against its timeout, so the + // total is bounded by `timeoutMs + the spin budget` rather than by + // `timeoutMs`. spinUs is 50 by default and the timeouts that reach + // here are milliseconds (the verb barrier's 30 s, the drain's bound), + // so the drift is under a part in a thousand - and erring LONG is the + // safe direction for a deadline whose expiry is reported as a timeout. + const auto deadline = timeoutMs == kWaitForever + ? std::chrono::steady_clock::time_point::max() + : std::chrono::steady_clock::now() + + std::chrono::milliseconds(timeoutMs); + + // ONE PARK PER Wait is what the ledger counts, and this loop may Park + // many times for one Wait; see the counting site below. + bool countedPark = false; for (;;) { // Announce, FENCE, then re-test. The fence is the mechanism - // see the file header - so setting the flag itself is relaxed. @@ -163,6 +280,34 @@ namespace MobileGL::MG_Remote::Transport { std::chrono::duration_cast(deadline - now).count(); chunkMs = remaining <= 0 ? 0 : static_cast(remaining); } + // COUNTED HERE, at the one place a waiter stops spinning and + // really blocks (P5d round 3, package T item 4). Unconditional + // rather than behind the stats latch: it is one relaxed add on a + // path that is about to enter a condition variable or a poll(), + // and a counter that only exists when MOBILEGL_PIPE_STATS is set + // could not be read by a test that wants to prove the spin budget + // avoided the park. + // + // TWO THINGS ARE COUNTED OUT, because this number is published as + // the SUBSET of the waits that blocked, and a subset that can + // exceed its own denominator is worse than no number at all: + // - RE-PARKS. One Wait can traverse this loop many times: a + // CondVarDoorbell remembers a Notify delivered while nobody + // was waiting, so Park returns at once, `ready()` is still + // false (the peer had already drained what it published) and + // the wait parks again. That is one wait that paid a park, not + // two, so only the first is counted. + // - POLLS. `chunkMs == 0` is the last sub-millisecond of a + // finite timeout rounding down; Park(0) does not block, and + // counting it would charge a park per iteration of a tight + // re-test loop that never left the CPU. + if (chunkMs != 0 && !countedPark) { + countedPark = true; + m_parkEntries.fetch_add(1, std::memory_order_relaxed); + if (parkTally != nullptr) { + parkTally->fetch_add(1, std::memory_order_relaxed); + } + } Park(chunkMs); // Clearing is relaxed on purpose: a notifier that reads a // stale 1 only rings a bell nobody is waiting on, which the @@ -188,6 +333,9 @@ namespace MobileGL::MG_Remote::Transport { protected: Doorbell() = default; + + private: + std::atomic m_parkEntries{0}; }; // Rings `bell` only when the peer said it is parked. diff --git a/MobileGL/MG_Remote/Transport/EventRing.h b/MobileGL/MG_Remote/Transport/EventRing.h index 13be1ca3..644664af 100644 --- a/MobileGL/MG_Remote/Transport/EventRing.h +++ b/MobileGL/MG_Remote/Transport/EventRing.h @@ -151,7 +151,7 @@ namespace MobileGL::MG_Remote::Transport { } // Publish, THEN ring - the same order as the forward direction, and for - // the same reason (Doorbell.h:186-193: the fence only orders what + // the same reason (NotifyIfParked's PRECONDITION: the fence only orders what // precedes it, so ringing first reopens the lost-wakeup window). void PublishAndNotify(Doorbell& clientBell, std::atomic& producerParked) { m_producer.Publish(); diff --git a/MobileGL/MG_Remote/Transport/Ring.cpp b/MobileGL/MG_Remote/Transport/Ring.cpp index b0617e92..a1473b50 100644 --- a/MobileGL/MG_Remote/Transport/Ring.cpp +++ b/MobileGL/MG_Remote/Transport/Ring.cpp @@ -466,7 +466,15 @@ namespace MobileGL::MG_Remote::Transport { if (!Valid() || m_selfBell == nullptr) { return SessionWait::TimedOut; } - if (m_selfBell->Wait(m_control->producerParked, ready, m_spinUs, timeoutMs)) { + // COUNTED BEFORE THE WAIT AND ONLY WHEN ONE REALLY HAPPENS - after the two refusals + // above, which answer TimedOut without ever reaching the bell. See Waits()/Parks() in + // the header for why the pair is the reading and either number alone is not. + m_waits.fetch_add(1, std::memory_order_relaxed); + // &m_parks is the OTHER half of the pair, and it is passed rather than read off the + // bell: the bell is this endpoint's and the encoder waits on it too, so its own + // ParkEntries() is not a subset of the waits counted one line above. See Parks() in + // SessionRings.h. + if (m_selfBell->Wait(m_control->producerParked, ready, m_spinUs, timeoutMs, &m_parks)) { return SessionWait::Reached; } // Wait == false && Dead() is "the session was shut down", and it is the diff --git a/MobileGL/MG_Remote/Transport/SessionRings.h b/MobileGL/MG_Remote/Transport/SessionRings.h index a25d9f3e..b3fd1c35 100644 --- a/MobileGL/MG_Remote/Transport/SessionRings.h +++ b/MobileGL/MG_Remote/Transport/SessionRings.h @@ -267,7 +267,7 @@ namespace MobileGL::MG_Remote::Transport { enum class SessionWait : std::uint32_t { Reached = 0, // The doorbell died: the peer shut the session down. The ONLY thing that - // can un-park a waiter on kWaitForever (Doorbell.h:211-221), and the + // can un-park a waiter on kWaitForever (CondVarDoorbell::Kill), and the // reason a bounded join is possible at all. ShutDown = 1, TimedOut = 2, @@ -302,7 +302,7 @@ namespace MobileGL::MG_Remote::Transport { bool Valid() const { return m_control != nullptr && m_cmd != nullptr; } // Publish the command ring's head, record submittedSeq, THEN ring - in - // that order and never any other. Doorbell.h:186-193: the fence only + // that order and never any other. NotifyIfParked's PRECONDITION: the fence only // orders what precedes it, so ringing before publishing reopens the very // lost-wakeup window the fences exist to close. RingTest.cpp:446 pins the // call order; this is the one place production code performs it. @@ -337,6 +337,29 @@ namespace MobileGL::MG_Remote::Transport { Doorbell* SelfDoorbell() const { return m_selfBell; } std::uint32_t SpinUs() const { return m_spinUs; } + // THE WAIT LEDGER'S CLIENT HALF (P5d round 3, package T item 4), and it is a PAIR + // because one number without the other says nothing. `Waits()` is every entry into + // Park - the R-1 barrier's wait, the present throttle and the ring-space wait, which + // simpleperf at head 56a77348 measured as WaitForApplied 27.9% self on the GL thread; + // `Parks()` is how many of them ran out of spin budget and actually blocked. A high + // wait count with near-zero parks is a thread spinning on a server that is nearly + // there; a high park count is a server that is not. The two readings are what tell + // those apart, and the split's whole frame budget turns on which one it is. + // + // Both counters are OURS, and the park one deliberately is not the bell's own + // ParkEntries(). Doorbell::Wait is the only code that knows whether its spin budget + // ran out, so it does the counting - but a bell belongs to an ENDPOINT, not to a wait + // path, and this one is shared: ClientSession hands the very same object to the + // encoder as its SEG_STAGE retirement bell (ClientSession::Start -> + // PipeWireCodec::SetStageRetirementDoorbell), whose waits never pass through + // SessionProducer::Park and so never reach m_waits. Reading ParkEntries() here would + // fold that second subsystem's parks into a number this header, PipeStats and the + // summary line all describe as the subset of OUR waits - and with the retirement + // wait's spinUs of 0 it parks every time, so the subset could print larger than the + // set it is a subset of. Wait's `parkTally` out-parameter exists for exactly this. + std::uint64_t Waits() const { return m_waits.load(std::memory_order_relaxed); } + std::uint64_t Parks() const { return m_parks.load(std::memory_order_relaxed); } + private: template SessionWait Park(Ready&& ready, std::uint32_t timeoutMs); @@ -347,6 +370,13 @@ namespace MobileGL::MG_Remote::Transport { Doorbell* m_selfBell = nullptr; std::uint32_t m_spinUs = kDefaultSpinUs; std::uint64_t m_lastPublishedSeq = 0; + // Relaxed atomics, not plain integers: the GL thread owns this producer, but + // ClientSession::Stop's bounded drain waits through it from whichever thread is tearing + // the session down, and a diagnostic is not worth a data race. Relaxed costs the same + // as a plain add on both architectures the split runs on. m_parks is incremented by + // Doorbell::Wait through the `parkTally` pointer, once per wait that really blocked. + std::atomic m_waits{0}; + std::atomic m_parks{0}; }; // ----------------------------------------------------------------------- @@ -449,7 +479,7 @@ namespace MobileGL::MG_Remote::Transport { // Ring the client's bell, but only when it said it is parked: a store to // a shared cache line otherwise burns a big core for a whole frame on a - // phone (Doorbell.h:13-22). + // phone (Doorbell.h's file header, the bidirectional argument). void NotifyClient(); std::uint64_t AppliedSeq() const { return m_appliedSeq; } diff --git a/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp b/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp index 512ce94d..f9446fa1 100644 --- a/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp +++ b/MobileGL/MG_State/GLState/BufferState/BufferObject.cpp @@ -39,9 +39,12 @@ namespace MobileGL::MG_State::GLState { // the same shape as Fatal{PipeLegacyMemosDisabled} - so a cleared subsystem bit 7 no // longer leaves the arm silently readable. Client-thread callers (the GL thread's // own state) are unaffected. - void RefuseLegacyBufferArmFromApplyThread(const char* accessor) { - if (!MG_Remote::Client::PersistentMapTracker::PushIsArmed()) return; - if (!MG_Remote::Client::PersistentMapTracker::OnServerRole()) return; + // The refusal's BODY, split out from its two probes (P5d round 3, package D). A caller + // that has already asked "am I the server role" for its own reasons - + // SyncPersistentMappedRange does, one line later - calls this and pays for + // ServerLoop::OnApplyThread() once instead of twice. It is a Fatal, so it never + // returns; the probes live in RefuseLegacyBufferArmFromApplyThread below. + [[noreturn]] void FatalLegacyBufferArmFromApplyThread(const char* accessor) { MGLOG_F("MGPipe: Fatal{RoleViolation, \"buffer-legacy-arm\"} - the apply thread " "called BufferObject::%s on a frontend object. With an active transport " "the server reads the applier's resource record and its own staged shadow; " @@ -50,6 +53,17 @@ namespace MobileGL::MG_State::GLState { accessor); std::abort(); } + + // The accessors' own guard: two loads and two branches on the client thread, which is + // where MappedData/IsMapped/GetChangeSerial/HasDefinedContent are called from per draw + // (IsLivePersistentMap reads two of them for every live map). PushIsArmed() is a read + // of MG_Config::Transport and OnServerRole() is now one relaxed load of the apply + // thread's key (ServerLoop.h), so the monolith answer costs a load each. + void RefuseLegacyBufferArmFromApplyThread(const char* accessor) { + if (!MG_Remote::Client::PersistentMapTracker::PushIsArmed()) return; + if (!MG_Remote::Client::PersistentMapTracker::OnServerRole()) return; + FatalLegacyBufferArmFromApplyThread(accessor); + } #endif } @@ -400,14 +414,27 @@ namespace MobileGL::MG_State::GLState { void BufferObject::SyncPersistentMappedRange() { #if MOBILEGL_BUILD_DISAGGREGATED - // P5c (hd): the named refusal comes FIRST - the silent return below used to let a + // P5c (hd): the named refusal comes FIRST - a silent return here used to let a // server-side caller slip through with one MGLOG_D's worth of evidence (B3). - RefuseLegacyBufferArmFromApplyThread("SyncPersistentMappedRange"); + // + // ONE ROLE PROBE, NOT TWO (P5d round 3, package D). This used to call + // RefuseLegacyBufferArmFromApplyThread and then re-ask PushIsArmed()/OnServerRole() + // for the early-out below - the same two questions, answered twice on the same line of + // execution, on a function the draw path reaches per mapped buffer. The refusal's + // condition and the early-out's condition were already IDENTICAL, which is why the + // early-out's `return` was unreachable: with the push armed, the apply thread aborts + // in the refusal and never gets there. Folding them keeps the refusal first and drops + // the duplicate probe rather than the guard. + // // Split's client pre-verb hook publishes these bytes. The retained backend sync // sites must do nothing on the apply thread: re-entering this producer there would - // overwrite the server shadow through the monolith adapter without crossing the wire. + // overwrite the server shadow through the monolith adapter without crossing the wire - + // and "do nothing" is spelled Fatal, not `return`, because a server-side caller here + // is a role violation and not a shape the design tolerates. if (MG_Remote::Client::PersistentMapTracker::PushIsArmed()) { - if (MG_Remote::Client::PersistentMapTracker::OnServerRole()) return; + if (MG_Remote::Client::PersistentMapTracker::OnServerRole()) { + FatalLegacyBufferArmFromApplyThread("SyncPersistentMappedRange"); + } MG_Remote::Client::PersistentMapTracker::Instance().PushBlocksFor(*this); return; } @@ -967,6 +994,16 @@ namespace MobileGL::MG_State::GLState { return m_resource.Bytes(); } +#if MOBILEGL_BUILD_DISAGGREGATED + SizeT BufferObject::ShadowAllocationBytes() const { + // The extent of the very pointer MappedData() hands out, read by the same caller + // (the tracker's registration) on the same thread: the same rule-E surface, the + // same refusal. + RefuseLegacyBufferArmFromApplyThread("ShadowAllocationBytes"); + return m_resource.ShadowAllocationBytes(); + } +#endif + Bool BufferObject::IsBackendPersistentMapped() const { return m_resource.IsGpuResident(); } diff --git a/MobileGL/MG_State/GLState/BufferState/BufferObject.h b/MobileGL/MG_State/GLState/BufferState/BufferObject.h index 9d853a2c..815663bc 100644 --- a/MobileGL/MG_State/GLState/BufferState/BufferObject.h +++ b/MobileGL/MG_State/GLState/BufferState/BufferObject.h @@ -228,6 +228,16 @@ namespace MobileGL { // object can express that. Split-only so the pull build's layout and inlining do // not move (G1). Bool HasOutstandingGpuWrite() const { return m_gpuWritePending; } + + // The shadow allocation's own extent in bytes - page-aligned base, page-granular + // size, every byte this shadow's and nobody else's (PipeResource.h, + // ShadowAllocationBytesFor). Read by the persistent-map tracker at registration + // so it can protect the mapped range's containing pages OUTWARD and hash no + // edges; zero for an adopted store, which the tracker never registers. Split-only + // for G1's reason, like the rest of this block, and out of line because it is a + // layer-1 read of client memory like MappedData (CONTRACT-P5C rule E) and carries + // the same apply-thread refusal. + SizeT ShadowAllocationBytes() const; #endif Bool IsMapped() const; diff --git a/MobileGL/MG_State/GLState/BufferState/PipeResource.h b/MobileGL/MG_State/GLState/BufferState/PipeResource.h index 3bd6f1a0..8fb16d6c 100644 --- a/MobileGL/MG_State/GLState/BufferState/PipeResource.h +++ b/MobileGL/MG_State/GLState/BufferState/PipeResource.h @@ -28,17 +28,39 @@ namespace MobileGL::MG_State::GLState { inline constexpr SizeT MIN_MAP_BUFFER_ALIGNMENT = 64; // The shadow's ALLOCATION alignment is a full page, not the advertised 64: a - // page-aligned shadow owns every byte of every page it occupies, which is what lets - // the persistent-map mprotect tracker write-protect the mapped interior without ever - // covering a neighbour allocation's byte (a foreign thread with signals blocked - // faulting on a shared page is a process kill). GL_MIN_MAP_BUFFER_ALIGNMENT keeps - // answering 64 - over-aligning an allocation is invisible to the application, and a - // glMapBufferRange base that is page-aligned is 64-aligned. The cost is allocator - // rounding on small buffers, paid so the tracker never has to make the unsafe choice. + // page-aligned shadow (page-granular in SIZE as well, in a split build - see + // ShadowAllocationBytesFor) owns every byte of every page it occupies, which is what + // lets the persistent-map mprotect tracker write-protect the mapped range's containing + // pages without ever covering a neighbour allocation's byte (a foreign thread with + // signals blocked faulting on a shared page is a process kill). + // GL_MIN_MAP_BUFFER_ALIGNMENT keeps answering 64 - over-aligning an allocation is + // invisible to the application, and a glMapBufferRange base that is page-aligned is + // 64-aligned. The cost is allocator rounding on small buffers, paid so the tracker + // never has to make the unsafe choice. inline constexpr SizeT SHADOW_ALLOCATION_ALIGNMENT = 4096; - // Allocator that gives every allocation SHADOW_ALLOCATION_ALIGNMENT. Deliberately - // minimal: the + // The bytes one shadow allocation actually asks the heap for, from the count std::vector + // asked the allocator for. In a split build the SIZE is rounded up to a whole number of + // SHADOW_ALLOCATION_ALIGNMENT pages, not just the base: alignment alone puts the first + // byte on a page boundary but lets the LAST page end mid-way, and whatever the heap + // packs after it shares that page. The persistent-map tracker then had to align its + // protected range inward and hash the partial-page edges on every draw (8.5% of the + // client thread at VD12 in XXH3, on nearly every buffer - almost no mapped end is + // page-aligned). With a page-granular size every byte of [base, base + this) is this + // allocation's own, so the tracker can protect the mapped range's containing pages + // outward and there are no edges to hash. ONE function for the allocator and for + // PipeResource::ShadowAllocationBytes(), because the extent the tracker widens into may + // never exceed what was allocated. The pull build keeps the exact count and does not + // see this function at all: it has no tracker to spend the rounding on, and G1 holds + // its symbol set and .text byte-identical only if allocate()'s own text does not move. +#if MOBILEGL_BUILD_DISAGGREGATED + inline constexpr SizeT ShadowAllocationBytesFor(SizeT count) { + return (count + SHADOW_ALLOCATION_ALIGNMENT - 1) & ~(SHADOW_ALLOCATION_ALIGNMENT - 1); + } +#endif + + // Allocator that gives every allocation SHADOW_ALLOCATION_ALIGNMENT (and, in a split + // build, a page-granular size - see ShadowAllocationBytesFor). Deliberately minimal: the // vectors it backs hold raw bytes and are only ever sized, so allocate/deallocate plus the // rebinding and equality boilerplate std::vector requires is the whole interface. template @@ -51,8 +73,13 @@ namespace MobileGL::MG_State::GLState { T* allocate(SizeT count) { if (count == 0) return nullptr; +#if MOBILEGL_BUILD_DISAGGREGATED + return static_cast(::operator new(ShadowAllocationBytesFor(count * sizeof(T)), + std::align_val_t{SHADOW_ALLOCATION_ALIGNMENT})); +#else return static_cast( ::operator new(count * sizeof(T), std::align_val_t{SHADOW_ALLOCATION_ALIGNMENT})); +#endif } void deallocate(T* pointer, SizeT) noexcept { ::operator delete(pointer, std::align_val_t{SHADOW_ALLOCATION_ALIGNMENT}); @@ -111,14 +138,42 @@ namespace MobileGL::MG_State::GLState { // glBufferStorage before any persistent map). Mirrors the previous // power-of-two reserve + exact resize of the old m_dataPtr. void ResizeShadow(SizeT size) { - m_shadow->reserve(std::bit_ceil(size == 0 ? SizeT{1} : size)); + const SizeT reserved = std::bit_ceil(size == 0 ? SizeT{1} : size); + m_shadow->reserve(reserved); m_shadow->resize(size); +#if MOBILEGL_BUILD_DISAGGREGATED + // RECORDED HERE, NOT INFERRED FROM capacity(). `reserved` is the count this + // call asked the allocator for, so ShadowAllocationBytesFor(reserved) is the + // block the allocator handed back whenever this reserve reallocated - and when + // it did not (the vector already held at least that much), the block behind + // Bytes() is an earlier, LARGER reserve's, so the value recorded here is at + // most the true extent either way. Reading capacity() back instead would tie + // the ownership claim to a library's promise that capacity() never exceeds + // the count it allocated - true of libc++, libstdc++ and MSVC today, pinned by + // nothing - and an over-report there would protect a page this allocation + // does not own, the very process-killer the extent exists to keep out. An + // under-report costs nothing: the tracker still needs rangeEnd <= extent, and + // size <= reserved always. + m_shadowExtent = ShadowAllocationBytesFor(reserved); +#endif } // Direct shadow access, used only by the backend's upload-from-shadow path, // which never runs for a GPU-resident (persistent) buffer. MapAlignedData& Shadow() { return *m_shadow; } const MapAlignedData& Shadow() const { return *m_shadow; } +#if MOBILEGL_BUILD_DISAGGREGATED + // The extent of the shadow's heap allocation in bytes, as ResizeShadow asked for + // it: every byte of [Bytes(), Bytes() + this) belongs to this shadow and to + // nothing else, and the extent is a whole number of SHADOW_ALLOCATION_ALIGNMENT + // pages. This is what lets the persistent-map tracker protect the mapped range's + // containing pages OUTWARD without ever covering a byte it does not own (a + // foreign thread with signals blocked faulting on a shared page is a process + // kill). Zero for an adopted (GPU-resident) store, whose shadow was released, and + // for a shadow that was never sized; the tracker excludes both. + SizeT ShadowAllocationBytes() const { return m_gpuMapped != nullptr ? 0 : m_shadowExtent; } +#endif + // Transition to persistent GPU residency: adopt the backend's coherent // mapped base as the source of truth and drop the CPU shadow. The caller // must have already seeded the GPU memory from the shadow (via the backend @@ -127,6 +182,10 @@ namespace MobileGL::MG_State::GLState { m_gpuMapped = mappedBase; m_shadow->clear(); m_shadow->shrink_to_fit(); +#if MOBILEGL_BUILD_DISAGGREGATED + // The block is gone with the shrink; the next ResizeShadow records the next one. + m_shadowExtent = 0; +#endif } // Give the adoption back: the bytes resolve against the shadow again (which @@ -150,5 +209,9 @@ namespace MobileGL::MG_State::GLState { SharedPtr m_shadow = MakeShared(); void* m_gpuMapped = nullptr; SharedPtr m_backend; +#if MOBILEGL_BUILD_DISAGGREGATED + // See ShadowAllocationBytes. Split-only so the pull build's layout does not move (G1). + SizeT m_shadowExtent = 0; +#endif }; } // namespace MobileGL::MG_State::GLState diff --git a/MobileGL/MG_State/GLState/Core.h b/MobileGL/MG_State/GLState/Core.h index db2d9519..6ed617f1 100644 --- a/MobileGL/MG_State/GLState/Core.h +++ b/MobileGL/MG_State/GLState/Core.h @@ -140,6 +140,14 @@ namespace MobileGL { m_textureState.NoteUnitTouched(unit, bindingChanged); } Int GetMaxTouchedTextureUnit() const { return m_textureState.GetMaxTouchedUnit(); } +#if MOBILEGL_PIPE_PUSH + // The IMAGE-unit high-water mark, which is a different namespace from the + // texture-unit one above - see TextureState::NoteImageUnitTouched for why it is a + // second mark and why it only grows. Push builds only, so the pull build's + // GLContext is untouched (G1). + void NoteImageUnitTouched(Int unit) { m_textureState.NoteImageUnitTouched(unit); } + Int GetMaxTouchedImageUnit() const { return m_textureState.GetMaxTouchedImageUnit(); } +#endif // Monotonic counter bumped whenever a texture bind/unbind/delete changes which // texture is bound at a unit; lets a backend skip re-resolving an unchanged // per-draw sampled-texture set. diff --git a/MobileGL/MG_State/GLState/TextureState/MipmapStorage.cpp b/MobileGL/MG_State/GLState/TextureState/MipmapStorage.cpp index 921fb312..ff9f5351 100644 --- a/MobileGL/MG_State/GLState/TextureState/MipmapStorage.cpp +++ b/MobileGL/MG_State/GLState/TextureState/MipmapStorage.cpp @@ -41,7 +41,7 @@ namespace MobileGL { if (!MG_Remote::Server::ServerLoop::OnApplyThread()) return; // The named exemption (MipmapStorage.h): Magma's texture sync is tx's // declared leftover and retires with P7. - if (MGPipeTextureLegacyArmScope::Active()) return; + if (MGPipeTextureLegacyArmScope::ActiveOnApplyThread()) return; MGLOG_F("MGPipe: Fatal{RoleViolation, \"texture-legacy-arm\"} - the apply thread " "called TextureObjectMipmap::%s on a frontend object. With an active " "transport the server reads the staged-texture store and the resource " @@ -70,18 +70,28 @@ namespace MobileGL { #if MOBILEGL_BUILD_DISAGGREGATED namespace { - thread_local Uint32 g_textureLegacyArmScopeDepth = 0; + // NOT thread_local (P5d round 3, package D), for hd's reason in + // MG_Impl/Pipe/SlotAllocator.cpp: RefuseLegacyTextureArmFromApplyThread above + // is this counter's ONLY reader in the tree and it returns before it looks + // unless the transport is active and ServerLoop::OnApplyThread() is true, so + // the apply thread is the only thread whose depth could change an answer. The + // scope below therefore counts only there, which makes the apply thread the + // single writer and the single reader - no race to defend, and no + // __emutls_get_address call per access (7.4% of the apply thread, 7.7% and the + // top symbol on the monolith's GL thread). + Uint32 g_textureLegacyArmScopeDepth = 0; } - MGPipeTextureLegacyArmScope::MGPipeTextureLegacyArmScope() { - ++g_textureLegacyArmScopeDepth; + MGPipeTextureLegacyArmScope::MGPipeTextureLegacyArmScope() + : m_counted(MG_Remote::Server::ServerLoop::OnApplyThread()) { + if (m_counted) ++g_textureLegacyArmScopeDepth; } MGPipeTextureLegacyArmScope::~MGPipeTextureLegacyArmScope() { - --g_textureLegacyArmScopeDepth; + if (m_counted) --g_textureLegacyArmScopeDepth; } - Bool MGPipeTextureLegacyArmScope::Active() { + Bool MGPipeTextureLegacyArmScope::ActiveOnApplyThread() { return g_textureLegacyArmScopeDepth != 0; } #endif diff --git a/MobileGL/MG_State/GLState/TextureState/MipmapStorage.h b/MobileGL/MG_State/GLState/TextureState/MipmapStorage.h index 3762c8c8..0e9c7a59 100644 --- a/MobileGL/MG_State/GLState/TextureState/MipmapStorage.h +++ b/MobileGL/MG_State/GLState/TextureState/MipmapStorage.h @@ -50,13 +50,27 @@ namespace MobileGL { // held, and greppable as exactly this name; retiring the scope is P7's server-side // Magma texture sync. Every OTHER apply-thread touch of a guarded surface is still // Fatal{RoleViolation, "texture-legacy-arm"}. + // + // The depth is counted ONLY on the apply thread (P5d round 3, package D), exactly + // as hd's two scopes in MG_Impl/Pipe/SlotAllocator.h are: the guard above is the + // counter's only reader and it returns before it looks unless + // ServerLoop::OnApplyThread() is true, so a depth kept on any other thread could + // never change an answer. m_counted remembers the constructor's decision so the + // destructor undoes exactly what the constructor did. The query is spelled + // ActiveOnApplyThread() rather than Active() because the precondition belongs in + // the name: off the apply thread it now answers false however many scopes are + // open, and a future second reader must be made to see that rather than trust a + // bare Active(). class MGPipeTextureLegacyArmScope { public: MGPipeTextureLegacyArmScope(); ~MGPipeTextureLegacyArmScope(); MGPipeTextureLegacyArmScope(const MGPipeTextureLegacyArmScope&) = delete; MGPipeTextureLegacyArmScope& operator=(const MGPipeTextureLegacyArmScope&) = delete; - static Bool Active(); + static Bool ActiveOnApplyThread(); + + private: + Bool m_counted; }; #endif diff --git a/MobileGL/MG_State/GLState/TextureState/TextureState.h b/MobileGL/MG_State/GLState/TextureState/TextureState.h index 0e416a2d..1d7dc7e9 100644 --- a/MobileGL/MG_State/GLState/TextureState/TextureState.h +++ b/MobileGL/MG_State/GLState/TextureState/TextureState.h @@ -108,6 +108,39 @@ namespace MobileGL::MG_State::GLState { } } Int GetMaxTouchedUnit() const { return m_maxTouchedUnit; } +#if MOBILEGL_PIPE_PUSH + // High-water mark of IMAGE units ever bound by glBindImageTexture, and it is a SECOND + // mark rather than a widening of the one above because the two index different + // namespaces: GL 4.6 core 8.22's image units are their own array, and folding them + // together would make a bind of image unit 7 push every per-draw TEXTURE-unit scan out + // to eight slots for nothing - and a bind of the last advertised image unit push it out + // to MAX_TEXTURE_IMAGE_UNITS (191), which is the whole array. + // + // WHAT IT IS FOR. The split client's per-draw GPU-write set swept ALL + // MAX_TEXTURE_IMAGE_UNITS image units looking for a writable image-BUFFER texture + // (MG_Remote/Client/GpuWritePending.cpp), because the frontend had no such mark - the + // 2026-09-17 inproc profile measured that sweep at 2.15% self / 3.59% inclusive of the + // GL thread in an application (Minecraft) that never binds an image at all. With the + // mark that application's sweep is one compare against -1. + // + // IT ONLY EVER GROWS, which is NoteUnitTouched's rule one notch stronger and is the + // whole safety argument: an unbind - glBindImageTexture(unit, 0, ...), or the + // delete-unbind in MarkTextureObjectForDeletion - leaves the mark where it was, so a + // scan bounded by it is an OVER-approximation. Over-approximating a GPU-write set costs + // a readback the narrowing channel removes; under-approximating it reads a stale buffer + // and says nothing, which is the one direction that set may not fail in. + // + // PUSH BUILDS ONLY, so the pull build's TextureState keeps its layout and its .text byte + // for byte (G1): the only reader is a push-build path and the only writer - + // MG_Impl/GLImpl/Texture/GL_Texture.cpp's BindImageTexture - is guarded the same way. + void NoteImageUnitTouched(Int unit) { + if (unit > m_maxTouchedImageUnit && unit < MAX_TEXTURE_IMAGE_UNITS) { + m_maxTouchedImageUnit = unit; + } + } + // -1 until an image unit is bound for the first time, i.e. "there is nothing to sweep". + Int GetMaxTouchedImageUnit() const { return m_maxTouchedImageUnit; } +#endif Uint64 GetTextureBindGeneration() const { return m_textureBindGeneration; } // Both counters below are pushed PipeInputs fields AND are moved by writes the // backends make into frontend objects during their own verb - a synthesised fallback @@ -164,6 +197,8 @@ namespace MobileGL::MG_State::GLState { #if MOBILEGL_PIPE_PUSH Uint64 m_anyTextureContentGeneration = 0; Uint64 m_anyTextureParamsGeneration = 0; + // See NoteImageUnitTouched: the image-unit twin of m_maxTouchedUnit, push builds only. + Int m_maxTouchedImageUnit = -1; #endif static Uint64 AllocateContextId(); diff --git a/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp b/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp index d60f7aa1..29925120 100644 --- a/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp +++ b/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp @@ -171,6 +171,11 @@ TEST_F(SplitBufferSet, Row2OnlyAWritableImageBufferTextureCounts) { } // Row 2's WALK, through the image unit the context actually holds. +// +// THE FIXTURE BINDS THE UNIT BY HAND, so it has to move the image-unit high-water mark by hand +// too (P5d round 3, package C): the walk is bounded by that mark now, and a case that writes the +// binding straight into the slot never went through glBindImageTexture, which is what feeds it. +// The mark's own producer has its own case - ImageEmit.TheBindFeedsTheImageUnitHighWaterMark. TEST_F(SplitBufferSet, Row2AWritableImageBufferTextureIsMarkedByADraw) { auto texture = MakeShared(8u); auto backing = MakeBuffer(14u, 128); @@ -178,6 +183,7 @@ TEST_F(SplitBufferSet, Row2AWritableImageBufferTextureIsMarkedByADraw) { auto& binding = MG_State::pGLContext->GetImageTextureBinding(0); binding.Texture = texture; binding.Access = GL_WRITE_ONLY; + MG_State::pGLContext->NoteImageUnitTouched(0); MG_Remote::Client::MarkGpuWritesForDraw(); @@ -186,6 +192,41 @@ TEST_F(SplitBufferSet, Row2AWritableImageBufferTextureIsMarkedByADraw) { binding = MG_State::GLState::ImageTextureBinding{}; } +// Row 2's BOUND, and it is the half an optimisation is allowed to get wrong in exactly one +// direction (P5d round 3, package C). The sweep used to be MAX_TEXTURE_IMAGE_UNITS wide on every +// draw - 2.15% self of the GL thread on the Minecraft inproc profile, in a workload with no image +// binding anywhere - and is now bounded by the frontend's image-unit high-water mark. +// +// THE CASE PINS BOTH ENDS OF THAT BOUND. Below the mark a writable image-buffer texture is still +// found; a mark that has never moved means the walk does not run at all, which is the whole +// saving and is what makes "the mark only ever grows" load-bearing rather than decorative. Revert +// the bound to `unit < MAX_TEXTURE_IMAGE_UNITS` and the second half goes red; drop +// NoteImageUnitTouched from glBindImageTexture and ImageEmit.TheBindFeedsTheImageUnitHighWaterMark +// goes red instead - the two together are what say the narrowing is safe. +TEST_F(SplitBufferSet, Row2TheSweepIsBoundedByTheImageUnitHighWaterMark) { + ASSERT_EQ(MG_State::pGLContext->GetMaxTouchedImageUnit(), -1) + << "a fresh context has bound no image unit, so the sweep has nothing to walk"; + + auto texture = MakeShared(9u); + auto backing = MakeBuffer(18u, 128); + texture->GetBufferBindingSlot().Bind(backing); + constexpr Int kUnit = 5; + auto& binding = MG_State::pGLContext->GetImageTextureBinding(kUnit); + binding.Texture = texture; + binding.Access = GL_READ_WRITE; + + MG_Remote::Client::MarkGpuWritesForDraw(); + EXPECT_EQ(MG_Remote::Client::ProducerMarkCount(GpuWriteProducer::WritableImageBufferTexture), 0u) + << "the mark has never moved, so the sweep must not walk a single unit"; + + MG_State::pGLContext->NoteImageUnitTouched(kUnit); + MG_Remote::Client::MarkGpuWritesForDraw(); + EXPECT_EQ(MG_Remote::Client::ProducerMarkCount(GpuWriteProducer::WritableImageBufferTexture), 1u) + << "a unit at the mark is inside the walk"; + + binding = MG_State::GLState::ImageTextureBinding{}; +} + // Row 3 - VulkanRenderer.cpp:11618. With no capture active there is nothing to mark, and that // gate is the half worth pinning: a mark taken with no active capture would mark whatever the // binding points happened to hold from a previous one. @@ -506,9 +547,195 @@ TEST_F(SplitBufferSet, OnlyAdoptTierTwoIsImplemented) { MG_Config::Ipc.AdoptTier = 2; } +// ===================================================================================== +// THE MPROTECT TRACKER'S OUTWARD ALIGNMENT (P5d round 3). A split build's shadow is +// page-aligned AND page-granular (PipeResource.h), so every page a mapped range touches is +// the shadow's own and the tracker protects the range's containing pages whole: no +// unprotected edge bytes, no per-draw XXH3 (8.5% of the client thread at VD12 before this). +// White-box on purpose - which pages a map protected is not observable through any push +// count, and the epoch (the one thing a protected page moves) is the tracker's own. +// ===================================================================================== + +// A sub-page map used to register with an EMPTY interior and two hashed edges; now it is +// one protected page, and a write through the pointer FAULTS - which is the whole point. +TEST_F(SplitBufferSet, ASubPageMapIsOneProtectedPageAndAWriteThroughItFaults) { + if (!PersistentMapTracker::MprotectArmAvailableForTest()) { + GTEST_SKIP() << "no mprotect arm on this host (handler not installable, or the kernel " + "page is not the tracker's 4 KB); the hash arm serves every map here"; + } + constexpr SizeT kSize = 100; // well under a page, and not a multiple of anything + auto buffer = MakeBuffer(26u, kSize); + ASSERT_EQ(buffer->ShadowAllocationBytes() % 4096u, 0u) + << "the split build's shadow allocator rounds the SIZE to whole pages, not just the base"; + ASSERT_GE(buffer->ShadowAllocationBytes(), kSize); + auto* mapped = static_cast(buffer->AcquireMemoryRange( + Range1D{0, kSize}, BufferMappingAccessBit::Write | BufferMappingAccessBit::Persistent)); + ASSERT_NE(mapped, nullptr); + ASSERT_EQ(reinterpret_cast(mapped) % 4096u, 0u) << "the shadow base is page-aligned"; + + const auto* slot = PersistentMapTracker::TrackedSlotForTest(*buffer); + ASSERT_NE(slot, nullptr) << "a page-aligned, page-granular shadow registers on the mprotect arm"; + EXPECT_EQ(slot->pageCount, 1u) + << "a sub-page map is ONE protected page under outward alignment, not an empty interior"; + EXPECT_FALSE(slot->hasEdges) << "outward alignment leaves no unprotected edge bytes to hash"; + const uintptr_t base = slot->base.load(); + const uintptr_t end = slot->end.load(); + EXPECT_EQ(base, reinterpret_cast(mapped)); + EXPECT_EQ(end, base + 4096u); + EXPECT_LE(end, reinterpret_cast(mapped) + buffer->ShadowAllocationBytes()) + << "the protected span never runs past the shadow's own allocation"; + + // Fresh state ships everything once, then re-arms; with nothing written a second push + // ships nothing (and, with no edges, hashes nothing). + MG_Remote::Client::PushPersistentMapsBeforeVerb(); + EXPECT_EQ(PersistentMapTracker::Instance().BlocksPushed(), 1u); + EXPECT_EQ(PersistentMapTracker::Instance().BytesPushed(), static_cast(kSize)); + MG_Remote::Client::PushPersistentMapsBeforeVerb(); + EXPECT_EQ(PersistentMapTracker::Instance().BlocksPushed(), 1u); + + // The one page IS protected: a write through the application's pointer faults into the + // handler (the epoch moves), and the next push ships the page's block clamped to the + // mapped range. Under the old empty-interior registration nothing faulted here. + const Uint64 epochBefore = PersistentMapTracker::FaultEpochForTest(); + mapped[kSize - 1] = 0x5A; + EXPECT_GT(PersistentMapTracker::FaultEpochForTest(), epochBefore) + << "the sub-page map's page was not protected: the write did not fault"; + MG_Remote::Client::PushPersistentMapsBeforeVerb(); + EXPECT_EQ(PersistentMapTracker::Instance().BlocksPushed(), 2u); + EXPECT_EQ(PersistentMapTracker::Instance().BytesPushed(), static_cast(2 * kSize)) + << "the push is clamped to [begin, end) even though the protected page is wider"; + + // The unmap retires the slot and restores the page writable: an API write into the + // shadow afterwards must not fault (an unowned fault chains to SIG_DFL and dies). + buffer->ReleaseMemory(false); + EXPECT_EQ(PersistentMapTracker::TrackedSlotForTest(*buffer), nullptr); + const Uint8 byte = 0x3C; + buffer->UploadSubData(DataPtr{const_cast(&byte), 1}, 0); +} + +// The inward fallback is kept for any shadow that fails the page-granular test, and the +// only way to reach it on this build - whose allocator never hands out such a shadow - is +// to register a range by hand with a base or an extent the tracker cannot vouch for. +TEST_F(SplitBufferSet, AShadowThatFailsThePageGranularTestKeepsTheInwardAlignmentAndItsEdges) { + if (!PersistentMapTracker::MprotectArmAvailableForTest()) { + GTEST_SKIP() << "no mprotect arm on this host (handler not installable, or the kernel " + "page is not the tracker's 4 KB); the hash arm serves every map here"; + } + // Three whole pages from the shadow allocator itself: page-aligned, page-granular. + MG_State::GLState::MapAlignedData store(3u * 4096u); + const auto storeBase = reinterpret_cast(store.data()); + ASSERT_EQ(storeBase % 4096u, 0u); + const SizeT extent = MG_State::GLState::ShadowAllocationBytesFor(store.capacity()); + ASSERT_EQ(extent, 3u * 4096u); + // Never a real buffer's id: BufferObject lifetime ids count up from 1. + constexpr Uint64 kId = ~0ull - 7u; + + // An UNALIGNED base (the shadow's own storage, offset by 64) with a range of two pages: + // only the one page fully inside [base+64, base+64+8192) is protected, both ends are + // edges - the pre-round-3 shape, and still the safe answer for a base like this. + const auto* slot = PersistentMapTracker::TrackForTest(kId, store.data() + 64, 0, 2u * 4096u, extent); + ASSERT_NE(slot, nullptr); + EXPECT_EQ(slot->pageCount, 1u); + EXPECT_TRUE(slot->hasEdges); + EXPECT_EQ(slot->base.load(), storeBase + 4096u); + EXPECT_EQ(slot->end.load(), storeBase + 2u * 4096u); + PersistentMapTracker::UntrackForTest(kId); + + // An aligned base with an extent the caller could NOT vouch for (0): inward as well. + slot = PersistentMapTracker::TrackForTest(kId, store.data(), 64, 2u * 4096u + 64, 0); + ASSERT_NE(slot, nullptr); + EXPECT_EQ(slot->pageCount, 1u); + EXPECT_TRUE(slot->hasEdges); + PersistentMapTracker::UntrackForTest(kId); + + // A range that runs PAST the extent it was told about is never widened into it. + slot = PersistentMapTracker::TrackForTest(kId, store.data(), 64, 2u * 4096u + 64, 2u * 4096u); + ASSERT_NE(slot, nullptr); + EXPECT_EQ(slot->pageCount, 1u); + EXPECT_TRUE(slot->hasEdges); + PersistentMapTracker::UntrackForTest(kId); + + // And the same range on the same aligned base with the TRUE extent widens outward to + // its three containing pages - exactly the extent, so the clamp sits on its boundary. + slot = PersistentMapTracker::TrackForTest(kId, store.data(), 64, 2u * 4096u + 64, extent); + ASSERT_NE(slot, nullptr); + EXPECT_EQ(slot->pageCount, 3u); + EXPECT_FALSE(slot->hasEdges); + EXPECT_EQ(slot->base.load(), storeBase); + EXPECT_EQ(slot->end.load(), storeBase + extent); + PersistentMapTracker::UntrackForTest(kId); + // Restored writable before the store goes out of scope: freeing a still-protected page + // would fault inside the allocator with no owner to answer. + store[0] = 1; + store[extent - 1] = 1; +} + +// THE DRAIN THE EPOCH SKIP RESTS ON. A fault marks a page and moves the epoch; the next +// draw's walk consumes the epoch. If that walk pushed only the buffers the draw bound, a +// faulted map the draw did NOT bind would be left marked, writable and unable to move the +// epoch ever again - and the next draw that does bind it would skip, and the server would +// draw its pre-write bytes. Before round 3 the skip's edge service drained every member +// with edges by accident (nearly all of them); with page-granular shadows nothing has +// edges, so the walk drains every marked member on purpose. Two maps, one bound: the write +// to the unbound one must ship before the draw that binds it. Driven through +// PushDrawConsumers itself - PushPersistentMapsBeforeVerb never reaches the skip. +TEST_F(SplitBufferSet, AFaultedMapTheDrawDidNotBindStillShipsBeforeTheDrawThatDoes) { + if (!PersistentMapTracker::MprotectArmAvailableForTest()) { + GTEST_SKIP() << "no mprotect arm on this host (handler not installable, or the kernel " + "page is not the tracker's 4 KB); the hash arm serves every map here"; + } + constexpr SizeT kSize = 4096; // one page, one block each + auto a = MakeBuffer(29u, kSize); + auto b = MakeBuffer(30u, kSize); + auto* aMapped = static_cast(a->AcquireMemoryRange( + Range1D{0, kSize}, BufferMappingAccessBit::Write | BufferMappingAccessBit::Persistent)); + b->AcquireMemoryRange(Range1D{0, kSize}, + BufferMappingAccessBit::Write | BufferMappingAccessBit::Persistent); + ASSERT_NE(aMapped, nullptr); + ASSERT_NE(PersistentMapTracker::TrackedSlotForTest(*a), nullptr) + << "both maps must be on the mprotect arm, or the epoch is not what decides"; + ASSERT_NE(PersistentMapTracker::TrackedSlotForTest(*b), nullptr); + auto& tracker = PersistentMapTracker::Instance(); + auto& point = MG_State::pGLContext->GetBufferBindingPoint(BufferTarget::ShaderStorage, 0); + + // Draw 1 binds B only. Both maps are fresh - every page marked, and the registration + // moved the epoch - so this walk ships both once: the unbound A as well. + point.Bind(b); + tracker.PushDrawConsumers(); + EXPECT_EQ(tracker.BlocksPushed(), 2u) << "first push ships everything once, the unbound map too"; + // Draw 2, nothing written: the epoch is unmoved and the skip ships nothing. + tracker.PushDrawConsumers(); + EXPECT_EQ(tracker.BlocksPushed(), 2u); + + // The application writes A through its pointer (a fault: A's page is marked, the epoch + // moves) and the next draw binds B only. + const Uint64 epochBefore = PersistentMapTracker::FaultEpochForTest(); + aMapped[kSize / 2] = 0x5A; + ASSERT_GT(PersistentMapTracker::FaultEpochForTest(), epochBefore) + << "the write did not fault: A's page was not re-armed by its first push"; + tracker.PushDrawConsumers(); + EXPECT_EQ(tracker.BlocksPushed(), 3u) + << "the walk that consumed the epoch must drain A's marked page although this draw " + "bound only B - nothing will ever move the epoch for that page again"; + + // Draw 4 binds A. The epoch is unmoved and the skip fires - correctly, because A's + // write already crossed. With the drain reverted, this is the draw that reads stale. + point.Bind(a); + tracker.PushDrawConsumers(); + EXPECT_EQ(tracker.BlocksPushed(), 3u) << "nothing left to ship"; + EXPECT_EQ(tracker.BytesPushed(), static_cast(3u * kSize)); + + point.Bind(nullptr); + a->ReleaseMemory(false); + b->ReleaseMemory(false); +} + #else -// THE SAME SIXTEEN NAMES, SO THE ctest NAME SET DOES NOT MOVE BETWEEN LANES. G2 compares the +// THE SAME NAMES, ONE FOR ONE, SO THE ctest NAME SET DOES NOT MOVE BETWEEN LANES. (The count +// is deliberately NOT written out here: it read "SIXTEEN" against seventeen stubs before this +// commit and "SEVENTEEN" against eighteen after, so it has never once matched the list it +// claims to describe - and the list below is the authority either way.) 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. @@ -516,6 +743,7 @@ TEST(SplitBufferSet, Row0EverySsboBindingPointIsMarkedByADraw) { MGL_SPLIT_ONLY_ TEST(SplitBufferSet, Row1EveryBoundAtomicCounterIsMarkedByADraw) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, Row2OnlyAWritableImageBufferTextureCounts) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, Row2AWritableImageBufferTextureIsMarkedByADraw) { MGL_SPLIT_ONLY_OR_SKIP(); } +TEST(SplitBufferSet, Row2TheSweepIsBoundedByTheImageUnitHighWaterMark) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, Row3TransformFeedbackTargetsAreOnlyMarkedWhileACaptureIsActive) { MGL_SPLIT_ONLY_OR_SKIP(); } @@ -531,6 +759,11 @@ TEST(SplitBufferSet, AZeroBlockSizeTurnsThePushOffRatherThanMakingItUnlimited) { TEST(SplitBufferSet, ADestroyedBufferLeavesTheSet) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, UnderSplitTheWritebackClearsThePendingFlagAndNotTheRequest) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, OnlyAdoptTierTwoIsImplemented) { MGL_SPLIT_ONLY_OR_SKIP(); } +TEST(SplitBufferSet, ASubPageMapIsOneProtectedPageAndAWriteThroughItFaults) { MGL_SPLIT_ONLY_OR_SKIP(); } +TEST(SplitBufferSet, AShadowThatFailsThePageGranularTestKeepsTheInwardAlignmentAndItsEdges) { + MGL_SPLIT_ONLY_OR_SKIP(); +} +TEST(SplitBufferSet, AFaultedMapTheDrawDidNotBindStillShipsBeforeTheDrawThatDoes) { MGL_SPLIT_ONLY_OR_SKIP(); } #endif // MOBILEGL_BUILD_DISAGGREGATED diff --git a/MobileGL/MG_Test/Pipe/FieldOwnershipTest.cpp b/MobileGL/MG_Test/Pipe/FieldOwnershipTest.cpp index c1ebcb9f..ad45fb11 100644 --- a/MobileGL/MG_Test/Pipe/FieldOwnershipTest.cpp +++ b/MobileGL/MG_Test/Pipe/FieldOwnershipTest.cpp @@ -166,6 +166,9 @@ TEST(FieldOwnershipTest, TheSevenStickyForwardsAgreeWithTheirFieldRows) { TEST(FieldOwnershipTest, VerbBoundaryOpsCoverEveryVerbShapedCall) { GTEST_SKIP() << "push not compiled in (MOBILEGL_PIPE_PUSH=OFF)"; } +TEST(FieldOwnershipTest, TheResidualFillsSuppliedMemoReKeysOnEveryInputThatMovesAnAnswer) { + GTEST_SKIP() << "push not compiled in (MOBILEGL_PIPE_PUSH=OFF)"; +} #else // MOBILEGL_PIPE_PUSH @@ -387,6 +390,124 @@ TEST_F(FieldOwnershipTest, VerbBoundaryOpsCoverEveryVerbShapedCall) { // that arms all of it, and nothing in a monolith lane stamps. // --------------------------------------------------------------------------------------- +// P5d round 3 (package C): THE RESIDUAL FILL's SUPPLIED-SET MEMO, and the one thing about it +// that can be wrong. The predicate is the same expression step 4 used to spell once per field +// per verb, so the change carries no new verdict - what it carries is a KEY, and a key that +// misses one of its inputs answers the PREVIOUS environment's question at the new one. In the +// direction that loses that means the fill SKIPS a field no call supplied - and that is SILENT, +// not an abort: the walk stamps FilledGen from the verb serial whether or not it copied, so the +// field reads FRESH with the previous verb's value and the draw goes out with a stale binding +// slot. Poison catches an UNSTAMPED read; it cannot catch a stamped-but-uncopied one. That is +// exactly why this case exists - there is no second line of defence behind it. +// +// So each input is moved on its own with the answer read back in between, and in an order where +// a memo that ignored that input would have to return the stale answer rather than the right one +// by luck. This case lives outside the split-only block because the memo is in the push build +// too (PipeFill.cpp is compiled at MOBILEGL_PIPE_PUSH), and the fields it names are the same +// three in both. +// +// THE FOURTH KEY INPUT - the P4a consumer signal - GETS STEP 4 AND A DIFFERENT SHAPE, because +// it cannot move an answer today and no honest case can pretend otherwise: every field whose +// emitter belongs to a P4a family is also a field the applier cannot supply whole (its storage +// is a frontend pointer), so that conjunct is DOMINATED and the mask is identical either way. +// Step 4 moves the signal for real and pins the domination instead, so that the day a P4a row +// gains a twin the case goes red and says what to write - the same day PipeFill.cpp's +// NoP4aFamilyFieldIsWhollySupplied() static_assert fires. +TEST_F(FieldOwnershipTest, TheResidualFillsSuppliedMemoReKeysOnEveryInputThatMovesAnAnswer) { + constexpr Uint64 kNoSubsystems = 0; + constexpr Uint64 kEverySubsystem = ~Uint64{0}; + + // 1. THE PUSH MASK. create_render_state carries GetRenderStateParameters whole and the + // applier writes it without deriving, so at a mask that carries the render-state bit the + // fill skips it - and at 0, which is what a unit lane runs at (Features.PipePush defaults + // to 0), nothing is emitted and every field is pulled. + EXPECT_FALSE(MGPipeResidualFillSuppliesField(MGPipeInputField::GetRenderStateParameters, + kNoSubsystems, true, false)); + EXPECT_TRUE(MGPipeResidualFillSuppliesField(MGPipeInputField::GetRenderStateParameters, + kEverySubsystem, true, false)); + EXPECT_FALSE(MGPipeResidualFillSuppliesField(MGPipeInputField::GetRenderStateParameters, + kNoSubsystems, true, false)) + << "the memo answered the previous mask: MOBILEGL_PIPE_PUSH is not in its key, and a " + "per-subsystem A/B would then fill from the wrong subsystem set"; + + // 2. THE DERIVATION LATCH. GetViewport reaches PipeInputs only through + // MGPipeDeriveRenderStateFields, so a build whose derivation is a stub must keep PULLING + // it - skipping it there leaves the mirror unwritten and the backend reading a default. + EXPECT_TRUE(MGPipeResidualFillSuppliesField(MGPipeInputField::GetViewport, kEverySubsystem, + true, false)); + EXPECT_FALSE(MGPipeResidualFillSuppliesField(MGPipeInputField::GetViewport, kEverySubsystem, + false, false)) + << "the memo answered the previous latch: ApplierDerivesRenderStateFields is not in its key"; + + // 3. P5c rv's WIRE GATE (CONTRACT-P5C.md §5.3). set_context_values has no producer without a + // live wire, so its eight value-class fields keep being pulled under monolith - G1's + // byte-for-byte rule - and are skipped only when the record really crosses. + EXPECT_FALSE(MGPipeResidualFillSuppliesField(MGPipeInputField::GetActiveTextureUnit, + kEverySubsystem, true, false)); + EXPECT_TRUE(MGPipeResidualFillSuppliesField(MGPipeInputField::GetActiveTextureUnit, + kEverySubsystem, true, true)) + << "the memo answered the previous wire state: contextValuesWireLive is not in its key, " + "and the emission's half of the gate and the fill's half would then disagree"; + + // 4. THE P4a CONSUMER SIGNAL, which is in the key and is DOMINATED, so this step pins the + // domination rather than pretending to move an answer. Under monolith the signal IS "did + // a backend register the resource op table" (P4aFamilyHasItsConsumer, PipeFill.cpp), and + // MG_Config::Transport is a constexpr Monolith in every lane but a split one - so the + // signal really moves here, at a mask that carries all four P4a bits, which is the only + // mask at which the fill reads it at all. + if (MG_Config::Transport == MG_Config::TransportMode::Monolith) { + const MGPipeResourceOps* const savedOps = MGPipeGetResourceOps(); + MGPipeResourceOps ops{}; + MGPipeFieldMask withConsumer{}; + MGPipeFieldMask withoutConsumer{}; + + // An EMPTY table is enough: the predicate only asks whether ONE IS REGISTERED, which is + // how MG_Test/Pipe arms this subsystem everywhere else. + MGPipeSetResourceOps(&ops); + ASSERT_NE(MGPipeGetResourceOps(), nullptr) + << "the fixture did not move the signal it names, so this step observes nothing"; + for (SizeT i = 0; i < kMGPipeInputFieldCount; ++i) { + if (MGPipeResidualFillSuppliesField(static_cast(i), kEverySubsystem, + true, true)) { + withConsumer.Words[i / 64] |= (Uint64{1} << (i % 64)); + } + } + + MGPipeSetResourceOps(nullptr); + ASSERT_EQ(MGPipeGetResourceOps(), nullptr); + for (SizeT i = 0; i < kMGPipeInputFieldCount; ++i) { + if (MGPipeResidualFillSuppliesField(static_cast(i), kEverySubsystem, + true, true)) { + withoutConsumer.Words[i / 64] |= (Uint64{1} << (i % 64)); + } + } + MGPipeSetResourceOps(savedOps); + + // THE NAMED ROW, both ways: GetFramebufferBindingSlot is emitted by set_framebuffer_state + // and so rides the framebuffer family's consumer gate - and is pulled regardless, because + // its storage is a BindingSlot no payload can carry. + EXPECT_FALSE(MGPipeFieldMaskHas(withConsumer, MGPipeInputField::GetFramebufferBindingSlot)); + EXPECT_FALSE( + MGPipeFieldMaskHas(withoutConsumer, MGPipeInputField::GetFramebufferBindingSlot)); + + for (SizeT i = 0; i < kMGPipeInputFieldCount; ++i) { + const auto field = static_cast(i); + EXPECT_EQ(MGPipeFieldMaskHas(withConsumer, field), + MGPipeFieldMaskHas(withoutConsumer, field)) + << kMGPipeInputFieldNames[i] + << "'s supplied answer moved with the P4a consumer signal. That is CORRECT " + "behaviour and this assertion is a trip wire, not a defect report: the " + "conjunct stopped being dominated, so the memo's fourth key input is now " + "observable and needs the move-it-and-read-it-back pair steps 1-3 give the " + "other three - under split through CapsMirrorInstance().Adopt() with and " + "without kMGPipeSubsystemResources in the CallMask, as " + "SanityTest.ACapsMaskWithoutTheResourceFamilyEmitsNothingAndCountsTheRefusal " + "already does. Rewrite this step into that pair; PipeFill.cpp's " + "NoP4aFamilyFieldIsWhollySupplied() static_assert fires on the same change"; + } + } +} + #if MOBILEGL_BUILD_DISAGGREGATED TEST_F(FieldOwnershipTest, AServerStampMakesRecordSuppliedFieldsFreshAndWithdrawsTheRest) { diff --git a/MobileGL/MG_Test/Pipe/ImageEmitTest.cpp b/MobileGL/MG_Test/Pipe/ImageEmitTest.cpp index f3c7f056..290dd289 100644 --- a/MobileGL/MG_Test/Pipe/ImageEmitTest.cpp +++ b/MobileGL/MG_Test/Pipe/ImageEmitTest.cpp @@ -356,7 +356,8 @@ TEST(ImageEmit, AMakeCurrentClearsTheImageSetAndAdvancesItsSerial) { X(ImageEmit, AnAccessModeChangeAloneStillEmitsTheSet) \ X(ImageEmit, AnInternalFormatChangeAloneStillEmitsTheSet) \ X(ImageEmit, TheApplicationsFormatAndAccessTravelUnrecast) \ - X(ImageEmit, AnImageBoundTextureIsMarkedShaderImageBoundAtTheBind) + X(ImageEmit, AnImageBoundTextureIsMarkedShaderImageBoundAtTheBind) \ + X(ImageEmit, TheBindFeedsTheImageUnitHighWaterMark) #define MGL_DECLARE_PULL_SKIP(Suite, Name) \ TEST(Suite, Name) { GTEST_SKIP() << "compiled only under MOBILEGL_PIPE_PUSH"; } @@ -410,10 +411,12 @@ namespace { // binds an image pays one integer test per draw, taken BEFORE any hash and before any // 192-entry walk. // - // The frontend has no image-unit high-water mark of its own - NoteUnitTouched is the - // TEXTURE-unit path and glBindImageTexture does not reach it - so the window is derived - // from the highest image unit the CURRENT PROGRAM names. A program with no image uniform - // names none, and the set is not emitted at all. + // The window is derived from the highest image unit the CURRENT PROGRAM names, and it stays + // that way now that the frontend DOES keep an image-unit high-water mark (P5d round 3, + // package C, TextureState::NoteImageUnitTouched): the mark answers "which units could hold a + // binding", and this emitter's question is the narrower "which units could a shader READ". + // A program with no image uniform names none, and the set is not emitted at all - even with + // a texture sitting on unit 0, as below. TEST(ImageEmit, AZeroHighWaterMarkEmitsNothingWithoutHashing) { EmitterScope scope; static const char* kNoImages = R"(#version 430 core @@ -562,6 +565,42 @@ void main() { imageStore(img, ivec2(0, 0), vec4(1.0)); } GL::UseProgram(0); GL::BindImageTexture(0, 0, 0, GL_FALSE, 0, GL_READ_ONLY, GL_RGBA8); } + + // P5d round 3, package C: glBindImageTexture IS THE PRODUCER OF THE IMAGE-UNIT HIGH-WATER + // MARK, and this case is the reason a reader may bound a walk with it. The split client's + // per-draw GPU-write sweep (MG_Remote/Client/GpuWritePending.cpp) stops at the mark instead + // of walking all MAX_TEXTURE_IMAGE_UNITS units; drop the NoteImageUnitTouched call from + // GL_Texture.cpp's BindImageTexture and this goes red here, where the entry point is, rather + // than as a missed GPU write in a scenario nobody runs on a desktop lane. + // + // AND THE UNBIND HALF IS THE OTHER ASSERTION. The mark only ever grows: a binding that goes + // away leaves it standing, so the walk stays an over-approximation of the units that hold a + // binding - the one direction a conservative GPU-write set may fail in. + TEST(ImageEmit, TheBindFeedsTheImageUnitHighWaterMark) { + EmitterScope scope; + const GLuint name = MakeImageTexture(); + // UNIT 1, LIKE EVERY OTHER CASE IN THIS FILE, and deliberately not a higher one: + // BindImageTexture refuses `unit >= GetAdvertisedImageUnitCount()` before it reaches + // either the slot or the mark, and that count comes from the backend's + // DynamicParameters.MaxImageUnits - so a case that needs unit 4 fails on a lane whose + // backend advertises fewer, for a reason that has nothing to do with the mark. Unit 1 + // still separates "the mark moved" from "the mark is trivially 0". + constexpr Int kUnit = 1; + GL::BindImageTexture(static_cast(kUnit), name, 0, GL_FALSE, 0, GL_WRITE_ONLY, GL_RGBA8); + // And the bind is checked before the mark is, so a REFUSED bind says so rather than + // reading as a missing NoteImageUnitTouched. + ASSERT_TRUE(Ctx().GetImageTextureBinding(kUnit).Texture != nullptr) + << "glBindImageTexture did not take: this lane's backend advertises fewer than " + << (kUnit + 1) << " image units, so the case cannot observe the mark at all"; + EXPECT_GE(Ctx().GetMaxTouchedImageUnit(), kUnit) + << "glBindImageTexture did not move the image-unit high-water mark, so every reader " + "that bounds a per-draw walk with it now walks past this binding"; + + GL::BindImageTexture(static_cast(kUnit), 0, 0, GL_FALSE, 0, GL_READ_ONLY, GL_RGBA8); + EXPECT_GE(Ctx().GetMaxTouchedImageUnit(), kUnit) + << "the mark must not shrink on an unbind: a walk bounded by it has to stay an " + "over-approximation"; + } } // namespace #endif // MOBILEGL_PIPE_PUSH diff --git a/MobileGL/MG_Test/Util/PipeStatsTest.cpp b/MobileGL/MG_Test/Util/PipeStatsTest.cpp index 7628252a..6148a498 100644 --- a/MobileGL/MG_Test/Util/PipeStatsTest.cpp +++ b/MobileGL/MG_Test/Util/PipeStatsTest.cpp @@ -171,6 +171,14 @@ namespace { // And the new ByteClass rides the ordinary bytes[] bracket under a short name that is // NOT "csob": the cso[] bracket above already prints csob= for the CSO bind count. EXPECT_NE(line.find("csob-blob="), String::npos) << line; + // P5d round 3's wait ledger. Four fields on a bracket of their own, and they are + // pinned here for the reason every other short name is: they are what the inproc + // performance work reads out of a run's log, so a rename or a dropped field breaks + // every recorded measurement of the split's handoff. + EXPECT_NE(line.find("wait[srv="), String::npos) << line; + EXPECT_NE(line.find("srvpark="), String::npos) << line; + EXPECT_NE(line.find("cli="), String::npos) << line; + EXPECT_NE(line.find("clipark="), String::npos) << line; #endif } diff --git a/MobileGL/MG_Test/Wire/InProcessTransportTest.cpp b/MobileGL/MG_Test/Wire/InProcessTransportTest.cpp index 41510cc6..6c4a120a 100644 --- a/MobileGL/MG_Test/Wire/InProcessTransportTest.cpp +++ b/MobileGL/MG_Test/Wire/InProcessTransportTest.cpp @@ -316,6 +316,84 @@ TEST(InProcessTransportTest, DoorbellReturnsImmediatelyWhenAlreadyReady) { EXPECT_EQ(parked.load(), 0u); } +// P5d round 3, package T item 3: the spin phase is a CALIBRATED ITERATION BUDGET and reads no +// clock. Two things have to be true of it and neither is visible from the outside without the +// park counter, which is why ParkEntries() exists: a condition that flips WITHIN the budget must +// be caught by the spin (no park, no syscall - that is the whole saving), and a condition that +// never flips must still reach the park (a budget that silently came out as zero, or a +// calibration that returned zero iterations per microsecond, would turn every wait into a park +// and would be invisible in a test that only checked the return value). +TEST(InProcessTransportTest, DoorbellSpinsWithoutParkingWhenReadyFlipsInsideTheBudget) { + std::unique_ptr client; + std::unique_ptr server; + InProcessTransport::CreatePair(client, server); + + // The budget itself must be a usable number: SpinItersPerUs() is what multiplies spinUs, + // so a zero here would mean "never spin" and the case below would pass for the wrong reason. + ASSERT_GT(SpinItersPerUs(), 0u); + + std::atomic parked{0}; + Doorbell& bell = client->SelfDoorbell(); + const std::uint64_t parksBefore = bell.ParkEntries(); + + // Flips on the 8th probe - far inside a 50 us budget on any core that can run this suite, + // and deliberately NOT on the first, so the spin loop really iterates. + int probes = 0; + EXPECT_TRUE(bell.Wait( + parked, [&probes] { return ++probes >= 8; }, kDefaultSpinUs, 5000)); + EXPECT_GE(probes, 8); + EXPECT_EQ(bell.ParkEntries(), parksBefore) + << "the wait blocked although its condition flipped inside the spin budget: the " + "clock-free spin is not spinning, and every verb barrier is paying a futex round " + "trip it does not need"; + EXPECT_EQ(parked.load(), 0u); +} + +TEST(InProcessTransportTest, DoorbellStillParksWhenTheSpinBudgetRunsOut) { + std::unique_ptr client; + std::unique_ptr server; + InProcessTransport::CreatePair(client, server); + + std::atomic parked{0}; + Doorbell& bell = client->SelfDoorbell(); + const std::uint64_t parksBefore = bell.ParkEntries(); + + EXPECT_FALSE(bell.Wait( + parked, [] { return false; }, kDefaultSpinUs, 30)); + EXPECT_GT(bell.ParkEntries(), parksBefore) + << "the wait never reached the blocking park, so a waiter with no deadline would burn " + "a core at full clock for ever"; + EXPECT_EQ(parked.load(), 0u); +} + +// The park number is published as the SUBSET of the waits that blocked, and it only stays one +// if a Wait that traverses the park loop twice counts once. That is not a contrived path: a +// CondVarDoorbell REMEMBERS a notify delivered while nobody was waiting, so the first Park +// returns at once, the condition is still false (the peer had already drained what it +// published) and the wait parks again. Counting per Park() rather than per Wait() made +// `srvpark`/`clipark` able to print LARGER than `srv`/`cli`, which turns the one ratio the +// ledger exists to publish into an argument for the opposite conclusion. +TEST(InProcessTransportTest, AWaitThatParksTwiceOnARememberedNotifyIsStillOnePark) { + std::unique_ptr client; + std::unique_ptr server; + InProcessTransport::CreatePair(client, server); + + std::atomic parked{0}; + Doorbell& bell = client->SelfDoorbell(); + // Delivered with nobody waiting: the bell remembers it, so the FIRST Park below returns + // immediately with the condition still false and the loop parks a second time. + bell.Notify(); + + const std::uint64_t parksBefore = bell.ParkEntries(); + EXPECT_FALSE(bell.Wait( + parked, [] { return false; }, kDefaultSpinUs, 30)); + EXPECT_EQ(bell.ParkEntries(), parksBefore + 1) + << "one wait counted more than one park: the ledger's parks are no longer a subset of " + "its waits, and `clipark > cli` reads as 'every verb pays a futex round trip' when " + "the truth is one remembered notify"; + EXPECT_EQ(parked.load(), 0u); +} + TEST(InProcessTransportTest, DoorbellTimesOutWhenNothingHappens) { std::unique_ptr client; std::unique_ptr server; diff --git a/MobileGL/MG_Test/Wire/RemoteClientTest.cpp b/MobileGL/MG_Test/Wire/RemoteClientTest.cpp index 5da3cd20..cba77bd5 100644 --- a/MobileGL/MG_Test/Wire/RemoteClientTest.cpp +++ b/MobileGL/MG_Test/Wire/RemoteClientTest.cpp @@ -1847,6 +1847,71 @@ MGL_BUFFER_GUARD_TEST(BufferSyncPersistentMappedRangeFromTheApplyThreadIsFatalBy MGL_BUFFER_GUARD_TEST(BufferHasDefinedContentFromTheApplyThreadIsFatalByName, 707, (void)buffer.HasDefinedContent()) #undef MGL_BUFFER_GUARD_TEST +// P5d round 3 (package D): the two NAMED EXEMPTION scopes of CONTRACT-P5C §3.1 / §5.4 stopped +// keeping their depth in a thread_local and now count ONLY while ServerLoop::OnApplyThread() is +// true (SlotAllocator.cpp). The claim that makes that legal is "the apply-thread guard is the +// counter's only reader, so a depth kept on any other thread cannot change an answer" - and +// these three cases are that claim in executable form. They are the red-once for the change: +// +// (a) drop the `if (m_counted)` from the constructor (or make it unconditional again on a +// thread that is not the apply thread) and (c) goes red - a GL-thread scope would start +// exempting the apply thread, which is exactly the leak the thread_local used to prevent; +// (b) drop the increment altogether and (a) goes red - the exemption stops exempting and +// the debt's own sites (DirectGLES' HandleOf probes) abort the server; +// (c) drop the guard's `MGPipeReverseAnnouncementScope::ActiveOnApplyThread()` / +// `FrontendKeyedRegistryScope::ActiveOnApplyThread()` rows and (b) - the control - stops +// being the only aborting arm. +TEST(RemoteGuards, AnAllocatorProbeFromTheApplyThreadOutsideEveryScopeIsFatalByName) { + const auto child = RunInChild([] { + StartControlSession(); + Srv::ServerLoopInstance().RunOnApplyThread( + +[](void*) -> MobileGLResult { + MG_Pipe::MGPipeRefuseAllocatorFromApplyThread("FindByLifetimeId"); + return MOBILEGL_OK; + }, + nullptr); + ClientSessionInstance().Stop(); + }); + ExpectNamedAbort(child, "Fatal{RoleViolation, \"MGPipeSlots\"}"); +} + +TEST(RemoteGuards, AnAllocatorProbeInsideAnExemptionScopeOnTheApplyThreadIsAllowed) { + const auto child = RunInChild([] { + StartControlSession(); + Srv::ServerLoopInstance().RunOnApplyThread( + +[](void*) -> MobileGLResult { + const MG_Pipe::MGPipeFrontendKeyedRegistryScope frontendKeyedRegistry; + MG_Pipe::MGPipeRefuseAllocatorFromApplyThread("FindByLifetimeId"); + const MG_Pipe::MGPipeReverseAnnouncementScope reverseAnnouncement; + MG_Pipe::MGPipeRefuseAllocatorFromApplyThread("Acquire"); + return MOBILEGL_OK; + }, + nullptr); + ClientSessionInstance().Stop(); + }); + ExpectChildSuccess(child); +} + +// THE ONE THAT PINS THE SINGLE-WRITER ARGUMENT. The scope is open on the GL thread for the +// whole of the posted request; the apply thread's probe must still abort, because the exemption +// belongs to the thread that entered the scope and never to another one. With the depth kept in +// a plain counter that every thread incremented, this case would go green - and a real +// apply-thread violation would be silently exempted for as long as any GL thread held a scope. +TEST(RemoteGuards, AnExemptionScopeHeldOnTheGLThreadDoesNotExemptTheApplyThread) { + const auto child = RunInChild([] { + StartControlSession(); + const MG_Pipe::MGPipeFrontendKeyedRegistryScope frontendKeyedRegistry; + Srv::ServerLoopInstance().RunOnApplyThread( + +[](void*) -> MobileGLResult { + MG_Pipe::MGPipeRefuseAllocatorFromApplyThread("FindByLifetimeId"); + return MOBILEGL_OK; + }, + nullptr); + ClientSessionInstance().Stop(); + }); + ExpectNamedAbort(child, "Fatal{RoleViolation, \"MGPipeSlots\"}"); +} + TEST(RemoteGuards, CapsMirrorFallbackWithNoServerBackendIsFatalByName) { const auto child = RunInChild([] { MG_Config::Transport = MG_Config::TransportMode::InProcess; diff --git a/MobileGL/MG_Test/Wire/ServerLoopTest.cpp b/MobileGL/MG_Test/Wire/ServerLoopTest.cpp index 078ee1f8..1a1cbfdc 100644 --- a/MobileGL/MG_Test/Wire/ServerLoopTest.cpp +++ b/MobileGL/MG_Test/Wire/ServerLoopTest.cpp @@ -326,6 +326,115 @@ TEST(ServerLoopTest, AControlRequestRunsOnTheApplyThreadAndUnparksIt) { fixture.Stop(); } +// P5d round 3, package T item 1: the mailbox's one-bit shadow. ControlIsPending() no longer +// takes m_controlMutex - it reads m_controlPosted - and that word has TWO failure modes, only +// one of which any existing case can see. +// +// THE SET SIDE is already covered: a poster that failed to publish the shadow leaves the park +// predicate with nothing to see, and AControlRequestRunsOnTheApplyThreadAndUnparksIt above says +// so in its own words within five seconds. +// +// THE CLEAR SIDE IS THIS CASE, and it is the one nothing else can reach. A shadow that the pump +// takes the request from but never clears makes `ready` permanently true: the loop then spins at +// full clock for the rest of the session, never sets consumerParked again, and every symptom is +// a performance one - the process stays correct, the phone gets hot and a core is gone. +// +// IT HAS TO OBSERVE THE WINDOW, NOT THE END STATE, and that is the whole shape of the case. +// PumpControlRequest clears the shadow TWICE: once when it takes the request (the load-bearing +// clear) and once defensively, on the next iteration, when it finds the shadow set with +// m_controlPending false. Assert only "the shadow is clear after the request was answered" and +// the defensive clear covers for a missing take-clear on the very next pump - the case then +// passes with the line it exists to pin deleted. So the request's work BLOCKS on a gate this +// thread holds, and the shadow is read while the apply thread is still inside work(): in that +// window m_controlPending is still true, the pump is not running, and the ONLY thing that can +// make ControlIsPending() false is the clear at the take. Delete it and this read returns true, +// deterministically, on every run. +TEST(ServerLoopTest, TheControlShadowIsClearedWhenThePumpTakesTheRequest) { + ServerFixture fixture; + ASSERT_TRUE(fixture.Handshake()); + ASSERT_TRUE(fixture.StartLoop()); + + Server::ServerLoop& loop = Server::ServerLoopInstance(); + ASSERT_TRUE(fixture.WaitUntilTrulyParked()) + << "the apply thread never parked in the first place, so this case could prove nothing " + "about it parking AGAIN"; + EXPECT_FALSE(loop.ControlIsPending()) + << "the shadow is set with no request posted: `ready` is true for ever and the loop " + "will never park"; + + // The gate the posted work blocks on. `running` is released by the apply thread once it is + // INSIDE work(); `release` is set by this thread once it has read the shadow. + struct Gate { + std::atomic running{false}; + std::atomic release{false}; + }; + Gate gate; + + // Posted from a helper for N-9's reason - a predicate that cannot see the request blocks the + // poster for ever, and the case has to say that rather than hit ctest's timeout. + std::atomic answered{false}; + MobileGLResult rc = MOBILEGL_ERR_INVALID_ARGUMENT; + std::thread poster([&] { + rc = loop.RunOnApplyThread( + +[](void* user) -> MobileGLResult { + Gate* g = static_cast(user); + g->running.store(true, std::memory_order_release); + const auto gateDeadline = + std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (!g->release.load(std::memory_order_acquire) && + std::chrono::steady_clock::now() < gateDeadline) { + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + return MOBILEGL_OK; + }, + &gate); + answered.store(true, std::memory_order_release); + }); + + // Wait until the work is really running, i.e. until the pump has taken the request. + const auto startDeadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (!gate.running.load(std::memory_order_acquire) && + std::chrono::steady_clock::now() < startDeadline) { + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + const Bool started = gate.running.load(std::memory_order_acquire); + if (started) { + // THE ASSERTION. The apply thread is inside work(), so nothing is pumping and + // m_controlPending is still true; the shadow is false only because the take cleared it. + EXPECT_FALSE(loop.ControlIsPending()) + << "the pump took a control request and left the shadow set: `ready` stays true for " + "as long as the request runs, and if the take-clear is gone entirely the idle " + "poll spins a core at full clock for the rest of the session"; + } else { + ADD_FAILURE() << "the posted control request never started running: the shadow's SET side " + "is broken, so the park predicate no longer carries the control flag"; + } + gate.release.store(true, std::memory_order_release); + + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (!answered.load(std::memory_order_acquire) && std::chrono::steady_clock::now() < deadline) { + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + const Bool gotAnswer = answered.load(std::memory_order_acquire); + if (!gotAnswer) { + ADD_FAILURE() << "the posted control request was never answered"; + fixture.Stop(); + poster.join(); + return; + } + poster.join(); + EXPECT_EQ(rc, MOBILEGL_OK); + + // And the loop must be able to BLOCK again - the end state the defensive clear also + // guarantees, kept because it is the symptom an operator would actually see. + EXPECT_TRUE(fixture.WaitUntilTrulyParked()) + << "the apply thread never re-entered the blocking park after a control request: the " + "shadow is stuck set, so the idle poll now spins a core at full clock for the rest " + "of the session"; + + fixture.Stop(); +} + // Re-entrancy is not a deadlock: ~BackendObject_DirectGLES reaches ReleaseEGLResources FROM the // apply thread, so a post from there must run inline. TEST(ServerLoopTest, APostFromTheApplyThreadItselfRunsInlineRatherThanDeadlocking) { @@ -808,6 +917,59 @@ TEST(ServerLoopTest, AForwarderCallAfterTheLoopStoppedReturnsNotInitializedAndDo EXPECT_FALSE(probe.ran) << "the work ran on the caller after the loop stopped"; } +// P5d round 3, package D: THE IDENTITY MUST DIE WITH THE THREAD, and this is the case that says +// so. OnApplyThread() is no longer `m_running && id == mine` - it is one relaxed load of +// Server::Detail::g_applyThreadKey compared against the caller's own thread pointer - so there +// is no second term left to make a stale identity harmless. If ApplyThreadMain's exit block +// stops clearing the key (deleted, or lost in a merge with a package that rewrites that same C2 +// critical section), the key keeps naming a thread that no longer exists, and libc hands that +// thread's freed TLS block - hence the very thread-pointer value the key holds - to the NEXT +// thread the process creates: a ShaderCompilePool worker, or the GL thread of a second context +// after a context-loss restart. That thread then answers OnApplyThread() TRUE, which inverts +// RunsAsTheServerRole() and PersistentMapTracker::OnServerRole() on the client, aborts +// BufferObject's accessors with Fatal{RoleViolation, "buffer-legacy-arm"}, and makes +// RunOnApplyThread run EGL work inline on it - the "EGL on the app thread" outcome R-1 exists to +// make impossible. +// +// Two halves, because the mechanism and the consequence fail differently. (a) is deterministic +// on every platform: after the join the key is 0, full stop. (b) is the one that would have +// caught the real bug: threads created after the join must not inherit the answer. (b) is not +// guaranteed to reproduce recycling on every allocator, which is exactly why (a) is here too. +TEST(ServerLoopTest, AThreadCreatedAfterTheLoopStoppedIsNotMistakenForTheApplyThread) { + ServerFixture fixture; + ASSERT_TRUE(fixture.Handshake()); + ASSERT_TRUE(fixture.StartLoop()); + ASSERT_TRUE(fixture.WaitUntilTrulyParked()); + + // The key really was published while the thread ran - otherwise (a) below would pass for the + // wrong reason (a key that was never set is also 0). + ASSERT_NE(Server::Detail::g_applyThreadKey.load(std::memory_order_relaxed), 0ull) + << "the apply thread is parked and running but published no identity: ApplyThreadMain's " + "first statement no longer stores Detail::CurrentThreadKey()"; + + fixture.Stop(); + ASSERT_FALSE(Server::ServerLoopInstance().Running()); + + // (a) the clear itself. + EXPECT_EQ(Server::Detail::g_applyThreadKey.load(std::memory_order_relaxed), 0ull) + << "the apply-thread identity outlived the apply thread: ApplyThreadMain's C2 exit block " + "no longer clears Detail::g_applyThreadKey beside m_running"; + + // (b) what the clear is FOR. Sequentially, so each thread is created after the previous one + // has been joined and is therefore the likeliest candidate to receive the recycled block. + for (int i = 0; i < 8; ++i) { + std::atomic onApplyThread{true}; + std::thread probe([&] { + onApplyThread.store(Server::ServerLoop::OnApplyThread(), std::memory_order_release); + }); + probe.join(); + ASSERT_FALSE(onApplyThread.load(std::memory_order_acquire)) + << "thread " << i << " created after the apply thread was joined answers " + "OnApplyThread() TRUE - it inherited the exited thread's TLS block and the stale " + "key still names it. Every role guard in the tree is now inverted on that thread"; + } +} + // M-7: the OTHER !m_running arm - a backend built but no thread (Start refused / std::thread // threw) - must be a NAMED Fatal, never a silent inline EGL-on-the-app-thread fallback. A split // lane that ran eglMakeCurrent on the app thread would render correctly with the context on the diff --git a/MobileGL/MG_Test/Wire/SessionTest.cpp b/MobileGL/MG_Test/Wire/SessionTest.cpp index de25334a..2d33969d 100644 --- a/MobileGL/MG_Test/Wire/SessionTest.cpp +++ b/MobileGL/MG_Test/Wire/SessionTest.cpp @@ -1121,6 +1121,43 @@ TEST(SessionTest, TheProducerRemembersWhatItPublishedEvenIfTheWatermarkLags) { EXPECT_EQ(session.Control().submittedSeq.load(), 4u); } +// P5d round 3, package T item 4. The client's wait ledger is a PAIR - waits, and the subset of +// them that blocked - and the summary line prints it as a ratio. That only reads as a ratio if +// both halves count the same events, which is why the park counter is the producer's own rather +// than the self bell's ParkEntries(): a bell belongs to an ENDPOINT and every waiter on that +// endpoint shares it. In the real client that second waiter is the encoder's SEG_STAGE +// retirement wait, which ClientSession points at this very bell +// (PipeWireCodec::SetStageRetirementDoorbell) and which never passes through +// SessionProducer::Park - so it would have inflated `clipark` while leaving `cli` untouched, and +// with its spinUs of 0 it parks EVERY time. +TEST(SessionTest, TheProducersParkLedgerCountsItsOwnWaitsAndNotTheBellsOtherWaiters) { + SessionFixture session; + ASSERT_TRUE(session.Build(TestSizes())); + + const std::uint64_t waitsBefore = session.producer.Waits(); + const std::uint64_t parksBefore = session.producer.Parks(); + + // A SECOND SUBSYSTEM waits on the same bell, exactly as the encoder does. spinUs 0 and a + // condition that never flips, so it goes straight to the blocking park. + std::atomic parked{0}; + EXPECT_FALSE(session.clientTransport->SelfDoorbell().Wait( + parked, [] { return false; }, 0, 20)); + EXPECT_EQ(session.producer.Waits(), waitsBefore) + << "a foreign wait on the shared bell moved the producer's wait count"; + EXPECT_EQ(session.producer.Parks(), parksBefore) + << "a foreign wait on the shared bell moved the producer's PARK count: `clipark` is not " + "the subset of `cli` the summary line says it is, and on a lane with stage " + "retirement it can print larger than the number it is a subset of"; + + // And the producer's own wait moves both, exactly once: 50 us of spin cannot outlast a + // 20 ms timeout, so this wait blocks, and it blocks once. + EXPECT_EQ(session.producer.WaitForApplied(1, 20), SessionWait::TimedOut); + EXPECT_EQ(session.producer.Waits(), waitsBefore + 1); + EXPECT_EQ(session.producer.Parks(), parksBefore + 1) + << "the producer's own blocking wait was not counted as a park, so the ledger would " + "report a spin budget that covers a handoff it does not"; +} + // M-3. FlatBuffers' Verifier::VerifyTable is `return !table || table->Verify(*this)`, so a NULL // union member PASSES verification: a 24-byte frame verifies, carries the file identifier, // reports msg_type() == Hello, and returns nullptr from msg_as_Hello(). Both handshakes fold diff --git a/MobileGL/MG_Util/Metrics/PipeStats.cpp b/MobileGL/MG_Util/Metrics/PipeStats.cpp index a988db8a..5590da31 100644 --- a/MobileGL/MG_Util/Metrics/PipeStats.cpp +++ b/MobileGL/MG_Util/Metrics/PipeStats.cpp @@ -530,6 +530,18 @@ namespace MobileGL::MG_Util::PipeStats { line += " ringwraps=" + std::to_string(Read(g_gauges[static_cast(Gauge::RingWraps)])); line += " ringpads=" + std::to_string(Read(g_gauges[static_cast(Gauge::RingWrapPads)])); line += " ringwaits=" + std::to_string(Read(g_gauges[static_cast(Gauge::RingWaits)])); + // P5d round 3's wait ledger, on a bracket of its own so one grep reads the whole + // family, and RUN TOTALS like the gauges above it for the same reason: both sides + // publish a monotone counter of their own, and a windowed difference of two counters + // published by two threads at two different moments is not a quantity either of them + // ever held. `srv`/`cli` are entries into Doorbell::Wait; `srvpark`/`clipark` are the + // subset that stopped spinning and blocked. Read them as a ratio: parks near zero means + // the spin budget is covering the handoff, parks tracking the waits means it is not and + // every verb is paying a futex round trip. + line += "] wait[srv=" + std::to_string(Read(g_gauges[static_cast(Gauge::ServerWaits)])); + line += " srvpark=" + std::to_string(Read(g_gauges[static_cast(Gauge::ServerParks)])); + line += " cli=" + std::to_string(Read(g_gauges[static_cast(Gauge::ClientWaits)])); + line += " clipark=" + std::to_string(Read(g_gauges[static_cast(Gauge::ClientParks)])); #endif line += "] gates["; for (Uint32 i = 0; i < kGateCount; ++i) { @@ -597,7 +609,16 @@ namespace MobileGL::MG_Util::PipeStats { json += " \"ring-wrap-pads\": " + std::to_string(Read(g_gauges[static_cast(Gauge::RingWrapPads)])) + ",\n"; json += " \"ring-waits\": " + - std::to_string(Read(g_gauges[static_cast(Gauge::RingWaits)])) + "\n"; + std::to_string(Read(g_gauges[static_cast(Gauge::RingWaits)])) + ",\n"; + // The wait ledger under its long names, same run totals as the summary line's wait[]. + json += " \"server-waits\": " + + std::to_string(Read(g_gauges[static_cast(Gauge::ServerWaits)])) + ",\n"; + json += " \"server-parks\": " + + std::to_string(Read(g_gauges[static_cast(Gauge::ServerParks)])) + ",\n"; + json += " \"client-waits\": " + + std::to_string(Read(g_gauges[static_cast(Gauge::ClientWaits)])) + ",\n"; + json += " \"client-parks\": " + + std::to_string(Read(g_gauges[static_cast(Gauge::ClientParks)])) + "\n"; #endif json += " },\n \"cmd-bytes-per-draw-histogram\": ["; for (Uint32 i = 0; i < kPayloadHistogramBuckets; ++i) { diff --git a/MobileGL/MG_Util/Metrics/PipeStats.h b/MobileGL/MG_Util/Metrics/PipeStats.h index bf7bce97..7b679375 100644 --- a/MobileGL/MG_Util/Metrics/PipeStats.h +++ b/MobileGL/MG_Util/Metrics/PipeStats.h @@ -226,6 +226,35 @@ namespace MobileGL::MG_Util::PipeStats { RingWraps, RingWrapPads, RingWaits, + // P5d ROUND 3's WAIT LEDGER, the four numbers that say where the split's frame went. + // Under `inproc` the lockstep frame is "client work + server apply + handoff", and the + // handoff is made of waits: simpleperf on the workload device measured + // SessionProducer::WaitForApplied at 27.9% self of the GL thread and the apply thread's + // idle poll at ~49% of its own. A wait that SPUN and a wait that PARKED cost three + // orders of magnitude apart, so each side publishes both: `ServerWaits` / + // `ClientWaits` are entries into Doorbell::Wait, `ServerParks` / `ClientParks` are the + // subset that ran out of spin budget and blocked. + // + // EACH SIDE PUBLISHES ITS OWN. CONTRACT-P5C rule E forbids either role naming the + // other's memory, so the client does NOT read ServerLoop::ParkCount() across the role + // line - the apply thread publishes the server pair from its own loop and the GL thread + // publishes the client pair from EmitPresent. PipeStats is the meeting point precisely + // because it is below both - IN ONE PROCESS. Under `spawn` the server pair is published + // into the SERVER process's PipeStats, so the client's summary line prints + // `srv=0 srvpark=0` for the whole run: a zero that means "another process", printed in + // the shape of a zero that means "never waited". That is how `rsp` and the rest of the + // server-published gauges already behave; the round this pair was added for is `inproc`. + // + // THE PARK NUMBER IS THE SUBSET OF THE WAIT NUMBER ON PURPOSE, and stays one only + // because both halves of each pair are counted by the same code path: Doorbell::Wait + // increments a tally the WAITER owns (its `parkTally` out-parameter), once per wait that + // really blocked. A per-bell counter would not do - a bell belongs to an endpoint and + // every waiter on it shares it - and a park counted per Park() call would not either, + // since one wait can park, wake on a remembered notify and park again. + ServerWaits, + ServerParks, + ClientWaits, + ClientParks, Count };