diff --git a/MobileGL/MG_IntegrationTest/Harness/PipeStatsWindow.h b/MobileGL/MG_IntegrationTest/Harness/PipeStatsWindow.h index 87915816..daa6a512 100644 --- a/MobileGL/MG_IntegrationTest/Harness/PipeStatsWindow.h +++ b/MobileGL/MG_IntegrationTest/Harness/PipeStatsWindow.h @@ -208,6 +208,32 @@ namespace MGITest::PipeStatsWindow { return Last(ReadWholeFile(ServerLibraryLogPath())); } + // ...AND SINCE A MARK, which is the form a counted window actually wants. + // + // The two above take the LAST summary line in the role's whole log, and that is one shutdown + // away from being the wrong line: the server writes a final, EMPTY window at teardown + // (frames=N window=0 draws=0, every counter zero), so a reader that runs after it - or that + // is slowed down enough for it to land first - reads four zeroes and reports them as the + // workload's shape. It has not bitten yet only because the scenarios read before teardown. + // + // Marking the log before the counted workload and reading only what was appended after + // removes the race entirely: with MOBILEGL_PIPE_STATS_PERIOD=1 the one eglSwapBuffers that + // closes the window emits exactly one line into that span, so "the last line since the mark" + // is that line and cannot be a later one. + inline Window LastFromClientLogSince(const LogMark& mark) { +#if MOBILEGL_BUILD_DISAGGREGATED + MGPipeSyncPeerLog(); +#endif + return Last(ReadFileSince(LibraryLogPath(), mark.client)); + } + + inline Window LastFromServerLogSince(const LogMark& mark) { +#if MOBILEGL_BUILD_DISAGGREGATED + MGPipeSyncPeerLog(); +#endif + return Last(ReadFileSince(ServerLibraryLogPath(), mark.server)); + } + // One counter out of that line, by its short name ("mpr", "draws", "csom"), or -1 when the // line does not carry it. The search includes the SEPARATOR before the name and the `=` after // it, so "draws" cannot match "draws/f=" and "mpr" cannot match a longer name ending in it - diff --git a/MobileGL/MG_IntegrationTest/Scenarios/TextureUploadShapeScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/TextureUploadShapeScenario.cpp index 1c72129e..7a126167 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/TextureUploadShapeScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/TextureUploadShapeScenario.cpp @@ -113,6 +113,26 @@ namespace MGITest { constexpr int kScatteredRects = 40; constexpr int kRectSize = 2; constexpr int kFrames = 3; + // THE SCATTER IS INSET BY ONE RECT, AND WITHOUT IT THIS GATE MEASURED NOTHING. + // + // The original stride was x = ((i*7) % (kAtlasSize/kRectSize)) * kRectSize and the same + // with 5 for y. Both are surjective onto 0..31 over 40 iterations (rect 9 lands at x=62, + // rect 19 at y=62), so the rects' UNION BOX is the whole 64x64 level - and the server's + // subRectEligible (Managers.cpp:8486-8491) requires !dirtyRegion.CoversWholeLevel, a + // BOUNDING-BOX test (MipmapStorage.h:30-33). Both scatter textures were therefore + // rect-INELIGIBLE on every arm, before any policy about rect lists was consulted: the + // box-versus-rect decision this scenario exists to pin was never reached, and the only + // texture that could move was the contiguous control band. + // + // Insetting by one rect keeps the same 40 writes and the same coarse stride but bounds + // the union box at (kRectSize, kRectSize)..(62, 54), which covers no level. The decision + // point is now live for the two scatter textures, which is what makes the gold row below + // a statement about the emission shape rather than about the workload's bounding box. + constexpr int kScatterMargin = kRectSize; + constexpr int kScatterCells = kAtlasSize / kRectSize - 2 * (kScatterMargin / kRectSize); + static_assert(kScatterMargin + (kScatterCells - 1) * kRectSize + kRectSize < kAtlasSize, + "the inset scatter must not reach the level's far edge, or its union box " + "covers the level again and the server stops looking at the rect list"); // The viewed texture's OWNER is twice the atlas so that its level 1 - which is what the // view opens onto - is exactly kAtlasSize and the same scattered pattern applies to it. constexpr int kViewedOwnerSize = kAtlasSize * 2; @@ -131,13 +151,35 @@ namespace MGITest { // these numbers describe a workload that no longer exists. The assert makes that a compile // error rather than a mysterious red. // - // WHY box IS THE WHOLE OF emit ON THIS TREE. GetDirtyRects (MipmapStorage.cpp:430-457) - // hands the emitter a rect list only when the list has 2..maxRects entries AND the rects' - // summed area is under 3/4 of the union box. This workload's rects are spread on a coarse - // stride and the cascade merges every pair that touches, so the emitter takes the box arm - // for all three textures in all three frames. That is the SHAPE being pinned: a change - // that starts emitting rect lists here is exactly the +6 ms/frame inversion, and it is - // what the red-once for this gate produces on purpose. + // WHY box IS THE WHOLE OF emit ON THIS TREE, and this paragraph was WRONG in its first + // version - the correction is worth keeping because the wrong version is the one a reader + // reaches for. + // + // The client really does build a rect list here: MipmapStorage's cascade merges only + // rects that overlap or abut (RegionsTouch, MipmapStorage.cpp:88-91), the inset scatter + // leaves 2-texel gaps, and GetDirtyRects hands back THIRTY rects whose summed area (120 + // texels) is far under 3/4 of the union box - so neither the 2..maxRects test nor the + // summedArea*4 >= unionArea*3 fallback takes the box arm. The record carries all thirty. + // + // THE SERVER DECIDES, and it decides BOX, at Managers.cpp:8649: + // + // if (BufferImpl::UnpackRingAvailable()) dirtyRectCount = 0; + // + // "through the unpack ring every glTexSubImage is a GPU copy job (Mali), so ~100 sprite + // rects become ~100 jobs whose fixed cost dwarfs the union box's extra bytes - measured + // +6 ms/frame... One box, one job." That is the existing Mali-cliff mitigation, and + // box=9 rect=0 is its INTENDED OUTPUT rather than an accident of this workload. Pinning + // it is the point: a change that starts emitting rect lists through the ring is exactly + // the +6 ms/frame inversion, and disabling the ring is what the red-once does. + // + // WHAT THE FIRST VERSION GOT WRONG, because the trap is easy to fall into twice: the + // original scatter stride reached x=62 and y=62 in a 64-px atlas, so the union box + // covered the whole level - and the server's subRectEligible (Managers.cpp:8486-8491) + // requires !dirtyRegion.CoversWholeLevel, a BOUNDING-BOX test (MipmapStorage.h:30-33). + // Both scatter textures were rect-INELIGIBLE before the ring policy was ever consulted, + // so the gold looked identical while measuring something else entirely, and the only + // texture that could move under any red-once was the contiguous control band. The + // kScatterMargin inset above is what makes this paragraph true. struct UploadShapeGold { long long Emissions; long long BoxEmissions; @@ -355,8 +397,8 @@ void main() { oColor = texture(uTex, vUv); } const std::uint8_t patch[kRectSize * kRectSize * 4] = { 0, 255, 0, 255, 0, 255, 0, 255, 0, 255, 0, 255, 0, 255, 0, 255}; for (int rect = 0; rect < kScatteredRects; ++rect) { - const int x = ((rect * 7) % (kAtlasSize / kRectSize)) * kRectSize; - const int y = ((rect * 5) % (kAtlasSize / kRectSize)) * kRectSize; + const int x = kScatterMargin + ((rect * 7) % kScatterCells) * kRectSize; + const int y = kScatterMargin + ((rect * 5) % kScatterCells) * kRectSize; glTexSubImage2D(GL_TEXTURE_2D, 0, x, y, kRectSize, kRectSize, GL_RGBA, GL_UNSIGNED_BYTE, patch); } @@ -433,6 +475,9 @@ void main() { oColor = texture(uTex, vUv); } // agree; this is what makes that a real question outside monolith. TextureEmitCountersPeek emitBefore{}; const bool emitPeekLive = PeekTextureEmitCounters(&emitBefore); + // The log mark the two role readings are taken against, so that a teardown window + // cannot be mistaken for the workload's (see PipeStatsWindow.h). + const PipeStatsWindow::LogMark statsMark = PipeStatsWindow::MarkLaneLog(); Image lastScattered; Image lastContiguous; @@ -480,15 +525,29 @@ void main() { oColor = texture(uTex, vUv); } // two-sided comparison into `N == 0`. So each side is read from ITS OWN role's log, and // the server bracket falls back to the client log only where there is no server half - // the pull build and the monolith lane, where one file IS the whole run. - const PipeStatsWindow::Window clientWindow = PipeStatsWindow::LastFromClientLog(); - const PipeStatsWindow::Window serverWindow = PipeStatsWindow::LastFromServerLog(); - ASSERT_TRUE(clientWindow.found) - << "no 'MGPipe stats:' line in " << PipeStatsWindow::LibraryLogPath() - << ", so the shape could not be read at all"; + const PipeStatsWindow::Window clientWindow = + PipeStatsWindow::LastFromClientLogSince(statsMark); + const PipeStatsWindow::Window serverWindow = + PipeStatsWindow::LastFromServerLogSince(statsMark); const PipeStatsWindow::Window& shapeWindow = (serverWindow.found && PipeStatsWindow::CounterOrAbsent(serverWindow, "emit") > 0) ? serverWindow : clientWindow; + // THE ASSERTION IS ON THE WINDOW THAT CARRIES THE BRACKET, not on the client's, and + // moving to a marked read is what showed why. The client role emits NO summary window + // of its own under inproc/spawn/tcp at the point this case reads - only the + // "counters ON" banner, which also contains the marker string. Asserting + // clientWindow.found over the whole file therefore passed on the BANNER in all three + // split arms and claimed a window had been read when none had. Since the mark there + // is no banner to hide behind, so the claim has to be made about the role that + // actually published the counters. + ASSERT_TRUE(shapeWindow.found) + << "no 'MGPipe stats:' window was appended to either role's log by the swap that " + "closes the counted window. client=" + << PipeStatsWindow::LibraryLogPath() + << " server=" << PipeStatsWindow::ServerLibraryLogPath() + << ". The shape could not be read at all - which is a different finding from a " + "shape that read zero."; RecordProperty("stats_line_client", clientWindow.line.c_str()); if (serverWindow.found) RecordProperty("stats_line_server", serverWindow.line.c_str()); diff --git a/docs/Disaggregated/MEASUREMENTS.md b/docs/Disaggregated/MEASUREMENTS.md index 83777b11..e7c3f826 100644 --- a/docs/Disaggregated/MEASUREMENTS.md +++ b/docs/Disaggregated/MEASUREMENTS.md @@ -790,12 +790,22 @@ P6（spawn transport）的实现包（a6 / c6 / lk / so / sm / cp / hs / dl / st P4a 的记录是两张纹理、无 client emitter 的 `emit=6 box=6 rect=0 jobs=6`（`ctu=0`），与本行 **不可比**：工作负载不同。 -`rect=0` 不是散点图案的偶然，而是 unpack-ring 策略的既定输出：`Managers.cpp:8652` 在 -`UnpackRingAvailable()` 时把 `dirtyRectCount` 清零（"One box, one job"，即 Mali 悬崖的既有缓解）， -`:8764` 的 `rectShape` 还要求 `dirtyRectCount >= 2`。因此**客户端任何谓词翻转都动不了这四个数** -——翻转 `summedArea*4 >= unionArea*3` 全绿，见 `notes/p34b/espryt-d2.md` 的四次尝试。红一次由 -"把 union box 切成两半 + `MOBILEGL_ESPRYT_DISABLE_UNPACK_RING=1`"做到：`box=6 rect=3 jobs=12`， -而 26 个像素用例全绿。 +`rect=0` 是 **unpack-ring 策略的既定输出**：client 确实建出 30 个 rect（2 texel 间隙， +`RegionsTouch` 不合并；summed area 120 远低于 union box 的 3/4，两道 client 侧谓词都不取 box 臂）， +记录把 30 个 region 全部带过去；是 server 在 `Managers.cpp:8649` 的 +`if (UnpackRingAvailable()) dirtyRectCount = 0;` 决定取 box——"One box, one job"，Mali 悬崖的既有 +缓解。门钉的就是这个策略的输出。 + +**散点图案本身是被修过的（评审 fable 指出）**：原来的 stride `x=((i*7)%32)*2` / `y=((i*5)%32)*2` +在 64 px 图集里会打到 x=62、y=62，union box 覆盖整个 level，而 server 的 `subRectEligible` +（`Managers.cpp:8486-8491`）要求 `!dirtyRegion.CoversWholeLevel`（`MipmapStorage.h:30-33`，**包围盒** +判据）——两张散点纹理在 ring 策略之前就已经 rect-ineligible，金标数字一样但量的不是同一件事。 +内缩一个 rect（`kScatterMargin`）后 union box 落在 (2,2)..(62,54)，决策点才真正带电。 + +红一次因此是**零补丁**的：`MOBILEGL_ESPRYT_DISABLE_UNPACK_RING=1` 一个环境变量就把两张散点纹理 +从 box 翻成 rect——`box=3 rect=6 jobs=183`（3 = 连续带 x3 帧；6 = 两张散点 x3 帧；183 = 3 + 6x30）， +`bytes/f[tex]` 同时从 104448 降到 9024，而 26 个像素用例全绿。内缩之前同一个变量是全绿的，这正是 +`CoversWholeLevel` 把决策点短路掉的证据。 ### 13.2 client 半边的读法（本包的修正） diff --git a/docs/Disaggregated/notes/p34b/espryt-d2.md b/docs/Disaggregated/notes/p34b/espryt-d2.md index d6a3d4c4..e1157254 100644 --- a/docs/Disaggregated/notes/p34b/espryt-d2.md +++ b/docs/Disaggregated/notes/p34b/espryt-d2.md @@ -138,42 +138,54 @@ client's true count in every arm, because the test process is the client under a ### Red-once (R-16) -**The prescribed flip does not reach this workload, and that is a finding.** Flipping -`summedArea * 4 >= unionArea * 3` in `MipmapStorage::GetDirtyRects` leaves the gate GREEN. Four -attempts, in order, all green: +**The prescribed flip does not reach this workload — but not for the reason this note first +gave.** Flipping `summedArea * 4 >= unionArea * 3` in `MipmapStorage::GetDirtyRects` leaves the +gate green, and so do two other client-side mutations: -1. flip `summedArea*4 >= unionArea*3` → green -2. `rects.size() < 2` → `rects.empty()` → green -3. remove the union-box re-seed in `MarkDirtyRegion` → green -4. `MOBILEGL_ESPRYT_DISABLE_UNPACK_RING=1` alone → green +1. flip `summedArea*4 >= unionArea*3` -> green +2. `rects.size() < 2` -> `rects.empty()` -> green +3. remove the union-box re-seed in `MarkDirtyRegion` -> green +4. `MOBILEGL_ESPRYT_DISABLE_UNPACK_RING=1` alone -> green **(before the geometry fix)** -The reason is two gates downstream of the predicate, either of which alone forces the box arm: +The first three are green because they are mutations of a decision the CLIENT has already made +correctly: with the inset scatter the client hands over thirty rects, and none of those three +predicates is the one that takes the box arm. Attempt 4 was green for a completely different and +much worse reason, and finding it is the reviewer's (fable, ID-P7-8): -* `Managers.cpp:8764` `rectShape = subRectEligible && dirtyRectCount >= 2` — the SERVER needs at - least two rects, and the client's list collapses to one for this workload; and -* `Managers.cpp:8652` `if (BufferImpl::UnpackRingAvailable()) dirtyRectCount = 0;` — **by design**: - "through the unpack ring every glTexSubImage is a GPU copy job (Mali), so ~100 sprite rects - become ~100 jobs whose fixed cost dwarfs the union box's extra bytes… One box, one job." +> the original stride `x = ((i*7) % 32) * 2`, `y = ((i*5) % 32) * 2` reaches x=62 (rect 9) and +> y=62 (rect 19) in a 64-px atlas, so the rects' union box covered the WHOLE LEVEL — and the +> server's `subRectEligible` (`Managers.cpp:8486-8491`) requires +> `!dirtyRegion.CoversWholeLevel`, a **bounding-box** test (`MipmapStorage.h:30-33`). Both +> scatter textures were rect-INELIGIBLE before `:8649`'s ring policy was consulted. -So `box=9 rect=0` is not an accident of the scatter pattern: it is the unpack-ring policy's -intended output, and no client-side predicate can move it while the ring is available. The gold -row pins the policy's output, which is the right thing to pin. +So the gate's numbers were right and its subject was wrong: the box-versus-rect decision it +exists to pin was never reached for the two textures the workload scatters into, and the only +texture that could move under any red-once was the contiguous control band. The first +substitute red-once's `box=6 rect=3 jobs=12` was exactly that band (64x8, cut in two, three +frames) — **neither scatter texture moved**, which is what gave the wrong explanation away. -The red-once that DOES bite makes `GetDirtyRects` hand back the union box cut in two AND disables -the unpack ring: +**The fix** is `kScatterMargin`: inset the scatter by one rect so the union box bounds at +`(2,2)..(62,54)` and covers no level. Same 40 writes, same coarse stride, decision point live. +The gold row is UNCHANGED at `emit=9 box=9 rect=0 jobs=9` — but it now means what it says, and +`rect=0` is genuinely the unpack-ring policy's output rather than a bounding-box accident. + +**The red-once after the fix is ZERO-PATCH**, which is stronger than any throwaway source edit: ``` -DirectGLES.TextureUploadShape. ... FAILED - the BOX/RECT split moved: 6 box emissions against a gold of 9 - the BOX/RECT split moved: 3 rect-list emissions against a gold of 0 - the JOB count moved: 12 against a gold of 9 - tex[emit=9 box=6 rect=3 jobs=12] +MOBILEGL_ESPRYT_DISABLE_UNPACK_RING=1 + tex[emit=9 box=3 rect=6 jobs=183] bytes/f[tex=9024, was 104448] + the BOX/RECT split moved: 3 box emissions against a gold of 9 + the BOX/RECT split moved: 6 rect-list emissions against a gold of 0 + the JOB count moved: 183 against a gold of 9 ``` -and all 26 pixel cases of `TextureView` / `TextureViewAlias` / `SampledSetStaleness` / -`ImageSizeAfterRespec` / `CopyImageLevelRange` / `ClearThenReadPixels` stay GREEN under the same -patch — the required property, and the point of the gate: SSIM and every pixel assertion are blind -to a shape that inverted. +**Which texture moved: BOTH SCATTER TEXTURES.** `box=3` is the contiguous control band alone +(one contiguous region -> one rect -> not >= 2 -> box), `rect=6` is the plain scatter atlas AND +the atlas written through the `glTextureView`, three frames each, and `jobs=183 = 3 + 6 x 30` +confirms thirty rects per scatter emission. All 26 pixel cases of `TextureView` / +`TextureViewAlias` / `SampledSetStaleness` / `ImageSizeAfterRespec` / `CopyImageLevelRange` / +`ClearThenReadPixels` stay GREEN under the same knob — the required property, and the point of +the gate: every pixel assertion is blind to a shape that inverted. ### The Mali frame-time delta is NOT published