mirror of
https://github.com/MobileGL-Dev/MobileGL
synced 2026-09-08 20:28:32 +09:00
[Docs] (Disaggregated): consolidate into a single MGPipe plan and drop the replica plan
- docs/Disaggregated/PLAN.md is now the one plan: the MGPipe design with the transport, control-plane, present, threading, EGL/process, monolith/build chapters inlined as real chapters (7-13) instead of references, sections renumbered 0-17 + appendices, every internal citation updated - the replica-GLContext plan and its review record are removed at the user's request; REVIEW.md is the MGPipe design-competition and adversarial-review record only, with the comparison verdicts dropped and the remaining finding text reworded to the new section numbers - day-43 GO/NO-GO now names its two outcomes (continue / shrink to headless tooling or re-evaluate) without any rollback path
This commit is contained in:
File diff suppressed because it is too large
Load Diff
+1941
-814
File diff suppressed because it is too large
Load Diff
File diff suppressed because one or more lines are too long
+274
-237
@@ -1,265 +1,302 @@
|
||||
# 拆分设计评审记录(feat/disaggregated)
|
||||
# 拆分设计评审记录(MGPipe)
|
||||
|
||||
> 生成于 2026-09-05,配合 `PLAN.md` 阅读。记录设计竞标的结论、对抗性审查发现及其处置,便于日后追溯"为什么是这个方案"。
|
||||
> 生成于 2026-09-05,配合同目录 `PLAN.md` 阅读。这一轮的前提是用户的方向修正:backend 应拥有贴近后端 API 的状态机并暴露 gallium 式显式接口;memo/`SharedPtr`/版本计数器无 wire 对应物是要解决的工程问题,不是否定薄后端的理由。
|
||||
|
||||
## 1. 候选方案与评分
|
||||
|
||||
四个独立架构方案(各自从不同角度出发),三位评审按 7 项加权打分(性能/roundtrip 0.20、实现成本与风险 0.20、GL 语义完整性 0.20、跨平台 0.10、monolith 保留 0.10、可测试/可增量 0.10、复用既有工作 0.10)。
|
||||
三个独立方案,三位评审按 5 项加权打分(边界清晰度/架构价值 0.25、改造成本与风险 0.20、性能 0.15、语义完整性 0.20、可增量/monolith 保留/可测试 0.20)。
|
||||
|
||||
| 方案 | 角度 | 三位评审加权分 |
|
||||
|---|---|---|
|
||||
| Replica-Server Disaggregation: a risk-first vertical slice to OpenRA-on-device, then one hard semantic at a time | Risk-first incremental delivery. The server is the same libMobileGL binary running a real MG_State GLContext driven by a | 8.6 / 8.8 / 8.9 |
|
||||
| MG_Mirror: a thin, delta-fed state model inside the server — split without rewriting the backends | Thin server / delta-consuming backend. The server owns its own state model (MG_Mirror, ~4k lines, zero MG_State translat | 6.4 / 6.3 / 6.6 |
|
||||
| MG_Wire: Disaggregating MobileGL by replaying MG_State mutations into a server-side replica GLContext | Replica-state first: the server process runs an unmodified MobileGL backend against its own MG_State::pGLContext, recons | 8 / 7.4 / 7.2 |
|
||||
| Wire: a replayed GL command stream over a lock-free shared-memory ring | Command-stream / render-thread first. The primary channel is an SPSC lock-free shared-memory ring carrying fixed-layout | 8 / 7.8 / 7.4 / 7.7 / 7.2 / 7.5 |
|
||||
|
||||
三位评审一致选择 **Replica-Server / risk-first**(server = 未改动 backend + replica `GLContext`)作为基底,并嫁接其余方案的要点:Design 1 的 per-level `serverAuthoritative` 位与 composite pipeline program 处理、Design 2 的 `nm --defined-only` + `.text` size monolith 门与 DirectVulkan 内部 shader 构建期烘焙、Design 3 的 SPSC shm ring + FlatBuffers `struct` 热路径与 shadow-in-shm 前移。
|
||||
| MGPipe: a split-first explicit backend interface (server owns its state machine, no MG_State replica) | SPLIT-FIRST PRAGMATIC. Keep PLAN.md's transport/data-plane/sync/present/threading/platform/build design essentially verbatim, and replace on | 8.2 / 8.8 / 8.4 |
|
||||
| MGPipe: a gallium-faithful explicit interface for MobileGL | GALLIUM-FAITHFUL. Introduce MGPipe — an MGPipeScreen/MGPipeContext pair modelled directly on pipe_screen/pipe_context (CSOs with create/bind | 7.3 / 7.65 / 7.7 |
|
||||
| MGPipe: a twin-derived explicit backend interface for MobileGL | Backend-native state machine first. The interface is not designed top-down from gallium; it is read off the memo/snapshot/twin structures Di | 8.45 / 8.6 / 8.25 |
|
||||
|
||||
### 评审指出的致命缺陷(已在综合稿中处理)
|
||||
|
||||
- DESIGN 2 — the conformance gate cannot detect the failure it exists to prevent. Its generated static_asserts check is_same_v on accessor SIGNATURES and sizeof/alignof/offsetof on shared PODs. Neither verifies SEMANTICS. A mirror IsStorageDirty, a dirty-rect merge, a change-serial bump rule or a persistent-map state transition that behaves differently from MG_State compiles clean, passes every assert, and renders wrong. This is the whole risk of the design and its named mitigation does not address it.
|
||||
- DESIGN 2 — the mirror's scope is materially under-counted. I grepped MG_Backend: the backends call 15 distinct MUTATOR families on frontend objects across 94 sites, not just readers — SyncPersistentMappedRange x20 (which re-enters BufferBackendOps::FlushMappedRange, so the mirror BufferObject must reproduce the entire persistent-map state machine), MarkStorageDirty x19, AllocateStorage x8, SetInternalFormat x7, WritebackFromBackend x8, EnsureGpuResidentStorage x3, UpdateMipmapSubData. The design's ~450-line BufferObject and ~1,100-line texture estimates do not cover this.
|
||||
- DESIGN 2 — it never mentions GetProgramForDraw's composite-pipeline path. Core.cpp:592-660 joins every stage program, computes ComputeDrawProgramSignature, and on a cache miss constructs and LINKS an unnamed ProgramObject(0u), mirroring uniform values and block bindings into it. The mirror's ~700-line ProgramObject must reproduce all of it, or every program-pipeline application breaks. Unpriced.
|
||||
- DESIGN 3 — GL-name-space divergence is detected, not prevented. The whole identity model rests on both processes running the same IndexGenerator over the same call sequence, including on-demand object creation inside BindBuffer_State. The mitigation is a periodic XOR checksum of live names. That catches drift after the fact; Designs 1 and 4 prevent it structurally by carrying the name and lifetime id in an explicit create record and calling ctx.CreateBufferObject(name) directly. For a silent-corruption failure mode, prevention is the correct choice.
|
||||
- DESIGN 3 — the texture path contradicts the replay premise. Section 3.4 states pixels are already resolved client-side by ProcessTexturePixelsDataUnpack and that the record carries 'level identity plus the dirty description, not the upload plan: {name, target, level, unionBox, rectCount, rects}'. That is a delta, not a replay of glTexSubImage2D, so the generator's claim to cover the entry-point table mechanically does not hold for the texture family — and the design never specifies how the server's replica MipmapStorage obtains the bytes (it says only 'route their allocator to SEG_SHADOW the same way', which for buffers required an explicit new PipeResource kSharedShadow mode that is never specified for textures).
|
||||
- DESIGN 3 — the emit-after-error rule silently drops partial-effect calls. Skipping the record when PendingErrorCount moved is conservative for the common case but wrong for the spec-level exceptions the design itself acknowledges, and the only detector named is a CTS run at P4.
|
||||
- DESIGN 1 — hooks are hand-placed inside MG_State mutators, i.e. inside the state authority, and a missed mutator is a silent divergence with no structural detector. Ops.def plus sizeof tripwires catch SCHEMA drift, not HOOK OMISSION. The design says so honestly, but it is the largest residual risk in the winner-adjacent option and it is why Design 4's approach (emit at the ~152 already-enumerated GLFunctionsTable/BufferBackendOps boundary sites, replay via mutators on the server) is the safer placement of the same idea.
|
||||
- DESIGN 4 — the reflectionDigest is too narrow to catch the divergence it is designed to catch. It hashes uniform (name, location, type) triples plus maxUniformLocation and the XFB layout. It does not cover the generated SPIR-V itself, nor uniformIndexInTProgram, explicitProgramOpaqueBindings or storageBlocksWithoutBinding. This project's own bisect history records that glslang reflection and generation ORDER is load-bearing and that desktop byte-identity is a corpus-limited false green — so a server relink could produce different SPIR-V, pass the digest, and render wrong. Fix: extend the digest to an xxHash over the SPIR-V modules and the full LinkArtifacts field set, and make it a hard Fatal, which the design already does for the narrow version.
|
||||
- ALL FOUR — none notes that ProgramObject.h transitively includes ShaderObject.h -> ShaderCompileTask.h and SpvcSession.h (verified). Any server that links a real MG_State ProgramObject therefore pulls the shader-compile machinery whether or not it runs it. This only invalidates a binary-size argument, and only Design 2 makes one (which it solves by not linking MG_State at all), but every plan that claims a 'glslang-free server' after ProgramPublish should verify it with nm rather than assert it.
|
||||
- DESIGN 2 — the 'zero MG_State symbols in the server' gate is contradicted by three verified sites the design under-costs: VulkanRenderer.cpp:4214/4222/4290/4300 construct MG_State::GLState::ShaderObject and :4233/:4313 call ->Link(false); UniformManager.cpp:161-179 constructs 8 MG_State::GLState::TextureObject* kinds; DirectGLES.cpp:170 constructs a free-standing MG_State::GLState::SamplerObject(0). The design budgets ~145 lines for the first and '0 logic change' for UniformManager — but zero lines in UniformManager means the mirror must implement AllocateStorage / SetInternalFormat / UpdateMipmapSubData / MarkStorageDirty across 8 texture kinds with faithful MipmapStorage semantics. The cost is not eliminated, only moved into the line-count estimate it is missing from.
|
||||
- DESIGN 2 (the decisive one) — the conformance generator closes only the half of drift a compiler can see. is_same_v on accessor signatures and sizeof/alignof/offsetof on PODs cannot detect BEHAVIOURAL divergence in MipmapStorage::InsertDirtyRect's cascade-merge and the summedArea*4 >= unionArea*3 threshold (MipmapStorage.cpp:287-312), VecRange1D::Add's 7%-of-span gap ratio, or PipeResource::ResizeShadow's bit_ceil. The backend consumes all of these directly (Managers.cpp:4304-4310, :1891-1970), and this is precisely the area where the project has already measured a +6 ms/frame cliff between rect-list and union-box upload shapes (Managers.cpp:4311-4319). The design names drift as its 'single real risk' and then mitigates only the mechanical half.
|
||||
- DESIGN 2 — mirror sizing. Verified: TextureState 3,144 lines, ProgramState 7,992, BufferState 1,154. The ~5,350-line total mirror estimate is defensible for ProgramObject (most of ProgramState is glslang link tasks and the job graph, which the mirror does not need) but not for textures, where the code the backend depends on is behaviour rather than generation. Expect ~8-10k lines, i.e. optimistic by roughly 2x.
|
||||
- DESIGN 3 — GL names as wire identity make name-space divergence a SILENT-CORRUPTION class. The mitigation (a per-kind XOR checksum of live names every 4096 records) detects the fault up to 4096 records after it happens, i.e. after a frame or more of wrong pixels. Every other design uses never-reused GetLifetimeId() handles with explicit create/delete records, which cannot drift by construction. Mitigable by auditing every batch instead of every 4096 records, but it is a structural weakness of the entry-point-replay approach, not an implementation detail.
|
||||
- DESIGN 3 — the claim that a thread_local pGLContext changes 'zero of the 1494 call sites' is false. I count 65 non-arrow uses across MG_Impl / MG_Backend / MG_State: GL_Debug.cpp:99 (.get()), GL_Program.cpp:1630 (== nullptr), DirectGLES.cpp:146 (.get()), Managers.cpp:3608/3737/3808/4663/7120/7128/7131/8678 (truthiness), BackendObject_DirectVulkan.cpp:388/788, DirectVulkan.cpp:347-386 (MOBILEGL_ASSERT). An operator-> shim needs get(), operator bool and null comparison too. Not fatal, but the claim is, and .get() returning a thread-local pointer changes lifetime semantics that MOBILEGL_ASSERT sites depend on.
|
||||
- DESIGN 3 — composite pipeline programs are unaddressed. GLContext::GetProgramForDraw() links a NEW ProgramObject on a pipeline cache miss (Core.cpp:592-640). Under entry-point replay the server independently reaches that miss and links its own composite, allocating a name from ITS IndexGenerator — a second, undiscussed source of exactly the name-space divergence the design's audit is meant to catch. Design 1 is the only design that solves this explicitly.
|
||||
- DESIGN 3 — the 11.6-week estimate is not credible for the scope: a 682-opcode generator plus ~40 hand normalizers, a rewrite of 312 _State forwarders plus ~367 new wrappers, a TLS refactor of the state authority, a name-audit subsystem, a shm ring with mirror-mapping, SCM_RIGHTS, three adoption tiers, Android packaging AND a production Service. Read it as 16-18 weeks and re-baseline the phase gates accordingly.
|
||||
- DESIGN 1 — the ~150 recorder hooks are the least enumerable completeness surface in the replica family. Unlike GL entry points (a machine-readable 682-line macro table, verified) there is no single list of MG_State mutators to generate from; I count 76 mutator-shaped methods in Core.h alone with the remainder spread across BufferObject, MipmapStorage, VertexArrayObject, FramebufferObject, SamplerObject and ProgramObject. MGWIRE_MUTATOR_COUNT is a cardinality tripwire, not a coverage proof: a hook placed on the wrong side of a mutator, or a mutator with a side effect on a sibling object, passes it.
|
||||
- DESIGN 1 — InstallPublishedLink's Visit() + sizeof static_assert is explicitly acknowledged to miss any LinkArtifacts field change that does not alter sizeof. Since LinkArtifacts drives every uniform location and every XFB stride, a silent miss produces misrouted glUniform deltas with no diagnostic. It needs Design 4's reflectionDigest cross-check as a runtime companion.
|
||||
- DESIGN 1 — the in-process mode's std::swap(pGLContext, m_replica) around ApplyBatch is a data race if anything on the app thread touches pGLContext concurrently. It is confined to a test transport, but it makes the in-process oracle less trustworthy than Design 3's TLS or Design 4's separate-process-first ladder — and an untrustworthy oracle is worse than none when it is the primary correctness gate for Phase 1.
|
||||
- DESIGN 4 — reconciler completeness has only a TEST tripwire (Phase-2 name-for-name parity), not a build tripwire. Anything the backend gates on that the WireMirror forgets to walk diverges silently until a scenario happens to exercise it. This is the winner's single weakest point and is why grafting Design 3's generated command table and Design 2's generated read-surface assert is not optional.
|
||||
- DESIGN 4 — Phase 1's 'server relinks from source' runs glslang and a full compile pool in BOTH processes for Phases 1-4, on a platform whose existing compile pool is already clamped to 4 workers purely as an RSS ceiling (ShaderCompilePool.h:77-82). The Phase-1 device gate is a single OpenRA trace so it will pass; Minecraft would not. This should be stated as an explicit Phase-1 non-goal so nobody measures MC before Phase 5.
|
||||
- DESIGN 4 — like Design 3, it never addresses the composite pipeline link at Core.cpp:592-640. Its applier calls the backend table, the backend calls GetProgramForDraw(), and on a pipeline cache miss the SERVER links a composite ProgramObject. This must be either client-resolved (Design 1's mechanism) or explicitly banned with an assert; leaving it implicit is a latent divergence.
|
||||
- CROSS-CUTTING (credit where due) — all four designs correctly diagnose that Feat/CS-Delta-IPC never implemented SCM_RIGHTS (LocalSocketTransport.cpp:296 hardcodes fd = -1), so its data plane could not move a byte cross-process on Linux or Android; that ServerHost/main.cpp does not compile while being in the default ALL target, so the branch tip cannot build; and that the committed per-draw fprintf(stderr) at DirectGLES.cpp:+2583-2590 poisoned every measurement taken on that branch. All four schedule fd-passing in the first transport commit and all four remove the uncommitted [IBOTX]/[BUFTX] fprintfs before baselining. None of the four repeats the prior branch's inversion of landing a state-model refactor before a triangle renders.
|
||||
- D2 — TextureLevelPull is a novel synchronous reverse stall in the middle of a draw, and its dismissal is wrong. Because the mirror deliberately does not retain texel bytes, any server-side driver-object re-mint must ask the client to re-send. D2 pre-empts only one of three causes (RequireImageBindableStorage, via imageBindableHint); it dismisses full format regeneration (Managers.cpp:3950-4195) with 'already re-uploads every level today, so it is not a new cost class'. That is false across processes: in the monolith the bytes are in the same address space and the re-upload is free; in the split it is a blocking server-initiated round trip the client did not initiate and cannot predict, on a path that fires on ordinary glTexImage format changes. This is the only genuinely new stall class any of the four designs introduces.
|
||||
- D2 — the drift guard is narrower than advertised. D2 claims divergence between MG_Mirror and MG_State is 'a build error, not a review item'. The generated is_same_v/sizeof/alignof/offsetof asserts catch signature and layout drift only. They cannot catch behavioural drift in the ~1,100 lines of mirror texture logic that reimplement IsStorageDirty/MarkStorageDirty/GetStorageDirtyRegion/GetStorageDirtyRects, including the 96-rect cascade-merge and the summedArea*4 >= unionArea*3 fallback. A semantic change to MipmapStorage compiles clean and renders wrong.
|
||||
- D3 — 'zero of the 1494 call sites change' is not accurate, and the shim is harder than stated. I measured ~71 non-arrow uses of pGLContext (34 as a passed argument, 3 as pGLContext.<member>, plus the assignment at GLState/Core.cpp:20 and the deliberately-leaked definition at :1487). Crucially the declared type is `extern UniquePtr<GLState::GLContext>&`, not a pointer, so a thread-local shim must emulate operator->, get(), operator=, operator bool and reference binding, and the Init/Destroy lifetime path must be reworked. Small in absolute terms, but it is presented as free and it lands on the monolith's hottest access path.
|
||||
- D3 — the divergence oracle is disabled precisely where the divergence risk lives. D3's correctness rests on GL name-space determinism holding across 682 entry points, on-demand object creation in BindBuffer_State, internal cross-domain GLImpl:: calls, and an emit-after-error rule. Its two guards are periodic name-set checksums and MOBILEGL_WIRE_VALIDATE_SERVER — but the latter is explicitly CI-only, short-circuited in shipping builds by kPrevalidated + g_wireSkipValidation. A shipped build therefore turns a name-space divergence into undefined behaviour rather than a GL error, with silent wrong pixels as the symptom.
|
||||
- D3 — the schedule is not credible for the stated scope. P1 is budgeted at 2.0 weeks for: a generator over all 682 entry points, ~40 hand-written pointer normalizers, one-line wrappers for the ~367 entry points that have no _State counterpart, the W-AUDIT-1 internal-caller audit across MG_Impl and MG_Backend, the server-side validation oracle, name-audit records, and EmitFullSnapshot. The total 11.6 weeks is the most aggressive of the four for the largest protocol surface of the four.
|
||||
- D1 — the persistent-map defaults are inverted. For coherent persistent maps D1 ships option (b), a 4KiB-block xxHash-gated whole-mapped-range copy, as the v1 default, with option (c) 'decline the persistent bit' behind a config switch. SyncPersistentMappedRange() is invoked by the backend at draw time (Managers.cpp:1547, DirectGLES.cpp:262/4412/4666-4667/4768-4769, MultiDraw.cpp:498), so (b) puts a hash scan of a potentially large mapped range on the per-draw path. The frontend already tolerates a null AcquirePersistentMap at three sites (BufferObject.cpp:174, 439-442, 470-472) and MOBILEGL_DISABLE_LARGE_BUFFER_ADOPTION already exists, so (c) is the correct default and (b) the opt-in.
|
||||
- D1 — the wire byte budget is internally inconsistent. It states ~35KB of opcode stream for a 3000-draw frame while also specifying 24-40B draw records and a rule that binds are never coalesced. 3000 draws alone is 72-120KB before any state or bind traffic. Does not change the ranking (the volume is small either way) but it is an unverified figure presented as a budget in a design that otherwise cites line numbers.
|
||||
- D4 (winner, so worth naming) — the reflectionDigest gate is narrower than the divergence it must catch. During Phases 1-4 the server relinks from source, and the digest covers only (uniformName, location, type) triples plus maxUniformLocation and the XFB layout. The backends additionally read GetUniformTypeFacts, GetUniformSamplerOrImageUnitIndex, GetUniformBlockBinding, GetShaderStorageBlockBindingOverrides, PointSizeDemoted, GetTransformFeedbackStride and GetTransformFeedbackPackedStride. A relink divergence in any of those passes the gate silently. Widen the digest to the full backend-read reflection set before Phase 1 lands.
|
||||
- D4 (winner) — server-side RSS during Phases 1-4 is unaccounted for. Relinking from source means a second full glslang link pipeline plus its arenas in a second process, on a device where ShaderCompilePool is already clamped to 4 workers purely as an RSS ceiling (ShaderCompilePool.h:77-82) and where the project has an LMK-kill history. The exposure peaks during MC startup, which links hundreds of programs — exactly the Phase 5 workload D4 measures. Add a server-RSS acceptance bound to Phase 1, not only to Phase 5.
|
||||
- Cross-cutting (all four, but D1/D2/D4 most): none of the four commits to a measured per-frame byte or call-volume number, because none exists in the tree — MG_Util/Metrics is format arithmetic only (BufferMetrics.h:14-27, TextureMetrics.h:15-44), Tracy has zones but no TracyPlot counters, and the MC 26.3 campaign's PANDIAG/STALLDIAG instrumentation is gone. Every ring size, batch threshold, inline-vs-shm cutoff and frames-ahead credit in all four designs is therefore an estimate. Whichever design proceeds should land the byte counters in its FIRST phase, not (as D2, D3 and D4 all schedule it) in a later performance phase.
|
||||
- Design 1 — internal schedule contradiction, and it is the axis this review weighs hardest. Its comparison section claims 'the earliest honest IPC frame on a trivial workload is day ~45-55, and a Minecraft frame ~day 120+'. Its own phase list places the first IPC frame in P11, which follows P0-P10 (8-11 + 10-14 + 8-11 + 12-16 + 12-16 + 9-12 + 35-44 + 24-30 + 26-33 + 8-12 + 10-14 = 217-283 days). The phase list is the binding artifact, so the real first frame is ~day 220. A plan that asks for 260-340 engineer-days with zero IPC value for ten months, against a verified 77-day alternative (PLAN.md P0..P9 sums to exactly 77), will be rejected on schedule regardless of its architectural merit — and its own comparison text obscures that rather than confronting it.
|
||||
- Design 1 — it takes the one gallium deviation the tree argues against, and takes it on the hottest path. Decomposing RenderStateParameters into blend/depth_stencil/rasterizer CSOs discards a documented layout invariant (ScissorBoxWrittenMask at RenderState.h:363 and ClipDistanceEnabledMask at :369 were deliberately placed in the tail span after LogicOp so DirectGLES's three-span memcmp at :2035-2046 catches them) and turns one 8-byte version compare into three hash computations plus three lookups per state transition. Content-addressing answers the correctness half but not the cost half, and DirectGLES still needs the blob per CSO anyway to diff against the driver and emit only changed GL calls — so the decomposition buys the server a handle compare while the client pays three hashes. Not fatal to the architecture; fatal to the claim that this is the cheapest shape.
|
||||
- Design 3 — the residual value block is a live semantic hole during the P5-P8 split window with only half a guard. The poison mask catches UNFILLED fields; it does not catch a block whose layout differs between the emitting client and the applying server, which is exactly the failure a union of heterogeneous PODs invites across a compiler/ABI boundary. The design specifies static_assert on sizeof but not on member offsets. Without per-member offsetof asserts (or serializing the block field-wise rather than memcpying it), a padding difference produces silently wrong render state in split mode that the monolith verify harness cannot see, because in monolith mode both sides are the same translation unit.
|
||||
- Design 3 — P7 (DirectVulkan, 48 days) is roughly half the independent 85-111 estimate for the same work, and it sits on the critical path for the second backend's split support. The design names this honestly and makes P3a the falsification point, which is the right response, but the 192-day total should be read as 192-260 and the plan should state that a P3a overrun by more than 50% re-baselines the whole schedule before P4a starts — which it says, but only in the risk list, not in the headline number.
|
||||
- All three — the central performance claim is unfalsified and cannot be settled from the tree. Every design argues the per-draw reachability traversal MOVES to the client rather than doubling (as the replica plan's does), and therefore that net CPU is <= monolith. Nothing in the tree measures per-frame bytes or calls: MG_Util/Metrics is format arithmetic and Tracy has zones but no plots. All three correctly put TracyPlot counters in P0, and all three correctly nominate per-thread CPU time rather than wall-clock frame time as the metric. But until those land, every ring size, every batching threshold, the render-state wire granularity decision and the headline CPU argument are estimates. Any adopted plan must treat the P0 counters as a hard prerequisite, not a nice-to-have.
|
||||
- All three — the server-initiated texture re-mint pull is a genuinely new stall class that the replica plan does not have, and its rate on the real corpus is unmeasured by all three. imageBindableHint pre-empts RequireImageBindableStorage (Managers.cpp:2813), but full format regeneration (:3950-4195) fires on ordinary glTexImage format changes and is not pre-emptible. All three ship the same three mitigations (hint, asynchronous park-and-re-emit so the stall lands on the apply thread, bounded retention LRU) and all three gate it with a scenario plus a published per-case pull counter, which is the right shape. The residual risk is identical across designs and should be tracked as a portfolio risk, not scored against any one of them.
|
||||
- Design 1 — the render-state CSO decomposition is wrong and its justification is internally inconsistent. I verified both halves of the counter-evidence: DirectGLES.cpp:2025-2050 does a three-span head/blend/tail memcmp guarded by static_assert(is_trivially_copyable_v<RenderStateParameters>), and RenderState.h:355-370 states verbatim that ScissorBoxWrittenMask and ClipDistanceEnabledMask were placed 'Deliberately beside ScissorBoxes so it shares their tail span (after LogicOp) and DirectGLES' span memcmp picks a transition up like any other state.' Design 1 §5.4 then proposes hashing 'the three spans DirectGLES already memcmps' to obtain three CSO handles — but head/blend/tail is not the blend/depth-stencil/rasterizer partition, so the proposed mechanism cannot produce the proposed handles. Beyond the inconsistency, decomposition introduces a hand-maintained field→CSO partition over a ~150-field struct with no completeness tripwire: a field added to RenderStateParameters and not assigned to a CSO is silently never pushed, whereas under the blob it rides along and a sizeof static_assert catches schema drift. Not fatal to the design as a whole — replace this one entry with Design 3's create/bind_render_state and Design 1 becomes competitive.
|
||||
- Design 2 — handle/data-structure mismatch. MGHandle is defined as the monotone, never-reused GetLifetimeId() (8 B), and the design then claims the six StateBackendObjectRegistry instances and thirteen Magma caches become 'arrays indexed by handle' and that this is what deletes TwinLookupMemo/OwnerEquals/g_fbSlotCache. A sparse monotone u64 cannot index an array; without a dense per-kind slot allocator the server keeps a hash map and retains most of the lookup cost the design books as deleted. The fix is Design 3's PipeHandle{slot, gen} with per-kind dense slots plus reserved bands — same 8 bytes, same ABA guarantee, and it actually delivers the array.
|
||||
- Design 2 — an asserted factual correction that is itself wrong. It opens by 'correcting' the evidence to 'exactly 71 function pointers plus one capability bool, GLFunctionsTable BackendObject.h:117-278 … not 67, not 73.' Measured: 67 function pointers in that range. Minor in substance, non-trivial in credibility for a design whose entire method is 'I re-measured the tree where the reports disagree.'
|
||||
- Design 3 — the day-62 milestone is narrower than it reads. Emulations (client vertex/index arrays, primitive-restart rewrite, indirect-count resolve, CopyImage mirror) are deliberately Fatal in split mode until P8, so 'first cross-process frame' means OpenRA on a reduced path. That is a legitimate engineering choice but it must be labelled at the go/no-go, or a stakeholder will read it as 'the split works' when the answer is 'the transport and five object classes work.'
|
||||
- Design 3 — the 192-day total is the least defensible number in the set, against a refactor-cost evidence range of 202-266 days for the backend work alone plus ~68 for IPC. The design concedes this and names a falsification (P3a overrun >50% ⇒ re-baseline before P4a), which is the right response, but the headline figure should be presented as a range with the P3a checkpoint attached.
|
||||
- All three — the central performance claim (the per-draw reachability traversal MOVES to the client and gets cheaper rather than doubling) is unmeasured, because the tree has no per-frame byte or call metric at all (MG_Util/Metrics is format arithmetic; Tracy has zones and no plots). All three correctly schedule TracyPlot counters in P0/M0 and all three correctly insist the metric be per-thread CPU time rather than wall clock. No design should be believed on CPU until that lands, and the first real datapoint (render state on both backends) must be a hard go/no-go, not a report.
|
||||
- All three — loss of PLAN.md's byte-identity monolith gate (nm --defined-only plus stripped .text equality) is unavoidable and all three say so explicitly. This is a shared cost, not a flaw of any one design, and the five-part replacement (purity grep + nm, per-draw field-wise MOBILEGL_PIPE_VERIFY, behavioural A/B across {monolith-pull, monolith-push, split}, per-thread CPU non-regression, coverage/poison/no-raw-pointer-memo asserts) is stronger semantically than what it replaces. It must be written down as a cost in the final doc, not buried.
|
||||
- DESIGN 1 — MAJOR, not strictly fatal but must be reversed before P0 freezes the header: decomposing RenderStateParameters into blend/depth_stencil/rasterizer CSOs (§1.2 D3, §3.2). Its own evidence contradicts it — RenderState.h:359-368 records that ScissorBoxWrittenMask and ClipDistanceEnabledMask were deliberately placed in the tail span so DirectGLES' three-span memcmp (DirectGLES.cpp:2035-2046, guarded by a static_assert(is_trivially_copyable_v) at :2033) picks a transition up like any other state. Espryt keeps a byte-for-byte value mirror precisely so it can emit only the changed GL calls, so the server must retain the blob per CSO regardless; the decomposition therefore buys a handle compare the versioned blob already provides and adds a span re-hash plus three cache lookups on every GetPipelineStateVersion move. Fix: adopt Design 2/3's versioned blob with a dirty-span mask (Design 3's client LRU makes a repeat cost 12 bytes), and let the server derive whatever CSOs it wants internally.
|
||||
- DESIGN 2 — CREDIBILITY, not architecture: the opening Verification note asserts 'GLFunctionsTable has exactly 71 function pointers plus one capability bool ... with Present/SetSwapInterval that is 74 members — not 67, not 73' and explicitly overrides the other reports. Measured at dev@81b17c0b: 67 function pointers + 1 Bool = 68 members, 70 with GlobalBackendFunctionsTable. It also states '50 include lines over 18 distinct MG_State headers' where I measure 50 lines over 15 distinct MG_State paths, and carries 169 DirectVulkan pGLContext reads where the actual count is 166 (VulkanRenderer 126 + DirectVulkan 18 + UniformManager 14 + VkRenderPassManager 3 + VkTextureManager 2 + BackendObject_DirectVulkan 2 + VkClearManager 1). A design whose central methodological claim is 'I re-derived this from the tree rather than copying the brief' cannot afford to be wrong in the one place it says so loudest. None of this invalidates the design, but every other unverified number in it now needs an independent check before it is used for sizing.
|
||||
- DESIGN 3 — SCHEDULE, acknowledged but under-absorbed: P7 (DirectVulkan, all subsystems) is priced at 48 days against the refactor-cost reader's 85-111 for the same scope, and the 192-day total sits below the reader's 202-266 for the backend refactor ALONE. Design 3 names this as a risk and supplies a falsification trigger (re-baseline if P3a overruns >50%), which is the right instinct, but the trigger fires on Espryt's wave-1 and cannot detect a Magma-specific overrun until P7 is already the critical path. Fix: add a second explicit re-baseline gate at P7 midpoint, and price the CTS turnaround (gl44to46 is ~56,271 cases) as a separate line rather than folding it into the phase estimates.
|
||||
- ALL THREE — completeness gap in the migration mechanism, shared and unaddressed: MG_Backend has 348 pGLContext mentions of which only 290 are arrow uses. All three designs propose a mechanical sed of 'MG_State::pGLContext->' to a macro/alias over '293 sites' and none accounts for the 58 non-arrow uses — the null-guards (Managers.cpp:3608, 3737, 3808, 4663, 8678; BackendObject_DirectVulkan.cpp:388, 788), the MOBILEGL_ASSERT truth tests, the raw-pointer capture at DirectGLES.cpp:146 (MG_State::GLState::GLContext* ctx = MG_State::pGLContext.get()), and the patch-parameter ternaries at Managers.cpp:7120-7131 that sit inside the transpile path. The patch reads are semantically covered by set_patch_state in all three catalogues, but the mechanical step is under-specified and the raw .get() capture defeats an accessor-shaped alias entirely. Whichever design is chosen must enumerate and convert those 58 sites explicitly, and the interface-purity gate must grep for 'pGLContext' (not 'pGLContext->').
|
||||
- NONE OF THE THREE is fatally incomplete on semantics. Each satisfies all 290 backend reads, both texture-byte channels, the 26 reverse pulls, XFB (CPU accounting client-side, capture writeback as a reply), queries and fences (client-minted, two-valued contract preserved), persistent maps (explicitly quarantined from the refactor, decided by a POST-probed tier), GPU-written buffer reads (conservative client pending set narrowed by an EvGpuWritten reply), share groups (one flat handle space in v1, screen/context split declared in the header from day one), and the composite pipeline program (never crosses; resolved by Core.cpp:592-744 as today). All three correctly identify the server-initiated texture re-mint pull as the one genuinely NEW stall class and mitigate it three ways with a dedicated gate and a per-trace-case counter.
|
||||
|
||||
### 评审建议嫁接的要点
|
||||
|
||||
- From Design 3 — the Track V / Track H accessor split. Roughly 55% of the class-B reads are value-typed (RenderStateParameters, PixelStoreParameters, IsCapabilityEnabled, GetStencilState, GetColorMaskIndexed, the ~22 Magma singletons) and need no reshaping whatsoever: the client memcpys, the server hands the backend a reference to its own copy. Only the 167 SharedPtr<MG_State...> points need real work. This is the decomposition that makes migration granularity one accessor rather than one subsystem, and it is the load-bearing premise under any split-first schedule. Neither Design 1 nor Design 2 states it.
|
||||
- From Design 3 — the residual value block with a compile-error retirement. One temporary set_residual_value_state carrying the union of not-yet-migrated value accessors, guarded by static_assert(sizeof(ResidualValueBlock) == MGL_RESIDUAL_BLOCK_SIZE) with the constant bumped DOWN each phase, ending at static_assert(sizeof(...) == 0). This is what lets the split run subsystem by subsystem instead of after a finished refactor, and it is the only temporary in any of the three designs with a mechanical (not procedural) retirement. Add the layout static_assert it omits: the block must be byte-identically laid out on both sides, so assert offsetof for every member, not only sizeof.
|
||||
- From Design 3 — the PipeInputs::m_filledMask poison. In debug and disaggregated builds, reading a field the tracker never pushed is Fatal{UnmigratedPipeInput, "GetStencilState"} on the first draw. Design 2's G5 written-once bitmask is the same idea, but Design 3's runtime-fatal formulation is the one that cannot be rendered past, and it works during the split window where Design 2's generated comparer needs both models live in one address space.
|
||||
- From Design 3 — the ordering rule that identity handle-ification precedes the first frame while memo re-keying follows it (P3a/P4a before P5/P6; P3b/P4b after). The wire needs handles; the 28 days of memo re-keying, dirty-flag inversion and program-staleness rework are optimizations that can land behind a working split. This single reordering is worth ~5 weeks of time-to-first-frame and neither other design exploits it.
|
||||
- From Design 3 — the explicit day-21 hedge: run PLAN.md's P0 verbatim (its hygiene, skeleton, spikes and byte counters are state-model-independent), then MGPipe P1+P2 (15 days), then decide. At day 21 you hold the verify harness proving push works, render state pushed on both backends, a measured monolith per-thread CPU delta on two devices, and the per-accessor cost of Track H sampled. That is a genuine, cheap decision point, and it is the only one offered in the set.
|
||||
- From Design 1 — the client-side content-addressed CSO cache modelled on Mesa's cso_context/cso_cache, with per-kind caps and LRU eviction issuing delete_*_state. Design 2's render-state LRU is the same idea applied to one blob; Design 1 generalizes it to vertex-elements, samplers and sampler views, and the property that two different programs setting identical state produce ZERO server-side transitions is a real per-draw win worth keeping even while shipping the render-state blob rather than three CSOs.
|
||||
- From Design 1 — the framing that inproc IS u_threaded_context: a push-only interface recorded into batches and applied on the server thread. Mesa proved this shape can be transparently threaded, and it reframes the monolith render-thread deliverable from 'an IPC side effect' to 'the interface's second consumer'. Worth stating explicitly in whatever plan is adopted, because it is the argument that the interface pays for itself even if the process split never ships.
|
||||
- From Design 1 — homing each emulation by gallium's own rule (state-tracker side when caps say the driver cannot, driver side when it is a driver lowering) with a named cap bit per decision: kCapPrimitiveRestart, kCapMultiDrawIndirectCount, kCapFloat64VertexAttrib, kCapNeedsHostIndexBytes. That turns the per-backend asymmetry (Magma's deliberately null ResidentSubData, the 8 null slots, PrefersCpuXfbPrimitiveAccounting) from a wart into the mechanism, and it replaces today's implicit slot-nullness capability probes at GL_Query.cpp:471/545/768.
|
||||
- From Design 2 — PipeCalls.def as one X-macro consumed by five generators (function table, monolith thunks, wire records with per-kind static_assert plus generated runtime bounds checks, the shadow-compare comparer, the written-once mask). Design 3 has the coverage generator but not the comparer/mask generators; generating the semantic gate from the same source as the call table is what stops the gate going stale as the catalogue grows.
|
||||
- From Design 2 — the D18 exception. Its D-class table is the only one that marks VkRenderPassManager::m_renderbufferResources / VkTextureManager::m_textureResources as UNCHANGED, with the reason (callers cache Resource* across further lookups; a table grow once relocated a cached &layout and BlitFramebuffer silently bailed at 'source image layout undefined'; ska's erase-shift makes it worse, not historical). Whichever plan is adopted must carry that postmortem verbatim into the review checklist, because converting those to slot arrays is exactly the change a refactor makes without reading the comment.
|
||||
- From Design 2 — the DERIVATION METHOD, adopted as the doc's opening chapter: build the call catalogue by inverting the backends' own key structures (SetupDrawSnapshot VulkanRenderer.h:948-1042, BackendTextureObject::IsDrawSyncClean Managers.h:1003-1020, ResolvedDrawBuffers Managers.h:697-717, ResolvedVertexBindings VulkanRenderer.h:1153-1218, g_syncedRenderStateParameters DirectGLES.cpp:1956, BufferBackendOps BufferObject.h:76-120), not top-down from gallium. This is both the honest justification for every entry and the reason the interface is complete: the inputs to those structures ARE the interface.
|
||||
- From Design 2 — PipeCalls.def as single source of truth with FIVE generators: function tables, monolith thunks, wire records with per-kind static_assert plus generated runtime bounds checks, the MOBILEGL_PIPE_VERIFY field-wise comparer, and the written-once bitmask. Generating both tripwires removes the hand-maintenance risk that is the design's own biggest exposure. Graft over Design 3's hand-written verify.
|
||||
- From Design 2 — the explicit two-kinds-of-generation statement: client-owned identity vs the twelve server-only epochs (g_bufferMutationEpoch, g_bufferBackendIdGeneration, g_attachmentBackendIdGeneration, g_backendContextGeneration, m_textureImageEpoch, m_resourceEraseEpoch, m_renderbufferImageEpoch, m_sliceEpochCounter, m_cacheStructureEpoch, m_evictionEpoch, m_recordingGeneration, m_frameSerial) that the client must never be asked about. Write this as a normative interface rule, not prose.
|
||||
- From Design 2 — D18 marked UNCHANGED with a review-checklist note: VkRenderPassManager::m_renderbufferResources and VkTextureManager::m_textureResources are deliberately node-based std::unordered_map, not the project's open-addressed UnorderedMap, because callers cache Resource* across further lookups (postmortem at VkRenderPassManager.h:375-397, a BlitFramebuffer silently bailing at 'source image layout undefined' after a table grow relocated a cached &layout). It is the only design that explicitly flags 'do not optimise this container back during the refactor.'
|
||||
- From Design 2 — the dirtySpanMask on the render-state wire. Compose with Design 3's CSO: on a CSO cache MISS ship only the changed spans of the blob plus the previous CSO handle as a base, rather than the full ~1.1 KiB. Cheapest of all three encodings.
|
||||
- From Design 1 — CAPS-GATED emulation homing, replacing fixed client/server assignment. MGPipeCaps carries kCapPrimitiveRestart, kCapPrimitiveRestartFixedIndex, kCapMultiDraw, kCapMultiDrawIndirectCount, kCapFloat64VertexAttrib, kCapResidentSubData, kCapCpuXfbPrimitiveAccounting, kCapNeedsHostIndexBytes, and each lowering (u_primconvert-style restart rewrite, indirect-count fallback, client-array upload) runs client-side only when the cap says the server cannot. This replaces today's implicit null-slot capability probes at GL_Query.cpp:471/545/768 and makes per-backend asymmetry (Magma's deliberately absent ResidentSubData, VkBufferManager.cpp:104-111) the mechanism rather than a wart.
|
||||
- From Design 1 — kCapNeedsHostIndexBytes specifically: it prices the monolith/split asymmetry of MGHostSpan honestly (a free pointer in-process, a copy on the wire) so a backend that never needs host index bytes does not pay.
|
||||
- From Design 1 — the explicit deviations-from-gallium table with a tree citation per row. Keep the format; replace only the render-state row with Design 3's blob-CSO.
|
||||
- From Design 3 — the render-state shape itself: create_render_state(cso, blob) + bind_render_state(cso, v, pipeV) with a client LRU. Graft into whichever design wins.
|
||||
- From Design 3 — PipeFramebufferState with a CLIENT-RESOLVED readSurface and inline attachment internalFormats. Two defect classes and one lookup deleted by struct shape alone.
|
||||
- From Design 3 — Track V / Track H accessor split, per-accessor migration granularity, and MOBILEGL_PIPE_PUSH as a per-subsystem bitmask latched at init like MOBILEGL_BACKEND_TYPE (ConfigLoader.cpp:212-225), so every commit has a same-binary A/B on either backend.
|
||||
- From Design 3 — every temporary gets a compile-error retirement: PipeInputs::m_filledMask poison giving Fatal{UnmigratedPipeInput, fieldName}, and static_assert(sizeof(ResidualValueBlock) == 0) before the pull path may be deleted. Adopt this rule wholesale; it is the difference between a strangler that finishes and one that ossifies.
|
||||
- From all three, unchanged — the EvLogLine severity split (level <= WARN lossy, level >= ERROR lossless plus a per-second rate limiter emitting 'N suppressed'), because backend program link failure is surfaced ONLY as MGLOG_E plus a bind-program-0 no-op (Managers.cpp:8091-8126, 8357-8372) and PLAN.md §7.4's uniform lossy policy would silently drop the system's most valuable diagnostic.
|
||||
- FROM DESIGN 2 — derive the interface from the backends' own key structures, not from gallium top-down. SetupDrawSnapshot (VulkanRenderer.h:948-1042) is a 40-field enumeration of everything Magma must have pinned for a draw; DrawTextureSyncKeys + IsDrawSyncClean (Managers.h:1003-1020) is the same for Espryt's textures; ResolvedDrawBuffers/ResolvedVertexBindings are the vertex-input statement; g_syncedRenderStateParameters is the render-state statement verbatim. This is a stronger completeness argument than any coverage table, and it is what produces the correct blob-not-CSO answer on render state. Design 3 should adopt this as the explicit derivation rationale for its call catalogue.
|
||||
- FROM DESIGN 2 — PipeCalls.def with five generators from one file: function table, monolith thunks, wire records + per-kind static_assert + generated runtime bounds checks, the MOBILEGL_PIPE_VERIFY field-wise comparer, and the written-once bitmask. Generating the verify comparer and the completeness tripwire from the same declaration as the call list means the gates cannot drift from the interface. Design 3 hand-writes both; it should generate them.
|
||||
- FROM DESIGN 2 — keying PipeInputs on MEMO KEYS rather than read sites. That is why the pushed block stays ~20 KB with a field set stable across the migration, and it is the reason per-accessor granularity actually works. Design 3's PipeInputs is described per-accessor, which is a larger and less stable field set.
|
||||
- FROM DESIGN 2 — D18 explicitly marked UNCHANGED with the VkRenderPassManager.h:375-397 postmortem carried verbatim into the review checklist, so nobody 'optimises' m_renderbufferResources/m_textureResources back to the project's open-addressed UnorderedMap. The ska erase-shift behaviour makes that hazard worse, not historical. Neither other design guards this.
|
||||
- FROM DESIGN 2 — MGHostSpan: one 32-byte accessor for the four host-byte classes (client vertex arrays, client index arrays, indirect/parameter command blocks, index bytes) whose fill policy differs by build. Zero monolith cost (one pointer load), and it is the abstraction that makes the disappearance of the 26 SyncPersistentMappedRange/SyncGpuWrites reverse pulls a mechanical consequence rather than a per-site argument.
|
||||
- FROM DESIGN 1 — the emulation-homing RULE (gallium's own: state-tracker lowering when a cap says the driver cannot, driver lowering when the driver forces it), with each emulation gated on a named capability bit — kCapPrimitiveRestart, kCapMultiDrawIndirectCount, kCapFloat64VertexAttrib, kCapNeedsHostIndexBytes. Designs 2 and 3 assign emulation ownership case by case; Design 1's rule generalises to a third backend and makes the assignment auditable.
|
||||
- FROM DESIGN 1 — kCapNeedsHostIndexBytes specifically: it prices the monolith-vs-split asymmetry (a shadow pointer costs nothing in-process, a copy in split) into the interface as a capability, so a backend that never needs host index bytes never pays.
|
||||
- FROM DESIGN 1 — the explicit 8-deviation ledger (each deviation from gallium named, justified by a file:line or a measured cliff, and numbered). This is the right way to document an interface that will outlive its authors; Designs 2 and 3 justify their deviations inline and less traceably.
|
||||
- FROM DESIGN 1 — MGPipeCallbacks as a single named struct of 8 reply/event kinds installed at context_create, rather than an ad-hoc event list. In the monolith they are direct calls; in split they are records. This makes the reverse channel a first-class part of the interface rather than an appendix.
|
||||
- FROM DESIGN 3 (keep) — dense per-kind slots in an 8-byte PipeHandle{slot, gen}. Designs 1 and 2 use sparse 64-bit lifetime ids as the wire handle, which keeps the server on a hash table; dense slots make the server's object tables literal arrays, which is what actually deletes the hashing/ABA layer rather than merely re-keying it. The lifetime id stays client-side as the tracker's own identity.
|
||||
- FROM DESIGN 3 (keep) — client-resolved readSurface in the framebuffer payload, and static_assert(sizeof(ResidualValueBlock)==0) as the retirement device for a deliberate temporary.
|
||||
|
||||
## 2. 对抗性审查(三个视角)
|
||||
|
||||
### GL 语义正确性(refuted=False,13 条)
|
||||
### GL 语义正确性(refuted=False,12 条)
|
||||
|
||||
- **[major] P1's day-13 on-device milestone omits the entire Android delivery chain it depends on**
|
||||
- 问题:§15 P1 lists only C++ components (WireMirror, EmitTable, BackendObject_Remote, ReplicaContext, Applier, ServerLoop, ServerMain, spawn) yet its acceptance step 3 is `run_android_retrace_local.py --case OpenRA --backend DirectGLES` on 35d0befa in split mode. That path needs three things not in the deliverables. (a) Env plumbing: every MobileGL knob reaching the device is an explicit Intent extra threaded through five files — `android-plugin/trace-replay-ci.sh:368-420` builds `--es/--ez` extras one by one, `android-plugin/app/src/trace/cpp/trace_replay_core.cpp:134-207` is a hand-written `setenv` list, plus TraceReplayActivity.java, the JNI Request marshalling, and `run_android_retrace_local.py:122-201`. There is no generic env passthrough. (b) Server packaging: the plugin APK's native build is split between `android-plugin/app/src/trace/cpp/CMakeLists.txt` (app module) and the root `CMakeLists.txt` via `implementation(project(":MobileGL"))`; a `libMobileGLServer.so` must come from the root build, and `MobileGL/build.gradle` sets no `targets` list, so the claim in §13 that AGP will package an `add_executable` renamed `lib*.so` is asserted, not verified. (c) Exec permission: the reader's SIGILL (exit 132) evidence was obtained through `run-as`, i.e. the runas_app domain, not from the app's own untrusted_app process, which is what the trace Activity is.
|
||||
- 修法:Move the Android delivery chain into P0 as a 30-minute spike with its own gate: build a trivial `libMobileGLServer.so` from the root CMakeLists, confirm AGP packages it into `lib/arm64-v8a/`, and have TraceReplayActivity `posix_spawn` it from `getApplicationInfo().nativeLibraryDir` and print a line — proving untrusted_app exec before any protocol work. Add a single generic `--es mobilegl_env "K=V;K=V"` passthrough to the trace path (one change in each of the five files) instead of a per-knob extra. Re-baseline P1 acceptance to the Linux `inproc` + `spawn` gates only, and make the device retrace the P2 exit criterion.
|
||||
- **[major] Nothing stops the spawned server from taking the remote branch and forking again**
|
||||
- 问题:§12 selects the split at `MG_Backend/Init.cpp` on `MG_Config::Transport`, which `ConfigLoader.cpp` reads from the environment (same shape as `features.CoherentAsFlush = QueryEnvFlag(...)` at ConfigLoader.cpp:185). §11 spawns the server with `fork`/`execve` and fd 3, so the child inherits `MOBILEGL_TRANSPORT=spawn`. §13 then says the server is a stub that `dlopen(libMobileGL.so)` + `dlsym("mobilegl_server_main")`; that entry must stand up a real backend, which runs `MG_Backend::Init()` (MobileGL/MG_Backend/Init.cpp:48-70). With the inherited variable still set, it constructs another `BackendObject_Remote` and spawns again — an unbounded fork chain on first GL call. The plan never states how the child's mode is forced.
|
||||
- 修法:Make `mobilegl_server_main` set `MG_Config::Transport = Monolith` before it can reach `MG_Backend::Init()`, AND scrub `MOBILEGL_TRANSPORT`/`MOBILEGL_IPC_*` from the child environment at spawn time (build an explicit envp rather than inheriting). Add a P0 `MG_Test/Wire` test that spawns a server and asserts the process tree gains exactly one child.
|
||||
- **[major] MG_IntegrationTest's fork pre-flight will spawn a second, orphaned server holding the GPU device**
|
||||
- 问题:`MobileGL/MG_IntegrationTest/Harness/HeadlessGL.cpp:344-368` forks a child that runs the complete EGL bring-up and then `_exit(step)`, with the comment at :364-366 stating this is deliberate — 'every atexit handler and static destructor in this address space belongs to the parent's copy of the world.' In split mode that child's bring-up reaches `MG_Backend::Init()` and spawns a server process; `_exit` runs no teardown, so that server is orphaned and lives until it notices EOF or hits `MOBILEGL_IPC_IDLE_EXIT_S` (default 30s per the plan's appendix). The parent then immediately brings up its own server against the same device. HeadlessGL.cpp:585-589 already names exactly this failure mode ('a leaked exclusive device, an environment the child did not have') as the reason it distinguishes 'pre-flight passed, parent failed'. §15's P1/P2 acceptance runs the whole integration suite through this path and the plan does not mention the pre-flight at all.
|
||||
- 修法:Make the server's EOF detection immediate and its exit unconditional (sub-second, not the 30s idle watchdog), and have the client spawn with the socket fd marked so `_exit` closes it deterministically. Add a readiness handshake with one bounded retry on device-busy so a lingering pre-flight server cannot flake the parent. Validate this specific interaction as part of P1 acceptance step 1, before any breadth work.
|
||||
- **[major] Server discovery via dladdr does not work for either desktop gate**
|
||||
- 问题:§11 locates the server with `dladdr(&MobileGL::Initialize)` → dirname → `libMobileGLServer.so`. But `MobileGL/MG_IntegrationTest/CMakeLists.txt:31-32` sets `MGL_ITEST_MOBILEGL_TARGET MobileGL_s`, i.e. the integration binary links MobileGL **statically** on desktop, so `dladdr` resolves to the test executable's own path, not a library directory. For trace replay, `tools/trace_replay/CMakeLists.txt:285-290` passes an explicit `-DMOBILEGL_LIBRARY=$<TARGET_FILE:MobileGL>`, whose directory is the MobileGL build output dir, while CMake places an `add_executable` in the defining directory's binary dir by default. Both P1 acceptance steps therefore fail to find the server as designed, and the plan's proposed `mgl_itest_join_environment(... "MOBILEGL_TRANSPORT=inproc" ...)` snippet does not set `MOBILEGL_IPC_SERVER_PATH`.
|
||||
- 修法:Make `MOBILEGL_IPC_SERVER_PATH` the primary discovery mechanism and `dladdr` the fallback. Set `RUNTIME_OUTPUT_DIRECTORY` of `MobileGLServer` to `$<TARGET_FILE_DIR:MobileGL>`, and add `"MOBILEGL_IPC_SERVER_PATH=$<TARGET_FILE:MobileGLServer>"` to every new ctest ENVIRONMENT list (joined via `mgl_itest_join_environment` with `${MGL_ITEST_COMMON_ENV}`) and to the new `SPLIT` argument of `add_trace_replay_test`. Confirm the absolute path survives the CI artifact hop — `.github/workflows/test.yml:174-185` rewrites only `cmake` paths inside `CTestTestfile.cmake`, not ENVIRONMENT values.
|
||||
- **[major] Split mode's ban on COHERENT_AS_FLUSH invalidates the P2 gate for the two Create/Flywheel fixtures, and app-native coherent persistent maps have no mitigation at all**
|
||||
- 问题:§6.8 states 'split mode must not apply MOBILEGL_COHERENT_AS_FLUSH'. `tools/trace_replay/trace_cases.json` has exactly two cases with `coherent_as_flush: true` — `minecraft-1.21.1-neoforge-create-indirect-in-world` and `minecraft-1.21.1-neoforge-create-instancing-in-world`. So P2's gate ('CI 全部 trace case … 在 Linux split 模式 SSIM ≥ 0.99' compared name-for-name against monolith) would run those two through a different buffer path in each mode, making the comparison meaningless for the two most buffer-stressing fixtures in the suite. Separately and more seriously, the plan addresses only the *rewrite* flag, not an application that requests `GL_MAP_PERSISTENT_BIT|GL_MAP_COHERENT_BIT` itself. With adoption declined in P1-P6 (§6.8 tier T2), such a map skips every early-out in `BufferObject::SyncPersistentMappedRange()` (MobileGL/MG_State/GLState/BufferState/BufferObject.cpp:238-250: returns early for GPU-resident, non-persistent, read-only, and FlushExplicit — a coherent persistent write map matches none of them) and reaches `NotifySubData(whole mapped range)` on **every draw**, which over IPC becomes a whole-buffer wire transfer per draw. The plan's copy-accounting table in §6.4 does not contain this row. `MG_Config::Features.CoherentAsFlush` defaults false (MobileGL/Config.h:174), so the ban itself is narrow — but the underlying cliff is not.
|
||||
- 修法:Two changes. (1) Run the two Create cases in split mode with the flag ON so the P2 comparison is honest, or state explicitly that they are excluded and why. (2) Add a third tier for non-adopted persistent-coherent maps in P1-P6: pull the shadow-in-shm work (currently P4.5) forward to cover *this* case specifically, or ship a dirty-range tracker for coherent maps, and add the row to the §6.4 copy table. Measure it on the two Create fixtures before P2 exit, not at P7.
|
||||
- **[minor] The byte-identical-monolith gate is contradicted by P4.5's allocator change**
|
||||
- 问题:§12 and P0's acceptance require `nm --defined-only` and stripped `.text` size on `libMobileGL.so` to be unchanged when `MOBILEGL_BUILD_DISAGGREGATED=OFF`, and §12 layer 1 says MG_Remote sources simply leave `SOURCE_FILES`. But P4.5 (§6.4, §15) changes `PipeResource`'s `MapAlignedAllocator` and `MipmapStorage`'s level vectors to use a shm arena for ≥256KiB — these live in `MG_State`, not `MG_Remote`, and changing a container's allocator changes the type. Unless every one of those edits is `#if MOBILEGL_BUILD_DISAGGREGATED`-guarded, the P0 gate goes red at P4.5 and the plan says nothing about it.
|
||||
- 修法:State that the shm arena is a guarded allocator specialization that compiles to the current `MapAlignedAllocator` when the option is OFF, and re-run the `nm`/`.text` gate as a phase-exit criterion for every phase (P0 through P9), not only P0.
|
||||
- **[minor] Non-arrow pGLContext usage count is understated 2x, and the lifecycle sites are omitted**
|
||||
- 问题:§12 and R8 say the `inproc` thread-local `pGLContext` shim must cover '约 65 处' non-arrow usages. Measured on dev@81b17c0b: `grep -rn pGLContext MobileGL/ --include=*.cpp --include=*.h | grep -v 'pGLContext->'` yields **133** lines. Of those only 2 are in MG_Impl (GL_Debug.cpp:99 `.get()`, GL_Program.cpp:1630 `== nullptr`); the bulk are in MG_Backend, including roughly 90 `MOBILEGL_ASSERT(MG_State::pGLContext, ...)` truthiness checks in DirectVulkan.cpp alone, plus `DirectGLES.cpp:146` `.get()` and ten `if (MG_State::pGLContext)` guards in Managers.cpp. It also omits the lifecycle sites the shim must handle: `MG_State/GLState/Core.cpp:20` (`pGLContext = MakeUnique<...>()`), `Core.cpp:1487` (the leaked-reference definition), `Core.h:564` (the `extern UniquePtr&` declaration), and `MobileGL/Init.cpp:63` (`pGLContext.reset()`).
|
||||
- 修法:Correct the count and note that the shim must provide `operator->`, `operator bool`, `get()`, `== nullptr`, assignment from `MakeUnique`, and `reset()`. Since the backend-side usages are exactly the ones that must see the *replica*, prototype the shim against `MG_Backend/DirectVulkan/DirectVulkan.cpp`'s assert block first — it is the densest cluster.
|
||||
- **[minor] The FlatBuffers submodule stays mandatory even with a committed generated header, and the option has no guard**
|
||||
- 问题:§13 says committing `protocol_generated.h` means 'cross-compilation never needs flatc', which is true — but the REUSE table (§14) keeps `Feat/CS-Delta-IPC:MobileGL/Protocol/CMakeLists.txt`'s flatc resolution, and that file at :22-38 does `add_subdirectory(3rdparty/flatbuffers)` with `FLATBUFFERS_BUILD_FLATC ON` whenever `MOBILEGL_FLATC_EXECUTABLE` is unset — i.e. the exact NDK trap the plan says it fixes is preserved by the reuse decision. Independently, the runtime headers are still needed: the same file at :61-64 adds `3rdparty/flatbuffers/include` to `MobileGL_Protocol`. So with `MOBILEGL_BUILD_DISAGGREGATED=ON` and the submodule not initialised, `MG_Remote/**` lands in `SOURCE_FILES` (§13) and the build fails with no guard, since the existing `if (EXISTS .../flatbuffers/CMakeLists.txt)` only wraps the Protocol subdirectory. The tree currently has 12 submodules and none is flatbuffers.
|
||||
- 修法:Do not reuse the flatc resolution block as-is: make codegen a `scripts/gen_protocol.py` developer target that is never part of the build graph, and delete `add_subdirectory(3rdparty/flatbuffers)` from the default path entirely (keep `MOBILEGL_FLATC_EXECUTABLE` only for the CI `flatc-check` step). Add an explicit guard that force-sets `MOBILEGL_BUILD_DISAGGREGATED=OFF` with a `message(WARNING ...)` when `3rdparty/flatbuffers/include` is absent.
|
||||
- **[minor] Ring decode has no stated bounds discipline, only the socket path does**
|
||||
- 问题:§7.2 specifies magic and 64MiB length validation on read for the CTRL socket, correctly citing the prior branch's unbounded `make_shared<std::vector<uint8_t>>(size)` (verified at Feat/CS-Delta-IPC:MobileGL/Remote/LocalSocketTransport.cpp, the `async_read` header handler). But §6.3's `RecHeader { kind; flags; size; }` is read out of `SEG_CMD`, a region the peer writes concurrently, and the plan's only integrity mechanism there is the `static_assert` on `sizeof(T)` at compile time. A corrupted or truncated `size` lets the applier's cursor walk past the ring; a `kind` whose record is shorter than `sizeof(T)` lets it read past the record.
|
||||
- 修法:State the invariant explicitly and generate it: alongside each `MGL_REC_SIZE_CHECK`, emit a runtime `size >= sizeof(T) && size <= remainingRingBytes && (size % 8) == 0` precondition in the applier's dispatch switch, and treat a violation as `Fatal{ProtocolCorruption}` rather than undefined behaviour.
|
||||
- **[minor] Two small test-infrastructure mechanics the plan understates**
|
||||
- 问题:(a) `add_trace_replay_test` names its test `MobileGLTraceReplay.${CASE_NAME}.${BACKEND}` (tools/trace_replay/CMakeLists.txt:330-332); a `SPLIT` argument as proposed in §13 would produce a duplicate ctest name for the same case+backend unless the name is extended. (b) The test command is `cmake -P run_trace_case.cmake` with ~18 `-DTRACE_*` variables; a new mode must be threaded through that script too, which the plan does not list among the files it touches. Neither is hard, but both sit on the P2 gate.
|
||||
- 修法:Extend the generated name to `MobileGLTraceReplay.${CASE_NAME}.${BACKEND}${SPLIT_SUFFIX}` and add `-DTRACE_TRANSPORT=` to the `-P` invocation plus its consumption in `run_trace_case.cmake`, listing both files in the P2 deliverables.
|
||||
- **[minor] Windows: 'inherited handles remove accept/connect' does not carry over to asio's overlapped requirement**
|
||||
- 问题:§11 says the Windows spawn uses 'CreateProcess + 继承句柄' and §16 R9 claims this 'completely removes accept/connect'. asio's `windows::stream_handle` (the transport the plan defaults to on Windows) requires an **overlapped** handle for its IOCP service; an anonymous pipe pair from `CreatePipe` is not overlapped-capable, so the pair must be constructed with `CreateNamedPipeW(..., FILE_FLAG_OVERLAPPED)` plus a matching `CreateFileW(..., FILE_FLAG_OVERLAPPED)` and only then inherited. The plan's AF_UNIX observation is correct — asio 1.38.2 defines `ASIO_HAS_LOCAL_SOCKETS` for everything except `ASIO_WINDOWS_RUNTIME` (3rdparty/asio/asio/include/asio/detail/config.hpp:1085-1092) — but that is the fallback, not the default.
|
||||
- 修法:Spell out the Windows handle-pair construction (named pipe with a GUID-unique name, both ends `FILE_FLAG_OVERLAPPED`, server end inherited) in §11, and keep the AF_UNIX evaluation in P6 as written.
|
||||
- **[minor] mobilegl_server_main will not be dlsym-able in the shipping configuration**
|
||||
- 问题:§13 makes the Android server a ~30-line stub that does `dlopen(libMobileGL.so)` + `dlsym("mobilegl_server_main")`. But `CMakeLists.txt:498-510` sets `C_VISIBILITY_PRESET hidden` / `CXX_VISIBILITY_PRESET hidden` / `VISIBILITY_INLINES_HIDDEN ON` on the shared target for every non-Debug build — which is exactly the RelWithDebInfo configuration the plugin and FCL ship (MobileGL/build.gradle's `fordebug` type forces `-DCMAKE_BUILD_TYPE=RelWithDebInfo`). The symbol will not be exported unless it is explicitly annotated, so this works in a Debug build and silently fails on device.
|
||||
- 修法:Declare the entry point `extern "C" __attribute__((visibility("default"))) int mobilegl_server_main(int, char**)` (and add it to `MG_Impl/DyldInterpose/ExportedSymbols.txt` and `wgl.def` equivalents if those platforms ever host a server), and add a `nm -D | grep mobilegl_server_main` assertion to the P0 acceptance alongside the existing `nm --defined-only` gate.
|
||||
- **[minor] Phase effort is optimistic where it matters most, and the plan's own risk register does not cover schedule**
|
||||
- 问题:P0 = 3 days covers SCM_RIGHTS fd passing (hand-rolled sendmsg/recvmsg on asio's native handle), a four-platform shm layer, the SPSC ring with RingControl, validating framing, the committed-header flatc pipeline with a CI diff gate, a code-generating coverage assert with a second CI diff gate, the RenderbufferObject change, working-tree cleanup, and Tracy byte counters. P1 = 10 days covers the entire client (WireMirror, EmitTable, EmitBufferOps, BackendObject_Remote, CapsMirror, ClientArrayBounds, CompositeResolver) and the entire server (ReplicaContext, Applier, ServerLoop, ServerMain, spawn), plus — implicitly, per finding 1 — the whole Android delivery chain. For calibration, Feat/CS-Delta-IPC produced 6,668 lines across 10 commits and never rendered a frame; its own HANDOFF records four days lost to a non-reproducible regression. The 74-day total is internally consistent (3+10+8+3+5+5+4+6+6+8+6+10=74) but the front-loaded milestone is the weakest claim in the document, and §16 has no schedule risk row.
|
||||
- 修法:Split P1 into P1a (client emit + inproc applier + Linux `inproc` gate, 6 days) and P1b (spawn transport + Linux `spawn` gate, 4 days), and make the device retrace a P2 exit criterion. Add a schedule row to §16 whose mitigation is the P2.5 falsification gate already in the plan — it is the right instrument, it is simply not linked to the schedule risk it retires.
|
||||
- **[major] The headline per-draw cost comparison (§10.2, §5.1) is a static-site-count vs dynamic-call-count category error; the baseline is overstated by roughly an order of magnitude**
|
||||
- 问题:§10.2's table and §5.1 price today's per-draw state acquisition as "Espryt 124 / Magma 169 accessor calls + version compares + a ~1.2KB three-span memcmp + CurrentUnitBindingsEpoch's per-unit owner walk + Magma's two lossy version sums + ~40 payload accessor walks". 124/169 are STATIC `pGLContext->` call sites (§2.1's own definition), not dynamic per-draw calls. Every one of those costs is already memo-gated in the tree: - `SyncRenderState` returns at the top on a single Uint16 compare (`MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp:2016-2018`: `if (!forceFullPush && !colorMaskWidenDirty && g_hasSyncedRenderState && currentRenderStateVersion == g_syncedRenderStateVersion) return;`). The three memcmps run only when the version moved. - `SyncNeccessaryTextures` steady state is a 6-value key compare plus `PairingsIntact` and a per-entry `IsDrawSyncClean` word compare (`DirectGLES.cpp:1537-1560`); the unit walk runs only on a miss. - `CurrentUnitBindingsEpoch` has a three-value fast gate and only walks owners when the bind generation moved (`DirectGLES.cpp:1421-1426`). - Magma's `TrySetupDrawFastPath` steady state is ~10 accessor calls and ~20 word compares (`MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp:6002-6300`), not 169. - `GetOrCreatePipeline` recomputes the pipeline-state hash only when `GetPipelineStateVersion()` moved (`VulkanRenderer.cpp:4982-4993`), and the "~40 payload accessor walk" at :5155-5200 runs only on a pipeline memo MISS. - `ApplyDynamicDrawStateTail` has a two-level gate: one version compare, then a value key built from one bulk fetch (`VulkanRenderer.cpp:5888-5893`). So the real steady-state pull cost is on the order of 10-25 accessor calls and a few dozen word compares per draw per backend. Comparing that against "1 dirty word test + N set_*" is a much narrower margin than the plan's table implies, and the plan's entire business case (B-R2, the day-24 GO/NO-GO in §0.6/P2, the "traversal is moved, not doubled" claim) is built on the inflated figure.
|
||||
- 修法:Restate §10.2's table in DYNAMIC terms and stop citing 124/169 as a per-draw cost anywhere in the document (they belong only in §2.1's coupling-surface argument). Add a per-draw dynamic counter (accessor calls executed, memo hit/miss per gate) to P0's TracyPlot deliverable list alongside the byte counters — the plan currently lands byte counters but no call counters, so it will still be guessing at P2. Then make the day-24 GO/NO-GO threshold an ABSOLUTE number (ns/draw of tracker cost measured on both devices) rather than "within the noise of monolith-pull", because relative-to-noise passes trivially when the true baseline is 20 calls, not 124.
|
||||
- **[major] The tracker is specified as a poll of existing counters, which is the same traversal it claims to eliminate — §5.2 and §10.2 are mutually inconsistent**
|
||||
- 问题:§1.1/§5.2 state "MG_State 零新增记账" and map every dirty bit onto an existing version counter; §5.4-2 explicitly requires the two high-water-mark walks (`TouchBindPoint`/`GetTouchedBindPointCount`, `NoteUnitTouched`/`GetMaxTouchedUnit`) to stay "in the tracker's walk". That means `m_dirty` is COMPUTED by polling, not SET by the mutators. But §10.2 and §5.1 price the steady state as "one 64-bit dirty word test + N set_* calls". These cannot both be true. `MGPIPE_NEW_SAMPLER_VIEWS` alone is mapped in §5.2 onto `GetContentVersion` + `GetShapeVersion` + `GetTextureParamsVersion` + `GetTextureBindGeneration` + `GetSamplingResolutionGeneration`. The first three are PER-TEXTURE, so computing that one bit requires walking the touched units and reading three counters per bound texture — which is exactly `SetupDrawSnapshot`'s `sampledContentSum`/`sampledParamsSum` walk (`VulkanRenderer.cpp:6253-6254`) that §4.7.3-D14 claims collapses to "one compare", and exactly Espryt's unit list walk. Same for `NEW_VERTEX_BUFFERS` (per-attribute `VertexAttributeVersion` triples) and `NEW_FRAMEBUFFER` (`Array<Uint16,40>` attachment versions). Gallium does not work this way: `st_invalidate_*` sets dirty bits from the GL entry points; `st_validate_state` never polls object versions. The plan adopts gallium's validate-time push but not gallium's dirty-marking, and then quotes gallium's cost.
|
||||
- 修法:Choose explicitly, in the design document, and price the choice. The correct answer is dirty-MARKING: have MG_Impl's mutating entry points call `MGPipeTracker::MarkDirty(group)` so validate is genuinely O(dirty groups). Then delete the "zero new bookkeeping in MG_State" claim, add the marking-site audit to B-R6 (it is the same completeness obligation as the reconciler, on a larger surface — every GL setter, not every backend read), and let the G5 written-once bitmask plus MOBILEGL_PIPE_VERIFY cover it. If instead polling is kept, §10.2 and §5.1 must be rewritten to say the tracker performs the same per-object walk as today's backend, and the net win reduces to the server-side memo deletions only.
|
||||
- **[major] The ~115-line unit-bindings epoch machinery is booked as deleted, but it cannot be deleted — only moved to the client**
|
||||
- 问题:§2.5, §4.7.3-D3 ("结构性删除") and §10.4-1 count `UnitBindingsSnapshot`/`CaptureUnitBindings`/`UnitBindingsUnchanged`/`CurrentUnitBindingsEpoch`/`UnitTextureSyncEntry`/`PairingsIntact` (~115 lines, `DirectGLES.cpp:1372-1489`) as a structural deletion, on the ground that "the push call IS the change signal". That is only true if the client can cheaply decide WHETHER to push. It cannot, for exactly the reason the machinery exists: `GetTextureBindGeneration()` bumps on REDUNDANT rebinds — the comment at `DirectGLES.cpp:1414-1420` records that MC 26.2 rebinds the same sampler around every texture-unit switch. If the tracker keys `set_sampler_views` on the bind generation it will push a full resolved view array on every redundant `glBindSampler`, which in the workload that motivated the machinery is per-batch. To avoid that it must do the same owner-comparison walk — i.e. the code moves to `MG_Impl/Pipe/Tracker.cpp`, it does not disappear. Worse, in split mode a spurious push is not just CPU: `set_sampler_views` is a `kVarTail` record carrying an `MGPSamplerView`-shaped entry per sampled unit, so a redundant push costs hundreds of ring bytes per draw. The same argument applies to `g_fboTextureSyncList` (D8) and, in weaker form, to `ResolvedTextureBindingMemo` (D9): the client needs its own memo keyed on the same epoch to avoid re-resolving completeness (`IsMipmapCompleteForFilter` / `SamplesAsIncompleteTexture` / `IsUndefinedDefaultTexture`) per draw, since §5.5 puts view resolution on the client.
|
||||
- 修法:Move these rows from "deleted" to "relocated" in §2.5, §4.7.3 and §10.4-1, and subtract them from the "~550 lines deleted" ledger (which then drops to roughly 350-400, of which the genuinely-deleted parts are TwinLookupMemo×3 + OwnerEquals, the six registry GC sweeps, `sourcePin`, and the placeholder-texture puppetry). Add the client-side epoch memo and its key to §5.5 as an explicit deliverable of P3b/P4b, and add a `set_sampler_views` push-count-per-frame counter to the P0 counter list so a regression to per-batch pushing is visible immediately.
|
||||
- **[major] D-B1's whole-block RenderStateCso re-creates the exact regression the two version counters exist to prevent**
|
||||
- 问题:`RenderState.h:519-528` documents why there are two counters: "Viewport, scissor, depth range, blend colour, line width, polygon offset, stencil write mask, the clear values, hints and the point-size family are all either dynamic pipeline state or not pipeline state at all, so changing one of them must not evict a cached pipeline. Keeping one counter for both made a glViewport call knock the next draw off the pipeline memo AND the draw fast path." Verified: `RenderState.cpp:639-640, 702-735` and neighbours bump only `++m_version` for those setters, never `BumpVersions()`. D-B1 makes the CSO identity the CONTENT of the whole `RenderStateParameters` block. Therefore `glViewport`, `glScissor`, `glBlendColor`, `glClearColor`, `glLineWidth`, `glStencilMask` and `glPolygonOffset` each produce a different content hash, hence a different CSO handle. Consequences: (a) a 64-entry client LRU (§4.5.2/§4.1) keyed on a block containing 16 viewports + 16 scissor boxes + 16 depth ranges + clear values will thrash under Iris shader packs and shadow-cascade rendering, which change viewport/scissor many times per frame; (b) each LRU miss re-sends a ~1.2 KB `create_render_state` blob; (c) a new CSO handle invalidates any per-CSO pipeline-hash memo the server keeps, which is the very thing §4.5.2 promises ("Magma 每 CSO 算一次 pipeline hash"). D-B1 and D3 ("CSO 边界跟 Vulkan 动态状态走") therefore contradict each other inside the same document.
|
||||
- 修法:Key the CSO on the pipeline-relevant subset only — the same field set `ComputePipelineStateHash` already enumerates (`VulkanRenderer.cpp:4826-4906`) and the same subset `m_pipelineStateVersion` guards — and carry viewport/scissor/depth-range/blend-colour/line-width/polygon-offset/stencil-ref-and-write-mask as a separate `set_dynamic_state` payload, mirroring `DynamicStateShadow` and `ApplyDynamicDrawStateTail`. Accept and state that this breaks the "reuse the existing head/blend/tail span division" argument (the head span starts with `Viewports` and also contains `LineWidth`/`PointSize`/`PolygonOffset*`, so the existing spans do not align with the pipeline/dynamic split); the span-memcmp layout invariant then applies inside the pipeline-subset blob and must be re-derived, which is cheaper than paying a CSO per glViewport.
|
||||
- **[major] Content-addressed CSOs make the single path the code names as hottest more expensive, not cheaper**
|
||||
- 问题:`DirectGLES.cpp:2029-2032` names the target: "a per-draw blend toggle used to re-diff all ~40 pieces of state field by field on every draw (Blaze3D brackets every batch with glEnable/glDisable(GL_BLEND), making this the hottest thing mc_state_toggle did)". Verified that a real toggle does move the version — `SET_CAPABILITY` short-circuits only on a REDUNDANT set (`RenderState.cpp:311-313`), and enable/disable pairs are not redundant. Today's cost on that path: three memcmps over ~1.2 KB, server-side, once per draw whose version moved. Under the plan the client must find the CSO by hashing, and it cannot shortcut via the version: `m_version` is monotonic (`++m_version`), so a version value never repeats and no version→CSO memo can ever hit on the alternating-content pattern. So the client pays an xxHash over the same ~1.2 KB plus a `ska::flat_hash_map` probe on every such draw. Then, because the handle changed, Espryt's 693-line body still runs its span memcmp — P2's deliverable explicitly keeps it "一行不动". Net: a full-block hash and a map probe ADDED, nothing removed. For Magma it is worse in a subtler way: `ComputePipelineStateHash` folds roughly 25-30 words out of one bulk fetch (`VulkanRenderer.cpp:4826-4906`) — far cheaper than an xxHash of the full 1.2 KB block. Moving pipeline-hash computation behind a CSO handle therefore trades a cheap server-side hash for an expensive client-side one on precisely the toggle pattern §4.5.2 cites as the justification.
|
||||
- 修法:Do not content-address on the full block. Derive the CSO key from the pipeline-subset field list (reuse `ComputePipelineStateHash`'s enumeration verbatim so the two can never disagree) plus the two version counters, and let the CSO cache hold the small key. Alternatively drop content addressing on the hot path entirely: mint a CSO per distinct `m_pipelineStateVersion` value and run a dedupe/coalesce pass off the draw path at frame boundaries. Either way, P2's acceptance must include a dedicated microbenchmark of the Blaze3D toggle pattern (enable/draw/disable/draw at MC batch rates) on both devices, because that single pattern decides whether §10.2's central claim survives.
|
||||
- **[major] §5.8.1's blanket reconcile rule adds a per-frame round trip on the *IndirectCount path that the monolith does not pay, on a named trace fixture**
|
||||
- 问题:§5.8.1 asserts that "every client-side scan/rewrite in the table above immediately follows `SyncPersistentMappedRange()` + `SyncGpuWrites()` in the monolith" and mandates "publish → wait for appliedSeq → drain events" at each. That is true for the restart rewrite and multi-draw flattening (`DirectGLES.cpp:4412-4413`, `MultiDraw.cpp:498-499`, `VulkanRenderer.cpp:3431, 4159`), but it is NOT true for the `*IndirectCount` CPU fallback, which §5.8's table also assigns to the client. Verified: `MultiDrawElementsIndirectCount` (`DirectGLES.cpp:4667-4668`) calls only `drawBuffer->SyncPersistentMappedRange(); parameterBuffer->SyncPersistentMappedRange();` and then reads the count and the command block straight out of `MappedData()` (`:4690-4694`). There is no `SyncGpuWrites()` and therefore no stall today. `SyncGpuWrites` is what triggers `ReadbackFromGpu` (`BufferObject.cpp:265-274`). If the plan applies its blanket rule here, every `glMultiDrawElementsIndirectCount` acquires a publish-and-wait round trip. The trace corpus contains `minecraft-1.21.1-neoforge-create-indirect-in-world` — a Create/Flywheel fixture whose indirect and parameter buffers are compute-written each frame — so this would be a per-frame, per-batch synchronous round trip on a named acceptance fixture, and the plan's §9.2 #10 dismisses it as "常见情况不 pending,代价为零".
|
||||
- 修法:Replace the blanket rule with a per-site table that reproduces the monolith's reconcile set exactly: `SyncPersistentMappedRange` only where the monolith calls only that, `SyncPersistentMappedRange + SyncGpuWrites` where the monolith calls both. Add the round-trip counter for the indirect-count path to the P8 acceptance and require it to read zero on `create-indirect`. Separately, note that the monolith's omission of `SyncGpuWrites` there may itself be a latent correctness gap — but that is a `dev` question, not something the split should silently fix by adding a stall.
|
||||
- **[major] The day-24 GO/NO-GO measures the one subsystem where push's benefit is smallest and its overhead is largest**
|
||||
- 问题:§0.5 and P2's acceptance make the day-24 decision on "monolith-push within monolith-pull's noise on p50 and p99 per-thread CPU" after converting only render state. But render state is the subsystem where push helps LEAST and the plan's CSO design costs MOST: - Espryt already holds a byte-exact value mirror with a version early-out and a span memcmp (`DirectGLES.cpp:2016-2047`) — there is almost nothing to save. - Magma already caches the pipeline-state hash under the version (`VulkanRenderer.cpp:4982-4993`) and gates the dynamic tail twice (`:5888-5893`). - The CSO overheads identified above (full-block hash on the client, CSO churn on glViewport) land squarely and only on this subsystem. So a GREEN P2 does not validate the claim it gates (that Track H handle-ization pays for itself across 200+ days), and a RED P2 is more likely to indict the CSO design than the push model. Either way the decision the gate is supposed to inform is not the decision it measures. §0.6 also asserts the fallback cost is "only 16 of the 24 days", which understates it: P1's 293-site sed plus the 58 hand-converted non-arrow sites plus the G4/G5 generators are not reusable by the earlier (since-dropped) design.
|
||||
- 修法:Extend the day-24 gate to require both (a) the render-state conversion and (b) one Track H slice — the plan already prices the cheapest ones: 0d handle infrastructure (5-7 days, §6.4) and Magma's `VertexInputStateFactory`/`VaoDrawMemo` re-key (2-3 days, §6.5-4, explicitly "低(纯结构性收益)"). That yields a real Track H unit cost, which is what B-R14's re-baselining actually needs. Add an explicit exit criterion that separates "push is slower" from "the CSO design is slower" by running P2 with content addressing disabled (a `MOBILEGL_PIPE_PUSH` sub-bit) as a negative control.
|
||||
- **[major] The interface-purity gate's shared-value-header allowlist is not achievable as written, and the nm gate cannot detect the failure**
|
||||
- 问题:§4.7.2 and §10.3-① define the purity gate as: `MG_Backend` may include only "a shared VALUE header allowlist (`RenderStateParameters` from RenderState.h, `SamplerParameters` from SamplerObject.h, `PixelStoreParameters`, `VertexAttribute`, texture/format enums)", plus `nm --undefined-only libMobileGLServer.so | grep -E 'MG_State::GLState::|glslang'` empty. Verified that the allowlist is not a leaf set: `MobileGL/MG_State/GLState/RenderState/RenderState.h:12` includes `MG_State/GLState/FramebufferState/FramebufferObject.h`, which at `:12-13` includes `MG_State/GLState/TextureState/TextureObject.h` and `MG_State/GLState/RenderbufferState/RenderbufferObject.h`. The dependency is structural: `RenderStateParameters` sizes two of its arrays with `MG_State::GLState::FramebufferObject::MAX_DRAW_BUFFERS` (`RenderState.h:263, 273`). So shipping `RenderStateParameters` to a "pure" MG_Backend drags the entire framebuffer/texture/renderbuffer class graph in with it. And the nm gate is blind to this: header inclusion of classes whose members are never called emits no undefined symbols, so `nm --undefined-only | grep MG_State::GLState::` can be empty while the include graph is fully coupled. The plan prices this cleanup inside P13's 6 days ("MG_Backend 的 MG_State include 收缩到共享值头白名单") as if it were a mechanical trim.
|
||||
- 修法:Make header extraction an explicit P0/P1 deliverable, not a P13 trim: move `MAX_DRAW_BUFFERS`, `PerBufferBlendState`, `StencilFaceState`, `PixelStoreParameters` and `RenderStateParameters` into a dependency-free `MG_Pipe/MGPipeValueTypes.h` that includes nothing from `MG_State/GLState`, and have `RenderState.h` include that instead. Then replace the nm gate with an INCLUDE-GRAPH gate — compile `MG_Backend` in the disaggregated configuration with `MG_State/GLState` removed from the include search path (or assert on `-H` output), which is the only check that can actually go red for the reason the gate exists.
|
||||
- **[minor] draw_vbo's payload construction is priced at parity with today's 3-scalar call, and mandates fields that are currently computed only where needed**
|
||||
- 问题:§10.2's first table row reads "每 verb 的分发: 1 次间接调用 (已经在付) → 1 次间接调用", implying parity. But today's entry is `DrawArrays(GLenum mode, GLint first, GLsizei count)` — three scalars in registers (`MG_Backend/BackendObject.h:117`). The replacement is `draw_vbo(const MGPDrawInfo*, Uint32, const MGPDrawIndirect*, const MGPDrawRange*, Uint)`, and `MGPDrawInfo` as specified in §4.5.7 is ~80 bytes (mode, indexSize, flags, pad, instanceCount, startInstance, restartIndex, minIndex, maxIndex, an 8-byte handle, a 32-byte `MGHostSpan`, and an 8-byte `xfbCpuCapturedVertices`) plus a 12-byte `MGPDrawRange`. That is ~90 bytes of stores constructed per draw where there were three register moves. Two of those fields are new work, not just new stores: `minIndex`/`maxIndex` come from an index scan that today runs only for client-memory arrays (`TryComputeMaxIndexFromHostBytes`, `VulkanRenderer.cpp:3407-3470`, used at `:3599`), and `xfbCpuCapturedVertices` is a `GetTransformFeedbackCapturedVertices()` read that today happens only inside the XFB scatter path (`DirectGLES.cpp:~900`). At MC draw rates this is small but not nothing, and §10.2 accounts for none of it.
|
||||
- 修法:State the payload cost explicitly in §10.2, gate `minIndex`/`maxIndex` and `xfbCpuCapturedVertices` behind `MGPDrawInfo::flags` so they are only computed when a consumer asked for them, and add per-draw payload bytes to the P0 counter set (`cmd-records` is per-frame; a per-draw histogram is what sizes SEG_CMD).
|
||||
- **[minor] The +50-60 MiB memory figure omits the retention LRU the same document introduces, and that LRU is probably unnecessary**
|
||||
- 问题:§7.11 (formerly the removed comparison table) gives the plan's memory as "transport segments (~48MiB) + POD slot records + an optional bounded ≤32MiB texel-retention LRU ≈ +50-60MiB". The arithmetic does not include the LRU it just described: §8.1's segment defaults are SEG_CMD 8 + SEG_STAGE 32 + SEG_REPLY 8 + SEG_EVENT 0.25 = 48.25 MiB, and `MOBILEGL_PIPE_TEXEL_RETAIN_MB` defaults to 32 (附 B). That is 80 MiB before §8.2's mandated SEG_STAGE growth for the four new byte classes. Separately, the retention LRU appears to be unnecessary. `MipmapStorage` keeps `Vector<Vector<Uint8>> m_data` — a complete CPU shadow of every level (`MobileGL/MG_State/GLState/TextureState/MipmapStorage.h:117`) — so a server-initiated pull (§7.5) can always be serviced from bytes the client already holds. The LRU therefore buys latency, not correctness, and its cost lands on the metric (memory) that §0.4 uses as the plan's strongest argument against the earlier (since-dropped) design in a project whose headline result was saving ~400 MB.
|
||||
- 修法:Correct the arithmetic to 48 MiB + SEG_STAGE headroom + POD records, and default `MOBILEGL_PIPE_TEXEL_RETAIN_MB=0`. Turn it on only if §7.5(d)'s measured per-trace pull rate justifies it — which is exactly the discipline §7.5 already commits to for the pull count itself.
|
||||
- **[minor] §9.1's "glGetTexImage = 0 round trips on DirectGLES" does not survive the plan's own generated-mipmap ownership split**
|
||||
- 问题:§9.1 claims zero round trips for `glGetTexImage`/`glGetTextureImage` on DirectGLES because the client shadow answers. Verified that MG_Impl routes to the backend only when the backend is DirectVulkan (`MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp:6453-6459`), otherwise calling `CopyTextureImageToClientOrPBO_State`. But §5.8's row for generated mipmaps splits ownership: "client 分配 level 存储 … server 生成". A GPU-generated mip level therefore has allocated-but-empty client storage. `CopyTextureImageToClientOrPBO_State` will happily answer from that empty shadow. The plan's answer is `on_mip_levels_generated` (§7.1), but that callback as specified carries only `{res, base, count}` — no texels — so it can only mark the levels as needing a pull, which converts the query into a blocking round trip (the same class as §9.2 #9), or the design must instead eagerly write back every generated level (potentially megabytes per `glGenerateMipmap` on an atlas). The plan never says which, and §9.1 books it as zero.
|
||||
- 修法:Decide explicitly in §5.8/§7.2 between eager `on_texture_writeback` of generated levels and lazy pull-on-query, and move the DirectGLES `glGetTexImage` row from §9.1 (zero) to §9.2 (conditional blocking) with the condition named. Add the generated-level case to `TextureRemintPullScenario` so the chosen path has a gate.
|
||||
- **[minor] Two smaller round-trip accountings are optimistic: map_persistent is per-respecify not per-object-lifetime, and MGHostSpan is not free**
|
||||
- 问题:(a) §9.2 #8 prices `map_persistent` under tier T1 as "每 store 生命桥期一次,不是每次使用". But storage respecification re-mints the store, and the plan's own P3a acceptance lists `StorageBufferRegrowScenario`. `TryAdoptLargeStorage` fires at storage-definition time, so a buffer that grows N times costs N blocking round trips, not one. For a workload that grows chunk arenas during world load this is a burst of stalls at exactly the moment the user perceives them. (b) §4.5.7 states "monolith 代价为零(一次指针加载)" for `MGHostSpan`. It is a 32-byte struct embedded in every `MGPDrawInfo` and read through `MGPipeHostBytes` which the same section describes as "一次分支,每次使用解析一次". That is a branch plus 32 bytes of payload on every draw record, whether or not the draw uses host bytes — which for VBO-based workloads (all of MC/Sodium) is every draw.
|
||||
- 修法:(a) Reword §9.2 #8 to "once per storage definition" and add a `map-persistent-roundtrips` counter to the P0/P11 counter set, with `StorageBufferRegrowScenario` publishing it. (b) Reword §4.5.7's cost line to "one predictable branch plus 32 bytes on the draw record", and consider moving `userIndices` out of `MGPDrawInfo` into the `kHostSpan` var-tail so draws that carry no host bytes do not pay for the field.
|
||||
|
||||
已验证的优点:
|
||||
- The core architectural decision (D1: server runs a real replica GLContext driven by mutator replay, backends untouched) is well-founded and the evidence cited for it checks out. Composite pipeline programs really are anonymously linked at MobileGL/MG_State/GLState/Core.cpp:644 (`MakeShared<ProgramObject>(0u)`), which is exactly the gap §5.7 identifies and solves.
|
||||
- Every DROP claim about Feat/CS-Delta-IPC verified true. MobileGL/ServerHost/main.cpp really writes `interface_.ops.Start(&interface_, &config)` on a `MobileGLTransport*` (compile error, branch tip cannot build ALL). MobileGL/Remote/LocalSocketTransport.cpp really has `asio::async_write(stream, asio::buffer(next), [this, next](...))` where `next` is a local moved-from vector (use-after-free on every send), really allocates `std::make_shared<std::vector<uint8_t>>(size)` straight from the wire length with no cap, and really hardcodes `out->fd = -1` in PollOffer — so the 'no POSIX fd passing, no Linux/Android data plane' conclusion is correct.
|
||||
- The Android flatc trap is real and correctly diagnosed: Feat/CS-Delta-IPC:MobileGL/Protocol/CMakeLists.txt:25-38 does `add_subdirectory(3rdparty/flatbuffers)` with `FLATBUFFERS_BUILD_FLATC ON` whenever the override is unset, while the root hook guards only CS.cmake with `NOT ANDROID`.
|
||||
- The FCL process-model correction is right and materially changes the design: FCL/src/main/AndroidManifest.xml:113 declares `.activity.JVMActivity` with no `android:process`, and the only `:jvm` entry is `com.tungsten.fclcore.download.ProcessService` at :137-141. The game really does run in the main process, so a second process must be created.
|
||||
- asio 1.38.2 is vendored (3rdparty/asio/asio/include/asio/version.hpp: ASIO_VERSION 103802) and does define ASIO_HAS_LOCAL_SOCKETS on Win32 (detail/config.hpp:1085-1092, excluded only for ASIO_WINDOWS_RUNTIME), so the plan's Windows transport reasoning starts from a correct premise.
|
||||
- The 'one hook point' claim (D2) is accurate: MobileGL/MG_Backend/Init.cpp:48-70 is a single switch on `MG_Config::ActiveBackendType` followed by `InitSpecificBackendLibs()`, which is the only place `gBackendFunctionsTable` and `pActiveBackendObject` are assigned. A single guarded branch there really does cover the whole boundary with no `#ifdef` at the ~250 downstream call sites.
|
||||
- RenderbufferObject genuinely lacks GetLifetimeId() (no match under MobileGL/MG_State/GLState/RenderbufferState/), so the P0 item is real and not busywork.
|
||||
- The integration-test extension point is exactly as described: `mgl_itest_join_environment` exists (MG_IntegrationTest/CMakeLists.txt:306), and the comment at :339-343 states verbatim that a ctest ENVIRONMENT property REPLACES rather than appends and that every list must build on MGL_ITEST_COMMON_ENV. Eleven `gtest_discover_tests` registrations already follow that shape, so a Split lane per backend is a genuine one-registration change.
|
||||
- `add_trace_replay_test` really does set an ENVIRONMENT property per test (tools/trace_replay/CMakeLists.txt:353-360), so threading a transport variable through the trace lane is mechanically available.
|
||||
- The P1-P4 'server relinks from source' scheme has the inputs it needs: ProgramObject::GetLinkedShaderSnapshot() exists (ProgramState/ProgramObject.h:157) and deliberately holds SharedPtrs to the linked shaders (comment at :1716), so shader sources survive glDeleteShader and can be shipped.
|
||||
- MG_Config::Features.CoherentAsFlush defaults to false (MobileGL/Config.h:174), so §6.8's prohibition is a narrow, low-blast-radius rule rather than a default flip — and the reasoning behind it is correct, since BufferObject::SyncPersistentMappedRange (BufferObject.cpp:238-250) early-returns on FlushExplicit exactly as the plan assumes.
|
||||
- The working-tree hygiene item is real: MobileGL/MG_Backend/DirectGLES/{DirectGLES,Managers}.cpp are the only two modified files in the tree, and P0's insistence on removing per-draw instrumentation before any measurement is the correct lesson from the prior branch's poisoned measurements.
|
||||
- Push at draw-validate time rather than at GL-setter time (推论 1 / §5.1) is the right call and is directly supported by the tree: `RenderState::SetCapability` short-circuits redundant sets (`RenderState.cpp:311-313`) but a real enable/disable pair does bump the version, and `DirectGLES.cpp:2029-2032` names the Blaze3D per-batch blend toggle as the hottest path. A per-setter push would have turned that into an interface call plus a server CSO lookup per toggle. The plan identifies this as its most-likely-to-be-implemented-wrong decision and writes it as a spec clause (B-R15).
|
||||
- The A/B/C/D/E read classification (§2.3) and the conclusion that the interface must push VALUES not invalidation is correct and load-bearing. Verified: Magma keeps no render-state mirror and rebuilds its payload from ~40 direct field reads on a pipeline miss (`VulkanRenderer.cpp:5155-5200` region) while Espryt keeps a byte mirror and diffs it (`DirectGLES.cpp:1956`, `:2035-2047`). A bump-a-version-and-let-the-server-pull interface would indeed regress to today's model.
|
||||
- `MOBILEGL_PIPE_VERIFY` (§10.3-②) is a genuine semantic gate that exists only because the interface lands in the monolith first, and the plan is right to require FIELD-WISE comparison rather than memcmp — `DirectGLES.cpp:2029-2032` documents that a `RenderStateParameters` memcmp can false-DIFFER on padding but never false-match, so a byte comparer would produce false positives in the verify harness. This is the specific defect prior candidate designs were judged on, and it is answered.
|
||||
- D-B5 is honest about the cost: the plan states plainly that the earlier byte-identity monolith gate dies by construction and puts the loss in the design document rather than hiding it. Verified that no configuration can preserve it — the backend stops reading `pGLContext`, memos re-key, and MG_Impl gains validate calls.
|
||||
- Keeping `resource_subdata` carrying BOTH the union box and the rect list with the shape decision server-side (§4.5.6, §7.3) correctly preserves a measured hardware cliff. `MipmapStorage.h:60-83` documents the 96-slot rationale and the ~100-sprites/frame Minecraft pattern that motivated it; putting the decision on the side that pays the GPU cost is the right call.
|
||||
- PBO readback becoming fire-and-forget (§9.1) is strictly better than the monolith, verified: `DirectGLES.cpp:9191-9204` maps the pack PBO with `GL_MAP_READ_BIT` and copies back synchronously inside `ReadPixels`, which stalls on the read regardless of whether the application ever touches the PBO. Likewise `glFinish`/`glFlush` are genuine no-ops today (`MG_Impl/GLImpl/Exporting/Definitions.cpp:111-112`), so the requirement that they stay free is achievable rather than aspirational.
|
||||
- Per-backend optionality as a first-class interface property (§4.4.4, B-R9) is faithful to the existing contract: `BackendObject.h:212-215` and `:265-269` already document null table entries as "not implemented, frontend falls back", DirectVulkan already leaves 8 entries null, and Magma's deliberate omission of `ResidentSubData` (`VkBufferManager.cpp:104-111`) is preserved rather than papered over. Choosing a function-pointer struct over a virtual base is correctly justified by this, not by dispatch cost.
|
||||
- The composite pipeline-program answer (§5.6.3) is correct and cost-free: `GLContext::GetProgramForDraw` (`Core.cpp:592`) already resolves and links the composite entirely frontend-side, so the client pushes one handle and the blocking `JoinLinkAndSpirv()` leaves the server draw path. This closes the objection that killed the prior thin-server design without adding machinery.
|
||||
- P0 landing per-frame byte and call counters BEFORE any migration, and clearing the uncommitted per-draw `fprintf` instrumentation first, is the right sequencing — the tree genuinely has no per-frame byte or call metrics today, so every ring size, batching threshold and wire-granularity decision would otherwise be a guess.
|
||||
- The identity model is sound where it matters: verified that the ABA hazards the re-key table addresses are real and documented in-tree (`TwinLookupMemo`'s owner-equality at `DirectGLES.cpp:83-90` exists precisely because a recycled heap address would otherwise hit a memo slot), and that a dense `{slot, gen}` array index genuinely replaces a Fibonacci-hashed probe plus two `owner_before` calls that touch a control block — a real per-draw win on three lookups per draw.
|
||||
|
||||
### 性能与异步(refuted=True,13 条)
|
||||
### 改造可行性与估时(refuted=False,13 条)
|
||||
|
||||
- **[fatal] Persistent-mapped writes are severed: no map/unmap delta exists, and SyncPersistentMappedRange has zero client-side callers**
|
||||
- 问题:Plan §5.3's trigger→delta table has no map/unmap state at all, and §6.8 defers adoption (AcquirePersistentMap returns nullptr) until P7, so every persistent map stays shadow-backed in P1-P6. The push-down for a shadow-backed persistent map is BufferObject::SyncPersistentMappedRange (MobileGL/MG_State/GLState/BufferState/BufferObject.cpp:238-250), whose first line is `if (!m_isMapped) return;` and whose last line is `NotifySubData(m_mappedRange.start, ...)`. `grep -rn SyncPersistentMappedRange MobileGL/` returns callers ONLY inside MG_Backend/: DirectGLES.cpp:262,4412,4666,4667,4768,4769; Managers.cpp:1547; MultiDraw.cpp:498; DirectVulkan.cpp:290,481,895; UniformManager.cpp:2022; VkBufferManager.cpp:573,620; VulkanRenderer.cpp:3432,3511,3826,7070,12015,12016. There is not one call in MG_Impl or MG_State. In the split that backend code runs on the SERVER against the replica, whose BufferObject::m_isMapped is false (no map delta was ever sent), so it returns immediately; and nothing on the client ever calls it. Worse, the backend's clean-check explicitly depends on the map bit: Managers.cpp:1446-1447 `// A live non-zero-copy map may owe a per-draw SyncPersistentMappedRange push` / `if (frontend->IsMapped()) return false;` — the replica reports the buffer clean and skips the sync entirely. Result: writes made through glMapBufferRange(PERSISTENT|WRITE) without FLUSH_EXPLICIT are silently lost. This is the exact failure class the project already burned a campaign on (memory note flywheel-indirect-lessons: 'unflushed persistent maps' as root cause #1 of the Create/Flywheel fix). It is not a tuning problem — a whole delta kind is missing from the design. The naive repair is also a performance trap the plan never budgets: SyncPersistentMappedRange emits the WHOLE mapped range every draw, so a persistently-mapped chunk arena with adoption disabled (the P1-P6 default) becomes a per-draw whole-range copy into SEG_STAGE plus a per-draw whole-range record.
|
||||
- 修法:Add map/unmap to the delta model: RecBufferMap{handle, range, accessFlags} and RecBufferUnmap{handle} emitted from glMapBuffer*/glUnmapBuffer, so the replica's m_isMapped/m_mappedRange/m_mappingAccess track the client's and both IsBufferDrawClean's IsMapped() gate and the server-side SyncPersistentMappedRange push behave as in monolith. Then make the CLIENT own the range narrowing that the whole-range push lacks: track dirty 64KiB blocks of the mapped span (the same block watermark P4.5 already proposes for WAR) and emit only touched blocks as RecBufferSubData, so the replica's push is a no-op. Add a Split integration scenario that maps PERSISTENT|WRITE|COHERENT without FLUSH_EXPLICIT, writes, draws, and reads back — today no gate in the plan would catch this.
|
||||
- **[fatal] Zero-timeout sync/query polls answered from a local watermark livelock: nothing publishes the ring, and fence completion becomes present-granular**
|
||||
- 问题:Plan §8 and D4 answer GetSyncStatus, ClientWaitSync(timeout=0), IsQueryResultAvailable and GetQueryResult64(wait=false) from a single acquire load on RingControl, with fence/query handles minted client-side and emitted fire-and-forget. Two independent breakages. (a) LIVELOCK: §7.2's Publish() triggers are 64KiB of records, SEG_STAGE below 1/4, any blocking request, Present, eglMakeCurrent, glFlush. A locally-answered poll is none of these. So the canonical LWJGL/Sodium idiom `do { r = glClientWaitSync(s, GL_SYNC_FLUSH_COMMANDS_BIT, 0); } while (r == GL_TIMEOUT_EXPIRED);` never publishes the ring, the server never sees the RecFenceSync record, the watermark never moves, and the loop spins forever. The repository documents that this flush is load-bearing: DirectVulkan.cpp:1150-1156 — 'GL_SYNC_FLUSH_COMMANDS_BIT: flush regardless of timeout, so a zero-timeout poll loop makes progress across calls'. MG_Impl forwards the flags unconditionally (GL_Sync.cpp:96 `return backendClientWaitSync(syncObject->backendHandle, flags, timeout);`), so the client cannot claim the app didn't ask. (b) GRANULARITY: the watermarks the plan proposes (retiredSeq / completedFrameSerial) are advanced on DirectGLES only inside Present() — DirectGLES.cpp:10626-10643 polls the 4-deep g_frameFenceRing after eglSwapBuffers — or inside WaitForFrameSerialCompleted (:10583). A fence created mid-frame therefore reports unsignalled until the NEXT present retires, i.e. fence completion degrades to frame-count inference. DirectVulkan.cpp:1120-1128 states in so many words that this is the bug that was fixed: 'The fence is signaled once that submission's VkFence has been observed signaled, so completion tracks the GPU itself rather than the frame-count inference; MC 1.21.5's fence-paced ring buffers depend on this to recycle their space instead of growing without bound.' The project memory note magma-mc1215-fence-oom records the consequence as a shipped native-heap OOM kill. The plan reintroduces it structurally.
|
||||
- 修法:(a) Make any ClientWaitSync/GetSynciv call carrying GL_SYNC_FLUSH_COMMANDS_BIT an unconditional non-blocking Publish() (release-store head + doorbell if parked) before it answers locally, and add an escalation: after N consecutive locally-answered TIMEOUT_EXPIRED on the same handle, promote to a blocking request. Do the same for IsQueryResultAvailable. (b) Do not resolve fences against a present watermark. Give each RecFenceSync a server-side real backend FenceSync() and publish EvFenceSignaled{handle} from the server's existing per-fence poll; the client's local fast path must be 'handle <= a watermark the server derived from actual per-fence retirement', which on DirectGLES means the server polls its own live syncs outside Present too (it already has WaitForFrameSerialCompleted's fence-selection logic at DirectGLES.cpp:10586-10600 to build on).
|
||||
- **[fatal] MarkGpuWritten modelled as a server→client event is a read-after-write race, not an optimisation**
|
||||
- 问题:Plan §5.6 routes MarkGpuWritten/EnsureGpuResidentStorage back to the client as EvGpuWritten and claims it is 'better than monolith' because the server can name only ranges a shader actually wrote. That inverts the ordering the flag exists to provide. In monolith the flag is set SYNCHRONOUSLY inside the draw call, before it returns: MarkShaderStorageBuffersGpuWritten (DirectGLES.cpp:459-467) walks GetTouchedBufferBindingPointCount(ShaderStorage) and calls obj->MarkGpuWritten(), and is invoked from SyncNeccessaryBuffers on the draw path (DirectGLES.cpp:687,697); likewise SyncAtomicCounterBuffers (:509) and MarkWritableImageBufferTexturesGpuWritten (:1809). In the split the draw is fire-and-forget, so `glDrawElements(...); glMapBufferRange(GL_SHADER_STORAGE_BUFFER, ..., GL_MAP_READ_BIT);` runs entirely on the client before the server has even applied the draw. AcquireMemoryRange calls SyncGpuWrites() (BufferObject.cpp:454), SyncGpuWrites returns immediately because m_gpuWritePending is false (BufferObject.cpp:266), and the app gets the STALE shadow with no error and no round trip. Compounding it, §7.4's event-drain points are glGetError, glGetQueryObject*, glClientWaitSync, eglSwapBuffers and kNeedsAck waits — glMapBuffer*, glGetBufferSubData and glGetNamedBufferSubData (GL_Buffer.cpp:957,995) are not on the list, so even a late-arriving event would not be observed. This breaks an entire family of existing gates the plan schedules for P4 (SsboArrayLengthScenario, AtomicCounterScenario, StorageBufferRegrowScenario) in a way that is data-dependent and will look like flakiness.
|
||||
- 修法:Invert the direction. The client already has every input MarkShaderStorageBuffersGpuWritten uses (GetTouchedBufferBindingPointCount / GetBufferBindingPoint), so WireMirror must set MarkGpuWritten() locally and conservatively at draw/dispatch emit time, mirroring DirectGLES.cpp:459-467, :509 and :1809 exactly. EvGpuWritten{handle, ranges[]} then becomes a pure narrowing hint that can clear the flag or shrink the readback range, and arriving late is harmless. Separately, add glMapBuffer/glMapBufferRange/glGetBufferSubData/glGetNamedBufferSubData to §7.4's drain-point list.
|
||||
- **[major] §6.4's copy table understates both monolith and split; the real split cost is 4 copies (3 after P4.5), not 2 (1)**
|
||||
- 问题:The table in §6.4 claims 'glBufferSubData → shadow store' is 1 copy in monolith, 2 in P1-4, 1 at P4.5. All three numbers are wrong. Monolith is already 2: (1) app→shadow in BufferObject::UploadSubData's Memcpy, then (2) shadow→destination inside FlushPendingRangesNow, which is `Memcpy(dst, bufferObject.MappedData() + start, size)` into an invalidating map (Managers.cpp:914) or `Memcpy(g_uploadRing.store.mappedPtr + ringOffset, bufferObject.MappedData() + start, size)` into the upload ring (Managers.cpp:922). Split P1-4 is 4: app→client shadow (1), client shadow→SEG_STAGE (2), then on the server the applier replays the mutator, so BufferObject::UploadSubData memcpys SEG_STAGE→REPLICA shadow (3), and the server's unchanged FlushPendingRangesNow then memcpys replica shadow→upload ring (4). P4.5 shadow-in-shm removes only copy (2), leaving 3 — it cannot remove (3), because SEG_SHADOW is client-owned/server-read-only by §6.1 while the replica BufferObject owns its own PipeResource allocation. Reaching the claimed 1 would require the applier to hand the backend ops the shm pointer directly instead of calling BufferObject::UploadSubData, which destroys the 'applier = mutator replay, therefore side-effect-identical' invariant that risk R1 rests on, and bypasses the change-serial bump IsBufferDrawClean compares (Managers.cpp:1453). The map path is worse still: glMapBufferRange(WRITE)+unmap is already 3 in monolith (seed staging from shadow at BufferObject.cpp:487, staging→shadow at :200-202, shadow→ring) and becomes 5 in the split. At the plan's own MC pan figure of ~9 MB/frame of section-mesh writes this is 27-36 MB/frame of memcpy, ~1.6-2.2 GB/s of phone memory bandwidth at 60 fps, against a monolith baseline of ~18 MB/frame.
|
||||
- 修法:Correct the table and re-derive the P4.5 target. Either (a) accept 3 and say so, or (b) give the replica BufferObject a PipeResource mode that ADOPTS the client's SEG_SHADOW mapping read-only — a third PipeResource state alongside shadow and gpuMapped, where Bytes() returns the mapped client segment — so the applier's UploadSubData becomes a no-op range note and only the server's ring copy remains (1 copy end to end). That keeps mutator replay intact for every side effect except the byte move. Whichever is chosen, put the TracyPlot byte counters from P0 on BOTH sides of the wire and gate P4.5 on the measured total, not on the client-side number alone.
|
||||
- **[major] The 64 KiB publish threshold serialises the two halves and pre-emptively kills the P2.5 hypothesis**
|
||||
- 问题:§7.2 sets Publish() at 'records ≥ 64KiB', SEG_STAGE below 1/4, blocking request, Present, eglMakeCurrent, glFlush. With the §6.3 record sizes (RecDrawArrays 32B, RecBindBuffer 24B, RecDrawElements 56B) 64 KiB is roughly 1200-2700 records — i.e. an entire Minecraft frame, which the plan itself sizes at 1000-4000 draws. The server therefore cannot begin a frame's work until the client has finished emitting it. That is not asynchrony; it is a pipeline with a one-frame bubble, and it adds a full frame of latency on top of the present credit. It also invalidates P2.5 before it runs: the stated purpose of inproc is to move PrepareForDraw off the GL thread so the two overlap, and a frame-granular publish guarantees zero overlap within a frame. There is no throughput reason for the threshold either — SEG_CMD is an SPSC ring, so 'publishing' is a release store of `head`; the only thing worth amortising is the doorbell write, and §6.2 already gates that on consumerParked.
|
||||
- 修法:Delete the byte threshold. Release-store `head` every record (or every 8-16 records to amortise the store), and ring the doorbell only when RingControl.consumerParked is set. Keep Present/blocking-request/glFlush as explicit doorbell points. Then measure the doorbell rate with the P0 Tracy counters; if the wakeup rate is the problem, raise the consumer's spin window rather than delaying the producer.
|
||||
- **[major] No wakeup path for any client-side wait: present credit, readback replies and ring-full escalation must all busy-spin**
|
||||
- 问题:§7.3 states explicitly: 'server 端不发 credit 消息:它对 RingControl 做 release store'. §6.2 specifies a doorbell only for the CONSUMER (consumerParked + a 1-byte socket write from the producer). There is no producer-side park/wake, so every place the client waits has nothing to block on: the present-credit wait in eglSwapBuffers when presentsSent - presentAckSerial >= 2 (§9), every kNeedsAck blocking request (readback, ClientWaitSync>0, GetQueryResult64 wait=true, AcquirePersistentMap at P7), and the §6.5 escalation's 'bounded 50ms wait on the oldest unretired batch'. All of them reduce to polling a shared cache line across a process boundary. On the target hardware a present-credit wait is up to a full frame (16.6 ms at 60 Hz) of spinning; on Android that is a big core held at full clock against the GPU and the game JVM, and the codebase has no affinity control to keep it off a little core (`grep -rn 'sched_setaffinity\|cpu_set_t' MobileGL/` → 0 hits). The 50 ms escalation wait is a 50 ms spin. This directly contradicts the plan's own framing that the client merely 'blocks on a socket/futex read instead of vkWaitForFences'.
|
||||
- 修法:Add the symmetric doorbell: a producerParked flag in RingControl plus a second byte-stream direction (the socketpair already exists — reserve one byte code for 'watermarks advanced'). Client waits become spin-N-microseconds → set producerParked → blocking read on the socket; server does a release store then, only if producerParked, one byte. Specify a bounded spin (e.g. 50 µs, tuned per phase) and make the spin budget a config knob so it can be measured on 35d0befa and 3B159D009VZ00000 rather than guessed.
|
||||
- **[major] SEG_EVENT has no overflow policy: a full event ring while the client waits on present credit is a two-sided deadlock**
|
||||
- 问题:§6.1 sizes SEG_EVENT at 256 KiB, server-owned, client-read-only, and §7.4 has the server produce EvQueryResult, EvFenceSignaled, EvGpuWritten, EvBufferWriteback, EvReadbackDone, EvGlError, EvDefaultFramebufferInfo, EvCompileEnvInvalidate and — unbounded — EvLogLine{level,len,text}, with the server's MGLOG and deferred diagnostics replayed 'in stream order into the client log stream'. The plan never says what the server does when that ring is full. It also never says the client drains it while WAITING, only 'at each entry point where it could observe them (…eglSwapBuffers)'. Concrete deadlock: the client is inside eglSwapBuffers waiting on presentsSent - presentAckSerial >= 2; the server's mgl-srv-apply thread emits log lines and EvGpuWritten while applying; SEG_EVENT fills; the apply thread blocks producing; presentAckSerial never advances; the client never leaves eglSwapBuffers, so it never drains. Both halves are stuck. This is precisely the 'client blocked on credit while server blocked on the client' shape, and the plan's risk table (R1-R13) does not contain it.
|
||||
- 修法:State an explicit policy: (1) the client MUST drain SEG_EVENT inside every wait loop (present credit, kNeedsAck, ring escalation), not only on entry-point boundaries; (2) EvLogLine is lossy — overwrite-oldest with a dropped-count field, since losing a log line must never stall rendering; (3) semantically load-bearing events (EvGpuWritten, EvReadbackDone, EvFenceSignaled, EvBufferWriteback, EvGlError) are non-lossy, and when the ring cannot take one the server sets an eventRingFull flag in RingControl and stops APPLYING rather than blocking mid-record, so the state is recoverable; (4) add a fault-injection test that fills SEG_EVENT while the client is credit-blocked, alongside P8's SIGKILL test.
|
||||
- **[major] Frames of lag compose: present credit 2 sits on top of the backend's own 2-3, giving 4-5 frames end to end**
|
||||
- 问题:§9 sets the present credit to 2 and argues it 'mirrors the existing budget' (MagmaFramesInFlight=3 clamped to [2, maxImageCount], Espryt's 4-deep fence ring at DirectGLES.cpp:10071-10074) and therefore 'introduces no new stall class'. The stall CLASS is indeed not new, but the LATENCY composes and the plan never adds it up. The server's own Present already blocks 2-3 frames deep before it returns: VulkanRenderer::Present ends by calling FrameContext::WaitAndAcquireNextImage, whose first statement is `vkWaitForFences(device, 1, &frame.imageInFlightFence, VK_TRUE, timeout)` (FrameContext.cpp:288-290). presentAckSerial can therefore only advance once that wait completes. A client allowed 2 outstanding presents ahead of a server that is itself 2-3 GPU frames ahead is 4-5 frames of end-to-end latency — 66-83 ms at 60 Hz — for a first-person game. None of the acceptance gates detects this: SSIM goldens are frame-content comparisons and bench.sh measures FPS, not input-to-photon. The risk register's R11 worries about Magma's present mode but not about the composition.
|
||||
- 修法:Default MOBILEGL_IPC_PRESENT_CREDIT to 1, not 2, and document the composition explicitly (client credit + server FIF + driver depth). Add an input-latency measurement to the P3 and P9 gates — the codebase already has GetGpuTimestampNs and the trace-replay --benchmark per-frame JSON to build a timestamp-to-present histogram — and only raise the credit if a measured throughput win pays for a measured latency cost.
|
||||
- **[major] Total CPU work per draw increases and there is no core-placement plan on a big.LITTLE phone**
|
||||
- 问题:§5.1 says outright that the reconciler 'is the PrepareForDraw reachability walk with sync replaced by emit — not a metaphor: the same set, the same order, the same gating'. That means the walk runs TWICE per draw: once in WireMirror on the client, once in the unchanged PrepareForDraw on the server (DirectGLES.cpp:2916-2975), plus encode and decode. Some of that walk is not cheap: CurrentUnitBindingsEpoch (DirectGLES.cpp:1421-1438) falls through to a full owner-equality walk over every touched texture unit whenever GetTextureBindGeneration() moved, and the code's own note says that happens on redundant re-binds ('26.2 re-binds the unit's own sampler around every texture-unit switch'). The split's entire performance case therefore rests on those two halves landing on two different cores that are both fast. But `grep -rn 'sched_setaffinity\|cpu_set_t\|affinity' MobileGL/ --include=*.cpp --include=*.h` returns zero hits — the library never sets affinity. The server is a separate process launched by fork/exec (§11), so it does not inherit whatever affinity the launcher applied, and the project's own memory (pojav-bigcore-affinity-trap) records that pojavBigCore=true pinned the entire game JVM and MobileGL workers to one core, invalidating a body of historical measurements. If mgl-srv-apply lands on a 1.55 GHz little core it performs strictly more work than monolith did on a 1.96 GHz big core, and the split is a regression by construction. §15's P3 gate ('split frame time within 10% of monolith') would fail for a reason nobody would attribute correctly.
|
||||
- 修法:State the total-CPU-work delta in the plan (client reconcile + encode + decode + server PrepareForDraw vs monolith PrepareForDraw) rather than only the per-side cost. Add explicit affinity: reuse ShaderCompilePool's existing big-core detection (ShaderCompilePool.cpp:73-96 ReadCpuMaxFrequencyKHz / DetectBigCoreCount) to pin mgl-srv-apply to a big core, behind MOBILEGL_IPC_SERVER_AFFINITY, and log the resolved mask. Make P2.5 report per-thread CPU time on both threads, not just wall-clock frame time, so a 'no win' result can be attributed to placement vs to encode cost.
|
||||
- **[major] A shipping build cannot have both runtime split selection and a zero-overhead monolith; the nm/.text proof only covers the OFF build**
|
||||
- 问题:§12's three-layer guarantee and decision D8 prove monolith preservation with `nm --defined-only` plus a stripped .text size diff — but only for MOBILEGL_BUILD_DISAGGREGATED=OFF. Every deployment story in the plan requires ON in the shipped libMobileGL.so: MOBILEGL_TRANSPORT selected via FCL's user-editable env preferences, the plugin APK V2 toggle table, ctest ENVIRONMENT variants, the /data/local/tmp CTS path. And §12 states that in ON builds, inproc mode makes pGLContext a thread-local behind an operator-> shim. That shim sits on the hottest path in the library: `grep -rho 'pGLContext->' MobileGL/MG_Impl | wc -l` = 1494, plus 124 in DirectGLES and 169 in DirectVulkan. On Android a dlopen'd shared library cannot reliably use initial-exec TLS, so each access becomes a __tls_get_addr call — a function call where there is currently a single load of a global reference (Core.h:564 `extern UniquePtr<GLState::GLContext>& pGLContext`). The plan's own estimate of the non-arrow sites is also a guess ('~65 places'); the measured count in MG_Impl alone is 4 (GL_Debug.cpp:99 `.get()`, GL_Program.cpp:1630 `== nullptr`, plus 2 in header/comment context), with ~20 more in MG_State/MG_Backend (Managers.cpp:3608,3737,3808,4663,7120,7128,7131,8678; DirectGLES.cpp:146; TextureObject.cpp:92; BackendObject_DirectVulkan.cpp:388,788; and ~11 MOBILEGL_ASSERT sites in DirectVulkan.cpp:347-461), so the shim must also supply get(), operator bool and equality — but the count being wrong is minor next to the TLS cost.
|
||||
- 修法:Split the option in two: MOBILEGL_BUILD_DISAGGREGATED (spawn/socket only, keeps pGLContext a plain global — one predictable branch in MG_Backend/Init.cpp and nothing on the GL path) and MOBILEGL_BUILD_DISAGGREGATED_INPROC (CI/debug only, adds the TLS shim). Ship the former. Extend the P0 nm/.text gate to run on BOTH the OFF build and the shipping ON build in monolith mode, and make the ON-build check a .text-symbol-level diff of MG_Impl translation units so any accidental indirection on the GL path shows up as a size delta.
|
||||
- **[minor] Memory doubling is unbudgeted: client segments plus a full replica context plus the server's own three rings**
|
||||
- 问题:Risk R4 only tracks server-side glslang RSS during P1-P4. The steady-state data-plane and replica footprint is never budgeted. Client side (§6.1): SEG_CMD 8 MiB + SEG_STAGE 32 MiB growing to 256 MiB + SEG_SHADOW allocations at P4.5. Server side: the replica GLContext holds its own PipeResource shadow for every buffer and its own MipmapStorage for every texture level (the client's shadow is separate unless SEG_SHADOW adoption lands), plus the unchanged backend rings — kUboRingInitialBytes/kUboRingMaxBytes 4→64 MiB, kUnpackRing 4→64 MiB, kUploadRing 4→64 MiB (Managers.cpp:82-96) — plus kMaxPoolBytes = 64 MiB of buffer pool (Managers.cpp:566). That is up to ~450 MiB of new committed memory beyond monolith, on a device where the project already values 'saving ~400MB' as a headline result of the adoption fix and where its own memory notes record blanket-immutable buffers causing LMK kills.
|
||||
- 修法:Add an explicit steady-state memory budget to the plan alongside the round-trip budget, and make P1's acceptance record RSS for BOTH processes (it currently only records the server's). Size SEG_STAGE's ceiling from measurement, not 256 MiB by default. Prioritise the P4.5 replica-adopts-client-shadow change (see the copy-accounting fix) since it removes the duplicate shadow, not just a copy.
|
||||
- **[minor] 'Zero round trip in steady state' is fixture-dependent: BeginConditionalRender always blocks and is not exercised by the chosen gate**
|
||||
- 问题:§8 lists glBeginConditionalRender among the unavoidable blocking points, citing GL_Query.cpp:705-706, and the source confirms it is unconditional: 'Resolved ONCE, here, and by WAITING even for the _NO_WAIT modes: the spec lets those render instead of stalling, so always waiting is conforming and is the only choice that gives the whole block one deterministic verdict.' But P3's acceptance criterion — 'the round-trip counter reads 0 in steady-state frames of minecraft-1.21.4-main-menu' — picks a fixture that exercises neither conditional render nor occlusion queries, so a green gate proves nothing about a renderer that uses them per frame. The same applies to glGetQueryObject(GL_QUERY_RESULT) on an unfinished query.
|
||||
- 修法:Either make the P3 gate assert '0 round trips' across the whole trace-case matrix rather than one menu fixture, or restate the claim as 'zero round trips for the draw/state/upload path' and publish the per-fixture round-trip counts as a table. Consider making conditional render's occlusion resolve a client-side speculative pass-through with a server-side correction, since the spec permits the _NO_WAIT modes to render rather than stall.
|
||||
- **[minor] retiredTail starves in present-less loops and the stated mitigation does not exist on DirectGLES**
|
||||
- 问题:Risk R12 says the server 'also advances that watermark from its own TryDrainFrameTransients / RefreshCompletedSubmits, and publishes it on a timer'. That is true for DirectVulkan but has no DirectGLES counterpart: g_completedFrameSerial is advanced in exactly two places — inside Present() by polling the frame-fence ring after eglSwapBuffers (DirectGLES.cpp:10626-10643), and inside WaitForFrameSerialCompleted (DirectGLES.cpp:10583-10607) which itself requires a live ring fence at or past the target and returns false when the slot was recycled. In a present-less workload — glcts (tools/cts run_cts_local.py), readback loops, MG_IntegrationTest scenarios that never swap — no fence is ever inserted, so retiredTail never advances, SEG_STAGE fills, and §6.5's escalation runs to the hard drain on every case. That converts a CTS run into a sequence of 50 ms spins plus full drains, and could be misread as a conformance regression.
|
||||
- 修法:Give the DirectGLES server an explicit non-present fence tick: insert a glFenceSync and poll the ring on a timer or every N applied records when no Present has occurred for a threshold, reusing the g_frameFenceRing machinery. Log ring-occupancy and escalation counts (the P0 Tracy counters) so a starved watermark is visible as a metric rather than as an unexplained stall, and add a present-less split-mode case to the P2 gate.
|
||||
- **[major] Stage-A snapshot is filled at 2 sites, but 48 of 70 backend entry points read pGLContext outside them**
|
||||
- 问题:§6.2.1 and §11 P1 place `SnapshotFromGLContext()` at exactly two points: the top of `PrepareForDraw` (DirectGLES.cpp:2916) and `SetupDraw` (VulkanRenderer.cpp:6371). §5.1's tracker has exactly four validate entry points (ValidateForDraw/Dispatch/Clear/BlitOrCopy). Both are far too few. Of the 70 distinct `gBackendFunctionsTable.GL.*` entries reached from MG_Impl (89 call sites), 48 are neither draw nor dispatch, and many read pGLContext on their own: `UpdateTextureBindingAtTarget` reads `GetActiveTextureUnit()`/`GetTextureUnitObject()` at DirectGLES.cpp:6051-6052 and is reached from CopyTexImage2D/CopyTexSubImage2D; `GenerateMipmap` reads them at :6876-6877; `GetTexImage` at :9254-9257; `BlitFramebuffer` reads both FBO slots at :5988-5989; `Clear` reads `GetRenderStateParameters().ClearColor` at :4106 and the draw FBO at :4165; the readback family reads pack state at :6129/:7614/:9101/:9480 and the pack PBO at :7622/:8604/:8834/:9144/:9570; DSA-by-name reads at :4038-4043 and :7417-7418. The code says so explicitly: the comment at DirectGLES.cpp:1501-1502 states the no-arg `CaptureDrawTextureSyncKeys` wrappers exist "for every non-draw call site (Clear, readbacks)". The G5 poison mask does not save this: it fires only on a field that was NEVER filled; a field filled by an earlier draw reads STALE, not poisoned.
|
||||
- 修法:Enumerate a validate/fill hook per non-draw backend entry class (texture-op, readback, blit, clear, xfb-span, query, DSA-by-name) in `PipeCalls.def` alongside the verbs, and make G5's written-once bitmask assert per CALL rather than per draw (a field written by draw N must not satisfy the read in the glTexSubImage that follows it). Alternatively make `PipeInputs` accessors lazily filled with a per-call fill generation. Until this is fixed P1's acceptance criterion ("40 traces green under MOBILEGL_PIPE_VERIFY") is unreachable, and §11's day-16 milestone should not be scheduled against the two-site design.
|
||||
- **[major] Pushing texture resource_subdata at GL-call time destroys the dirty-rect coalescing the plan's own +6 ms/frame evidence rests on**
|
||||
- 问题:§5.1 states the rule "only resource mutations push at GL-call time — which is exactly what BufferBackendOps does today". That is true for buffers and false for textures. `glTexSubImage*` never calls the backend table at all: MG_Impl/GLImpl/Texture/GL_Texture.cpp:1817, :1937, :2004 only call `MarkStorageDirtyRegion`. Espryt coalesces the ACCUMULATED region at sync time (Managers.cpp:4274-4311), where MipmapStorage's 96-rect cascade merge and the `summedArea*4 >= unionArea*3` union-box fallback run, and then deliberately collapses the rect list to one box when the unpack ring is live (`if (BufferImpl::UnpackRingAvailable()) dirtyRectCount = 0;`, :4321) with the in-tree measurement "~100 sprite rects become ~100 jobs ... measured +6 ms/frame of GPU time in MC's animated-atlas ticks. One box, one job." Emitting one `resource_subdata` per glTexSubImage call reproduces exactly the ~100-job shape. §7.3 gestures at a deferred "emission cursor" but never resolves the contradiction with §5.1, and §5.1 is the section an implementer will follow because it is written as the design's most emphatic rule.
|
||||
- 修法:Amend §5.1 to say the GL-call-time rule applies only to the ops that already dispatch at GL-call time today (the seven BufferBackendOps hooks). State that texture subdata is accumulated in the client's existing MipmapStorage rect model and emitted at the next validate/flush point, so the merge heuristic keeps running before anything crosses the interface. Add a MOBILEGL_PIPE_STATS counter for `resource_subdata` emits per frame with an explicit ceiling on the MC animated-atlas fixture.
|
||||
- **[major] Sub-rect texture upload is gated on pointer identity and whole-level stride arithmetic that no MGPBlobRef can satisfy in split mode**
|
||||
- 问题:§5.4 prices subsystem 5's repack family as "unchanged in place, only the input changes from a pulled shadow pointer to an MGPBlobRef (the same pointer in monolith)". The code does not permit that. Managers.cpp:4278-4283 gates the whole sub-rect path on `uploadData == mipData` — literally "the upload source IS the whole level shadow" — and :4288-4293 computes `regionPtr = uploadData + z*levelSliceBytes + y*levelRowBytes + x*bpp`, striding into the FULL level with UNPACK_ROW_LENGTH; `rectShadowPtr` (:4321-4326) does the same per rect. The comment at :4270-4273 says conversion fallbacks "rewrite the whole level into a fresh buffer, so they stay on the full-level path" — i.e. the moment the source is not the level shadow, sub-rect upload is disabled by design. In split mode the client can stage (a) the whole level every time, which destroys the bandwidth benefit and contradicts §0.4's "零副本 / +50-60MiB" headline claim, (b) tightly-packed regions, which makes `uploadData == mipData` false and silently forces full-level uploads, or (c) nothing — requiring a server-side whole-level mirror, which IS the duplicated MipmapStorage the plan's strongest argument against the earlier (since-dropped) design says it avoids. §4.5.6's "carry both box and rect list, server picks the shape" does not address the stride source at all.
|
||||
- 修法:Redefine MGPSubData so each region carries {dstBox, srcRowStride, srcSliceStride, blob} and rework Managers.cpp:4274-4326 to take a strided-source descriptor instead of comparing pointers, so the server can set UNPACK_ROW_LENGTH from the descriptor over a tightly-packed staged region. Move this out of "原地不动" and into subsystem 5's day estimate, and add a Mali-device gate that publishes the box-vs-rect job count and frame-time delta at P3b/P4b exit — the plan already names this as B-R5's cliff but assigns it no work.
|
||||
- **[major] The XFB scatter path is a read-modify-write of the client's buffer shadow, and MGPipeCallbacks has no buffer pull**
|
||||
- 问题:§7.2 assigns all 8 `WritebackFromBackend` sites to `MGPReplySlot` (readback) plus `on_buffer_writeback` (XFB capture, PBO readback) — all one-way server→client. But `ScatterCapturedRecords` (DirectGLES.cpp:928) does `Memcpy(staged.data(), target.buffer->MappedData() + target.start, rangeBytes)`: it STARTS from the application's existing bytes so that the holes `gl_SkipComponents` asks for keep whatever the application had put there (the comment at :891-895 says this is "the whole point of the feature"), patches only the captured varyings in, then writes back and re-uploads. The server has no `MappedData()`, and §7.1's callback table has `on_texture_pull_request` but no buffer equivalent. As specified the scatter either zero-fills the skip holes — a conformance break; DirectGLES.cpp:882-883 names `KHR-GL46.transform_feedback.capture_special_interleaved_test` as the case that reaches this path — or needs an unnamed synchronous reverse buffer read at glEndTransformFeedback, a stall class the plan's §9.2 roundtrip table does not list.
|
||||
- 修法:Move the scatter to the client: the server pushes the packed scratch bytes via `on_buffer_writeback`, and the client — which owns the destination shadow and already has `GetTransformFeedbackVaryings()`/`GetTransformFeedbackStride()`/`GetTransformFeedbackPackedStride()` from the reflection archive — performs the patch and re-emits the range as an ordinary `resource_subdata`. If the scatter must stay server-side, add an explicit `resource_read_host(res, off, size)` reverse request to §7.1 and price its stall in §9.2 next to the texture pull.
|
||||
- **[major] The unit-bindings debouncer is deleted while its dirty signal is replaced by the very counter it exists to filter**
|
||||
- 问题:§2.5, §10.4-1, and §4.7.3 D3/D9 book ~115 lines at DirectGLES.cpp:1372-1489 as deleted because "the push call IS the change signal". But the comment at DirectGLES.cpp:1412-1421 states why `CurrentUnitBindingsEpoch` exists: `GetTextureBindGeneration()` bumps on REDUNDANT re-binds (26.2 re-binds the same sampler around every texture-unit switch), so the counter is untrustworthy and the epoch is built to "move exactly when WHAT is bound changes, never on a redundant re-bind". §5.2 then names `GetTextureBindGeneration()` as a dirty-bit input for NEW_SAMPLER_VIEWS. The tracker therefore re-emits `set_sampler_views` on every redundant re-bind, and D9's replacement (`viewSetSerial` bumped by the server inside `set_sampler_views`) invalidates the server's resolved-binding and sampler-pass memos on every batch — a per-batch regression on the exact workload the project optimises for, concealed inside a claimed 115-line deletion. `set_sampler_views` is a kVarTail `set_*`, not a CSO, so §4.2.3's "content addressing gives N=0 for repeated state" does not cover it; the same holds for `set_shader_images` and `set_shader_buffers`.
|
||||
- 修法:State that the debounce MOVES to the client rather than disappearing: the tracker must hash the resolved view/image/buffer sets and suppress the emit on an unchanged hash (`MGPFramebufferState::contentHash` already demonstrates the pattern — extend it to the other var-tail set_* calls and use it client-side as an emit suppressor, not only as the server's memo key). Re-charge ~115 lines to MG_Impl/Pipe/Tracker.cpp and correct §10.2's per-draw arithmetic and §10.4's deletion count accordingly.
|
||||
- **[major] Multi-draw cannot be split by a static screen cap: tier selection is per-batch and depends on backend-only program facts**
|
||||
- 问题:§5.8 assigns "CPU tier on the client (!kCapMultiDraw); compute tier stays server-side". `ResolveTierForBatch` (MultiDraw.cpp:282-320) chooses among five tiers PER BATCH using `programReadsDrawID` — a property of the transpiled ESSL, which exists only on the server — plus `perSubDrawBaseVertex` and the batch's index totals against `kMaxFlattenedIndices` (MultiDraw.cpp:72, 1<<24) and `kMaxComputeFlattenedIndices` (:82). The auto ladder is Ext → BaseVertex → MultiIndirect → Indirect → DrawElements (:241-243), so the CPU-flatten `DrawElements` tier is a FALLBACK reached only after the batched tiers decline for reasons the client cannot evaluate. A client that flattens whenever `!kCapMultiDraw` bypasses the BaseVertex and compute tiers; a client that does not flatten leaves the server-side fallback with no index bytes in split mode. `kCapMultiDraw*` as a lowering-ownership switch is therefore not expressible.
|
||||
- 修法:Keep all five tiers server-side. Carry what they need through the interface instead: `draw_vbo(info, indirect, MGPDrawRange[], numDraws)` plus a `kCapNeedsHostIndexBytes`-gated `MGHostSpan` for the index data, with the server deciding the tier. Delete `kCapMultiDraw`/`kCapMultiDrawIndirect`/`kCapMultiDrawIndirectCount` from §5.8's ownership table and replace them with a single rule: the server always owns multi-draw tiering; the client supplies index bytes when the caps say the server may need them.
|
||||
- **[major] on_texture_pull_request can park a twin forever: there is no negative completion**
|
||||
- 问题:§7.5(b) says the server marks the twin not-ready and the client re-emits on its next publish, and §9.2-9 says the resulting stall lands on mgl-srv-apply. But the client may have nothing to send. `RequireImageBindableStorage` (Managers.cpp:2789-2822) re-dirties every level of every upload target, and the replay reads the shadow — while :2810-2812 already skips levels whose `GetMipmapByteSize(...)` is 0, and a level whose content came from rendering, from a `glCopyTexSubImage` into a shape `CanMirrorCopyImageShadow` declines (DirectGLES.cpp:7068-7073), or from a GPU-side mip generation has no client bytes at all. With no negative completion the apply thread blocks on a twin that never becomes ready. B-R4 and the `TextureRemintPullScenario` gate address the RATE of pulls, never the unanswerable pull.
|
||||
- 修法:Make the pull a request/response pair terminated by an explicit `resource_subdata_complete(res, target, firstLevel, levelCount)` that may carry zero regions, and specify that the server proceeds with allocated-and-empty storage on an empty answer (matching today's monolith behaviour) with a logged diagnostic. Add the unanswerable case — a texture whose only content came from rendering, then image-bound — to TextureRemintPullScenario, and require the scenario to be red before the terminator lands.
|
||||
- **[major] MOBILEGL_PIPE_VERIFY is the plan's only semantic gate, and P13 deletes the code that produces its reference**
|
||||
- 问题:§13.3-② calls the per-draw per-field shadow compare "the decisive one" and §0.4 D-B5 makes it the whole justification for abandoning the earlier byte-identity monolith gate. Verify computes its reference by calling `SnapshotFromGLContext()` (§6.2.1 stage B). §6.7 and §11 P13 then say: "delete SnapshotFromGLContext(), the MGB_CTX macro, MOBILEGL_PIPE_PUSH ... KEEP the MOBILEGL_PIPE_VERIFY harness for later work." With the snapshot gone, verify has nothing to compare against; after P13 the design has no semantic tripwire at all. Open question 11 half-acknowledges the same hole for split-only diagnosis ("the plan's server has no MG_Impl, so a split-only rendering bug has no second opinion") without connecting it to the loss of verify.
|
||||
- 修法:Decide this before P0 freezes the gate list, because it changes what P13's purity gate may assert. Either keep SnapshotFromGLContext() compiled only under MOBILEGL_PIPE_VERIFY past P13 and scope the purity gate's `grep -c 'pGLContext' MG_Backend/` to the non-verify build, or replace it at P13 with the recorded-golden mode the plan already sketches at §10.4-9: turn MG_Test's mock backend into an MGPipe recorder, capture pushed state per draw on a set of fixtures, and diff future builds against the stored trace.
|
||||
- **[minor] Texture parameters are modelled only on sampler-view CSOs, but they are per-texture-object state that non-sampled textures still need**
|
||||
- 问题:§4.7.1 maps the "TexParam / SamplerParam" delta class (9 read points) entirely onto `create_sampler_view` (base/max level, swizzle, dsMode) plus `create_sampler_state`. But Espryt calls `SyncTextureParamsToBackend` for every touched unit binding AND every draw-FBO attachment texture (DirectGLES.cpp:1548-1560 for the unit list, :1580-1601 for the attachment list), and `RequireImageBindableStorage` sets `m_forceTextureParamsResync` precisely because a channel-widened carrier needs a swizzle override the frontend params version never moves (Managers.cpp:2815-2821). A texture that is only an FBO attachment, only an image-unit binding, or only a `glCopyImageSubData` endpoint has no sampler view, so under §4.7.1 its `glTexParameter` state has no carrier across the interface.
|
||||
- 修法:Put base/max level, swizzle, depth-stencil mode and the LOD clamps on `MGPResourceDesc` or a dedicated `set_texture_params(res, ...)` call, and let `MGPSamplerView` carry only the view restriction (min/num level, min/num layer, alias format). This also keeps `glTextureView` modellable as what it actually is — a real texture object with its own parameters that can itself be an FBO attachment and a glTexSubImage destination (TextureObjectView.cpp:281, :290) — rather than the "ordinary view CSO" §4.5.4 reduces it to.
|
||||
- **[minor] The client's per-(texture, uploadTarget, level) emission cursor aliases across glTextureView and its storage owner**
|
||||
- 问题:§7.3 inverts dirty ownership and gives the client a cursor keyed on `(texture, uploadTarget, level)` that it clears on emit. But `TextureObjectView` forwards `IsStorageDirty`, `MapMipmapData` and `GetStorageDirtyRegion` to the storage OWNER's mipmap with index remapping (TextureObjectView.cpp:290-322, and :281 writes into the owner's data). A view and its owner therefore share one underlying dirty state while carrying two independent cursors: whichever emits first clears the flag the other still needed, or both emit the same texels. The plan's own §4.7.3-D18 discipline about not "optimising" a documented hazard away applies here too, but the aliasing is never mentioned.
|
||||
- 修法:Key the emission cursor on `(storageOwner, ownerUploadTarget, ownerLevel)` — resolve through `GetViewStorageOwner()` and the view's `ToOwnerUploadTarget()`/`ToOwnerLevel()` mapping before consulting or clearing. Add a scenario that uploads through a view and samples through the owner (and the reverse) across a draw boundary.
|
||||
- **[minor] The OOM-ack story names entry points that never reach the backend**
|
||||
- 问题:§7.4 and §9.2-7 mark "glRenderbufferStorage*, the failure-capable forms of glTexImage*/glTexStorage*/glCopyTexImage*, and glBufferStorage" as kNeedsAck so the OOM-probe idiom works. The texture family never calls the backend table at all: MG_Impl/GLImpl/Texture/GL_Texture.cpp only calls `MarkStorageDirty(..., true)` at :2515, :2671, :2755, and Espryt allocates lazily at sync time. `RecordGLError` (DirectGLES.cpp:6309-6324) — the texture-side error reporter — has exactly one caller, glGenerateMipmap at :6916. Even the one genuine synchronous allocation, `glRenderbufferStorage*`, runs its OOM check inside `BackendRenderbufferObject::SyncToBackend` (Managers.cpp:8674-8684), i.e. also lazily. So kNeedsAck as specified has no producer for the texture family, and the renderbuffer case would need a forced sync at the GL call to be ackable at all.
|
||||
- 修法:Enumerate the actual synchronous allocation points rather than the GL entry points that look like them. State plainly that texture allocation OOM is already deferred to sync time in the monolith so the split changes nothing observable, and restrict kNeedsAck to the one case that can be made synchronous (renderbuffer storage, if forced to sync at the GL call) plus glBufferStorage. Otherwise §9.2-7's "rare and already expensive, so the ack is nearly free" is pricing a mechanism that does not fire.
|
||||
- **[minor] SEG_STAGE sizing omits the largest single-call payload the plan itself moves to the client**
|
||||
- 问题:§8.2 lists four new byte classes for SEG_STAGE (client vertex arrays, client index arrays, multi-draw argument blocks, client-resolved indirect command blocks) and claims "byte volume unchanged — they are re-uploaded per draw today". The whole-EBO primitive-restart rewrite that §5.8 moves to the client is not among them, and it is bounded at `kMaxRestartRewriteBytes = SizeT{1} << 26` — 64 MiB (DirectGLES.cpp:4218) — twice the default `MOBILEGL_IPC_STAGE_MB=32` in Appendix B. Unlike client vertex arrays these bytes are not re-uploaded per draw today: the rewrite lands in a backend scratch buffer the driver keeps. The multi-draw flattened index stream (kMaxFlattenedIndices = 1<<24 indices, MultiDraw.cpp:72) is in the same class.
|
||||
- 修法:Add the restart-rewrite blob and the multi-draw flattened index stream to §8.2's list, size SEG_STAGE against them or specify the grow/decline path for a single record larger than the segment, and keep the ceiling check with its `m_valid=false` decline and MGLOG_E_ONCE on the client (DirectGLES.cpp:4401-4409) so the diagnostic still fires on the thread that issued the draw.
|
||||
- **[minor] The fixed validate order puts set_shader_images after set_draw_program, contradicting D-B3's own argument**
|
||||
- 问题:§5.3's order is 1 framebuffer, 2 program, 3 sampler views / images / buffers / global constants, 4 render state, 5 vertex. D-B3 (§0.5) and §5.3 both claim the fixed order is what retires `ImageUnitFormatsStillMatch` (Managers.cpp:6545-6573, whose comment says it is "not expressible as a monotone version") by telling the server the image formats before the program build — but images are pushed at step 3, after the program at step 2. It only works because D-B2 defers specialization to draw time. And once specialization is deferred to `draw_vbo`, the framebuffer-before-program ordering argument carries no weight either: what actually retires the fragColor-broadcast workaround at DirectGLES.cpp:2712-2732 is LATE specialization, not call order. An implementer who takes §5.3 literally will build ordering assumptions the design does not need and does not honour.
|
||||
- 修法:Replace the numbered order with the invariant that actually holds: all set_* for a command complete before the verb, and the server specializes the shader at the verb from whatever has been pushed. Then §5.3's list is a convenience, and D-B3's claim should be restated as "late specialization plus complete state at the verb" rather than "framebuffer strictly first".
|
||||
|
||||
已验证的优点:
|
||||
- The replica-GLContext decision is correct and the cited evidence holds. IsBufferDrawClean opens with a raw-pointer identity compare before any version check (Managers.cpp:1435-1436, 'Identity first: a respecify path can hand the frontend a NEW resource'), and CurrentUnitBindingsEpoch (DirectGLES.cpp:1421-1438) resolves its epoch by an owner-equality walk over the live binding slots precisely because the bind generation moves on redundant re-binds. Neither has a wire-field analogue. Rewriting the backends to consume deltas would require re-deriving this invalidation model, which is what sank Feat/CS-Delta-IPC.
|
||||
- D3 — not shipping version counters and letting the replica bump them through mutator replay — is sound and avoids the failure mode of the prior branch. The counters that gate re-sync really are wrapping Uint16 paired with pointer identity, and replaying mutations makes both sides run the same wrap logic instead of maintaining monotonicity on the wire. It also avoids adding Install* setters, which is how Feat/CS-Delta-IPC's b50f3348 leaked RenderState's private members to public.
|
||||
- The claim that unpack pixel-store never crosses the boundary is verified: all six backend reads pass false (PACK) — DirectGLES.cpp:6129, 7614, 9101, 9480; Utils.cpp:2301; VulkanRenderer.cpp:10622. Confining PixelStoreBlob to the PACK direction is correct and removes a delta kind.
|
||||
- Using FlatBuffers structs as fixed-layout records inside an SPSC ring, with tables reserved for the rare/variable control-plane messages, is the right call: structs have no vtable, no offset indirection and need only a bounds check rather than a verifier walk. The per-kind static_assert in Records.def is a genuine fix for the exact bug Feat/CS-Delta-IPC hit (its single assert on the first union member could not catch mid-list insertion).
|
||||
- The observation that glReadPixels into a pack PBO can become fire-and-forget and thereby beat the monolith is correct: today DirectGLES maps the whole PBO back and writes it into the frontend shadow inside the call (DirectGLES.cpp:9189-9205), so there is no asynchronous PBO readback path at all. Same for deferring glEndTransformFeedback's unconditional infinite ClientWaitSync (GL_Drawing.cpp:1326-1337). Both are real wins and both are correctly identified as standalone monolith improvements worth landing on dev first.
|
||||
- Keeping Present strictly 1:1 with the application's eglSwapBuffers is correct and well-justified: DirectGLES.cpp:10646-10649 retires the UBO/unpack/upload rings and trims the buffer pool only there, and the Magma side does all four OnFrameBoundary agings plus the BeginFrame calls inside Present. Batching frames would starve those drains.
|
||||
- Porting the ring reclamation discipline from the existing PersistentRing is well-grounded: the RingFrameMark {frameSerial, headAtPresent} structure and the monotonic head/tail with 'in-flight bytes = head - tail must stay <= size' invariant are exactly as described (Managers.cpp:659-706), as is the grow → bounded-wait → hard-drain-plus-generation-bump escalation.
|
||||
- P0's demand to remove the uncommitted per-draw instrumentation is necessary and verified: Managers.cpp:875-877 contains a live std::fprintf(stderr, "[BUFTX] FlushPendingRangesNow res=%p serial=%llu' + NL + '", ...) inside the pendingMutex critical section on the buffer flush path, with a literal ' + NL + ' in the format string. Measuring anything before removing it would repeat the prior branch's mistake.
|
||||
- The refutation of Feat/CS-Delta-IPC's BFA C-ABI, UtilRuntime C-ABI-isation and share-group-sessioning-first ordering is well-founded, and the replacement ordering (thinnest end-to-end path first, device render at day 13, P2.5 as an early falsification gate at week 5) is the right risk sequencing. Making SCM_RIGHTS a P0 deliverable rather than a deferred 'P6' item directly fixes the defect that left the prior branch's data plane inoperable on Linux and Android.
|
||||
- The dead-code cleanups are real and verifiable wins for the monolith independent of the split: GetInteger64i_v and GetProgramiv have no MG_Impl callers, and routing glDispatchCompute's three per-dispatch GetIntegeri_v validation queries to the already-captured CompileEnv limits removes a genuine per-dispatch cost.
|
||||
- The dead-capability finding is real and independently verified: CapabilityInput::FramebufferSrgb and DepthClamp exist as enum values (RenderState.h:165, :168) but SetCapability falls to `default: // not supported currently` (RenderState.cpp:380) and IsCapabilityEnabled returns false at the `default:` arm (:428-429). All six backend consumers therefore read a constant false today. §10.4-6 is right to demand an answer before the render-state blob is frozen; writing the interface down genuinely surfaced this.
|
||||
- The dirty-ownership inversion (§7.3) is sound and rests on a fact I verified: `grep -rn 'IsStorageDirty|GetStorageDirtyRects|GetStorageDirtyRegion' MG_Impl/` returns exactly 0 hits — the frontend never reads its own texture dirty state, only sets and clears it. Deleting PLAN.md §5.6a's ack protocol and risk R6 is therefore justified.
|
||||
- The backend-memo-writeback asymmetry is exactly as claimed: DirectGLES writes zero Set*Memo calls into frontend objects (0 grep hits under MG_Backend/DirectGLES/), while DirectVulkan writes four — ProgramFactory.cpp:3448 and VertexInputStateFactory.cpp:60/78/83, with :78 storing a raw backend-heap pointer (`vao.SetBackendStateMemo(&entry, m_evictionEpoch)`). D12's verdict of "delete outright, do not translate" is the right call and the D13 VaoDrawMemo replacement really does already exist.
|
||||
- D21 is a genuine latent bug, verified: `VulkanRenderer::CurrentXfbCounterSlot` (VulkanRenderer.cpp:11136-11146) keys `m_xfbCounterSlotByObject` on `GetBoundTransformFeedbackName()` — a raw, LIFO-recycled GL name with no generation — so a deleted-and-regenerated XFB object inherits the predecessor's counter slot. Landing this on `dev` independently at P0 is correct sequencing.
|
||||
- The composite-pipeline-program answer ("nothing to do") is correct. GLContext::GetProgramForDraw (Core.cpp:592-660) already performs the whole flattening frontend-side, including both J1 join sites, `ComputeDrawProgramSignature()`, and `MakeShared<ProgramObject>(0u)` at :644 with the in-code rationale "deliberately not a named program ... backend registries key on the object, not the name". Deleting PLAN.md's proposed `SetReplicaResolvedDrawProgram` hook is justified, and this answers the prior judges' "unpriced composite" objection.
|
||||
- Moving the CopyImage shadow mirror to the client is correct and does delete a whole reverse byte channel. `MirrorCopyImageIntoDestinationShadow` (DirectGLES.cpp:7085-7148) is a pure shadow→shadow row memcpy whose eligibility (`CanMirrorCopyImageShadow`, :7068-7073 — single upload target, not 1D-array) and whose bounds/texel-size checks are all decidable from frontend data alone, and it deliberately does not mark dirty.
|
||||
- `RecProgramLinkOp` really is impossible, not merely undesirable: ProgramObject.h:11 includes ShaderObject.h, which at :12 includes ShaderCompileTask.h and at :145 returns `const SharedPtr<glslang::TShader>&`; ProgramObject.h:14 pulls SpvcSession.h. Collapsing PLAN.md's two program tiers to one, deleting phase P5, and promoting `nm -D | grep glslang` to a P7 acceptance criterion all follow correctly.
|
||||
- §2.4's catalogue of the 58 non-arrow `pGLContext` uses is a real gap no prior design caught, and DirectGLES.cpp:146 (`MG_State::GLState::GLContext* ctx = MG_State::pGLContext.get();`) is verified as sed-invisible. The adjacent `using FbBindingSlot = std::remove_reference_t<decltype(MG_State::pGLContext->GetFramebufferBindingSlot(...))>` at :142 is a second wrinkle in the same family. Making the purity gate grep `pGLContext` rather than `pGLContext->` is the right response.
|
||||
- The interface-purity gate (§4.7.2) is a genuinely stronger completeness argument than the prior branch's 477-row read inventory: making `MG_State::pGLContext` undeclared in the MGPipe build turns every unsatisfied read into a named compile error rather than a catalogue entry that can go stale. Keeping the inventory only as a G6 coverage checklist is the right demotion.
|
||||
- Carrying the CPU-modelled XFB vertex count on MGPDrawInfo is correct on the point I expected to be wrong: `AccountTransformFeedbackPrimitives(mode, count)` runs BEFORE the backend draw call (GL_Drawing.cpp:1132-1133, :1140-1141), so the value pushed with a draw already includes that draw's contribution.
|
||||
- The function-pointer-struct-not-vtable decision (§4.1) is well grounded in this codebase: the boundary already is a function-pointer struct installed at one hook point, null entries already mean "not implemented, frontend falls back", and that is the natural expression of a partially migrated subsystem during the strangler. A pure-virtual class would need stub overrides that lie.
|
||||
- D18 being the single identity row marked UNCHANGED — the deliberate node-based `std::unordered_map` for VkTextureManager/VkRenderPassManager resources, with the BlitFramebuffer "layout undefined" postmortem carried verbatim into the review checklist — is exactly the right instinct for a refactor of this size, and B-R8 names the failure mode (someone "optimising" it back) correctly.
|
||||
- The plan is honest about the two things that most threaten it: D-B5 states in the open that the earlier byte-identity monolith gate dies by construction and is a cost of this design, and B-R2 states that the central performance claim (the reachability traversal moves rather than doubles) is unmeasured and that the tree has no per-frame byte or call metric today. Landing TracyPlot counters and clearing the working-tree per-draw fprintf in P0, before any migration, is the correct ordering.
|
||||
|
||||
### 可行性/平台/交付(refuted=False,12 条)
|
||||
### 性能(refuted=False,14 条)
|
||||
|
||||
- **[fatal] Shadow-backed persistent (COHERENT) maps are never published to the server — app writes are silently lost**
|
||||
- 问题:`BufferObject::SyncPersistentMappedRange()` (MobileGL/MG_State/GLState/BufferState/BufferObject.cpp:238-250) is the ONLY publisher of writes an application makes through a persistent, non-FLUSH_EXPLICIT, non-adopted map: it emits `NotifySubData(m_mappedRange)`. Every one of its call sites lives inside MG_Backend/ (verified by grep: DirectGLES.cpp:262,4412,4666,4667,4768,4769; Managers.cpp:1547; MultiDraw.cpp:498; DirectVulkan.cpp:290,481,895; UniformManager.cpp:2022; VkBufferManager.cpp:573,620; VulkanRenderer.cpp:3432,3511,3826,7070,12015,12016). There is ZERO caller in MG_Impl or MG_State. Plan §6.8 makes tier T2 (`AcquirePersistentMap` returns nullptr) the default for phases 1-6. `AcquireMemoryRange` (BufferObject.cpp:459-475) then falls back to the shadow and hands the app `m_resource.Bytes() + range.start`. The app writes into the CLIENT's shadow and makes no further GL call — that is the entire point of a coherent persistent map. In split mode the backend runs against the replica, so it calls `SyncPersistentMappedRange()` on the REPLICA's BufferObject, which is not mapped by anything. The client's emit-ops table is never invoked, no delta is produced, and the server draws from whatever the shadow held at map time. The plan's §6.8 rationale explicitly reasons only about FLUSH_EXPLICIT ('FLUSH_EXPLICIT 恰是跨进程的好情况') and concludes the coherent case is covered by declining adoption. It is not: declining adoption is precisely what routes into the unpublished path. `MOBILEGL_COHERENT_AS_FLUSH` defaults to false (Config.h:174), so an app that itself passes GL_MAP_COHERENT_BIT — the modern streaming idiom, and the reason Config.h:168-174 exists at all — lands here unconditionally. No listed gate before P7 covers this. OpenRA (the P1 gate) does not use persistent maps.
|
||||
- 修法:Make the client the publisher. WireMirror must call `SyncPersistentMappedRange()` on every currently-mapped buffer reachable from the operation at each emit point, mirroring the backend's 13 call sites (VAO attribute buffers, index buffer, indirect/parameter buffers, UBO/SSBO/atomic binding points, XFB capture targets) BEFORE it samples `GetChangeSerial()`. Keep a client-side `ska::flat_hash_set` of live persistent-mapped buffers so the walk is O(mapped) not O(all). Add a P1 acceptance scenario (`PersistentCoherentMapScenario`) that maps PERSISTENT|WRITE|COHERENT, writes with no further GL call, draws, and reads back — and require it green before P1 is declared done, not at P7.
|
||||
- **[fatal] The applier replays GLFunctionsTable, not MG_Impl — MG_State mutations MG_Impl performs around table calls never reach the replica**
|
||||
- 问题:Plan §3, §5.2 and risk row R1 all rest on 'applier = mutator replay, so the replica's versions bump exactly when the client's did' and on R1's claim that divergence would require 'the client's ENTRY POINT doing something the applier did not replay, and that is a bounded, auditable surface (the 91 MG_Impl call sites into GLFunctionsTable)'. That surface is exactly where the bugs are, it is not bounded by anything the plan gates, and I found two concrete, shipped instances: (a) glGenerateMipmap. `GLImpl::GenerateMipmap` (MG_Impl/GLImpl/Texture/GL_Texture.cpp:6681-6691) runs `EnsureGeneratedMipmapStorageAllocated(*mipmapTexture)` BEFORE `GenerateMipmap_Backend`. That helper (GL_Texture.cpp:501-545) calls `AllocateStorage` for levels 1..N, `MarkStorageDirty(...,false)` (:528), `TruncateMipmapLevels` (:533) and `BumpContentVersion()` (:537). The comment at :534-537 states why the version bump exists: without it 'a cached sampled VkImageView built for the pre-generate level range would otherwise stay stale and clamp LOD>0 sampling to mip 0'. An applier that only calls the table reproduces that exact known bug on the replica. Same for `GenerateTextureMipmap` (:6705-6711) and `MaybeAutoGenerateMipmap` (:1625-1635). (b) Transform feedback CPU accounting. `AccountTransformFeedbackPrimitives` (MG_Impl/GLImpl/Drawing/GL_Drawing.cpp:172-236) mutates six GLContext counters on every captured draw: `AddTransformFeedbackPausedPrimitives` (:177), `AddTransformFeedbackInputPrimitives` (:184), `AddTransformFeedbackGeometryCaptureDraw` (:214), `AddTransformFeedbackPrimitives` (:231), `AddTransformFeedbackCapturedVertices` (:232), `AddTransformFeedbackAccountedCaptureDraw` (:237). DirectGLES reads `GetTransformFeedbackCapturedVertices()` at DirectGLES.cpp:900 to size the scattered capture, and DirectVulkan reads `GetTransformFeedbackPausedPrimitiveCounter()` at DirectVulkan.cpp:1384 and folds the frontend delta into the query result at :1337. On the replica every one of these stays 0: scattered XFB captures nothing and PRIMITIVES_WRITTEN/PRIMITIVES_GENERATED are wrong. None of these counters has a version counter; none appears in the plan's §5.3 trigger table or its §5.1 reconcile walk. They are additionally saved/restored per XFB object on bind (Core.cpp:1273,1296; Core.h:313-357), so a naive 'ship the scalar' patch must follow the object swap. The §5.9 coverage generator cannot catch this class: it scans MG_Backend/** for READS and maps them to delta kinds, so a backend read of `GetTransformFeedbackCapturedVertices` would be classified and pass, while the PRODUCER half in MG_Impl is never audited.
|
||||
- 修法:Add a second generated inventory to §5.9: every `pGLContext->` MUTATOR call in MG_Impl that occurs in a function which also calls `gBackendFunctionsTable.GL.*` or `pActiveBackendObject->`. Each entry must be marked replayed-by-applier, shipped-as-delta, or explicitly client-only, with a `#error` on unmapped — the same compile-time gate the read side gets. Concretely: (1) factor `AccountTransformFeedbackPrimitives` and `EnsureGeneratedMipmapStorageAllocated` into shared helpers the applier also runs, or ship them as explicit `RecXfbAccounting` / `RecGenerateMipmapLevels` deltas; (2) move the P8 Xfb* scenario gate earlier, into P2, so this class of divergence surfaces before four more phases are built on the assumption.
|
||||
- **[major] Read-after-GPU-write is gated by a flag the client can never set in time — glMapBufferRange(READ) returns stale bytes with no round trip**
|
||||
- 问题:`BufferObject::SyncGpuWrites()` (BufferObject.cpp:265-274) early-returns unless `m_gpuWritePending`, and that flag is set only by `MarkGpuWritten()` (BufferObject.cpp:260-263), whose only callers are in MG_Backend/ (DirectGLES.cpp:465, 509, 1809; UniformManager.cpp:1073, 1229; VulkanRenderer.cpp:11210) plus the resident-SubData branch in `UploadSubData`, which cannot fire client-side while adoption is off (§6.8 T2). Every reconciliation point calls it: `AcquireMemory` (:405), `AcquireMemoryRange` (:454), `UploadSubData`, `FillSubData` (:351), `CopyDataFrom` (:383), and MG_Impl's `glGetBufferSubData`/`glGetNamedBufferSubData` (GL_Buffer.cpp:957, 995). In the split the client's flag is set only if an `EvGpuWritten` event happens to have been drained already. Plan §6.7 lists 'glGetBufferSubData / glMapBuffer(READ) on gpuWritePending' as a round trip and says it is 'narrowed by EvGpuWritten{ranges}' — but nothing establishes the flag in the first place. An app that dispatches a compute shader writing an SSBO and immediately maps it for read gets the stale shadow, silently, with zero round trip. Same for atomic counters, XFB capture targets, and pack PBOs after the fire-and-forget ReadPixels of §6.7.
|
||||
- 修法:The client must own a conservative pending set, mirroring what DirectGLES already does at DirectGLES.cpp:459-467/687/697: at every emitted draw/dispatch, mark every buffer bound to SHADER_STORAGE / ATOMIC_COUNTER / an image-buffer texture unit, every active XFB capture target, and any pack PBO named by a ReadPixels record, recording the emit seq. On any read entry point, if the buffer is in that set: publish, wait for `appliedSeq >= recordedSeq`, drain events, then read. `EvGpuWritten` becomes a pure narrowing optimisation (it may cancel or range-limit the wait), never the thing that establishes existence.
|
||||
- **[major] Sync and query poll loops deadlock: the polling entry points are not Publish triggers**
|
||||
- 问题:Plan §8 answers `GetSyncStatus`, `ClientWaitSync(timeout==0)`, `IsQueryResultAvailable` and `GetQueryResult64(wait=false)` from a single `RingControl` acquire load with zero round trips. Plan §7.2's Publish trigger list is: 64 KiB of records, SEG_STAGE below 1/4, any blocking request, Present, eglMakeCurrent, glFlush. None of the poll paths appears. The canonical GL idioms are `glFenceSync(); while (glClientWaitSync(s, GL_SYNC_FLUSH_COMMANDS_BIT, 0) == GL_TIMEOUT_EXPIRED) {}` and `while (!avail) glGetQueryObjectuiv(id, GL_QUERY_RESULT_AVAILABLE, &avail);`. With no other GL call in the loop, the `FenceSync` / `EndQuery` record sits in the shm ring with no release-store of `head` and no doorbell; the server never observes it; the watermark never advances; the loop spins forever. This is a hang, not a slowdown. It is also a spec violation the codebase already cares about: GL_Sync.cpp:71-82 validates GL_SYNC_FLUSH_COMMANDS_BIT specifically so a caller cannot 'think it had asked for a flush it never got'. `glGetSynciv(GL_SYNC_STATUS)` (GL_Sync.cpp:172-181) is the same shape.
|
||||
- 修法:Add `glClientWaitSync` (any timeout), `glGetSynciv(GL_SYNC_STATUS)`, `glGetQueryObject*(GL_QUERY_RESULT_AVAILABLE | GL_QUERY_RESULT_NO_WAIT)` to the Publish trigger list — publish (release-store + doorbell) without waiting. Make GL_SYNC_FLUSH_COMMANDS_BIT publish unconditionally, since the spec mandates the flush. Add a starvation escape: after N consecutive polls with no watermark movement, promote to one blocking round trip so a server that has stalled cannot spin the client.
|
||||
- **[major] §5.6's 'the client never clears its texture dirty flags' is provably wrong and makes every texture update ship the whole level**
|
||||
- 问题:`MipmapStorage::MarkDirtyRegion` (MG_State/GLState/TextureState/MipmapStorage.cpp:198-235) UNIONS the incoming box into `m_dirtyRegions[level]` and appends to `m_dirtyRects[level]` for as long as `m_isDirty[level]` is true; only `MarkDirty(level,false)` (MipmapStorage.cpp:171-190) resets them. Plan §5.6 asserts the client never clears ('client 的 dirty flag 从不被清 ... 已发送状态存在 WireMirror 里') while §5.5 rule 3 derives the shipping shape from `GetStorageDirtyRegion`/`GetStorageDirtyRects`. `ShipRecord` (§5.1) holds three `Uint64` version words — no region can be reconstructed from it. Consequences: after the first sub-image the union box only grows, the rect list saturates at `kMaxDirtyRects`, and `GetDirtyRects` returns 0 the moment `summedArea*4 >= unionArea*3` (MipmapStorage.cpp:305). Every animated-atlas tick then ships the entire level — the exact opposite of the §5.5 tuning the plan claims to preserve. `MarkDirtyRegion`'s rect-seeding branch (`if (!m_isDirty[level]) rects.clear(); else if (rects.empty() ...) rects.push_back(region)`, :214-221) is written for a consumer that clears; never clearing changes its behaviour too. Secondary factual error in the same paragraph: the frontend does clear dirty flags itself, at five sites — GL_Texture.cpp:528, 701, 5547, 5621, 5691. The good news I verified: MG_Impl contains no `IsStorageDirty(`/`GetDirtyRects(`/`GetDirtyRegion(` call site at all, so clear-on-emit is safe for the frontend.
|
||||
- 修法:Have the client clear on emit — `MarkStorageDirty(uploadTarget, level, false)` immediately after appending the texture record. That reinstates the ack question §5.6 claims to have dissolved; close it by (a) making `ResyncSnapshot` always ship whole levels from the intact shadow (it can — the shadow is never dropped), and (b) deferring the clear until the record is past a drain-safe watermark, or accepting resync-on-drain. Rewrite §5.6's dirty-flag paragraph accordingly; it is currently the load-bearing justification for a design decision that does not hold.
|
||||
- **[major] `inproc` mode cannot work as specified: the backend function table, the active backend object and the default-FBO info are single process globals**
|
||||
- 问题:§12 hooks the split by replacing `MG_Backend::gBackendFunctionsTable` and `MG_Backend::pActiveBackendObject` — both assigned once, process-wide, at MG_Backend/Init.cpp:43-44 and :53-61. In `inproc` both roles live in one process, so once the client installs the emit table there is no path by which the applier reaches the real DirectGLES/DirectVulkan table, and no path by which server-side MG_Impl code reaches it either. Server-side MG_Impl code exists and reads that global: `GenerateMipmap_Backend` (GL_Texture.cpp:1621), the `GetTexImage` fallback chain (GL_Texture.cpp:6713-6725), `FixupGsStripCaptureOrder`. Worse, `MG_Impl::GLImpl::FramebufferImpl::pDefaultFramebufferInfo` is a second process global (defined GL_Framebuffer.cpp:3344) read by the client at GL_Framebuffer.cpp:495, 1827, 1837, 1897, 1905, 1913, 1927, 1936, 2549, 2590, 2598, 2608, 2611 and by the server-side backend at DirectGLES.cpp:1917, 2838, 2867, 9675 and SwapchainObject.cpp:276. One process cannot hold both a client default-FBO description and a server one; SwapchainObject writes the server's view straight into it. §12 addresses only `pGLContext` (correctly noting the 65 non-arrow uses — I counted exactly 65). This is not a corner: §12 calls `inproc` a product deliverable and P2.5 makes it the plan's EARLIEST falsification gate, so a broken `inproc` removes the week-5 go/no-go entirely.
|
||||
- 修法:Extend the role-scoping mechanism chosen for `pGLContext` to `gBackendFunctionsTable`, `pActiveBackendObject` and `pDefaultFramebufferInfo` — thread-local pointer plus an `operator->`/`get()`/`operator bool` shim, all under `#if MOBILEGL_BUILD_DISAGGREGATED`. Re-scope the D2/§12 claim of 'one hook point in MG_Backend/Init.cpp:47-70': it is four globals, and add the cost to P0's estimate. Alternatively drop `inproc` to a test-only mode with the applier holding an explicitly-passed table pointer and no MG_Impl on the server side — but then P2.5 no longer measures the monolith render-thread deliverable it is supposed to.
|
||||
- **[major] Late glGetError breaks the standard allocation-probe idiom for the backend's GL_OUT_OF_MEMORY sites**
|
||||
- 问题:§5.6 routes backend `RecordError` (DirectGLES.cpp:6319; Managers.cpp:8679; DirectVulkan.cpp:816; VulkanRenderer.cpp:1242, 1246, 1302) through an `EvGlError` event that is explicitly allowed to be observed 'a batch late', with `MOBILEGL_IPC_STRICT_ERRORS` reserved for the CTS lane. Those sites are GL_OUT_OF_MEMORY on renderbuffer and texture allocation. The universal application idiom is `glRenderbufferStorage(...); if (glGetError() == GL_OUT_OF_MEMORY) { fall back to a smaller target; }`. Late delivery makes the app take the success branch and then render into storage the server never allocated — a divergence that shows up as corrupt output or a later server-side failure, far from the cause. The plan is right that glGetError must stay client-local for the hot path (GL_Getter.cpp:2811-2817; the GL-thread-owned invariant at Core.cpp:48-49). The error is treating all backend errors as one class.
|
||||
- 修法:Split the class. Mark only the allocation-class entry points as `kNeedsAck` — `glRenderbufferStorage*`, `glTexImage*`/`glTexStorage*`/`glCopyTexImage*` where the backend can fail, `glBufferStorage`. They are rare and already expensive, so the ack is nearly free, and it makes the OOM probe exact. Everything else keeps late delivery. Drop the global `MOBILEGL_IPC_STRICT_ERRORS` from the CTS-only ghetto; with this split it should not be needed.
|
||||
- **[minor] P4's 'move glCopyTexSubImage wholly to the server' contradicts the existing implementation and references an event the protocol does not define**
|
||||
- 问题:`glCopyTexSubImage*` is already an entirely frontend operation: `CopyTexSubImage{1,2,3}D_State` (GL_Texture.cpp:3955, 3980) call `CopyReadFramebufferIntoMipmapRegion`, which borrows a backend `ReadPixels` into CPU scratch, memcpys into the mipmap shadow, and calls `MarkStorageDirty(uploadTarget, level, true)` at GL_Texture.cpp:1095. Left alone in the split it costs exactly one blocking ReadPixels round trip and the resulting dirty region ships as an ordinary texture delta — correct, and it needs no new command. P4 instead proposes moving it server-side plus an `EvTexWriteback` event to update the client shadow. That event does not appear in §7.4's event list (which has `EvBufferWriteback` but no texture equivalent), it still costs a round trip (the client shadow must be current for `glGetTexImage`), and it adds a command with no counterpart in `GLFunctionsTable`. `glClearTexImage` (GL_Texture.cpp:985-1006) has the same frontend-only shape.
|
||||
- 修法:Leave `glCopyTexSubImage*` and `glClearTexImage` frontend-side; delete the P4 item and the undefined `EvTexWriteback`. Keep the per-level `serverAuthoritative` bit only for the two cases whose shadow writes genuinely happen in the backend: generated mip levels (DirectGLES.cpp:6270-6271, 6861) and the `CopyImageSubData` destination mirror (DirectGLES.cpp:7144).
|
||||
- **[minor] Client-side vertex-array bounding can scan a stale index buffer**
|
||||
- 问题:§6.10 correctly identifies that the index scan (`TryComputeMaxIndexFromHostBytes`, VulkanRenderer.cpp:3406-3470) must run client-side. But the monolith runs `indexBuffer->SyncGpuWrites()` immediately before every such scan — DirectGLES.cpp:4413, MultiDraw.cpp:499, VulkanRenderer.cpp:3431, 4159 — precisely because the EBO may have been written by a compute shader or XFB. On the client that scan reads the client shadow, and per the pending-flag flaw the reconciliation will not fire, so the computed `maxIndex` is derived from stale bytes and the vertex array is under-copied: missing or garbage geometry, or an out-of-range read of the app's array. The same stale-shadow exposure applies to the primitive-restart rewrite (DirectGLES.cpp:4412-4414) and to the `*IndirectCount` parameter-buffer read (DirectGLES.cpp:4666-4693, 4768-4793).
|
||||
- 修法:Fold into the conservative pending-set fix: `ClientArrayBounds` and the restart/indirect-count readers must force the readback (publish + wait + drain) before touching the shadow, exactly where the monolith calls `SyncGpuWrites()`. Add a `ClientArrayAfterComputeWriteScenario` to the P2 gate.
|
||||
- **[minor] SEG_STAGE has no cursors in RingControl, and P4.5 shadow arena blocks have no retirement rule**
|
||||
- 问题:Two data-plane bookkeeping gaps. (1) §6.2's `RingControl` defines one `head`/`appliedTail`/`retiredTail` triple, but §6.1 gives SEG_STAGE its own 32-256 MiB ring and §7.2 makes 'SEG_STAGE 余量 < 1/4' a Publish trigger. Occupancy of a second ring is not computable from the first ring's cursors, and stage slots borrowed by `PendingResidentWrite` (§P6) retire on `retiredSeq`, not `appliedSeq`, so they need their own pair. (2) §6.4's 64 KiB block send-watermark covers overwriting a LIVE shadow, but says nothing about freeing one: `glDeleteBuffers` or a `glBufferData` respecify releases or reallocates the SEG_SHADOW arena block while records carrying `{segId, offset, size}` into it may still be unapplied — the server then reads another object's bytes.
|
||||
- 修法:Give SEG_STAGE its own `{head, appliedTail, retiredTail}` triple in `RingControl` (there is room in the 4 KiB page). Retire shadow-arena blocks through the same watermark as ring slots — a freed block goes on a pending list and is only returned to the arena once `appliedSeq` (or `retiredSeq` for borrowed slots) has passed the last record that referenced it — rather than being released at object destruction.
|
||||
- **[minor] A lifetimeId mismatch on create is repaired by a destructive re-create, which GL forbids for a still-referenced object**
|
||||
- 问题:§5.4 says the server keys replica objects by `(kind, name)` and that 'if a create's lifetimeId does not match the record, destroy first then create'. On the replica that object may still be legally referenced by FBO attachments, binding slots, texture views (`GetViewStorageOwner`) or XFB capture targets, all of which hold `SharedPtr`s; GL keeps such an object alive until the last reference drops. A forced destroy either leaves dangling replica references or silently detaches them, and it converts a protocol bug into a rendering bug that will be attributed to the backend. The surrounding design is sound — §5.4's 'identity plus counter, never the counter alone' is the right lesson from the packed_pixels postmortem (DirectGLES.cpp:2823-2831) — it is only the repair action that is wrong.
|
||||
- 修法:Make the mismatch `Fatal{IdentityDivergence}` (or a forced `ResyncSnapshot` under `MOBILEGL_IPC_RESPAWN`). It cannot occur if the protocol is correct, so a loud stop is strictly better than a silent destructive repair; the debug cost of an unexplained missing attachment far exceeds the cost of a crash with a named reason.
|
||||
- **[minor] RenderbufferObject::GetLifetimeId is listed as a 3-line P0 addition but has no version counter either**
|
||||
- 问题:§5.4 and §14 correctly flag that `RenderbufferObject` lacks `GetLifetimeId()` while Buffer (BufferObject.h:208), Framebuffer (:158), Program (ProgramObject.h:1620), VAO (VertexArrayObject.h:120), Sampler (SamplerObject.h:141) and Texture (TextureObject.h:83,161) have one. But the §5.3 trigger table also has no row for renderbuffer state at all: `BackendRenderbufferObject::SyncToBackend` (Managers.cpp:~8620-8700) caches `{internalFormat, width, height, samples}` and there is no accessor in the plan's walk that would tell the client a `glRenderbufferStorageMultisample` happened. It is reachable only transitively through `FboAttach`, which is gated on `GetAllFramebufferAttachmentVersions()` — a re-storage of an already-attached renderbuffer need not bump that.
|
||||
- 修法:Add both `GetLifetimeId()` and a `GetVersion()` to `RenderbufferObject` in P0 (same shape as `SamplerObject::GetVersion`), add a `RecRenderbufferStorage` row to §5.3, and add renderbuffer re-storage to the §5.1 reconcile walk step 6 (per-attachment). Regenerate `BackendStateSurface.inc` after adding the accessor so the §5.9 gate covers it.
|
||||
- **[major] Program reflection payload cannot be decoded without linking glslang — the plan's own enforcement gate is unreachable and the fix is unbudgeted**
|
||||
- 问题:§4.5.5 defines MGPProgramDesc.reflection as "Visit() 归档的 LinkArtifacts + SpirvArtifacts(全结构体)", and §5.7/§11-P7 make `nm -D libMobileGLServer.so | grep glslang` empty the "整个论点的强制执行点". But all five payload types are declared INSIDE ProgramObject.h: TypeFacts at MG_State/GLState/ProgramState/ProgramObject.h:44, ResourceReflection :76, XfbVarying :1146, LinkArtifacts :1210, SpirvArtifacts :1409. ProgramObject.h:11 includes ShaderObject.h (which exposes `SharedPtr<glslang::TShader>` at ShaderObject.h:146 and at :12 includes ShaderCompileTask.h, which itself pulls MG_Util/Async/JobNode.h, MG_Util/ShaderTranspiler/CompileEnv.h and MG_State/GLState/BufferState/BufferState.h), and ProgramObject.h:14 includes MG_Util/ShaderTranspiler/SpvcSession.h, which at :11 includes spirv_reflect.h. The server must have the *definitions* of LinkArtifacts/SpirvArtifacts to deserialize into, so it must include the exact header the gate forbids. ProgramObject.h is 1803 lines with 10 in-tree includers. The plan never budgets this extraction in any phase, and open question 5 concedes the MG_Util/MG_State seam "没有审计过" — while P7 acceptance depends on it.
|
||||
- 修法:Insert an explicit phase (before P4a, ~5-8 days) that extracts TypeFacts/ResourceReflection/XfbVarying/LinkArtifacts/SpirvArtifacts into a standalone MG_State/GLState/ProgramState/ProgramArtifacts.h with no ShaderObject.h/SpvcSession.h dependency, update the 10 includers, and add a CI assert that ProgramArtifacts.h's transitive include closure contains no glslang, no SPIRV-Cross and no spirv_reflect header. Only then is `nm -D | grep glslang` a gate rather than a wish.
|
||||
- **[major] Per-draw named-uniform-block bytes have no MGPipe call — the "all 26 reverse pulls disappear" claim is false and SEG_STAGE is under-sized**
|
||||
- 问题:§7.2 asserts the 20 SyncPersistentMappedRange sites "作为反向调用彻底消失" because "每一处都紧挨着一次对客户端字节的 CPU 读,而那些读全部搬到了 client(§5.8)". Verified counter-example: UniformManager::ResolveUniformBufferPayload calls bufferObject->SyncPersistentMappedRange() at MG_Backend/DirectVulkan/Renderer/UniformManager.cpp:2022 and then reads `outData = bufferObject->MappedData() + rangeStart` at :2052 (with a zero-padding copy at :2053-2057) to pack the block into Magma's own UBO ring — a per-draw read whose consumer is server-side, so it cannot move to the client. §5.8's ownership table does not list it; §4.4.3 and 附A define set_shader_buffers(cls, start, count, const MGPBufferRange*, writableMask) with flags V only, no kHasBlob and no MGHostSpan. §5.7/D6's set_global_constants covers only the DEFAULT uniform block (SpirvArtifacts::globalUboScratch), not named blocks. So every Iris/MC draw with a named UBO has an uncarried data dependency, and §8.2's SEG_STAGE sizing list (client vertex arrays, client index arrays, multi-draw args, resolved indirect blocks) omits it.
|
||||
- 修法:Either (a) add kHasBlob/MGHostSpan to set_shader_buffers for cls==Uniform and price the per-draw byte volume with the P0 counters before freezing the payload, or (b) land a separate dev PR making Magma descriptor-bind the resident VkBuffer range instead of ring-packing it, with its own perf gate on the Iris traces. Then re-audit all 26 sites individually (they are 20+6 and enumerable) and publish the per-site disposition rather than a blanket claim.
|
||||
- **[major] Phase days contradict the plan's own per-subsystem tables; P3a's re-baseline checkpoint fires by construction**
|
||||
- 问题:§11-P3a is "slot 基建、buffer、VAO(12 天)" and its deliverable list is exactly §6.4 rows 0b (handle infra, 5-7 d), 2 (buffer + 7 BufferBackendOps, 10-13 d) and 3 (VAO/vertex elements, 7-9 d) = 22-29 days. The phase then declares "⚠ 再基线检查点 1:若 P3a 超期 >50%(>18 天)… 必须重定基线" — i.e. the plan's own subsystem table already predicts the checkpoint trips. Same shape at P4a: 16 days for §6.4 row 4 (7-9) plus the identity halves of rows 5 (20-26) and 6 (14-18). P7 is stated 48-85 against §6.5's own total of 85-111, and B-R14 admits "P7 的 48 天下界明显低于同口径的 85-111" yet the headline 199-236/200-260 still uses 48. Espryt subsystem 7 (XFB, 5-7 d) has no phase home at all — it appears only in P9's split acceptance list. Summing §6.4 (89-120) + §6.5 (85-111) + shared infra + the 51 days of IPC phases (P5 12 + P6 5 + P9 10 + P10 6 + P11 8 + P12 10) gives ~245-310 excluding CTS, versus the advertised 200-260 including IPC.
|
||||
- 修法:Rebuild §11's day column by summing §6.4/§6.5 rows per phase rather than assigning budgets independently; publish the arithmetic. Set P3a's checkpoint at the subsystem-derived number (e.g. >36 days) and give Espryt XFB an explicit phase. Restate the headline as ~245-310 person-days excluding CTS turnaround, or split P3a into P3a-i (handle infra) / P3a-ii (buffer) / P3a-iii (VAO) so each has a checkpoint that can actually fire early.
|
||||
- **[major] The verify harness — the plan's decisive replacement for the byte gate — is structurally blind in the subsystem the plan calls most dangerous**
|
||||
- 问题:§10.3-② and §6.2.1 stage B make MOBILEGL_PIPE_VERIFY (tracker fills a second PipeInputs via SnapshotFromGLContext, G4 compares field-wise per draw) the mechanism that "在语义上严格强于任何符号 diff" and the answer to every prior review. But §7.3 inverts texture dirty ownership: the client keeps the MipmapStorage rect model, maintains a per-(texture, uploadTarget, level) emission cursor, and "在发射后清自己的标志". Once the client has cleared the flags, a from-scratch snapshot recompute cannot reconstruct the dirty rect set, so the comparator has no independent second opinion for resource_subdata payloads — precisely subsystem 5, which §6.4 and B-R5 both single out as "全表最危险" because of the measured +6 ms/frame box-vs-rects cliff (Managers.cpp:4311-4319) and the 7 fallback-repack paths whose eligibility test requires uploadData == mipData. The same blindness applies to any group where the push path consumes-and-clears rather than reads.
|
||||
- 修法:Add a verify-only retention mode: under MOBILEGL_PIPE_VERIFY the tracker keeps the pre-clear dirty set for the draw and G4 compares emitted (box, rectCount, rects[]) against a snapshot recompute. Additionally record the pull-mode upload shape per texture per frame into a golden and compare it in a TextureUploadShapeScenario, so the +6 ms cliff is gated by shape equality, not only by SSIM.
|
||||
- **[major] After stage C the MOBILEGL_PIPE_PUSH knob is no longer an A/B against the old backend, and the plan claims otherwise**
|
||||
- 问题:§6.7 states "任何一次提交都能在同一份二进制上按子系统 A/B" and "设备回归可以二分到'哪个子系统'", and §12-B-R1/B-R3 lean on this as the migration-risk mitigation. But stage C (§6.2.1) changes the PipeInputs field TYPE from SharedPtr<FrontendObject> to MGPipeHandle + POD descriptor, rekeys the backend memos to {slot,gen}, and (P3a) replaces the six StateBackendObjectRegistry hash tables (Managers.h:270-390, instances at :806/:1123/:1216/:1731/:1830/:1858) with slot arrays while deleting TwinLookupMemo x3 and OwnerEquals. With the bit cleared, SnapshotFromGLContext must still synthesise the handle from the client slot map and the backend still executes the rekeyed memo code — so both arms run the same new code. A rekeying bug (exactly the D1/D2/D3/D11/D13 hazard class the plan is trying to close) is present in both arms and cannot be bisected by the knob. The plan never states this narrowing.
|
||||
- 修法:State in §6.7 that the bitmask A/B is scoped to stage-B value fields. For P3a and P4a add a second, compile-time switch (e.g. MOBILEGL_PIPE_LEGACY_MEMOS) that keeps the registry/TwinLookupMemo implementations alive behind the same PipeInputs surface, so the first two handle waves retain a true old-vs-new arm on device; retire it at P13 with the pull path.
|
||||
- **[major] P2's day-24 GO/NO-GO measures the one face where the pull model is already nearly free, so a green result does not de-risk the central claim**
|
||||
- 问题:§0.6 and §11-P2 make day 24 the GO/NO-GO for "可达性遍历是搬走了而不是翻倍", on monolith-push per-thread CPU after only render state, pack state, patch state and attrib defaults have moved. But Espryt's render-state pull already early-outs on a single Uint16 compare before ever touching the block: DirectGLES.cpp:2007 reads GetRenderStateParametersVersion(), :2016-2018 returns when it matches g_syncedRenderStateVersion, and only then is GetRenderStateParameters() read at :2021 and the three-span memcmp run at :2042-2047. The tracker replaces that with an xxHash over the same ~1.2 KB plus a 64-entry CSO LRU probe — roughly neutral for Espryt, a clear win for Magma (~55 reads), and in neither case representative. The costs the claim actually rests on are the ones P2 does not move and that become NEW client work at P3a/P4a: the touched-unit sampler walk over Array<TextureUnit,192> (TextureState.h:41,128), the 84-per-target buffer binding-point walk, the 32-attribute VAO walk, and the per-texture content/params version reads. §3's own table concedes "这是主张,不是测量".
|
||||
- 修法:Move one object-valued group into the GO/NO-GO — set_sampler_views over the GetMaxTouchedUnit prefix is the cheapest honest candidate — and measure that. Otherwise relabel day 24 as "mechanism proven, zero product risk" and place the real GO/NO-GO at the P3a exit, where the first Track-H walk exists; adjust B-R1's "退回the earlier (since-dropped) design 只损失 16 天" accordingly (it becomes ~36 days).
|
||||
- **[major] "Zero new bookkeeping in MG_State" and "one 64-bit dirty word test" cannot both hold for object-valued groups; the mutator-enumeration obligation plan A had is not deleted, only renamed**
|
||||
- 问题:§5.2 promises the dirty bits come entirely from existing counters with "MG_State 零新增记账"; §5.1 and §10.2 price steady state at "一次 64 位 dirty word 测试 + N 次 set_*". For NEW_SAMPLER_VIEWS the listed sources are per-object and per-slot — ITextureObject::GetContentVersion/GetShapeVersion/GetTextureParamsVersion plus GetTextureBindGeneration()/GetSamplingResolutionGeneration() — and there is no aggregate covering "did any bound texture's content move". That is exactly why Magma resorts to the lossy sampledContentSum/sampledParamsSum (VulkanRenderer.h:975-1000). So the tracker must either walk the touched units at every validate (not O(1), and it is new client work the backend's ResolvedTextureBindingMemo currently skips), or add aggregate generations to TextureState (new bookkeeping), or set dirty bits from every MG_Impl mutator entry point — MobileGL implements desktop GL 4.6 and MG_Impl/GLImpl alone references 181 distinct gl* names. §0.4-4 claims plan A's "第七个面" and gen_impl_mutation_surface.py vanish because there is no replica to replay into; but plan A enumerated MG_Impl mutations to REPLAY them and plan B must enumerate them to MARK them dirty. The generator is deleted; the enumeration is not, and no phase budgets it. B-R6 names the risk but its three mitigations (written-once bitmap, poison, verify) all detect omissions, none enumerate the surface.
|
||||
- 修法:Decide per group and write it down: for value groups use the existing counter; for object groups either add an explicit aggregate generation to TextureState/BufferState/VertexArrayState (and price it as MG_State work), or keep gen_impl_mutation_surface.py in a repurposed form that enumerates the MG_Impl mutators which must set each MGPIPE_NEW_* bit and fails CI on an unmapped mutator. Then correct §10.2's steady-state cost row to show the per-group walk that survives.
|
||||
- **[minor] P1's byte-identity acceptance is contradicted by P1's own deliverables**
|
||||
- 问题:§11-P1 acceptance: "pull 构建里 nm --defined-only + 剥调试信息 .text size 与替换前完全一致——本阶段可证明是一次替换(这是最后一次这条等式成立)". But P1's deliverables include the §2.4 conversion list, of which the ~22 real null guards generate code: 7 `if (MG_State::pGLContext)` (e.g. Managers.cpp:3608, verified: the guard wraps three assignments in BackendTextureObject::StampViewSyncKeys), 14 `!= nullptr` and 1 `== nullptr`. Deleting or unconditionalising those changes .text in RelWithDebInfo. Only the 34 MOBILEGL_ASSERT sites are genuinely free — Defines.h:114 defines the macro as empty outside debug builds (verified). P1 also installs SnapshotFromGLContext() at the top of PrepareForDraw (DirectGLES.cpp:2916) and SetupDraw (VulkanRenderer.cpp:6371) with no stated #if guard, which adds a call in the pull build.
|
||||
- 修法:Guard SnapshotFromGLContext and the G4/G5 machinery behind MOBILEGL_PIPE_PUSH/_VERIFY/debug, defer the null-guard and ternary rewrites to P2 (where the fields are genuinely always-valid), and restate P1's acceptance as "nm --defined-only unchanged; .text within N bytes with the delta attributable line-by-line" rather than exact equality.
|
||||
- **[minor] P1 snapshots only at the two draw-prepare sites, but a large share of the pull reads are in non-draw verbs — the poison mask will Fatal on the first glGenerateMipmap/glReadPixels**
|
||||
- 问题:§11-P1 places SnapshotFromGLContext() at PrepareForDraw and SetupDraw only, while arming G5's poison mask so that reading an unfilled field is Fatal{UnmigratedPipeInput} "发生在第一个 draw 上", and then requires "全部 40 个 trace 与 367 个集成测试在 MOBILEGL_PIPE_VERIFY=1 下零分歧". Verified non-draw reads that would be unfilled: DirectGLES.cpp:6051-6052 (GetActiveTextureUnit + GetTextureUnitObject inside the GenerateMipmap path), :6129 and :7614 (GetPixelStoreParameters(false) in readback paths), :6643-6644, :6738-6739, :6876-6877 (texture verbs resolving the active unit), :6319 (RecordError). §5.1 does declare ValidateForClear/ValidateForBlitOrCopy/ValidateForDispatch, but P1's deliverable list does not enumerate them or the texture/readback verbs.
|
||||
- 修法:Make the per-verb snapshot points an explicit P1 deliverable derived from PipeCalls.def: generate, per kCtxVerb/kCtxObject call, the set of PipeInputs fields it may read, and emit the snapshot/validate call at each of the ~89 MG_Impl boundary sites accordingly. This also converts G5 from "catches an omission at some draw" into "catches it at the specific verb that needed it".
|
||||
- **[minor] §4.5.7 and §5.8 disagree on where primitive-restart rewrite and indirect-count resolve live; either answer moves the A/B baseline a second time**
|
||||
- 问题:§4.5.7's MGHostSpan consumer table says for restart rewrite / multi-draw flattening: "monolith 填法: ptr 指向 shadow" (server does it) / "split 填法: 暂存,或 client 已重写". §5.8's ownership table says client, gated on !kCapPrimitiveRestart. Both backends actually perform the rewrite — DirectGLES.cpp:4283 RewriteRestartIndices, :4377 ScopedRestartIndexSubstitution, whole-EBO bounded by kMaxRestartRewriteBytes = 1<<26 at :4218; VulkanRenderer.cpp:3990/:4089/:4161 — so the cap is false on both and the client always does it, i.e. a monolith behaviour change scheduled at P8 (day ~97-111), long after §10.3-③'s name-for-name integration baseline was taken at P2. If instead it is split-only, monolith and split run different implementations of a whole-buffer correctness-critical transform and the name-for-name gate compares two different programs. Open question 12 flags the diagnostic-thread change but not the baseline problem.
|
||||
- 修法:Choose client-side unconditionally, land it as an independent dev PR before P2 together with the decline-diagnostic relocation (resolving open question 12), so the monolith baseline moves exactly once and before any comparison is taken. Delete the conflicting row from §4.5.7's table.
|
||||
- **[minor] set_sampler_views/bind_sampler_states import a per-stage slot space that MobileGL's state model does not have**
|
||||
- 问题:§4.4.3 defines set_sampler_views(stage, start, count, const MGPBoundView*) and bind_sampler_states(stage, start, count, const MGPipeHandle*). Verified model: TextureState::m_textureUnits is Array<TextureUnit, MAX_TEXTURE_IMAGE_UNITS> with MAX_TEXTURE_IMAGE_UNITS = 192 (TextureState.h:41, :128) — one COMBINED unit space, with the per-stage limit only an advertised number (:42). TextureUnit holds Array<BindingSlot<ITextureObject>, TextureTargetCount> plus a single sampler (TextureUnit.h:20, :24-25). The same combined unit can be sampled by two stages, and both backends bind by combined unit (g_boundTexturesCache[192][TargetCount]). A stage parameter forces the client either to duplicate views under each stage or to invent a stage attribution GL does not define, and it adds a dimension the server must collapse again.
|
||||
- 修法:Drop the stage parameter from both calls and address the combined unit space directly — which is also what LinkArtifacts::uniformSamplerOrImageUnitIndex already yields for the client-side resolution described in §5.5. Keep stage only where the target API genuinely needs it (Magma's descriptor stage flags), derived server-side from the reflection archive.
|
||||
- **[minor] The monolith benefit is argued on ~550 deleted lines with no accounting of the code added**
|
||||
- 问题:§2.5, §3's comparison table and §10.4-1 lead the monolith case with "~550 行 per-draw 失效发现机制删除". Nowhere does the plan estimate the permanent additions: PipeCalls.def plus six generators (G1-G6), MG_Impl/Pipe/{Tracker, SlotAllocator, CsoCache, HostResolve, CompositeResolver}, MG_Pipe/{MGPipeTypes, MGPipeHandles, MGPipeCallbacks, MGPipeHostSpan}, MG_Backend/MGPipe/{PipeInputs, two impl files}, plus MG_Remote's emitter and PipeApplier/PipeObjectTables. For a ~72-call interface with ~14 POD payloads across two backends that is plainly an order of magnitude more than 550 lines, all permanently maintained, and it is added to a codebase where MG_Backend is already 68k lines and MG_Impl 37k.
|
||||
- 修法:Publish a net-LOC estimate and, more importantly, a net per-draw instruction/cache-line estimate next to the deletion list, and make §10.3-④'s per-thread CPU number — not the deletion count — the stated monolith case. This also gives B-R2 a falsifiable prediction rather than a qualitative claim.
|
||||
- **[minor] A block of SamplerObject.h citations point at lines that do not exist in the file**
|
||||
- 问题:The document header asserts "全部 file:line 引用针对工作树 dev@81b17c0b". MG_State/GLState/SamplerState/SamplerObject.h is 160 lines at 81b17c0b (identical at HEAD): BorderColorForm is at :66-70 and struct SamplerParameters at :72-96. But §4.5.4 cites ":468-492" for SamplerParameters, ":462-466" for BorderColorForm and ":455-461" for its rationale; §5.2 cites ":532, 551" for GetVersion/m_version; §4.2.1 cites ":533-537" for GetLifetimeId. All are past end-of-file. The substance is correct and is in the file (borderColorForm is mandatory because all three representations are always populated, :60-66; BumpVersion also bumps the context-wide sampling-resolution generation, :152-158), so this is an inherited transcription error rather than an invented fact — but the plan is meant to be an implementation spec, and every other citation I sampled was exact (293 arrow / 58 non-arrow pGLContext, 89 gBackendFunctionsTable.GL. sites, 40 pActiveBackendObject-> sites, 354/709 MG_State:: mentions, 50 include lines over 18 headers, DirectGLES.cpp:2035 static_assert, :2042-2047 three-span memcmp, RenderState.h:363/:369/:522/:529 all verified).
|
||||
- 修法:Re-verify the SamplerObject.h block and anything else inherited from the same reader report before P0 freezes MGPipeTypes.h, and add a cheap CI lint that every file:line in docs/Disaggregated/*.md resolves to a line that exists at the referenced baseline.
|
||||
- **[minor] The day-64 "first inproc IPC frame" milestone is unfalsifiable as specified**
|
||||
- 问题:§11-P5 delivers InProcessTransport and claims the milestone "★ 第 64 天 — 首个 IPC 帧(inproc)", honestly flagged as a reduced path. But nothing in §11-P5 or §8.1 says whether inproc goes through the same G3-generated encode/decode as spawn or short-circuits it. If it passes PipeInputs by pointer inside one address space, the subsystems not yet handle-ified at P5 (Espryt XFB, which has no phase at all; readback beyond the single blocking read_pixels) keep working via SharedPtr and the milestone proves nothing about wire completeness — while P6 (spawn, day 69) would then discover the gap five days later, on the critical path.
|
||||
- 修法:Specify that InProcessTransport uses the identical G3 serialization and differs only in the doorbell/copy mechanism, and add a debug assertion in PipeApplier that no SharedPtr or raw frontend pointer crosses the applier boundary in any transport. Then day 64 and day 69 differ only by process boundary, which is what the milestone is meant to assert.
|
||||
|
||||
已验证的优点:
|
||||
- The replica-GLContext decision is correct and the evidence for it is stronger than the plan states. I confirmed the backend memos written into frontend objects are only 4 sites and DirectVulkan-only (ProgramFactory.cpp:3448; VertexInputStateFactory.cpp:60, 78, 83), and that VertexInputStateFactory.cpp:78 really does store a raw pointer into the backend's own heap. Under the replica model these are free; under any delta-apply rewrite they are a redesign. DirectGLES has zero such sites.
|
||||
- RenderStateBlob as a single whole-struct delta is well chosen. I verified RenderStateParameters (RenderState.h:222-370) genuinely carries PatchVertices (:242), PatchDefaultOuter/InnerLevel (:248-249), ClampReadColor (:313), ProvokingVertexModeSetting (:300), PrimitiveRestartIndex (:322), PolygonMode front/back, the 16-viewport arrays and ScissorBoxWrittenMask. So one blob really does subsume the ~40 individual fixed-function accessors plus the patch-parameter reads at DirectGLES.cpp:2807-2814 and Managers.cpp:7120-7132.
|
||||
- Deleting GetInteger64i_v and GetProgramiv from the wire is right. I confirmed no MG_Impl call site reaches those table entries: glGetInteger64i_v answers locally and delegates leftovers to the 32-bit form (GL_Getter.cpp:1240, :1302-1307) and glGetProgramiv routes to GetProgramiv_State (GL_Program.cpp:2478-2479), which answers from ProgramObject.
|
||||
- tableSlotMask is a necessary addition the prior branch lacked. `BeginOcclusionQuery != nullptr` really is used as a capability probe at GL_Query.cpp:471, 545 and 768 (COUNTER_BITS answers 32/1/0 off it), so a remote client must reproduce which slots the far side actually registered.
|
||||
- The plan's read of GetQueryResult64's contract is accurate and load-bearing: GL_Query.cpp:292-311 reads 0, does NOT cache, and keeps the backend handle when the backend cannot produce a result yet. That is genuinely deferred-reply-friendly and makes watermark-predicted answers conformant.
|
||||
- §5.7's identification of the composite-pipeline hazard is correct and non-obvious. GLContext::GetProgramForDraw (Core.cpp:612-660) joins every stage, computes a signature, and on a cache miss does `MakeShared<ProgramObject>(0u)` and links an anonymous composite; RefreshCompositeUniforms/MirrorUniformValues then mutate it per draw. A publish-mode server with no sources genuinely cannot do this, so client-side resolution plus SetReplicaResolvedDrawProgram is the right fix.
|
||||
- Declining AcquirePersistentMap really is tolerated by the frontend at all three request sites — TryAdoptLargeStorage (BufferObject.cpp:173-176), EnsureGpuResidentStorage (:436-443) and AcquireMemoryRange (:470-473) all handle a null return — so §6.8's tier T2 is a safe default from the frontend's point of view (the failure is elsewhere, see the persistent-map finding).
|
||||
- MOBILEGL_COHERENT_AS_FLUSH defaults to false (Config.h:174, ConfigLoader.cpp:185), so §6.8's prohibition costs nothing on the default configuration and cannot regress the Create/Flywheel fixtures by itself.
|
||||
- The frontend never reads its own texture dirty state — zero IsStorageDirty/GetDirtyRects/GetDirtyRegion call sites in MG_Impl — which is what makes the clear-on-emit fix to §5.6 safe. The plan reached the wrong conclusion from the right underlying fact.
|
||||
- The §12 note about pGLContext is precise: it is `extern UniquePtr<GLState::GLContext>&` (Core.h:564) and I counted exactly 65 non-arrow uses across MG_Impl/MG_State/MG_Backend, matching the plan's '约 65 处'. The shim requirements it lists (operator->, get(), operator bool, equality) are the right set.
|
||||
- The critique of Feat/CS-Delta-IPC is accurate on the points I spot-checked: it really did leave POSIX fd passing unimplemented, its ServerHost really does not compile, and its bfa.h really does hand FlatBuffers table pointers across a nominal C ABI. Making SCM_RIGHTS a P0 deliverable with its own test is the correct inversion.
|
||||
- Keeping glFinish/glFlush free (Definitions.cpp:111-112) and glGetError client-local (GL_Getter.cpp:2811-2817, invariant at Core.cpp:48-49) is right, and the plan is correct that turning them into round trips would be a self-inflicted regression.
|
||||
- The pull-surface accounting is exact and better than every prior design's. Verified at dev@81b17c0b: 293 `pGLContext->` occurrences and 58 lines using pGLContext without the arrow, with the plan's §2.4 breakdown reproducing precisely (34 MOBILEGL_ASSERT truth tests, 14 `!= nullptr`, 7 `if (`, 1 `== nullptr`, 1 `.get()` at DirectGLES.cpp:146, 1 comment at VertexInputStateFactory.h:133). Identifying the `.get()` capture as invisible to sed, and specifying that the purity gate greps `pGLContext` rather than `pGLContext->`, closes a real hole the three earlier candidate designs all left open.
|
||||
- The function-pointer-table-over-vtable decision is correctly argued from this codebase rather than from gallium. Verified: GLFunctionsTable + GlobalBackendFunctionsTable contain 69 function pointers (BackendObject.h:117-285), reached from 89 `gBackendFunctionsTable.GL.` sites and 40 `pActiveBackendObject->` sites in MG_Impl, installed at the single hook point MG_Backend/Init.cpp, and null entries already mean "not implemented, frontend falls back" (documented at BackendObject.h:212-215, 265-269). A null `set_*` is a native expression of "this subsystem is not migrated"; a pure-virtual class would need stub overrides that lie.
|
||||
- D-B1 (ship RenderStateParameters as one blob, not three gallium CSOs) is grounded in verified in-tree evidence rather than preference: `static_assert(std::is_trivially_copyable_v<RenderStateParameters>)` at DirectGLES.cpp:2035, the head/blend/tail memcmp at :2042-2047 keyed on offsetof(...,BlendStates)/offsetof(...,LogicOp), and the load-bearing field placement of ScissorBoxWrittenMask (RenderState.h:363) and ClipDistanceEnabledMask (:369). Carrying both m_version (:522) and m_pipelineStateVersion (:529) on the wire is likewise correct and correctly justified by the glViewport-evicts-pipeline-memo regression recorded at :523-528.
|
||||
- The texture dirty-ownership inversion rests on a fact I confirmed independently: MG_Impl contains zero `IsStorageDirty(`, `GetStorageDirtyRects(` and `GetStorageDirtyRegion(` call sites while calling `MarkStorageDirty(` 14 times. Deleting plan A's §5.6a ack protocol and risk R6 on that basis is sound, and keeping the box-vs-rects upload-shape decision server-side (MGPSubData carrying both payloads) correctly leaves the choice on the side that paid for the +6 ms/frame measurement at Managers.cpp:4311-4319.
|
||||
- D-B4 — leave AcquirePersistentMap completely untouched through the entire monolith refactor and isolate it to the IPC step behind a week-one POST spike — is the right structural call. It is already an explicit call returning a pointer (BufferObject.h), so it genuinely passes through unchanged, and refusing to let one platform unknown gate ~200 days of interface work is exactly the right sequencing judgement.
|
||||
- The two backend-internal MG_State usages that the previous review round priced at zero are correctly identified and costed. Verified: UniformManager::MakePlaceholderTextureObject at UniformManager.cpp:161-181 with the real construction at :1417-1424, :1479-1496 (including SetSamples(2) for VUID-RuntimeSpirv-samples-08726 and TruncateMipmapLevels at :1496) and :1620; and the two internal shaders at VulkanRenderer.cpp:4211 and :4287 building MakeShared<MG_State::GLState::ShaderObject> (:4214, :4222, :4290, :4300), a ProgramObject (:4230) and calling Link(false) (:4233). Preferring checked-in SPIR-V guarded by an in-tree-glslang byte-compare MG_Test over a host-tool build step is the right trade for this repo's four build lanes.
|
||||
- VertexInputStateFactory's backend-heap-pointer write-back into the frontend VAO is correctly classified D12 "delete, do not translate", and D18 (VkRenderPassManager/VkTextureManager's deliberate node-based std::unordered_map) is correctly the single UNCHANGED row with a mandate to carry its postmortem comment verbatim into the P7 review checklist. Naming the one thing a large refactor must not "optimise back" is exactly the discipline these reviews usually find missing.
|
||||
- The milestone labelling is honest where a weaker plan would have overclaimed: P5/P6 are explicitly marked 缩减路径 with emulation Fatal in split until P8; §3 concedes plan A wins first-frame time by 4-5x; D-B5 states outright that the byte-identity gate dies by construction and calls it a cost that must be written down rather than hidden; and §9.3 refuses a blanket zero-round-trip claim in favour of published per-trace-case round-trip and texture-pull counters.
|
||||
- The design surfaced two genuine in-tree defects as by-products and routed them correctly: D21, m_xfbCounterSlotByObject keyed on the raw GL name (VulkanRenderer.cpp:11136-11146), so a deleted-and-regenerated XFB object resumes a capture that should restart — scheduled as an independent dev PR in P0; and the dead CapabilityInput::FramebufferSrgb/DepthClamp with no storage (RenderState.cpp:380, :428-429) feeding six constant-false backend reads, correctly made a blocking question before the render-state blob is frozen.
|
||||
- Ordering the strangler so framebuffer precedes textures and programs (D-B3, §6.6 step 4) is right and well-evidenced: the four cross-object masks are derived from attachment formats at Managers.cpp:5616-5619 and consumed by the render-state push (DirectGLES.cpp:2014) and the program staleness test (:2769-2770), and inlining internalFormat into MGPSurface lets them be derived at push time with no lookup — which genuinely retires the fragColor re-derivation workaround at :2712-2732 rather than porting it.
|
||||
|
||||
## 3. 修订记录(综合稿 → 定稿)
|
||||
## 3. 综合稿的关键决定
|
||||
|
||||
- FATAL persistent-map: verified SyncPersistentMappedRange (BufferObject.cpp:238-250) has zero MG_Impl/MG_State callers (all 19 production call sites are in MG_Backend/). Added new section 5.10: RecBufferMap/RecBufferUnmap records + client-side 64KiB-block push from PublishImplicitState + PersistentCoherentMapScenario as a P1a gate. Also fixes the IsBufferDrawClean IsMapped() gate (Managers.cpp:1447) that the replica would otherwise get wrong.
|
||||
- FATAL MarkGpuWritten: verified all 6 callers are backend-only. New section 5.6b makes the CLIENT set the flag conservatively at every draw/dispatch emit point (mirroring DirectGLES.cpp:459-467/509/1809), records emitSeq, and forces publish+wait+drain at every read entry; EvGpuWritten demoted to a narrowing hint. Section 7.4 drain points extended with glMapBuffer*/glGetBufferSubData/glGetNamedBufferSubData/glCopyBufferSubData.
|
||||
- FATAL poll livelock: section 7.2 rewritten. glClientWaitSync/glGetSynciv(SYNC_STATUS)/glGetQueryObject*(AVAILABLE|NO_WAIT) are now Publish triggers, GL_SYNC_FLUSH_COMMANDS_BIT publishes unconditionally (cites DirectVulkan.cpp:1158-1160), plus a starvation escalation (MOBILEGL_IPC_POLL_ESCALATE). Dedicated P3 gate added.
|
||||
- FATAL fence granularity: added a subsection to section 8 requiring real per-fence server-side polling + EvFenceSignaled instead of a present-granular watermark, citing DirectVulkan.cpp:1120-1128 and the magma-mc1215-fence-oom history. Section 9.3 adds a non-present fence tick for DirectGLES.
|
||||
- Applier-replays-table gap (new boundary face (g) in section 2): verified EnsureGeneratedMipmapStorageAllocated (GL_Texture.cpp:501-541, incl. BumpContentVersion at :538 with its stale-VkImageView rationale) and AccountTransformFeedbackPrimitives (GL_Drawing.cpp:172-236, 6 counters read by DirectGLES.cpp:900 and DirectVulkan.cpp:1337/1384). Added section 5.9b: a SECOND generated inventory (gen_impl_mutation_surface.py + MutationCoverage.def) making unmapped MG_Impl mutations a #error, plus RecGenerateMipmapLevels/RecXfbAccounting records and MG_Remote/Shared/ helpers.
|
||||
- Texture dirty flags: verified MipmapStorage::MarkDirtyRegion unions forever unless MarkDirty(level,false) runs, and that MG_Impl has ZERO IsStorageDirty/GetDirtyRects/GetDirtyRegion readers. Section 5.6a now requires clear-on-emit and closes the ack question via intact-shadow resync + re-send of un-applied texture records after a hard drain.
|
||||
- inproc globals: verified pDefaultFramebufferInfo is a second process global (22 refs; client MG_Impl reads 13, server backend reads 4 + SwapchainObject writes 1) and that gBackendFunctionsTable is read by server-side MG_Impl too. Section 12 split into 12.1/12.2/12.3: two CMake options (shipping spawn build keeps all four globals plain, no TLS on the GL hot path), full shim requirement list, and an explicit P0 go/no-go on isolating vs downgrading inproc.
|
||||
- Non-arrow pGLContext count corrected from '~65' to the measured 133 (2 in MG_Impl, the bulk in MG_Backend incl. ~90 DirectVulkan asserts), with the lifecycle sites (Core.cpp:20/1487, Core.h:564, Init.cpp:63) added to the shim requirements.
|
||||
- Publish policy: deleted the 64KiB byte threshold (it was a full MC frame and pre-killed the P2.5 hypothesis). Now release-store cmdHead every record (or every 8-16), doorbell only when consumerParked.
|
||||
- Added the symmetric producer-side doorbell (new section 6.2a: producerParked + reverse byte / condvar) so present-credit, kNeedsAck and ring-full waits block instead of cross-process spinning on a phone big core.
|
||||
- Added SEG_EVENT overflow policy (section 7.4): drain inside every wait loop, lossy EvLogLine with eventDropped counter, non-lossy semantic events with an eventRingFull stop-applying flag, plus a P4 fault-injection gate for the credit-blocked deadlock.
|
||||
- RingControl gained an independent {head, appliedTail, retiredTail} triple for SEG_STAGE (section 6.2), since the 'stage below 1/4' publish trigger cannot be computed from the cmd cursors and stage slots retire on retiredSeq.
|
||||
- Added SEG_SHADOW block retirement rule (section 6.1): freed/reallocated arena blocks go on a pending list gated by appliedSeq/retiredSeq, not released at object destruction.
|
||||
- Copy accounting table (6.4) corrected: monolith is 2 (not 1), P1-4 is 4 (not 2), P4.5 is 3 (not 1); added rows for map+unmap and for the new persistent-map push. Added optional plan B (replica adopts client SEG_SHADOW read-only as a third PipeResource mode) as a P6 candidate, and required TracyPlot counters on BOTH sides of the wire.
|
||||
- Errors: section 5.6c splits the class - only allocation-class entry points (glRenderbufferStorage*, some glTexImage*/glTexStorage*/glCopyTexImage*, glBufferStorage) become kNeedsAck so the GL_OUT_OF_MEMORY probe idiom stays exact; MOBILEGL_IPC_STRICT_ERRORS demoted from CTS-required to a diagnostic switch. P4 gains an OOM-probe gate.
|
||||
- Present credit default lowered from 2 to 1 with the latency composition spelled out (client credit + server FIF + driver depth; FrameContext.cpp:288-290 shows Present itself already waits), and input-latency histogram gates added to P3 and P9.
|
||||
- Added a core-placement plan (section 10): total-CPU-work delta must be stated, mgl-srv-apply pinned to a big core reusing ShaderCompilePool.cpp:73-96 detection via MOBILEGL_IPC_SERVER_AFFINITY, and P2.5/P3 must report per-thread CPU time.
|
||||
- Added a non-present fence tick for DirectGLES (9.3) so retiredTail does not starve in glcts/readback loops, plus a present-less split case in P2.
|
||||
- lifetimeId mismatch on create changed from destructive re-create to Fatal{IdentityDivergence} (section 5.4), because the replica object may still be legally referenced by attachments/views/binding slots.
|
||||
- RenderbufferObject now gets BOTH GetLifetimeId() and GetVersion() in P0, with a RecRenderbufferStorage row in 5.3 and per-attachment version reads in the 5.1 walk (a re-storage of an already-attached RBO need not bump the FBO attachment versions).
|
||||
- glCopyTexSubImage*/glClearTexImage kept frontend-side (6.6): verified CopyReadFramebufferIntoMipmapRegion (GL_Texture.cpp:1044-1097) is already pure-frontend borrowing one ReadPixels. Dropped the P4 'move to server' item and the undefined EvTexWriteback; serverAuthoritative bit narrowed to generated mips and the CopyImageSubData mirror.
|
||||
- Client index scans / restart rewrite / IndirectCount parameter reads must go through the pending-set force-readback at exactly the sites where the monolith calls SyncGpuWrites() (6.10), with a new ClientArrayAfterComputeWriteScenario in P2.
|
||||
- Added runtime bounds discipline for ring records (6.3): the same X-macro generates size >= sizeof(T) && size <= remainingRingBytes && (size%8)==0 preconditions, Fatal{ProtocolCorruption} on violation.
|
||||
- MOBILEGL_COHERENT_AS_FLUSH ban REMOVED (5.10/6.8): with client-side persistent-map push, both rewritten and app-native coherent maps are correct, so the two Create fixtures run the same buffer path in split and monolith and the P2 name-for-name comparison is honest.
|
||||
- Android delivery chain moved into P0 as spike A (server .so packaging verified through AGP, posix_spawn from the app's own untrusted_app process rather than run-as, generic --es mobilegl_env passthrough across the five trace files). External-memory feasibility became spike B so P7's schedule is known in week 1.
|
||||
- P1 split into P1a (client + inproc applier, Linux gate) and P1b (spawn transport, Linux gate); device OpenRA retrace moved to the P2 exit criterion. Total re-estimated 74 -> 77 person-days with milestones at weeks 3/5/6.
|
||||
- Spawned server must scrub MOBILEGL_TRANSPORT/MOBILEGL_IPC_* from its envp AND force Transport=Monolith before MG_Backend::Init (11.1), with a P1b process-tree count gate - otherwise an unbounded fork chain on first GL call.
|
||||
- HeadlessGL fork pre-flight orphan-server issue addressed (11.3): immediate EOF exit, bounded readiness retry, pgrep gate in P1b; cites HeadlessGL.cpp:344-368 and its own :585-589 'leaked exclusive device' note.
|
||||
- Server discovery reworked (11.1): MOBILEGL_IPC_SERVER_PATH primary with dladdr fallback, RUNTIME_OUTPUT_DIRECTORY aligned to the MobileGL library dir, env injected into every new ctest ENVIRONMENT - verified the itest links MobileGL_s statically (CMakeLists.txt:28-35) and retrace passes an explicit -DMOBILEGL_LIBRARY.
|
||||
- mobilegl_server_main declared extern "C" with explicit default visibility plus an nm -D assertion in P0 (11.2), because CMakeLists.txt:497-510 sets hidden visibility on every non-Debug build and RelWithDebInfo is what ships.
|
||||
- FlatBuffers: add_subdirectory(3rdparty/flatbuffers) removed from the default build path entirely (7.1/13) - the prior branch's flatc block IS the NDK trap - plus a CMake guard that forces the option OFF with a warning when 3rdparty/flatbuffers/include is absent.
|
||||
- Windows handle pair spelled out (11.5): GUID-named CreateNamedPipeW + CreateFileW both with FILE_FLAG_OVERLAPPED and the server end inherited, because asio's windows::stream_handle IOCP service needs an overlapped handle and CreatePipe does not give one.
|
||||
- trace-replay SPLIT plumbing detailed (13): test name gains a SPLIT suffix (current name MobileGLTraceReplay.CASE.BACKEND would collide) and -DTRACE_TRANSPORT= must be threaded through run_trace_case.cmake; both files listed as P2 deliverables.
|
||||
- Added a steady-state memory budget requirement (R14) covering client segments + full replica context + the server's three 4->64MiB rings + the 64MiB buffer pool (~450MiB), with P1a recording RSS for BOTH processes and SEG_STAGE's ceiling set by measurement.
|
||||
- P3's 'zero round trips' gate reworded to cover the whole trace-case matrix with per-fixture round-trip counts published, rather than resting on minecraft-1.21.4-main-menu which exercises neither conditional render nor occlusion queries.
|
||||
- The nm/.text monolith preservation gate is now a phase-exit criterion for every phase P0-P9, and the P4.5 allocator change is explicitly required to be #if MOBILEGL_BUILD_DISAGGREGATED-wrapped (PipeResource/MipmapStorage live in MG_State, so an unguarded allocator swap would turn the gate red).
|
||||
- Added a schedule-risk row (R15) calibrated against Feat/CS-Delta-IPC's 6668 lines / zero frames, with P2.5 named as its annealer.
|
||||
- Section 14 REUSE/CHANGE/DROP updated: the prior branch's Protocol/CMakeLists.txt flatc block moved from REUSE to CHANGE-with-deletion, HandleSessionGeneration.md gains the RBO GetVersion and Fatal-on-mismatch edits, and gen_impl_mutation_surface.py noted as having no counterpart there.
|
||||
- Wrote 5 files (part2 split into 2a/2b): part1=§0-3, part2a=§4, part2b=§5-6, part3=§7-10, part4=§11-14+附. Single title in part1 only; §0-§14+附 headings in required order; each file ~35-49KB UTF-8 ≈ 12-16K Chinese chars, well under the cap.
|
||||
- Base = winning Design 3 (split-first) phase plan, grafted with Design 2's twin-derived interface derivation (SetupDrawSnapshot / IsDrawSyncClean / ResolvedDrawBuffers / g_syncedRenderStateParameters / BufferBackendOps as the source of the call catalogue), its PipeCalls.def six-generator toolchain, its two-kinds-of-generation split (client identity vs 12 server-only MGGen epochs), its D18-UNCHANGED node-container discipline, and its MGHostSpan; plus Design 1's caps-gated emulation-homing rule, its numbered gallium-deviation ledger, and MGPipeCallbacks as a named struct.
|
||||
- Resolved Design 1's fatal flaw: render state ships as ONE versioned blob behind a content-addressed CSO handle (create_render_state(blob) + bind_render_state 12B, client 64-entry LRU keyed on the three existing memcmp spans), never decomposed into blend/depth-stencil/rasterizer CSOs — cited RenderState.h:359-368 (field order load-bearing), DirectGLES.cpp:2035 static_assert + :2042-2047 three-span memcmp, and the :523-528 two-counter regression.
|
||||
- Resolved Design 2's fatal flaw: MGPipeHandle is {slot:Uint32, gen:Uint32} with CLIENT-ALLOCATED DENSE PER-KIND SLOTS (not a sparse 64-bit lifetimeId), which is what actually turns the 6 StateBackendObjectRegistry hash tables and 13 Magma caches into arrays; GetLifetimeId() stays client-side as the tracker's own identity; 2^32 slot-reuse wrap documented and asserted.
|
||||
- Re-measured every contested count against the working tree rather than inheriting any report: GLFunctionsTable = 67 function pointers + 1 Bool (BackendObject.h:117-278), 69 fps with GlobalBackendFunctionsTable (not 73 or 71); 293 pGLContext-> occurrences over 290 lines + 58 non-arrow lines; 50 MG_State include lines over 18 distinct headers; 95 backend->frontend mutator sites over 17 methods; 7 BufferBackendOps hooks; 89 MG_Impl table sites + 40 pActiveBackendObject->; 1494 MG_Impl pGLContext->; 367 TEST_F / 428 TEST( / 40 trace cases at SSIM 0.99; PLAN.md phases sum to exactly 77 days.
|
||||
- Closed the shared migration gap all three designs missed: the 58 non-arrow pGLContext uses (≈40 MOBILEGL_ASSERT truth tests, ~10 null guards, 3 patch-param ternaries, the DirectGLES.cpp:146 .get() raw capture that sed cannot catch, 2 != nullptr conditions, 1 comment) are enumerated by form in §2.4, made an explicit P1 deliverable, and the purity gate greps 'pGLContext' not 'pGLContext->'.
|
||||
- Hardened the residual value block (the split-first accelerant): per-member offsetof static_asserts in addition to sizeof, AND field-wise serialization in split mode instead of a bulk memcpy — because the monolith verify harness cannot see a layout mismatch when both sides are the same TU; retirement is a compile error via static_assert(sizeof(ResidualValueBlock)==0) at P13.
|
||||
- Priced the schedule honestly: 200-260 engineer-days (single track 199-236, P7/Magma 48-85), first inproc IPC frame day 64 and first cross-process frame day 69 — both explicitly labelled REDUCED PATH (emulations Fatal in split until P8, full function at day 111) — against PLAN.md's verified 77 days and day-15 cross-process frame; added TWO re-baseline checkpoints (P3a overrun >50%, P7 midpoint <40% complete) and priced CTS turnaround (~56,271 cases) as a separate tiered-gating line, not folded into phase estimates.
|
||||
- Stated D-B5 as an explicit cost in the TL;DR: PLAN.md's byte-identity monolith gate dies by construction, replaced by a five-part gate (purity grep+nm, per-draw field-wise MOBILEGL_PIPE_VERIFY shadow-compare, behavioural A/B across {monolith-pull, monolith-push, split}, per-thread-CPU non-regression, coverage+poison+handle-recycle asserts) with two surviving nm equalities kept as assertions and .text drift published as informational.
|
||||
- Kept the texture re-mint pull as a named NEW stall class with all three mitigations shipping together (imageBindableHint pre-emption, asynchronous park-and-re-emit so the stall lands on mgl-srv-apply not the app thread, bounded 32MiB retention LRU), a dedicated TextureRemintPullScenario, and a per-trace-case pull counter that is PUBLISHED rather than asserted to zero.
|
||||
- Corrected PLAN.md §7.4 with evidence: backend program link/compile failure is surfaced ONLY as MGLOG_E plus a bind-program-0 no-op (Managers.cpp:8091-8126, 8357-8372, rationale at :7098/:7247-7249/:6478/:7827), so on_log must split by severity — <=WARN lossy, >=ERROR lossless with a per-second rate limiter emitting 'N errors suppressed' — with a log-flood fault-injection gate.
|
||||
- Quarantined AcquirePersistentMap from the refactor entirely (it is already an explicit pointer-returning call and survives P0-P13 untouched; only IPC breaks it), deferring it to PLAN.md §6.8's three POST-probed tiers with spike B in week one, so no platform unknown blocks 200 days of interface work.
|
||||
- Inherited PLAN.md §6-§13 essentially verbatim with a per-section table in §8.1 (no re-derivation), and listed every delete/change/add against it in §8.2 and §14.1 — including that the copy account drops to 3/2 (PLAN.md's own 'the plan' target) and inproc isolation drops from four process globals to two, which makes PLAN.md's earliest falsification gate cheap.
|
||||
|
||||
## 4. 被驳回的审查意见
|
||||
## 4. 修订记录(综合稿 v1 → 定稿 v2)
|
||||
|
||||
- 'inproc is a category error because the server must not hold MG_State' - not a flaw in this plan: the replica model deliberately links MG_State into the server, and the verified evidence (UniformManager.cpp:1418-1497 constructing real TextureObjects, VulkanRenderer.cpp:4211-4356 driving ShaderObject::Compile/ProgramObject::Link) shows a thin server is impossible regardless.
|
||||
- 'The 167 handle-ify hits mean a huge conversion surface' - already handled: the plan's own section 14 notes those counts include GLFunctionsTable declarations at BackendObject.h:158-186 and the static global at DirectGLES.cpp:55, and the replica model means no SharedPtr-keyed twin registry needs converting at all.
|
||||
- 'MarkStorageDirty(...,true) at Managers.cpp:2813 (RequireImageBindableStorage re-dirty) needs a client-visible ack protocol' - it is purely server-initiated by a server-side re-mint, is unpredictable by the client by construction, and the re-upload happens entirely on the replica; no wire traffic is needed (documented as such in the section 5.6 table).
|
||||
- 'BeginConditionalRender should become a client-side speculative pass-through' - the spec latitude is real but GL_Query.cpp:705-706 documents the always-wait choice as the only one giving the whole block one deterministic verdict; changing it is an independent monolith behaviour change, not a split concern. Kept as a listed blocking point instead.
|
||||
- [stage-A fill sites] Verified only ~22 of the 70 table entries MG_Impl uses are draw/dispatch; confirmed non-draw entries read pGLContext themselves (DirectGLES.cpp:6051-6052 GenerateMipmap path, :6129 pack state, :4106/:4165 Clear, :5988-5989 Blit, :1501-1502 comment). Replaced the 2-site SnapshotFromGLContext with G5-generated per-verb-class fill/validate points at the ~93 MG_Impl boundary sites; Tracker grows from 4 to 8 validate entries (§5.1, §6.2.1, P1).
|
||||
- [poison granularity] Upgraded G5's written-once bitmask to a per-verb generation (m_filledGen[f] == m_currentVerbSerial, sticky fields listed explicitly), so a field filled by draw N no longer satisfies the read in the following glTexSubImage; poison now fires on the verb that needed it (§6.2.2).
|
||||
- [texture push timing] Verified glTexSubImage* never calls the backend table (GL_Texture.cpp has 3 MarkStorageDirtyRegion sites only) and that Espryt coalesces at sync time with the union-box collapse at Managers.cpp:4386-4390 (+6 ms/frame). Rewrote 推论 1 and added §5.1.1: the GL-call-time push rule applies only to the seven BufferBackendOps hooks; texture subdata accumulates in the client's rect model and is emitted as one resource_subdata at the next validate/flush point, with a per-frame emit counter and an MC animated-atlas ceiling.
|
||||
- [sub-rect upload] Verified the `uploadData == mipData` gate (Managers.cpp:4278-4283) and whole-level stride arithmetic (:4288-4293, :4321-4326), and that the unpack-ring path already uses a strided source descriptor (UnpackStagingBlock, :4340-4390, tightly repacked). Redefined MGPSubData to carry MGPSubRegion{dstBox, srcRowStride, srcSliceStride, srcOffset} plus sourceIsVerbatimLevelShadow, reworked Managers.cpp:4274-4326 to read strides from the descriptor, moved this out of 原地不动 and priced it into Espryt subsystem 5 (+3-4 days).
|
||||
- [XFB scatter] Verified ScatterCapturedRecords does a read-modify-write of the client shadow (DirectGLES.cpp:928, rationale :889-892, case KHR-GL46.transform_feedback.capture_special_interleaved_test). Moved the scatter to the client: server pushes packed scratch bytes via on_buffer_writeback + new on_xfb_scatter_ready{packedStride, vertices}; client patches and re-emits an ordinary resource_subdata. No new reverse read is introduced (§7.2.1).
|
||||
- [unit-bindings debouncer] Confirmed GetTextureBindGeneration bumps on redundant re-binds (DirectGLES.cpp:1414-1420). Reclassified the ~115 lines from 'deleted' to 'relocated': the debounce becomes a client-side resolved-set xxHash emit suppressor (m_lastSetHash[]) covering every kVarTail set_*, and D9's viewSetSerial now has that as an explicit precondition. §2.5 split into ~372 lines truly deleted vs ~175 relocated; §3, §10.2 and §10.4 ledgers corrected.
|
||||
- [multi-draw / restart ownership] Verified ResolveTierForBatch (MultiDraw.cpp:282-320) selects per batch using programReadsDrawID (a server-only ESSL fact) and that both backends perform the restart rewrite. Deleted kCapPrimitiveRestart/kCapPrimitiveRestartFixedIndex/kCapMultiDraw/kCapMultiDrawIndirect/kCapMultiDrawIndirectCount as ownership switches (D-B7); all five tiers and the restart rewrite stay server-side, fed in split mode by a new incrementally-maintained Server/IndexHostMirror gated on kCapNeedsHostIndexBytes (budgeted, counted, with a per-draw shipping fallback). Resolves the §4.5.7-vs-§5.8 contradiction and closes open question 12.
|
||||
- [texture pull terminator] Added resource_subdata_complete(res, target, firstLevel, levelCount, pullSerial) which may carry zero regions; server proceeds with allocated-and-empty storage (matching monolith EnsureGenerateMipmapStorageAllocated at DirectGLES.cpp:6270-6271) plus a logged diagnostic. TextureRemintPullScenario must include the unanswerable case (render-only texture later image-bound) and be red before the terminator lands (§7.5e, P9).
|
||||
- [verify survives P13] SnapshotFromGLContext and its MG_State includes are now kept behind #if MOBILEGL_PIPE_VERIFY past P13; the three purity gates run only on the non-verify build; P13 additionally delivers the MGPipe recorder golden mode as a long-term MG_State-free semantic gate and as the answer to open question 11 (D-B5, B-R17).
|
||||
- [texture params] Verified SyncTextureParamsToBackend runs for FBO attachment textures (DirectGLES.cpp:1580-1601) and that RequireImageBindableStorage sets m_forceTextureParamsResync (Managers.cpp:2815-2821). Added set_texture_params(res, ...) carrying base/max level, swizzle, depth-stencil mode, LOD clamps and forceResync; MGPSamplerView reduced to view restriction only (new gallium deviation D10, plus a gate for attachment-only / image-only / CopyImage-endpoint textures).
|
||||
- [emission cursor aliasing] Verified TextureObjectView forwards IsStorageDirty/MapMipmapData/MarkStorageDirty(Region)/GetStorageDirtyRegion to the storage owner with index remapping (TextureObjectView.cpp:281, 290-322). Keyed the client emission cursor on (storageOwnerHandle, ownerUploadTarget, ownerLevel) and added a view/owner aliasing scenario.
|
||||
- [OOM ack] Verified the texture family never reaches the backend table and that even glRenderbufferStorage allocates lazily in SyncToBackend (Managers.cpp:8674-8684). Narrowed kNeedsAck to glBufferStorage plus, conditionally, glRenderbufferStorage*; P0 must answer whether the corpus actually contains a glRenderbufferStorage OOM probe. Stated plainly that texture allocation OOM is already deferred in the monolith so the split changes nothing observable (§7.4, §9.2-7).
|
||||
- [SEG_STAGE sizing] Rewrote the new-byte-class list to six items including named-UBO host payloads and tightly repacked texture regions; removed the 64 MiB restart rewrite and the multi-draw flattened stream from SEG_STAGE entirely (they are served by the index host mirror), and required G3 to define a chunking/degradation path for a single record larger than the segment (§8.2, open question 9).
|
||||
- [validate order] Replaced the numbered order contract with the invariant 'all set_* for a command complete before the verb; the server specializes at the verb'. D-B3 restated: what retires the fragColor workaround and ImageUnitFormatsStillMatch is late specialization, not framebuffer-first ordering (§5.3, D-B3).
|
||||
- [reflection payload / glslang gate] Verified TypeFacts/ResourceReflection/XfbVarying/LinkArtifacts/SpirvArtifacts all live in ProgramObject.h, which includes ShaderObject.h (glslang) and SpvcSession.h (spirv_reflect), with 7 in-tree includers. Added a new prerequisite phase P0.5 that extracts them into ProgramArtifacts.h with a CI include-closure assertion, without which P7's `nm -D | grep glslang` criterion is unreachable (§0.4, §4.5.5, P0.5).
|
||||
- [named UBO bytes] Verified UniformManager::ResolveUniformBufferPayload syncs at UniformManager.cpp:2022 and reads MappedData()+rangeStart at :2052 into Magma's own UBO ring - a server-side consumer that cannot move. Added an optional MGHostSpan payload to set_shader_buffers(cls==Uniform) gated by a new kCapNeedsHostUboBytes, plus a stage-ubo-named counter, and forbade freezing the payload shape before P0 gives byte volumes (D-B8, §5.7, §7.2).
|
||||
- [phase arithmetic] Rebuilt every phase day count as the sum of the §6.4/§6.5 rows it contains and published the arithmetic; total changed from 200-260 to 267-337 person-days excluding CTS turnaround; milestones moved to days 25 / 43 / 99 / 104 / 145 / 187 / 267; re-baseline checkpoints set at the summed upper bound +50% (P3a >27d, P4a >39d); Espryt XFB given an explicit phase home in P3b/P4b (§11.5, B-R14).
|
||||
- [verify blind spot] Added a verify-only retention mode: under MOBILEGL_PIPE_VERIFY the tracker keeps the pre-clear dirty set and G4 compares the emitted (unionBox, regionCount, regions[]) against a snapshot recompute; added TextureUploadShapeScenario recording upload shape and job count as a golden, because SSIM is insensitive to the +6 ms/frame box-vs-rect cliff (§7.3, §10.3-②, P3b/P4b).
|
||||
- [stage-C A/B narrowing] Stated in §6.7 that MOBILEGL_PIPE_PUSH stops being an old-vs-new arm after stage C (both arms run the rekeyed memo code), and added a compile-time MOBILEGL_PIPE_LEGACY_MEMOS switch keeping the registry/TwinLookupMemo implementations alive through P3a/P4a, retired with the pull path at P13 (+1 day per phase, costed; new risk B-R16).
|
||||
- [GO/NO-GO scope] Extended P2 to include one Track H slice per backend (Espryt 0b handle infrastructure, Magma subsystem 4) plus a Blaze3D blend-toggle microbenchmark and a CSO-content-addressing negative control, so day 43 measures the decision it gates; fallback cost restated honestly as 28-39 days rather than 16 (§0.6, P2, B-R1).
|
||||
- [dirty marking vs polling] Verified no aggregate exists for 'did any bound texture's content move' (which is why Magma uses lossy sampledContentSum/sampledParamsSum). Added 推论 4: value groups keep the polling model with zero new bookkeeping; object groups get 5 new aggregate generations in MG_State (~20 lines at existing bump points), and gen_impl_mutation_surface.py is repurposed as gen_pipe_dirty_surface.py enumerating MG_Impl mutators to aggregate generations with a CI failure on any unmapped mutator (§0.3, §5.2, §10.3-⑤, B-R6 layer 4).
|
||||
- [P1 byte identity] Verified MOBILEGL_ASSERT compiles away outside debug (Defines.h:114) but that the 7 null guards, 14 != nullptr conditions and 3 ternaries do generate code. Deferred those rewrites to P2, guarded SnapshotFromGLContext/G4/G5 behind build switches, and restated P1's acceptance as 'nm unchanged; .text delta attributable line by line' (P1).
|
||||
- [restart/indirect ownership conflict] Resolved the §4.5.7-vs-§5.8 contradiction by keeping restart rewrite and multi-draw tiering server-side (D-B7), which also means the monolith's behaviour and diagnostic thread do not change and the name-for-name baseline moves only once (open question 12 closed).
|
||||
- [stage parameter] Verified MobileGL has one combined 192-unit texture space (TextureState.h:41,128; TextureUnit.h:20,24-25) with the per-stage 32 being an advertised number only. Dropped the stage parameter from set_sampler_views and bind_sampler_states; stage flags are derived server-side from the reflection archive where the target API needs them (§4.4.3).
|
||||
- [net LOC honesty] Added §2.7 estimating MGPipe's permanent additions (~6,650 hand-written + ~4,000 generated in the monolith, excluding MG_Remote) against ~372 lines truly deleted, demoted the deletion ledger to supporting evidence, and made §10.3-④'s per-thread CPU number the primary monolith argument (new risk B-R18).
|
||||
- [citations] Verified SamplerObject.h is 160 lines and corrected every reference (BorderColorForm :60-70, SamplerParameters :72-96, GetLifetimeId :141, BumpVersion :151, m_version :155); added scripts/check_doc_citations.py as a P0 CI lint that every file:line in the docs resolves at the baseline commit.
|
||||
- [per-draw cost口径] Verified the dynamic early-outs (SyncRenderState :2016-2018, SyncNeccessaryTextures, CurrentUnitBindingsEpoch :1418-1436, TrySetupDrawFastPath, GetOrCreatePipeline :4982-4993, ApplyDynamicDrawStateTail :5888-5893) and added §2.3.1: the real steady-state pull is ~10-25 accessor calls per backend per draw, not 124/169. Rewrote §10.2 in dynamic terms, added dynamic call/memo-hit counters to P0's deliverables, and required an absolute ns/draw threshold at the GO/NO-GO instead of a relative-to-noise one.
|
||||
- [render-state CSO] Verified the two-counter rationale (RenderState.h:519-528) and that viewport/scissor/line-width setters bump only ++m_version while SET_CAPABILITY bumps BumpVersions (RenderState.cpp:312). Rewrote D-B1: the blob still travels whole for Espryt's span memcmp, but the CSO identity is the pipeline subset only (MGPipeComputePipelineSubsetHash moved verbatim out of VulkanRenderer.cpp:4826-4906 into MG_Pipe/), the dynamic subset goes through a new set_dynamic_state, the server keeps one working RenderStateParameters, and G7 generates a setter-consistency test asserting pipelineSubsetHash changes iff m_pipelineStateVersion changes. Client gates the hash on m_pipelineStateVersion so glViewport costs zero hashing and never evicts Magma's pipeline memo.
|
||||
- [reconcile discipline] Verified MultiDrawElementsIndirectCount calls only SyncPersistentMappedRange (DirectGLES.cpp:4666-4667), never SyncGpuWrites. Replaced §5.8.1's blanket publish/wait/drain rule with a per-site table reproducing the monolith's set exactly, and added a P8 acceptance requiring roundtrips-per-frame to read zero on the create-indirect fixture; flagged the monolith's own omission as a separate dev question the split must not silently fix (open question 15).
|
||||
- [purity gate] Verified RenderState.h:12 includes FramebufferObject.h which includes TextureObject.h/RenderbufferObject.h, and that RenderStateParameters sizes arrays with FramebufferObject::MAX_DRAW_BUFFERS (:263, :273), so the value-header allowlist is not a leaf set and nm --undefined-only is blind to include coupling. Split the purity gate into three: an include-graph gate (compile MG_Backend with MG_State/GLState off the search path) backed by a new MGPipeValueTypes.h extracted in P0.5, the symbol gate, and the undeclared gate - all run only on the non-verify build.
|
||||
- [draw payload cost] Stated MGPDrawInfo's real cost against today's three-register DrawArrays, flag-gated minIndex/maxIndex and xfbCpuCapturedVertices (computed only where a consumer asked), moved the 32-byte MGHostSpan out of the fixed header into the var-tail, and added a per-draw payload-byte histogram to P0's counters (§4.5.7, §10.2).
|
||||
- [memory arithmetic] Corrected §0.4-1 to a full table: 48.25 MiB transport + 0-32 MiB SEG_STAGE headroom + 0-64 MiB index host mirror (split only) + ~1-2 MiB records, with MOBILEGL_PIPE_TEXEL_RETAIN_MB defaulted to 0 because MipmapStorage keeps a complete CPU shadow so retention buys latency, not correctness. Typical +50-60 MiB, worst case ~+145 MiB.
|
||||
- [generated mipmaps] Verified EnsureGenerateMipmapStorageAllocated does AllocateStorage + MarkStorageDirty(false) with no content (DirectGLES.cpp:6270-6271), so GPU-generated levels are allocated-and-zero in the monolith too. Decided explicitly that on_mip_levels_generated carries shape only, glGetTexImage stays 0 round trips on DirectGLES, and only the CPU fallback path produces texels via on_texture_writeback (§9.1).
|
||||
- [map_persistent frequency] Corrected 'once per store lifetime' to 'once per storage definition' (TryAdoptLargeStorage fires at storage-definition time, so a regrowing arena pays N times) and required StorageBufferRegrowScenario to publish a map-persistent-roundtrips counter (D-B4, §8.3, §9.2-8).
|
||||
- [MGHostSpan cost] Restated the monolith cost as one predictable branch plus 32 bytes carried only when kHasUserIndices is set, rather than 'zero'.
|
||||
- [P5 inproc honesty] Added a specification clause that InProcessTransport uses the identical G3 serialization and differs only in doorbell/copy mechanism, plus a PipeApplier debug assertion that no SharedPtr or raw frontend pointer crosses the applier boundary in any transport, so the day-99 milestone actually proves wire completeness (P5).
|
||||
- [P2 baseline definition] Defined the name-for-name functional baseline as 'the refactored monolith at P1 exit' (itself proven equivalent to 81b17c0b by verify), with 81b17c0b retained only as the performance anchor (§10.3-③, B-R3).
|
||||
- [gate list] Added HandleRecycleScenario / TextureRemintPullScenario (with the unanswerable case) / TextureUploadShapeScenario / view-owner cursor aliasing scenario / attachment-only glTexParameter scenario / ClientArrayAfterComputeWriteScenario, each with an explicit statement of what must make it red before the corresponding fix lands.
|
||||
- [callbacks] MGPipeCallbacks grew from 9 to 10 (added on_xfb_scatter_ready) plus the forward terminator resource_subdata_complete; set_* grew from 14 to 17 (set_dynamic_state, set_texture_params, and set_shader_buffers gaining kHostSpan); appendix A and the call-count totals updated throughout.
|
||||
|
||||
## 5. 被驳回或部分驳回的审查意见
|
||||
|
||||
- [performance #11, partial] 'glGetTexImage = 0 round trips does not survive the generated-mipmap ownership split' - the demand for an explicit decision was accepted, but the implied conclusion (it must become a blocking round trip or an eager multi-megabyte writeback) is refuted. EnsureGenerateMipmapStorageAllocated (DirectGLES.cpp:6270-6271) does AllocateStorage + MarkStorageDirty(false) with no content, so a GPU-generated level's shadow is allocated-and-zero in the monolith too; CopyTextureImageToClientOrPBO_State answers from it identically in both modes. on_mip_levels_generated therefore carries shape only and the row stays in §9.1 at zero round trips; only the CPU fallback path (RGB16F/RGB32F, :6811-6861) needs on_texture_writeback. Documented as an explicit decision in §9.1 rather than a fix.
|
||||
- [skeptic framing on §0.4-4] The claim that gen_impl_mutation_surface.py 'vanishes' was corrected rather than accepted as-is: the replay obligation genuinely disappears (there is no replica), but the enumeration obligation reappears as dirty-marking, so the generator is repurposed (gen_pipe_dirty_surface.py) rather than deleted. Listing it as a pure deletion in §0.4-4 was the error; listing the enumeration obligation as unbudgeted was also inaccurate once the generator is repurposed - it is now a P2 deliverable.
|
||||
- [correctness #6, partial] The proposed fix 'delete kCapMultiDraw* and let the client supply index bytes when caps say the server may need them' was accepted for tiering ownership but rejected in its transport form: shipping index bytes per draw through MGHostSpan would put up to 1<<24 indices on the ring per batch. Replaced with an incrementally-maintained server-side index host mirror (D-B7) that costs zero per-draw wire traffic, at the price of a budgeted, counted memory duplication limited to element-array-bound buffers in split mode only - stated openly in the §0.4-1 memory table as the design's one data copy.
|
||||
|
||||
Reference in New Issue
Block a user