feat: d.arrayOf constant origin when all elements constant - #2872
feat: d.arrayOf constant origin when all elements constant#2872vende11s wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates WGSL snippet origin tracking so arrays composed solely of constant values can be treated as constant, and extends test coverage to validate this behavior for array literals and d.arrayOf(...) instantiations.
Changes:
- Propagate
originthroughd.arrayOf(...)([ ... ])instantiation when the argument is anArrayExpression. - Preserve constant-ness through array copies via
fallthroughCopyOrigin(...). - Add tests asserting
originfor captured array snippets isconstantvsruntimebased on element sources.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/typegpu/tests/tgsl/wgslGenerator.test.ts | Adds regression test coverage for constant-origin detection in array captures. |
| packages/typegpu/src/tgsl/wgslGenerator.ts | Adjusts snippet origin inference/propagation for array literals and array instantiation/copies. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return snip( | ||
| instantiated.value, | ||
| instantiated.dataType, | ||
| /* origin */ arg.origin, |
| const allConstant = values.every((value) => value.origin === 'constant'); | ||
|
|
||
| return snip( | ||
| new ArrayExpression(arrayType, values), | ||
| arrayType, | ||
| /* origin */ 'runtime', | ||
| /* origin */ allConstant ? 'constant' : 'runtime', | ||
| values.some((v) => v.possibleSideEffects), | ||
| ); |
There was a problem hiding this comment.
I wouldn't worry about this one too much
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — Reviewed PR #2872, which gives inline array literals ([...]) and d.arrayOf(...)([...]) a constant origin when every element is constant, so const-indexed accesses can emit WGSL const instead of let.
- Constant origin for all-constant array literals (
wgslGenerator.ts): the array-expression path now setsconstantorigin when every element resolves toconstant, instead of hardcodingruntime— e.g.const v = array<u32, 4>(2u, 1u, 3u, 7u)[1i]is now emitted where it used to be alet. - Origin propagation through
d.arrayOf(...)([...])(wgslGenerator.ts): the instantiated array now inherits the argument array literal's origin, and the copy-from-value branch usesfallthroughCopyOrigin(arg.origin)(identical to the oldruntimefor every non-constant origin) instead of a hardcodedruntime. - Regression test (
wgslGenerator.test.ts): aCAPTURE-based test covering plain literals,d.arrayOfover literals with mixed constant/runtime elements, and outer-constvs. inner-letcapture.
I verified the mutation flow is unaffected (let a = [1, 2, 3]; a[0] = idx still emits var, since arrays are not naturally ephemeral and deferred variable types resolve off mutation tracking, not origin), confirmed the emitted const RHS is valid WGSL (spec §8.5.3 allows i32/u32 const indices), confirmed the new test genuinely fails when the fix is reverted, and ran the full typegpu public suite (175 files / 2346 tests) plus package typecheck and lint — all clean.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — Reviewed the delta since the prior pullfrog review (commit c369a6a "apply review suggestions"), which restructures the test coverage added for constant array origins; the source hunks in wgslGenerator.ts are unchanged since the initial review (which was approved).
- Split the single capture test into two (
wgslGenerator.test.ts): separatearray literals origin should be constant when all of its elements are constantandd.arrayOf origin should be constant when all of its elements are constanttests, in line with the reviewer's request. - Clarified the fixtures: each test now declares its own
const x/let yand definesybefore use inside the fn, making the constant-vs-runtime assertions self-contained and readable.
I verified the reworked tests are sound (values defined before use, assertions match the source all-constant logic) and ran tests/tgsl/wgslGenerator.test.ts — all 89 tests pass, including the two reworked cases.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — Reviewed the delta since the prior pullfrog review (c369a6a "apply review suggestions"), namely commit 52f3f7a "fix typeInstantiation".
d.arrayOf(...)([...])now uses the instantiated array's own origin (wgslGenerator.ts): the wrapped literal now passesinstantiated.origininstead ofarg.origin. Both derive from the same elements being constant, so this is behavior-preserving, but it no longer reaches into the argument's origin.- Composite constructors of all-constant args now yield
constant(typeInstantiation,wgslGenerator.ts:1174-1179): the multi-arg return changed its origin from a hardcodedruntimetoargs.every((arg) => arg.origin === 'constant') ? 'constant' : 'runtime'. This is what lets non-array-literal constructor calls (e.g.d.arrayOf(u32, 4)([2, 1, 3, x]), vectors, matrices) becomeconstant, keeping them consistent with the array-literal origin from the earlier commits.
I verified the origin computation is consistent (constant only when every argument is constant — the strict check means constant-immutable-def / runtime elements still fall through to runtime, conservatively), ran the full typegpu suite (175 files / 2347 tests, including the matrix/vector/struct constructors the broadened origin touches) and the package typecheck — all clean. No new issues.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
iwoplaza
changed the title
52f3f7a to
0628976
Compare
|
pkg.pr.new packages benchmark commit |
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.81, 1.62, 3.59, 5.97, 7.51, 9.52, 21.52, 22.97]
line [0.91, 1.75, 3.83, 6.16, 6.94, 9.70, 20.96, 20.61]
line [0.91, 1.82, 3.90, 5.50, 6.42, 10.30, 19.74, 23.67]
---
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.32, 0.43, 0.59, 0.75, 1.00, 1.02, 1.18, 1.34]
line [0.33, 0.52, 0.67, 0.76, 1.01, 1.12, 1.26, 1.40]
line [0.27, 0.46, 0.60, 0.72, 1.06, 1.12, 1.32, 1.44]
---
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.76, 1.76, 3.06, 6.74, 10.69, 23.33, 49.31, 104.35]
line [0.76, 1.96, 3.17, 6.15, 10.88, 23.04, 51.17, 104.31]
line [0.81, 2.02, 3.03, 6.93, 11.04, 22.84, 50.66, 102.04]
|
Bundle size comparison (
|
| 🟢 Decreased | ➖ Unchanged | 🔴 Increased (max 0.02%) | ❔ Unknown |
|---|---|---|---|
| 0 | 304 | 21 | 0 |
import { ... } in PR vs import * as ... in PR (is the library tree-Shakeable?):
| Test | tsdown |
|---|---|
| tgpu_init.ts | 263.15 kB ( |
| tgpu_initFromDevice.ts | 262.60 kB ( |
| tgpu_resolve.ts | 161.79 kB ( |
| tgpu_resolveWithContext.ts | 161.73 kB ( |
| tgpu_bindGroupLayout.ts | 62.32 kB ( |
| tgpu_mutableAccessor.ts | 57.04 kB ( |
| tgpu_accessor.ts | 57.04 kB ( |
| tgpu_privateVar.ts | 55.73 kB ( |
| tgpu_workgroupVar.ts | 55.73 kB ( |
| tgpu_const.ts | 55.15 kB ( |
| tgpu_lazy.ts | 54.95 kB ( |
| tgpu_fragmentFn.ts | 39.68 kB ( |
| tgpu_fn.ts | 39.62 kB ( |
| tgpu_vertexFn.ts | 39.50 kB ( |
| tgpu_computeFn.ts | 39.20 kB ( |
| tgpu_vertexLayout.ts | 28.33 kB ( |
| tgpu_comptime.ts | 15.93 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.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — Reviewed the delta since the prior pullfrog review (52f3f7a8, superseded by a force-push; compared the old SHA's file state against current HEAD 0628976a5). Both changes respond to human-reviewer feedback from the August 24–26 reviews:
- Removed the redundant
ArrayExpressionre-wrap (wgslGenerator.ts:784-786):d.arrayOf(...)([...])now returnstypeInstantiation(...)directly instead of re-wrapping its result insnip(...)with identical fields. Behavior-preserving; addresses the "just recreating the same snippet" concern. - Added
sets origin of external arrays to constant(wgslGenerator.test.ts:2042-2051): covers thefallthroughCopyOrigin(arg.origin)copy-branch change, which previously had no coverage.
I verified the new test genuinely pins the fix — reverting fallthroughCopyOrigin(arg.origin) to 'runtime' makes it fail (expected 'runtime' to be 'constant') — and ran tests/tgsl/wgslGenerator.test.ts (90/90 pass). No new issues.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
No description provided.