impr: Warn when schema is not uniform aligned - #2750
Conversation
|
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.99, 1.95, 4.28, 6.28, 7.82, 13.40, 24.39, 25.64]
line [0.96, 1.97, 3.92, 6.18, 7.56, 12.00, 23.29, 23.35]
line [1.03, 2.02, 4.60, 6.68, 7.60, 11.68, 23.25, 23.23]
---
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.28, 0.51, 0.67, 0.82, 1.10, 1.19, 1.40, 1.58]
line [0.32, 0.50, 0.69, 0.85, 1.10, 1.21, 1.45, 1.56]
line [0.29, 0.50, 0.66, 0.78, 1.10, 1.18, 1.38, 1.59]
---
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.88, 2.20, 3.81, 7.30, 12.54, 26.48, 58.21, 116.84]
line [0.85, 2.14, 3.91, 7.13, 12.42, 26.30, 55.84, 114.44]
line [0.76, 2.12, 3.66, 6.99, 12.26, 26.96, 55.58, 111.22]
|
Bundle size comparison (
|
| 🟢 Decreased | ➖ Unchanged | 🔴 Increased (max 0.60%) | ❔ Unknown |
|---|---|---|---|
| 0 | 38 | 286 | 0 |
import * as ... in PR vs import * as ... in target (did bundle size increase?):
| Test | tsdown |
|---|---|
| STATIC_tgpu.ts | 285.75 kB ( |
| tgpu_fn.ts | 285.75 kB ( |
| tgpu_init.ts | 285.75 kB ( |
| tgpu_lazy.ts | 285.75 kB ( |
| tgpu_slot.ts | 285.75 kB ( |
| tgpu_const.ts | 285.75 kB ( |
| tgpu_unroll.ts | 285.75 kB ( |
| tgpu_resolve.ts | 285.75 kB ( |
| tgpu_accessor.ts | 285.75 kB ( |
| tgpu_comptime.ts | 285.75 kB ( |
| tgpu_vertexFn.ts | 285.75 kB ( |
| tgpu_computeFn.ts | 285.76 kB ( |
| tgpu_fragmentFn.ts | 285.76 kB ( |
| tgpu_privateVar.ts | 285.76 kB ( |
| tgpu_vertexLayout.ts | 285.76 kB ( |
| tgpu_workgroupVar.ts | 285.76 kB ( |
| tgpu_initFromDevice.ts | 285.76 kB ( |
| tgpu_bindGroupLayout.ts | 285.76 kB ( |
| tgpu_mutableAccessor.ts | 285.76 kB ( |
| tgpu_resolveWithContext.ts | 285.76 kB ( |
| STATIC_allImports.ts | 311.45 kB ( |
import { ... } in PR vs import * as ... in PR (is the library tree-Shakeable?):
| Test | tsdown |
|---|---|
| tgpu_init.ts | 276.22 kB ( |
| tgpu_initFromDevice.ts | 275.68 kB ( |
| tgpu_resolve.ts | 174.83 kB ( |
| tgpu_resolveWithContext.ts | 174.76 kB ( |
| tgpu_bindGroupLayout.ts | 74.50 kB ( |
| tgpu_mutableAccessor.ts | 69.22 kB ( |
| tgpu_accessor.ts | 69.22 kB ( |
| tgpu_privateVar.ts | 67.91 kB ( |
| tgpu_workgroupVar.ts | 67.91 kB ( |
| tgpu_const.ts | 67.33 kB ( |
| tgpu_lazy.ts | 67.12 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.
|
@pullfrog review |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one minor polish item inline.
Reviewed changes — the PR introduces runtime warnings for schemas that violate WGSL uniform-buffer layout constraints, moves buffer-count limit checks into a shared webgpuLimitations.ts module, removes now-misaligned uniform usage from two confetti examples, and adds tests for the new warnings.
- Add
warnIfNotUniformAligned— warns when a schema used asuniformhas struct member offsets or array strides that are not compatible with the default WGSL uniform address space layout. - Move
limitsOverflow.tstowebgpuLimitations.ts— consolidates WebGPU validation helpers; updates imports inapplyPipelineState.tsandlimitsOverflow.test.ts. - Clean up confetti examples — drops unused
'uniform'buffer usage for particle data that now triggers the new warning. - Add test coverage — snapshots legitimate schemas and several misalignment cases in
buffer.test.ts.
Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
|
The newly added Referenced at
|
There was a problem hiding this comment.
Important
The new Uniform alignment tests are failing because their inline snapshots include an extra } in the warning prefix (e.g. [uniform-schema-misaligned}]). Fix the snapshots before merging.
Reviewed changes — added a runtime warning for schemas used as uniform that don't meet WebGPU's strict uniform-buffer layout constraints, plus tests and example updates.
- Merge
limitsOverflow.tsintowebgpuLimitations.ts— consolidates WebGPU limit warnings in one file. - Add
warnIfNotUniformAligned— checks array stride, struct member offset alignment, and spacing for nested structs in uniform buffers. - Wire warning into
$usage('uniform')— catches bothcreateUniform()and explicit$usage('uniform'). - Update confetti examples — removes
uniformusage from particle data buffers that are not uniform-aligned. - Add tests — covers legit schemas, misaligned structs, nested structs, and arrays.
⚠️ Consider enabling the warning in production
uniform-schema-misaligned is currently enabled only in dev/test, unlike webgpu-limits-exceeded which is also enabled in production. Since both are WebGPU-portability warnings that can cause runtime failure on some devices, consider adding the new type to the production subset in packages/typegpu/src/tgpuLogger.ts.
Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
|
Line-anchored details for the findings in my review above:
|
| continue; | ||
| } | ||
|
|
||
| const minimumDifference = roundUp(16, sizeOf(thisValue)); |
There was a problem hiding this comment.
| const minimumDifference = roundUp(16, sizeOf(thisValue)); | |
| const minimumDifference = roundUp(sizeOf(thisValue), 16); |
There was a problem hiding this comment.
Should be fixed now, I also added a test for this
| `\ | ||
| Schema '${getName(schema.elementType) ?? '<unnamed>'}' is used in an array in an uniform buffer, and its stride (${stride}) is not a multiple of 16. | ||
| This is not portable (see https://www.w3.org/TR/WGSL/#address-space-layout-constraints), and will break on some devices. | ||
| To address this, wrap the element in 'd.align(16, ...)'.`, |
There was a problem hiding this comment.
You cannot wrap array element with d.align
There was a problem hiding this comment.
Addressed, see if I did it correctly please
…-is-not-uniform-aligned
There was a problem hiding this comment.
Pull request overview
This PR introduces a new portability warning that detects misaligned uniform-buffer schemas (per WGSL uniform layout constraints) and emits a structured logger warning when a schema is likely to break on some devices. It also consolidates WebGPU-related checks by moving the existing warnIfOverflow logic into the new webgpuLimitations.ts module and updates examples/tests accordingly.
Changes:
- Add
warnIfNotUniformAlignedand invoke it when a buffer is marked with uniform usage. - Add/extend test coverage for uniform-alignment warnings and update the relocated
warnIfOverflowimport. - Update docs examples to avoid using a large particle buffer as
uniform, and update pipeline imports to the new module.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/typegpu/tests/internal/limitsOverflow.test.ts | Updates warnIfOverflow import to its new module location. |
| packages/typegpu/tests/buffer.test.ts | Adds new tests asserting when uniform-alignment warnings should/shouldn’t fire. |
| packages/typegpu/src/tgpuLogger.ts | Adds the new warning type (uniform-schema-misaligned) to the allowed warning list. |
| packages/typegpu/src/core/pipeline/webgpuLimitations.ts | New module combining overflow checks with new uniform-alignment warning logic. |
| packages/typegpu/src/core/pipeline/renderPipeline.ts | Updates warnIfOverflow import to webgpuLimitations.ts. |
| packages/typegpu/src/core/pipeline/computePipeline.ts | Updates warnIfOverflow import to webgpuLimitations.ts. |
| packages/typegpu/src/core/pipeline/limitsOverflow.ts | Removes the old standalone overflow module (logic moved). |
| packages/typegpu/src/core/buffer/buffer.ts | Calls warnIfNotUniformAligned when $usage('uniform') is applied. |
| apps/typegpu-docs/src/examples/simulation/confetti/index.ts | Removes uniform usage from the particle data buffer in the docs example. |
| apps/typegpu-docs/src/examples/react/confetti/index.tsx | Same as above for the React docs example. |
Suppressed comments (2)
packages/typegpu/src/core/pipeline/webgpuLimitations.ts:75
- Grammar in this warning message should be “a uniform buffer”, not “an uniform buffer”.
Schema '${getName(schema) ?? '<unnamed>'}' is used in an uniform buffer, and its property '${key}' does not meet required alignment (offset is ${offset}, required alignment is ${requiredAlignment}).
packages/typegpu/src/core/pipeline/webgpuLimitations.ts:104
- Grammar in this warning message should be “a uniform buffer”, not “an uniform buffer”.
Schema '${getName(schema) ?? '<unnamed>'}' is used in an uniform buffer, and the difference between memory offsets of '${thisKey}' and '${nextKey}' props (${difference}) is less than recommended (${minimumDifference}).
💡 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 — Since the prior pullfrog review, the changes on this PR are limited to fixing the failing inline snapshots and syncing the branch with origin/main.
- Fix warning-prefix snapshots — removed the stray
}so the prefix is⚠️ [uniform-schema-misaligned], resolving the previously flagged failingUniform alignmenttests. - Sync with
origin/main— merged upstream, pulling in thewarnOncelogger endpoint (used by command-encoder / draw-state / timeable, from the logger work) plus unrelated buffer tests; none of this toucheswarnIfNotUniformAlignedor the example changes, which are unchanged since the prior review and remain correct.
No stale references to the removed limitsOverflow.ts remain. One informational note carried forward from the prior review still stands and is left to the author's judgment: uniform-schema-misaligned is not in the production-enabled warning subset, unlike the related webgpu-limits-exceeded.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — Since the prior pullfrog review, the only change on the branch is a merge of main into impr/warn-when-schema-is-not-uniform-aligned, which resolved cleanly without touching any of this PR's feature code.
- Merged
origin/main— pulled in ~20 upstream PRs (operator>>>, unplugin obfuscation, ShaderGenerator lifetime,hasIndexBufferfix, typegpu-threevaryings, etc.). The merge touchedcomputePipeline.tsandrenderPipeline.ts(which this PR also modified for thewarnIfOverflowimport relocation) but with unrelated main-side content only. - Verified the feature survived the merge — both pipeline files still import
warnIfOverflowfromwebgpuLimitations.ts;webgpuLimitations.ts,buffer.ts,tgpuLogger.ts, the confetti examples, and the tests are unchanged since the prior review. - Green after merge — full
typegpuvitest suite passes (170 files / 2268 tests, includingbuffer.test.tsandlimitsOverflow.test.ts), andpnpm --filter typegpu test:typesis clean.
No new issues found in the merged state. The informational note from the prior review still stands (the uniform-schema-misaligned warning is dev/test-only, unlike webgpu-limits-exceeded); a human thread also remains open on the grammar of "an uniform buffer", left to the author's discretion.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…-is-not-uniform-aligned
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Blocked by #2781
I checked and all the fixes suggested indeed do silence the errors