Skip to content

fix(@typegpu/gl): release unused uniforms and destroyed textures - #3258

Open
Kaleidosium wants to merge 2 commits into
software-mansion:mainfrom
Kaleidosium:fix-gl-retention
Open

Kaleidosium wants to merge 2 commits into
software-mansion:mainfrom
Kaleidosium:fix-gl-retention

Conversation

@Kaleidosium

@Kaleidosium Kaleidosium commented Oct 22, 2026 •

Copy link
Copy Markdown

Fixes #3153.

Long-lived WebGL roots retained unused uniforms and destroyed texture wrappers until root destruction.

Discover uniform bindings from shader references instead of keeping a root-wide uniform history, and unregister textures when they are destroyed. Cleanup callbacks reference the texture registry weakly, allowing surviving texture handles to outlive the root without retaining sibling textures.

Tests:

  • Regressions for bug(@typegpu/gl): long-lived roots retain unused uniforms and destroyed texture wrappers #3153, failing on main: unused uniforms and their buffers, uniforms no longer used by any pipeline, destroyed textures, and optimized-out uniforms are all collected while their root stays alive.
  • Guards for the new design, passing on main as well: a retained texture keeps neither its root nor its siblings alive, a retained destroyed texture releases its cleanup references, uniforms from both stages bind to locations of the program being drawn and pick up later writes, and remaining textures and framebuffers are destroyed exactly once by root.destroy().
  • The uniform-binding test gives its second pipeline a different fragment shader, so it stays valid whether or not pipelines with identical GLSL share one linked program (as they do with perf(@typegpu/gl): cache linked programs per root #3257).

Behaviour changes:

Not addressed here: samplers still accumulate in the root's sampler cache for the root's lifetime.

Validation:

  • Reproduced on latest stable (@typegpu/gl 0.12.7 / typegpu 0.12.8) and main (7c853fa3) using Metal and SwiftShader.
  • Fixed browser checks confirm collection after each allocation batch and correct native deletion counts. Actual pixel rendering also passes on both backends.
  • 142 GL tests pass against source and built output, with two existing skips. Merged with perf(@typegpu/gl): cache linked programs per root #3257, the GL suite passes too (154 passed, two skipped).
  • Mutation testing: all nine originally injected faults are detected. A second pass of 25 mutants (root-wide histories, retained shader state, stale uploads, locations reused across programs, strong cleanup references, missed or doubled deletions) catches 23; the two survivors only remove redundant cleanup.
  • The garbage-collection tests showed no flakiness across 30 consecutive runs, 24 full-suite runs with eight in parallel, and the threads and vmThreads pools.
  • Workspace type, style, build, and circular-dependency checks pass. All 3,736 source tests and 3,266 built tests pass.

One existing documentation example exceeded its five-second timeout during the unrestricted parallel run. It passed unchanged in isolation, and the complete built suite passed with four workers and unchanged timeout limits.

Copilot AI balanced review requested due to automatic review settings October 22, 2026 11:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No blocking issues were identified, and focused regression tests cover the changed ownership, binding, and cleanup behavior.

No open issues 🎉

What changed in this PR

Fixes #3153 by allowing unused uniforms and destroyed texture wrappers to be collected while a WebGL root remains alive.

Changes:

  • Discover uniform bindings from shader references instead of root-wide history.
  • Unregister destroyed textures through callbacks that weakly reference the texture registry.
  • Add regression coverage for resource lifetimes, uniform bindings, and native cleanup.
File Description
packages/​typegpu-gl/​tests/​webglFallback.test.ts Verify texture and framebuffer deletion exactly once by identity.
packages/​typegpu-gl/​tests/​uniforms.test.ts Cover cross-stage bindings, writes, optimized-out uniforms, and program-specific locations.
packages/​typegpu-gl/​tests/​garbageCollection.test.ts Verify uniform, buffer, root, and texture collection behavior.
packages/​typegpu-gl/​src/​webglTexture.ts Invoke and clear the destruction callback.
packages/​typegpu-gl/​src/​tgpuRootWebGL.ts Remove uniform history and introduce weakly referenced texture-registry cleanup.

Review effort: Balanced

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • tgpuRootWebGL.ts — uniform retention: dropped the root-wide #uniforms registry. TgpuWebGLRenderPipelineImpl now discovers bindings by iterating crossShaderStageState.globalIdentifierMap and filtering instanceof WebGLUniformImpl, so a createUniform result is no longer pinned for the root's lifetime; only pipelines that actually bind a uniform retain it, and optimized-out uniforms (getUniformLocation === null) are not retained.
  • tgpuRootWebGL.ts / webglTexture.ts — texture retention: #textures is now a Set. createTexture installs an onDestroy callback that holds a WeakRef to the registry, and WebGLTextureImpl.destroy() invokes it exactly once after native deletion and then clears it. A surviving texture handle therefore cannot keep the registry or its sibling textures alive.
  • Tests: added GC regression tests (garbageCollection.test.ts), a uniform-binding test that exercises both stages, per-program locations, and optimized-out uniforms (uniforms.test.ts), plus an exactly-once texture/framebuffer teardown test (webglFallback.test.ts).

Traced the reference graph by hand: the texture callback captures only the WeakRef, so no strong edge leads from a texture back to the set or root, and pipeline #uniformBindings retain only the uniforms they bind. root.destroy() iterating the set while onDestroy deletes the current entry is safe JS Set semantics. Verified locally: the full typegpu-gl suite passes (142 passed, 2 skipped), tsc passes, and oxlint reports no issues on the changed files.

ℹ️ TgpuRootWebGL.destroy() no longer destroys uniforms

With the root-wide uniform registry gone, destroy() no longer calls uniform.destroy(). That is behavior-neutral today because WebGLUniformImpl.destroy() is a no-op, but the file carries a TODO(#2510) to make uniforms UBO-backed — at which point root teardown would silently stop releasing uniform GPU resources.

Technical details
# Uniform cleanup owner is no longer the root

## Affected sites
- `packages/typegpu-gl/src/tgpuRootWebGL.ts` (`destroy()`) — iterates buffers, textures, and samplers, but no longer uniforms.
- `packages/typegpu-gl/src/tgpuRootWebGL.ts` (`WebGLUniformImpl.destroy()`) — currently a no-op.

## Required outcome
- None for this PR (no observable behavior change). When #2510 lands and `WebGLUniformImpl` allocates native UBOs, uniform teardown must be owned somewhere — either revisit root-level tracking (e.g. a `WeakRef` registry mirroring `#textures`) or have the pipeline release bindings it created.

## Open questions for the human
- Is uniform resource lifetime intended to be owned by the pipeline rather than the root going forward?

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

…haring

The test assumed a second pipeline built from the same descriptor links a second program. With a program cache (software-mansion#3257) both pipelines share one, so the lookup failed. Check that every upload targets a location queried on the program bound for that draw, and give the second pipeline a different fragment shader so the per-program check stays meaningful.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • uniforms.test.ts — program-sharing-agnostic binding assertions: replaced the program-index-based uniformLocation helper with an uploadedValues() helper that decodes each recorded uniform1f/uniform4fv upload back to its { program, name } and asserts the program matches the one bound by the draw's useProgram. The second pipeline now uses a different fragment shader so its linked program differs from the first, keeping the test valid whether or not identical-GLSL pipelines share one linked program (#3257).

The change is test-only; the production fix and other test files are byte-identical to the prior review. Ran the three touched GL suites locally: 27 passed, 2 skipped.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(@typegpu/gl): long-lived roots retain unused uniforms and destroyed texture wrappers

2 participants