mirror of
https://github.com/MobileGL-Dev/MobileGL
synced 2026-11-15 22:13:24 +09:00
[MG_IntegrationTest, docs] (P7): the upload-shape gate was measuring its own bounding box; inset the scatter so the box/rect decision is reachable
Rework round 2 of 4 after the D2 review (fable, ID-P7-8), which found that this gate's
numbers were right and its SUBJECT was wrong.
The scatter wrote 40 2x2 rects at x=((i*7)%32)*2, y=((i*5)%32)*2 into a 64-px atlas.
Both strides are surjective onto 0..31 over 40 iterations - rect 9 lands at x=62, rect 19
at y=62 - 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 on every arm before
:8649's unpack-ring policy was ever consulted. The box-versus-rect decision the scenario
exists to pin was never reached for the two textures it scatters into, and the only
texture that could move under any red-once was the contiguous control band - which is
exactly what the old substitute red-once's box=6 rect=3 jobs=12 was.
kScatterMargin insets the scatter by one rect: same 40 writes, same coarse stride, union
box bounded at (2,2)..(62,54). The gold row is UNCHANGED at emit=9 box=9 rect=0 jobs=9 on
all four arms - but it now means what it says.
THE RED-ONCE IS NOW ZERO-PATCH, which is stronger than the throwaway source edit it
replaces: MOBILEGL_ESPRYT_DISABLE_UNPACK_RING=1 alone gives box=3 rect=6 jobs=183 with
bytes/f[tex] falling from 104448 to 9024, while all 26 pixel cases stay green. BOTH
SCATTER TEXTURES moved - box=3 is the contiguous band alone, rect=6 is the plain atlas
AND the atlas written through the glTextureView, and jobs=183 = 3 + 6x30 confirms thirty
rects per scatter emission. The same knob was green before the inset, which is the
evidence that CoversWholeLevel was short-circuiting the decision.
The three prose sites are rewritten accordingly: the scenario's gold-table rationale, the
MEASUREMENTS.md 13.1 reading and the package note's red-once section now explain
attempts 1-3 as mutations of a decision the client already makes correctly, and attempt 4
by CoversWholeLevel rather than by the ring.
Also PipeStatsWindow gains LastFrom{Client,Server}LogSince, and the gate reads its two
role windows since a mark taken before the counted workload. The server writes an EMPTY
teardown window after the counted one, so "the last line in the file" is one shutdown
away from reporting four zeroes as the workload's shape. Moving to a marked read exposed
a second latent defect in the same assertion: ASSERT_TRUE(clientWindow.found) over the
whole file was satisfied by the "counters ON" BANNER in all three split arms, claiming a
window had been read when none had. The assertion is now on the window that actually
carries the tex[] bracket.
This commit is contained in:
@@ -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 -
|
||||
|
||||
@@ -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());
|
||||
|
||||
|
||||
@@ -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 半边的读法（本包的修正）
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user