From 5722094d6f5c0b28614a53e4c9d5bec6edafebac Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Mon, 10 Aug 2026 07:31:58 -0400 Subject: [PATCH] [Docs] (MG_State): the early AST drop frees only the re-parsed shaders - the compile node co-owns the rest --- .../GLState/ProgramState/ProgramLinkTask.h | 14 ++++++--- .../GLState/ProgramState/ProgramSpirvTask.cpp | 31 ++++++++++++++++--- 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.h b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.h index b0d0a33f..36534289 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.h +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.h @@ -93,11 +93,15 @@ namespace MobileGL::MG_State::GLState { // // MEMORY NOTE: this is the one thing the split makes live LONGER than it used to - // a glslang arena per stage, megabytes for a shaderpack, now alive from the end of - // phase A until phase B has generated its SPIR-V instead of dying with the link - // body. Phase B clears this vector as soon as GlslangToSpv returns, but a deep - // phase-B backlog still holds one arena per queued program. If peak RSS ever - // becomes the binding constraint on a pack load, THIS is the field to attack (by - // bounding the backlog, or by moving GlslangToSpv back into phase A). + // phase A until phase B runs instead of dying with the link body, so a deep + // phase-B backlog holds one arena per queued program. Phase B clears this vector + // as soon as GlslangToSpv returns, but read that call site's comment before + // relying on it: for the COMMON case (a shader linked into exactly one program) + // the compile node co-owns the same TShader and phase A pins that node, so the + // clear frees nothing and only the re-parsed CAS-loser shaders are actually + // released. If peak RSS ever becomes the binding constraint on a pack load, THIS + // is the field to attack - by bounding the backlog, by releasing the compile + // node's own reference at claim time, or by moving GlslangToSpv back into phase A. Vector> shaders; // GL enum per entry of `in.shaders`, in the same order (GetSpirvBinaryFromProgram // walks it to pick the intermediates). diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramSpirvTask.cpp b/MobileGL/MG_State/GLState/ProgramState/ProgramSpirvTask.cpp index bfc3943e..311568f4 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramSpirvTask.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramSpirvTask.cpp @@ -109,11 +109,32 @@ namespace MobileGL::MG_State::GLState { MGLOG_D("ProgramObject %u: Starting SPIR-V generation", externalIndex); GenerateSpirv(handoff, externalIndex); - // GlslangToSpv was the only consumer of the parsed ASTs, and they are by far the - // largest thing this node keeps alive (one glslang arena per stage, megabytes for a - // shaderpack). Everything after this point works on the SPIR-V and on the TProgram's - // own self-contained reflection pool, so drop them here rather than at the end of the - // body - spirv-opt plus routing is ~87% of this node's runtime. + // GlslangToSpv was the only consumer of the parsed ASTs; everything after this point + // works on the SPIR-V and on the TProgram's own self-contained reflection pool. Drop + // them here rather than at the end of the body, which is ~87% of this node's runtime + // earlier (spirv-opt plus routing). + // + // WHAT THIS ACTUALLY FREES, precisely - it is LESS than "the glslang arenas", and the + // difference matters for the peak-RSS story: + // * CAS-LOSER shaders (the re-parse in ShaderCompileTask::ClaimParsedShader, i.e. + // the 2nd..Nth link of a shared shader): freed here in full. The handoff is their + // ONLY owner. + // * CAS-WINNER shaders (the common case - one shader object linked into one + // program, which is every program of an Iris pack load): NOT freed here. The + // winner branch returns a COPY of ShaderCompileTask::artifacts.shader + // (ShaderCompileTask.cpp:320) and the node never releases its own reference, while + // phase A holds that node through in.shaders[i].compiled for its whole life - and + // phase A lives until PhaseAReleaser fires at the end of this body. So the + // refcount goes 2 -> 1 here and the arena dies where it would have died anyway. + // + // Making it free the winner's arena too means releasing whatever pins the TShader + // inside the compile node, and neither obvious route is safe as a drive-by: moving out + // of artifacts.shader at claim time races ShaderObject::GetCompiledShader() on the GL + // thread and breaks JobNode's "a terminal node is immutable" invariant, and dropping + // phase A's in.shaders[i].compiled reference only helps when nothing else holds the + // node (the adoption map is a WeakPtr index, so it would also change which nodes stay + // adoptable). Both belong in a change that can be reviewed against the consume-once + // and adoption semantics on their own terms. handoff.shaders.clear(); MGLOG_D("ProgramObject %u: Building global-UBO routing tables", externalIndex);