fix: Simpler ShaderGenerator lifetime - #2809
Conversation
|
Run failed. View the logs →
|
Resolution Time Benchmark---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Random Branching (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.99, 1.93, 4.02, 7.01, 8.30, 11.66, 21.44, 24.22]
line [1.01, 1.96, 4.10, 6.86, 7.97, 12.88, 22.06, 27.14]
line [1.00, 2.00, 4.75, 6.80, 7.96, 13.05, 23.96, 26.36]
---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Linear Recursion (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.34, 0.52, 0.76, 0.84, 1.19, 1.22, 1.48, 1.57]
line [0.27, 0.54, 0.68, 0.81, 1.12, 1.24, 1.43, 1.56]
line [0.31, 0.57, 0.77, 0.90, 1.29, 1.31, 1.58, 1.65]
---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Full Tree (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.79, 2.15, 4.16, 7.09, 12.60, 26.29, 55.78, 112.76]
line [0.91, 2.11, 3.93, 7.54, 12.91, 26.20, 56.23, 115.46]
line [1.02, 2.21, 4.75, 8.22, 13.44, 27.11, 58.70, 119.51]
|
There was a problem hiding this comment.
Pull request overview
This PR refactors TypeGPU’s shader generator lifecycle to be instantiated per ResolutionCtx (constructor-injected context) instead of relying on a long-lived singleton instance with a separate initGenerator step. This aligns the unstable unstable_shaderGenerator option with a “class/newable” contract and updates tests/docs accordingly.
Changes:
- Replace
ShaderGenerator.initGenerator(ctx)with constructor injection (new (ctx: ResolutionCtx)), introducingShaderGeneratorClass. - Remove default-export singleton generator instances (
wgslGenerator,glslGenerator) and update call sites to pass generator classes. - Update tests/docs to reference
WgslGeneratorand adapt snippet-capture utilities.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/typegpu/tests/utils/parseResolved.ts | Updates snippet extraction to use a generator class instead of an instance. |
| packages/typegpu/tests/tgsl/wgslGenerator.test.ts | Renames describe label to WgslGenerator. |
| packages/typegpu/tests/tgsl/infixOperators.test.ts | Renames describe label to WgslGenerator. |
| packages/typegpu/tests/tgsl/consoleLog.test.ts | Renames describe label to WgslGenerator. |
| packages/typegpu/src/tgsl/wgslGenerator.ts | Makes WgslGenerator context-bound via constructor; removes default singleton export. |
| packages/typegpu/src/tgsl/shaderGenerator.ts | Introduces ShaderGeneratorClass; removes initGenerator from the interface. |
| packages/typegpu/src/shared/normalizeMetadata.ts | Updates docs text to reference WgslGenerator. |
| packages/typegpu/src/resolutionCtx.ts | Instantiates a new generator per context; updates option type to ShaderGeneratorClass. |
| packages/typegpu/src/core/simulate/tgpuSimulate.ts | Passes WgslGenerator class to simulation context. |
| packages/typegpu/src/core/root/init.ts | Updates init options to accept ShaderGeneratorClass. |
| packages/typegpu/src/core/resolve/tgpuResolve.ts | Updates resolve options to accept ShaderGeneratorClass. |
| packages/typegpu-testing-utility/src/capture.ts | Updates capture helper to pass a generator class instead of an instance. |
| packages/typegpu-gl/src/glslGenerator.ts | Removes default singleton export of GlslGenerator. |
| packages/typegpu-gl/src/glOptions.ts | Updates WebGL options to pass GlslGenerator class. |
| apps/typegpu-docs/src/content/docs/advanced/explaining-the-magic.mdx | Updates docs wording from wgslGenerator to WgslGenerator. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
638b221 to
dd30022
Compare
|
pkg.pr.new packages benchmark commit |
Bundle size comparison (
|
| 🟢 Decreased | ➖ Unchanged | 🔴 Increased (max 0.07%) | ❔ Unknown |
|---|---|---|---|
| 0 | 301 | 21 | 0 |
import { ... } in PR vs import * as ... in PR (is the library tree-Shakeable?):
| Test | tsdown |
|---|---|
| tgpu_init.ts | 267.85 kB ( |
| tgpu_initFromDevice.ts | 267.32 kB ( |
| tgpu_resolve.ts | 168.42 kB ( |
| tgpu_resolveWithContext.ts | 168.35 kB ( |
| tgpu_bindGroupLayout.ts | 73.79 kB ( |
| tgpu_mutableAccessor.ts | 68.51 kB ( |
| tgpu_accessor.ts | 68.51 kB ( |
| tgpu_privateVar.ts | 67.20 kB ( |
| tgpu_workgroupVar.ts | 67.20 kB ( |
| tgpu_const.ts | 66.62 kB ( |
| tgpu_lazy.ts | 66.42 kB ( |
| tgpu_fragmentFn.ts | 38.92 kB ( |
| tgpu_fn.ts | 38.86 kB ( |
| tgpu_vertexFn.ts | 38.73 kB ( |
| tgpu_computeFn.ts | 38.44 kB ( |
| tgpu_vertexLayout.ts | 27.57 kB ( |
| tgpu_comptime.ts | 15.17 kB ( |
| tgpu_unroll.ts | 1.75 kB ( |
| tgpu_slot.ts | 1.70 kB ( |
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
This PR currently fails to typecheck — pnpm --filter typegpu test:types reports 4 errors — so it cannot merge as-is. The root cause is the unstable_shaderGenerator option now typed as ShaderGeneratorClass while the root's internal plumbing still works with a ShaderGenerator instance. See the inline init.ts comment for the exact fix.
Reviewed changes
The new commit 638b221 ("fix: Simpler ShaderGenerator lifetime") finalized the generator-lifetime refactor on top of the previously reviewed state:
- Replaced
TgpuRootWebGL's shared#shaderGeneratorsingleton with directGlslGeneratorclass references in eachtgpu.resolvecall. - Made
CapturingGenerator/ExtractingGeneratorabstract and scoped their captured state to per-call closures via anonymous subclasses, addressing the non-reentrancy concern from the prior review. - Exported
ShaderGeneratorClassfromtypegpu/~internal.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
dd30022 to
b397138
Compare
There was a problem hiding this comment.
ℹ️ The blocking typecheck issue from the prior review is fixed and no new issues were found; the one remaining prior item (a minor test-utility nit) is still open, so this review isn't a full approval.
Reviewed changes
The branch was force-pushed to a single commit (b397138) on the rebased base, so this run re-reviewed the full generator-lifetime refactor plus the correction since the prior review.
- Fixed the prior blocking typecheck break —
TgpuRootImpl.shaderGenerator(field + constructor param ininit.ts) andExperimentalTgpuRoot.shaderGenerator(rootTypes.ts) are now typedShaderGeneratorClass, matching theunstable_shaderGeneratoroption; no moreShaderGenerator-instance plumbing intoresolve/ResolutionCtxImpl. - Completed the lifetime refactor —
WgslGenerator/GlslGeneratorare now constructor-injected perResolutionCtx(new (opts.shaderGenerator ?? WgslGenerator)(this)inresolutionCtx.ts:452),initGeneratoris removed, and the singleton default exports (wgslGenerator,glslGenerator) are deleted; everyunstable_shaderGeneratorcall-site passes a generator class and the capture/snippet-test helpers use closure-scoped per-call state.
Verified: pnpm --filter typegpu test:types, @typegpu/gl test:types, and the testing-utility tsc all pass; the tgsl suite (317 tests) and @typegpu/gl suite (28 tests) pass.
The prior review's only still-open thread — the dead "Cannot inspect multiple return values" guard in packages/typegpu/tests/utils/parseResolved.ts (latestReturnedSnippet is backed by a setter-only accessor, so the guard read at line 29 always sees undefined and the last return silently wins) — is unchanged by the new commit. It's a test-utility edge rather than a correctness/merge blocker, but per the open thread it's not treated as addressed here.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
b397138 to
7c2cc8f
Compare
There was a problem hiding this comment.
ℹ️ The incremental change — making
getTargetShaderLanguage()report the active generator's actual language — is clean, correctly implemented, and well-tested, with no new issues found. This isn't a full approval only because the prior minorparseResolved.tsnit remains open.
Reviewed changes
Since the prior pullfrog review (b397138):
- Made
getTargetShaderLanguage()returnctx.gen.languageKeyinstead of a hardcoded'wgsl', so code generating GLSL now correctly reports'glsl'(blocked off insimulate/top-level by the existinggetExecModeguard). - Added
readonly languageKey: stringto theShaderGeneratorinterface and wired it as a prototype property onWgslGenerator('wgsl') andGlslGenerator('glsl'), usingdeclare+ a static block so subclass overrides resolve correctly through the prototype chain. - Added a regression test in
@typegpu/glasserting a GLSL resolve reports'glsl'— this test fails under the prior hardcoded behavior, so it's real coverage. - Updated the
std.getTargetShaderLanguageJSDoc and theutils.mdxdocs table to describe the language-agnostic semantics.
Verified: typegpu and @typegpu/gl typechecks pass; tests/std + tests/tgsl (691 tests) and glslGenerator.test.ts pass.
The prior thread at packages/typegpu/tests/utils/parseResolved.ts:52 (setter-only accessor leaving the "Cannot inspect multiple return values" guard dead) is untouched by this delta and remains open — a test-utility nit, not a merge blocker.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
7c2cc8f to
17b58ae
Compare
There was a problem hiding this comment.
ℹ️ No functional issues found in the incremental change — the pivot to instance-per-resolution with an eager
initGeneratorguard is internally coherent, typechecks, and passes thetgsl/std/@typegpu/glsuites I ran (767 + 29 tests). Two rough edges on the unstable API surface below.
Reviewed changes
Since the prior pullfrog review (7c2cc8f):
- Reversed the generator lifetime from constructor injection (
new (ctx)) to a fresh instance per resolution:ResolutionCtxImplnow defaults toopts.shaderGenerator ?? new WgslGenerator()and callsinitGenerator(this)eagerly in its constructor, andWgslGenerator.initGeneratorgained a double-init guard that throws. - Slimmed
ShaderGeneratorClassdown tonew (): ShaderGenerator, kept it for root storage only, and made render/compute pipelines instantiatenew class()per resolve. - Renamed the root field
shaderGenerator→shaderGeneratorClass(updating thenonTransferablePriorskey and theTgpuRootPicklist accordingly). - Restored
unstable_shaderGeneratorfortgpu.resolveto an instance, switchedTgpuRootWebGL/glOptionsto pass a freshnew GlslGenerator()per resolve call, and deleted thewgslGenerator/glslGeneratordefault singleton exports. - Reverted
parseResolved.ts/capture.tsto the simple fresh-instance-per-call form (dropping them out of the PR), which also resurrects the previously-dead "Cannot inspect multiple return values" guard.
The previously-open thread on the dead parseResolved.ts guard is addressed by that revert and was resolved.
ℹ️ Nitpicks
- The single-use contract of a generator instance is only enforced at runtime, not expressed by the type: constructing
new GlslGenerator()is fine, but handing the same instance to twotgpu.resolvecalls throws "Cannot initialize shader generators twice" at the second call (wgslGenerator.ts:224). A cachedglOptions()result, or an options object reused across resolves, would trip it in a place the type system can't flag. The error message does point users at the right remedy, so this is fine for an unstable API — just noting that the guard silently turns what used to be a harmless reuse into a hard failure.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
eea883d to
fc67705
Compare
fc67705 to
3359a48
Compare
The main idea behind this change is to create a generator per resolution pass (tgpu.resolve call, root.unwrap(pipeline), ...). This makes it very easy to not have to think about nested resolves (which we want to support because of @typegpu/three)