Repository navigation
chore: Refactor GPU function call mechanism - #2919
Conversation
ae39d1b to
08937bf
Compare
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.83, 1.72, 3.91, 6.23, 6.49, 10.43, 21.63, 20.80]
line [0.83, 1.71, 3.79, 5.98, 7.48, 12.31, 22.42, 25.37]
line [1.09, 2.08, 4.78, 7.51, 8.64, 14.66, 27.26, 29.80]
---
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.20, 0.40, 0.55, 0.66, 0.90, 1.03, 1.23, 1.28]
line [0.24, 0.45, 0.60, 0.74, 0.97, 1.02, 1.15, 1.29]
line [0.29, 0.53, 0.77, 0.93, 1.17, 1.29, 1.51, 1.73]
---
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.77, 2.06, 3.79, 6.96, 12.69, 23.07, 50.99, 101.57]
line [0.80, 2.07, 3.49, 6.71, 11.36, 24.90, 50.67, 105.94]
line [0.95, 2.56, 4.04, 8.54, 15.28, 30.82, 61.87, 129.17]
|
08937bf to
2c95c24
Compare
|
pkg.pr.new packages benchmark commit |
Bundle size comparison (
|
| 🟢 Decreased | ➖ Unchanged | 🔴 Increased (max 497.51%) | ❔ Unknown |
|---|---|---|---|
| 0 | 3 | 353 | 0 |
import * as ... in PR vs import * as ... in target (did bundle size increase?):
Click to reveal the results table (65 entries).
| Test | tsdown |
|---|---|
| d_struct.ts | 23.18 kB ( |
| d_Void.ts | 850 B ( |
| d_sampler.ts | 858 B ( |
| d_isPtr.ts | 868 B ( |
| d_comparisonSampler.ts | 869 B ( |
| d_isSizeAttrib.ts | 870 B ( |
| d_isWgslArray.ts | 870 B ( |
| d_isAlignAttrib.ts | 871 B ( |
| d_isAtomic.ts | 871 B ( |
| d_isWgslStruct.ts | 871 B ( |
| d_isBuiltinAttrib.ts | 873 B ( |
| d_isDecorated.ts | 874 B ( |
| d_isLocationAttrib.ts | 874 B ( |
| d_isInvariantAttrib.ts | 875 B ( |
| d_isInterpolateAttrib.ts | 877 B ( |
| d_atomic.ts | 895 B ( |
| d_ptrHandle.ts | 967 B ( |
| d_ptrUniform.ts | 968 B ( |
| d_ptrStorage.ts | 972 B ( |
| d_ptrPrivate.ts | 974 B ( |
| d_ptrFn.ts | 975 B ( |
| d_ptrWorkgroup.ts | 976 B ( |
| d_textureExternal.ts | 989 B ( |
| d_textureStorage1d.ts | 1.13 kB ( |
| d_textureStorage2d.ts | 1.13 kB ( |
| d_textureStorage3d.ts | 1.13 kB ( |
| d_textureStorage2dArray.ts | 1.14 kB ( |
| d_isDisarray.ts | 1.23 kB ( |
| d_isUnstruct.ts | 1.23 kB ( |
| d_isLooseDecorated.ts | 1.23 kB ( |
| d_isLooseData.ts | 1.27 kB ( |
| d_isWgslData.ts | 1.42 kB ( |
| d_arrayOf.ts | 34.97 kB ( |
| d_builtin.ts | 37.49 kB ( |
| common_fullScreenTriangle.ts | 50.50 kB ( |
| d_unstruct.ts | 1.78 kB ( |
| tgpu_slot.ts | 1.79 kB ( |
| d_isData.ts | 1.92 kB ( |
| STATIC_common.ts | 58.71 kB ( |
| STATIC_d.ts | 73.27 kB ( |
| std_atomicCompareExchangeWeak.ts | 24.64 kB ( |
| tgpu_unroll.ts | 2.46 kB ( |
| std_frexp.ts | 39.13 kB ( |
| std_modf.ts | 39.26 kB ( |
| tgpu_accessor.ts | 69.85 kB ( |
| tgpu_mutableAccessor.ts | 69.85 kB ( |
| tgpu_computeFn.ts | 47.03 kB ( |
| tgpu_vertexFn.ts | 47.58 kB ( |
| tgpu_fragmentFn.ts | 47.76 kB ( |
| common_writeChannels.ts | 5.58 kB ( |
| tgpu_lazy.ts | 64.14 kB ( |
| tgpu_const.ts | 64.34 kB ( |
| tgpu_workgroupVar.ts | 64.92 kB ( |
| tgpu_privateVar.ts | 64.93 kB ( |
| tgpu_bindGroupLayout.ts | 71.58 kB ( |
| tgpu_comptime.ts | 21.73 kB ( |
| std_extensionEnabled.ts | 21.94 kB ( |
| std_range.ts | 22.21 kB ( |
| d_disarrayOf.ts | 22.43 kB ( |
| d_deepEqual.ts | 23.08 kB ( |
| d_schemasEqualNominally.ts | 23.08 kB ( |
| d_schemasEqualStructurally.ts | 23.08 kB ( |
| std_isBeingTranspiled.ts | 22.24 kB ( |
| std_getShaderStage.ts | 22.29 kB ( |
| std_getTargetShaderLanguage.ts | 22.30 kB ( |
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.
There was a problem hiding this comment.
Pull request overview
Refactors TypeGPU’s GPU-callable invocation model by moving call behavior onto schemas/functions themselves and separating strict signature metadata from the call implementation, with WGSL generation updated to route all calls through a unified ShaderGenerator.call(...) path.
Changes:
- Replace the
$gpuCallableobject-with-.call(...)pattern with a direct callable symbol function and introduce$gpuCallableStrictSignaturefor strict signature metadata. - Centralize call handling in
WgslGenerator.call(...), including strict-signature-based argument conversion and improved argument count diagnostics. - Update schemas (struct/array) and standard library helpers to implement the new calling convention; adjust tests to match updated error messages and typing behavior.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/typegpu/tests/tgsl/wgslGenerator.test.ts | Updates inline snapshot for new strict-signature argument count error message. |
| packages/typegpu/tests/struct.test.ts | Updates snapshot message and removes non-null assertions to match new call/type behavior. |
| packages/typegpu/tests/bindGroupLayout.test.ts | Removes non-null assertion in schema call usage. |
| packages/typegpu/tests/array.test.ts | Updates inline snapshot for new strict-signature argument count error message. |
| packages/typegpu/src/types.ts | Introduces GPUCallableStrictSignature, reshapes GPUCallable, and tightens generateLog args type to readonly. |
| packages/typegpu/src/tgsl/wgslGenerator.ts | Adds centralized call(...) method and refactors call-expression generation to use it. |
| packages/typegpu/src/tgsl/shaderGenerator.ts | Adds call(callee, args) to the ShaderGenerator interface. |
| packages/typegpu/src/tgsl/forOfUtils.ts | Updates arrayLength GPU-callable invocation to the new $gpuCallable(ctx, args) form. |
| packages/typegpu/src/std/numeric.ts | Adjusts variadic helper typing to accept readonly tuples. |
| packages/typegpu/src/std/matrix.ts | Updates std matrix wrappers to bind the new $gpuCallable function directly. |
| packages/typegpu/src/std/environment.ts | Converts $gpuCallable implementation to a direct function. |
| packages/typegpu/src/shared/symbols.ts | Adds $gpuCallableStrictSignature symbol. |
| packages/typegpu/src/data/wgslTypes.ts | Refactors WgslStruct/WgslArray types to extend DualFn for unified call typing. |
| packages/typegpu/src/data/vertexFormatData.ts | Converts $gpuCallable implementation to a direct function. |
| packages/typegpu/src/data/struct.ts | Moves struct schema call behavior into $gpuCallable and provides strict signature metadata. |
| packages/typegpu/src/data/snippet.ts | Makes MapValueToSnippet<T> readonly to better align with readonly argument flows. |
| packages/typegpu/src/data/schemaCallWrapper.ts | Updates schema GPU-call wrapper to use $gpuCallable(ctx, args) form. |
| packages/typegpu/src/data/ref.ts | Converts _ref GPU-callable implementation to a direct function. |
| packages/typegpu/src/data/array.ts | Moves array schema call behavior into $gpuCallable and provides strict signature metadata. |
| packages/typegpu/src/core/unroll/tgpuUnroll.ts | Converts unroll GPU-callable implementation to a direct function. |
| packages/typegpu/src/core/slot/accessor.ts | Updates GPU-callable accessors to use $gpuCallable(ctx, []). |
| packages/typegpu/src/core/function/dualImpl.ts | Splits strict signature metadata onto $gpuCallableStrictSignature and updates $gpuCallable to a direct function. |
| packages/typegpu/src/core/function/createCallableSchema.ts | Converts callable schema GPU-callable implementation to a direct function. |
| packages/typegpu/src/core/function/comptime.ts | Moves comptime GPU-callable behavior onto a shared prototype and updates internal metadata shape. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — single commit 2c95c245 ("chore: Refactor GPU function call mechanism"): struct and array schemas now own their call semantics through $gpuCallable, the $gpuCallable symbol changed shape (callable (ctx, args) => Snippet plus a separate $gpuCallableStrictSignature), and argument-snippet generation was split out of call resolution into a new WgslGenerator.call(callee, args) funnel that _expression and generators can share.
Verified end-to-end: the full packages/typegpu suite (176 files / 2362 tests) and @typegpu/gl (58 tests) pass, and test:types is green on both packages. The struct/array codegen that moved out of _expression into the schema impls maps onto typeInstantiation with identical output for the WGSL generator and the GLSL GlslGenerator override (verified by tracing the array/struct paths through typeInstantiation), so downstream behavior is unchanged. The rewritten over-arity error messages have their snapshots updated in all four touched tests. No leftover consumers of the old object-shaped $gpuCallable anywhere, and no other package implements the symbol.
ℹ️ Redundant second conversion of strict-signature arguments
On AST-backed calls to a strict-signature callable, argument snippets are converted twice: once in the _expression branch via _typedExpression(arg, argType) (which also sets ctx.expectedType, needed for literal typing) and again inside WgslGenerator.call at the isGPUCallable/strict-signature branch. The second pass is a type-preserving no-op (tryConvertSnippet short-circuits on target === dataType), and the schema impls re-run tryConvertSnippet a third time on the already-typed value, so the behavior is correct — just redundant. This includes the pre-existing arrangement, so it is not a regression, but since call() is now the documented funnel for generators, it is worth deciding where conversion is owned once.
Technical details
# Strict-signature argument conversion happens three times
## Affected sites
- packages/typegpu/src/tgsl/wgslGenerator.ts:819-836 (_expression strict-signature branch) — converts via `_typedExpression`
- packages/typegpu/src/tgsl/wgslGenerator.ts:461-471 (call()'s strict-signature branch) — converts via `tryConvertSnippet`
- packages/typegpu/src/data/array.ts:137 and packages/typegpu/src/data/struct.ts:100 — schema impls convert a third time
## Required outcome
- No behavior change; evaluate whether the second and third conversions can be removed without losing the `expectedType` hint that `_typedExpression` provides.ℹ️ Naming: call sits ambiguously next to emitCall
The new ShaderGenerator.call(callee, args) (returns a Snippet) reads confusingly against the existing emitCall(name, templateParams, args) (returns a string that emits the resolved call text), which std codegenImpls and GlslGenerator override. Since ShaderGenerator is already marked unstable, a more distinct name for the invoke-and-resolve path (e.g. callGpu/resolveCall) would spare downstream generator authors a footgun.
Technical details
# Naming ambiguity: `call` vs `emitCall`
## Affected sites
- packages/typegpu/src/tgsl/shaderGenerator.ts:126 — `call(callee: AnyFn, args: readonly Snippet[]): Snippet`
- packages/typegpu/src/tgsl/shaderGenerator.ts:127 — `emitCall(name: string, templateParams: readonly Snippet[], args: readonly Snippet[]): string`
## Required outcome
- Decide on a name for `call` that does not collide conceptually with `emitCall`; renames only touch the unstable interface and the typegpu-gl generator, which inherits from WgslGenerator.Also nice touch: dropping the now-unnecessary ! non-null assertions in the two struct/bind-group tests under the new DualFn-based WgslArray typing.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
2c95c24 to
8a1f015
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — incremental delta (2c95c24 → 8a1f0151) on this force-pushed single-commit branch:
- Split
WgslGenerator._typedExpressioninto_expressionWithTypeSuggestion(setsctx.expectedType, evaluates, no conversion) and_expressionWithTypeConversion(suggestion thentryConvertSnippet, preserving theAutoStructskip). All conversion call sites renamed accordingly acrosswgslGenerator,glslGenerator, andparseResolved. - The tinyest
NODE.callstrict-signature branch now evaluates args via_expressionWithTypeSuggestiononly, moving the single conversion intoWgslGenerator.call()'s strict-signature branch. This eliminates the redundant double/triple conversion flagged in the prior review — behavior is preserved (suggestion+convert semantics identical at every call site,AutoStructhandling intact). - Made
generateLog'sargsparamreadonlyin theResolutionCtx,LogGenerator, andGPUCallabletypes (implementation keeps a mutable signature, valid under method bivariance), and adjusted the std variadicreadonlytuple typing. - Added
emitBinaryOpto theShaderGeneratorinterface (already implemented inWgslGeneratorand overridden inGlslGenerator) and documented thecallvsemit*split — addressing the prior naming-ambiguity note via docs rather than a rename, which is a reasonable call.
I verified the split is behavior-preserving at each call site and ran the green checks: typegpu 198 tests, tgsl 406, @typegpu/gl 58, and test:types on typegpu. Both prior informational concerns are cleanly addressed; no new issues found.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
|
Hey, is this reviewable? |
8a1f015 to
976ae30
Compare
There was a problem hiding this comment.
✅ No new issues found.
( ◦°^°◦)
Avast! I re-boarded this here PR, compared the latest commit with the prior review, and checked the new diagnostics, source-node handlin', and rebase adaptations. The whole fleet o' typegpu tests passed, so I be approvin' this voyage.
Reviewed changes — incremental delta (8a1f0151 → 976ae308) since the previous pullfrog review:
- Rebased the refactor onto current
main, dropping the branch-localisKnownAtComptimein favor of upstreamasComptimeacross the comptime fast paths (comptime,dualImpl,callableSchema, generator eval sites), and adapting toisStoredInMemoryand theimmediateorigin. WgslGenerator.call()now accepts the callee's source snippet and re-attaches source nodes to strict-signature-converted arguments, restoring descriptiveFunction 'fns[0]' is not marked with the 'use gpu' directivediagnostics now thatcall()no longer receives a callee snippet.- Over-arity diagnostics now report the signature's maximum arity (
expected at most N argument(s)), with snapshots updated in the array, struct, and generator tests; removed the dead duplicate "not marked with 'use gpu'" throw in the AST call branch. Boid().posmember-access snapshot origin changedruntime→constantbecause zero-arg struct construction now resolves throughtypeInstantiation(empty args areconstant), which is behavior-neutral.
Verified: full typegpu suite (200 files, 2980 tests), @typegpu/gl (154 passed, 2 skipped), and tsc --noEmit for both packages are green. Both prior informational concerns remain resolved; no new actionable issues.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
✅ No new issues found.
( ◦°^°◦)
Aye, a fresh commit sailed in while I were writin' the last review. I inspected it close: it be a tidy fix to the comptime prototype chain, and the new test guards it. Shipshape, says I.
Reviewed changes — incremental delta (976ae308 → 646f147) since the previous pullfrog review:
- Restored
Function.prototypeat the end of the comptime prototype chain (Object.setPrototypeOf(ComptimePrototype, Function.prototype)), sotgpu.comptimeresults keepcall/apply/bindand satisfyinstanceof Functionafterimpl's prototype was replaced by the plainComptimePrototypeobject. The existingtoStringoverride still reports'comptime'. - Added a regression test in
tests/tgsl/comptime.test.tscoveringinstanceof Function,call,apply,bind, andString(double).
Verified at 646f147: full typegpu suite (200 files, 2981 tests) and tsc --noEmit are green. The fix is correct and targets exactly the gap it describes; no new actionable issues.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
646f147 to
d54b257
Compare
There was a problem hiding this comment.
✅ No new issues found.
✅ ( ◦°^°◦)
Avast! I re-boarded this here PR, compared the fresh commit against the last review, and spied naught but a tidy prototype reshuffle below deck. The whole typegpu fleet still sails green, so I be approvin' this voyage.
Reviewed changes — incremental delta (646f147 → d54b257) since the previous pullfrog review:
- Vertex-format schemas moved to a prototype
$gpuCallable—TgpuVertexFormatDataImplnow implements[$gpuCallable](ctx, [v])as a prototype method rather than assigning a per-instance arrow property in its constructor, and relocates the[$internal]marker from an own field to the prototype via astatic {}initializer. This matches the existingWgslStructImpl/WgslArrayImplprototype pattern. - Behavior-preserving — every call site (
schemaCallWrapperGPU) invokes it asschema[$gpuCallable](...), sothis.typestays correctly bound; host-side[$cast]and theisPackedData/isMarkedInternalchecks resolve the same through the prototype chain. - The comptime
Function.prototypefix and its regression test are unchanged from646f147and were already reviewed.
Verified at d54b257: full typegpu suite (200 files / 2981 tests) and tsc --noEmit are green.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
d54b257 to
e98f49b
Compare
There was a problem hiding this comment.
( ◦°^°◦) ✅
Avast! I re-boarded PR #2919, spied the fresh commits, and found the only new cargo be the cap'n's rebase onto a newer main — nary a line o' the refactor itself changed. I re-checked the seams and the tests sail green, so I be approvin' again.
✅ No new issues found.
Reviewed changes — incremental delta (d54b257 → e98f49b) since the previous pullfrog review:
- Pure rebase — no authored changes. A direct tree diff (
git diff-tree d54b257 e98f49b) shows the PR's own files are byte-identical to the previously reviewedd54b257; the branch is now squashed to two commits on currentmain(0069f48). #3279arrived via the rebase — addsObject.setPrototypeOf(<Impl>, Function.prototype)toWgslArrayImpl/WgslStructImpl/DisarrayImpl/UnstructImplplustests/schemaPrototype.test.ts. Already merged upstream, so these sit as context (not+lines) in the PR diff.#3252arrived via the rebase —attribute()now replaces a same-type attribute (withAttribute/WithoutAttribute) andschemasEqualcompares the exact deduped attrib list;tests/attributes.test.ts.- Refactor + comptime fix unchanged — the
$gpuCallable/$gpuCallableStrictSignatureshape, theWgslGenerator.callfunnel, the struct/array schema impls, and the comptimeFunction.prototypefix with its regression test are all identical to the previously approved state.
Verified at e98f49b: tests/array, tests/struct, tests/attributes, tests/schemaPrototype, tests/bindGroupLayout, and tests/tgsl are green (29 files / 662 tests), and tsc --noEmit for packages/typegpu is clean. All prior pullfrog reviews carry zero open threads.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
e98f49b to
6bcd630
Compare
| // we can use internally to work with generic arrays. | ||
| export interface WgslArray<out TElement extends BaseData = BaseData> extends BaseData { | ||
| export interface WgslArray<out TElement extends BaseData = BaseData> | ||
| extends BaseData, GPUCallable<[elements?: Infer<TElement>[]]> { |
There was a problem hiding this comment.
Is there a reason why array extends GPUCallable but struct extends DualFn?
There was a problem hiding this comment.
To keep it consistent, I moved both to use GPUCallable, I think it's cleaner anyway
6bcd630 to
d280a66
Compare
There was a problem hiding this comment.
✅ No new issues found.
( ◦°^°◦) 🏴☠️
Avast! I re-boarded this here PR, spied the fresh "Review fixes" commit, and found naught but two tidy repairs to the cap'n's review — shipshape and all hands green.
Reviewed changes — incremental delta (d280a66 → 8bfb92d) since the previous pullfrog review:
- Restored
this === undefinedfor non-arrow comptime functions —ComptimePrototype[$gpuCallable]now invokes the wrapped function via.apply(undefined, args)instead ofthis[$internal].func(...args), which had boundthisto the callable object. This matches the pre-refactor closure call, and the host-sideimpl(...)wrapper already behaved this way. - Struct copy origin now falls through from its argument —
WgslStructImpl[$gpuCallable]usesfallthroughCopyOrigin(arg.origin)instead of a hard-coded'runtime', matchingarray.tsandWgslGenerator.typeInstantiation. The only behavioral delta isStruct(<constant snippet>)now yielding a'constant'snippet; object-literal arguments stay'runtime'(theobjectExprbranch always produces'runtime') andtgpu.constreads stay'constant-immutable-def'→'runtime'.
Both are fixes for the open human review feedback on struct copy origin and comptime this binding. Verified at 8bfb92d: full typegpu suite green (206 files / 3084 tests), @typegpu/gl green (154 passed, 2 skipped), and tsc --noEmit for packages/typegpu clean.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
8bfb92d to
57c05ae
Compare
There was a problem hiding this comment.
✅ No new issues found.
( ◦°^°◦) 🏴☠️
Avast! I re-boarded this here PR, spied the fresh "Review fixes" commit, and found naught but a tidy type alignment — shipshape and all hands green.
Reviewed changes — incremental delta (8bfb92d → 57c05ae9) since the previous pullfrog review:
WgslStructswitched fromDualFntoGPUCallable— it nowextends BaseData, TgpuNamable, GPUCallable<[props?: Prettify<InferRecord<TProps>>]>with one explicit(props?: ...): ...signature, so struct and array schemas express their call type the same way. This resolves the open human review question onwgslTypes.ts.- Structurally identical to the previous declaration —
DualFn<T> = T & GPUCallable<Parameters<T>>, and the old(props)/()overloads collapse into a single optional-param signature, so there is no runtime, codegen, or diagnostic change.test:types(tsc --noEmit) forpackages/typegpuis green.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
57c05ae to
b5ca2aa
Compare
b5ca2aa to
d6224cd
Compare
d6224cd to
fca6640
Compare

Changes:
constantorigin where applicable.$gpuCallableobject into just having the method be on the symbol, and strictSignature be a separate symbol:$gpuCallableStrictSignature. This was done so that the method can live on the class prototype and refer to properties of the instance.tinyestnodes is now a seperate step from actually calling a function. This allows other parts of the generator to call helper functions without needing to reach into the deep internals via symbols (helpful in GLSL generation as well as the future matrix std function rewrites)