Repository navigation
impr: Skip uniform alignment analysis when its warning is off - #3238
Conversation
|
pkg.pr.new packages benchmark commit |
There was a problem hiding this comment.
🟢 Approval recommended
The guard respects existing warning configuration, with no blocking issues identified.
No open issues 🎉
What changed in this PR
Avoids unnecessary uniform-alignment analysis when its warning is disabled, reducing production buffer-creation overhead.
Changes:
- Adds an internal logger query for warning enablement.
- Skips alignment analysis when disabled while preserving enabled-warning behavior.
| File | Description |
|---|---|
| packages/typegpu/src/tgpuLogger.ts | Adds isEnabled using current warning state. |
| packages/typegpu/src/core/pipeline/webgpuLimitations.ts | Returns early when alignment warnings are disabled. |
Review effort: Balanced
![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.
( ◦°^°◦) 🏴☠️ Ahoy! I read the whole diff, checked who calls warnIfNotUniformAligned, and made sure the new isEnabled be the only implementer's burden. The ship be seaworthy — no leaks found.
Reviewed changes
- Skip disabled alignment analysis —
warnIfNotUniformAlignednow returns early whenuniform-schema-misalignedis off, avoiding a full schema walk and layout computation that produced no output in production. - Internal
Logger.isEnabled— newisEnabled(type)on theLoggerinterface, implemented onTgpuLoggeras#enabledWarnings.has(type).
I verified the output is byte-for-byte identical in every environment (the old logger.warn already no-op'd when the type was disabled), that dev/test keep all warning types enabled so the warning tests still pass, and that TgpuLogger is the only implementer of the non-exported Logger interface. The skipped memoryLayoutOf/alignmentOf/sizeOf calls are pure memoizers with no side effects beyond caching, so eliding them is safe. Nice, well-scoped perf win.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
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.84, 1.84, 3.60, 5.50, 6.23, 10.07, 20.19, 19.17]
line [0.79, 1.54, 3.26, 5.28, 6.68, 9.45, 20.01, 20.53]
line [0.74, 1.70, 3.36, 5.00, 6.01, 8.44, 18.19, 20.22]
---
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.23, 0.44, 0.58, 0.71, 1.02, 1.00, 1.22, 1.38]
line [0.22, 0.44, 0.57, 0.77, 1.00, 0.99, 1.24, 1.36]
line [0.23, 0.39, 0.57, 0.68, 0.93, 0.91, 1.07, 1.24]
---
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.67, 1.86, 3.29, 6.06, 11.09, 22.18, 44.37, 96.66]
line [0.86, 2.06, 3.28, 6.16, 11.13, 22.20, 48.46, 94.29]
line [0.76, 1.82, 2.86, 5.76, 10.70, 21.01, 48.65, 90.59]
|
Bundle size comparison (
|
| 🟢 Decreased | ➖ Unchanged | 🔴 Increased (max 0.17%) | ❔ Unknown |
|---|---|---|---|
| 0 | 39 | 316 | 0 |
No notable changes.
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.
316298f to
f7922a6
Compare
f7922a6 to
beb7ab4
Compare
beb7ab4 to
e31b14a
Compare
There was a problem hiding this comment.
✅ No new issues found.
( ◦°^°◦) 🏴☠️ Ahoy! I read the new commit stem to stern — the enablement check be hoisted out o' the recursive walk into its own helper, just as the reviewer asked. No leaks in the hull.
Reviewed changes
- Guard hoisted out of recursion —
warnIfNotUniformAlignednow evaluateslogger.isEnabled('uniform-schema-misaligned')once and hands the recursive schema walk to a new privatecheckUniformAlignmenthelper, so the check no longer re-runs at every array/struct node.
This addresses the review feedback to move the enablement check out of the recursive function. The refactor is behavior-preserving: when the warning is disabled the exported entry point still returns before any traversal; when enabled, checkUniformAlignment performs the same recursion and emits the same warnings as the previous self-recursive warnIfNotUniformAligned. The only caller (packages/typegpu/src/core/buffer/buffer.ts:365) is unaffected, and the helper is module-private.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
Every `$usage('uniform')` walked the schema and computed memory layouts to decide whether to warn about non-portable uniform alignment (`uniform-schema-misaligned`). That warning is off by default in production builds, so the work was wasted there.
- Adds `isEnabled(type)` to the internal logger, so callers can skip computing what a warning needs when it won't be shown.
- `warnIfNotUniformAligned` returns early when its warning is disabled. In development and tests nothing changes.
Node 22 with `NODE_ENV=production`, creating `root.createBuffer(Scene).$usage('uniform')` for a scene struct (camera matrices plus an array of 16 light structs). Medians of 9 fresh processes:
| | main | this PR |
|---|---|---|
| First buffer | 1.90 ms | 0.45 ms |
| Each of the next 200 | 69.2 µs | 3.8 µs |
e31b14a to
4d4f9dd
Compare
Every
$usage('uniform')walked the schema and computed memory layouts to decide whether to warn about non-portable uniform alignment (uniform-schema-misaligned). That warning is off by default in production builds, so the work was wasted there.isEnabled(type)to the internal logger, so callers can skip computing what a warning needs when it won't be shown.warnIfNotUniformAlignedreturns early when its warning is disabled. In development and tests nothing changes.Node 22 with
NODE_ENV=production, creatingroot.createBuffer(Scene).$usage('uniform')for a scene struct (camera matrices plus an array of 16 light structs). Medians of 9 fresh processes: