Repository navigation
fix: Respect custom alignment of nested loose schemas - #3260
cieplypolar wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.
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 [1.06, 2.20, 4.75, 7.75, 8.14, 13.36, 25.58, 28.91]
line [1.03, 2.06, 4.82, 7.71, 8.80, 12.60, 27.33, 28.52]
line [1.09, 2.15, 4.79, 7.61, 8.88, 14.49, 29.28, 30.27]
---
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.60, 0.73, 0.86, 1.22, 1.28, 1.48, 1.66]
line [0.31, 0.49, 0.69, 0.86, 1.13, 1.21, 1.46, 1.63]
line [0.33, 0.53, 0.75, 0.89, 1.20, 1.31, 1.53, 1.69]
---
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 [1.02, 2.37, 4.62, 8.87, 15.52, 28.19, 61.07, 123.61]
line [0.83, 2.30, 4.51, 8.00, 14.25, 28.45, 60.35, 121.98]
line [0.87, 2.24, 3.96, 8.12, 14.74, 28.24, 59.91, 124.60]
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
This PR does not compile. tsc fails with two TS2322 errors in packages/typegpu/src/data/dataIO.ts, which breaks the typegpu build and every CI job that depends on it (build-and-test on built packages, test:types/test-misc, docs, treeshake, publish). This needs to be fixed before merge.
( ◦°^°◦) 🏴☠️ Avast, me hearties! I read the whole diff, ran the core test suite (2969 tests, all passin'), typechecked the core package, and spied the build barrrking at a type error in the new array writer — she'll never make port with that readonly unknown[] keel.
Reviewed changes
alignmentOfrecursion —unstruct/disarraynow delegate tocustomAlignmentOfinstead of peeking at the first property's raw custom alignment, so custom alignment propagates through nesting.offsetsForPropstrailing padding — usessizeOf(struct)directly, dropping spurious trailing padding forunstruct(no change forWgslStruct, whose size is already rounded).- Fallback reader/writer rewrite —
struct/array/unstruct/disarrayare now placed byoffsetsForPropsoffsets androundUp(sizeOf(elementType), alignmentOf(schema))stride, andalignIOwas removed from thedecorated/loose-decoratedpaths, aligning the fallback with the already-offsets-based compiled writer. getPatchInstructionsstride — derives disarray/array element stride fromalignmentOf(arrSchema)rather thanalignmentOf(elementType).- Tests — added nested-alignment coverage in
disarray.test.ts,internal/arrayBufferIO.noEval.test.ts,internal/partialIo.test.ts, andunstruct.test.ts.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
|
pkg.pr.new packages benchmark commit |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run re-reviewed the one commit added since the prior pullfrog review (f7ddd896, type-only). The TS2322 build break is resolved and no new issues were introduced.
- Typed the shared fallback writer params —
writeStructLikeandwriteArrayLikenow declareInferInput<wgsl.WgslStruct | Unstruct>/InferInput<wgsl.WgslArray | Disarray>instead ofRecord<string, unknown>/readonly unknown[], so thesatisfies CompleteDataWritersassignment accepts the typed-array members ofInferInput. Verified locally:pnpm --filter typegpu test:typesandtsc -p tsconfig.build.json --noEmitboth exit 0.
Prior pullfrog review feedback (the writeArrayLike parameter type) is addressed; the corresponding thread is resolved.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
Bundle size comparison (
|
| 🟢 Decreased (max -0.23%) | ➖ Unchanged | 🔴 Increased (max 0.51%) | ❔ Unknown |
|---|---|---|---|
| 34 | 320 | 1 | 0 |
import * as ... in PR vs import * as ... in target (did bundle size increase?):
| Test | tsdown |
|---|---|
| d_alignmentOf.ts | 30.01 kB ( |
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.
2fc983f to
346e20f
Compare
346e20f to
0a01cb7
Compare
| }); | ||
|
|
||
| it('reads wgsl structs packed into an unstruct', () => { | ||
| const s = d.unstruct({ a: d.u32, s: d.struct({ v: d.vec3u, w: d.u32 }) }); |
There was a problem hiding this comment.
Could you convince me as to why structs should pack tightly instead of aligning based on the alignment of their natural alignment? Is it because vectors don't either?
There was a problem hiding this comment.
First, to clarify, structs only relax their starting offsets. Any internal or trailing padding is preserved.
Second, the docs state: Loose schemas are not subject to alignment restrictions. To me, it feels intuitive that unstruct drops starting offset restriction but preserves struct's inner layout.
This is also consistent with vectors and arrays.
Btw, thanks for this comment. While reading it, I realized that I haven't updated vertex layout calculations.
iwoplaza
left a comment
iwoplaza
left a comment
There was a problem hiding this comment.
The implementation looks very solid! I am mostly wondering about the behavior, but I am easily persuaded 😆
0a01cb7 to
fabf065
Compare
Changes:
alignmentOfforunstructanddisarray. Now, it recurses into the schema.offsetsForPropsno longer adds trailing padding tounstruct.unstructs.