impr: Unnamed externals#2593
Draft
aleksanderkatan wants to merge 2 commits into
Draft
Conversation
|
pkg.pr.new packages benchmark commit |
📊 Bundle Size Comparison
👀 Notable resultsStatic test results:No major changes. Dynamic test results:No major changes. 📋 All resultsClick to reveal the results table (355 entries).
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu. |
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.24, 2.77, 6.02, 9.26, 11.55, 16.35, 33.47, 33.60]
line [0.92, 1.95, 3.98, 6.69, 7.48, 11.01, 20.67, 21.54]
line [0.95, 1.89, 3.92, 6.44, 7.73, 9.43, 20.52, 24.42]
---
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.45, 0.81, 1.13, 1.40, 1.81, 1.94, 2.32, 2.57]
line [0.30, 0.53, 0.68, 0.83, 1.09, 1.16, 1.42, 1.56]
line [0.31, 0.53, 0.65, 0.83, 1.12, 1.22, 1.40, 1.61]
---
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.14, 3.04, 5.12, 9.43, 19.19, 39.58, 82.76, 167.96]
line [0.84, 2.04, 3.72, 6.06, 12.45, 25.20, 53.57, 109.51]
line [0.75, 1.98, 4.19, 6.17, 11.99, 26.63, 53.73, 110.47]
|
There was a problem hiding this comment.
Pull request overview
This PR improves external dependency handling during WGSL resolution by ensuring unnamed externals nested inside plain JS objects are discovered and assigned stable names (so property-based externals like ext.prop are correctly named and emitted).
Changes:
- Introduces
renameAndMergeExternalsto traverse external maps and attach names to previously-unnamed nested externals before merging. - Updates core resolution paths (
tgpuResolve,fnCore, raw snippets, and declarations) to use the new rename+merge flow. - Adds regression tests for nested external renaming and array behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/typegpu/tests/function.test.ts | Adds test coverage for nested external renaming and ensures arrays aren’t renamed. |
| packages/typegpu/src/core/resolve/tgpuResolve.ts | Switches resolve-time dependency merging to renameAndMergeExternals. |
| packages/typegpu/src/core/resolve/externals.ts | Adds recursive renaming pass and new exported merge helper. |
| packages/typegpu/src/core/rawCodeSnippet/tgpuRawCodeSnippet.ts | Uses renameAndMergeExternals when composing snippet externals. |
| packages/typegpu/src/core/function/fnCore.ts | Uses renameAndMergeExternals for function externals, including plugin-provided externals. |
| packages/typegpu/src/core/declare/tgpuDeclare.ts | Uses renameAndMergeExternals for declaration externals. |
Comments suppressed due to low confidence (1)
packages/typegpu/src/core/resolve/externals.ts:47
mergeExternalswill treat arrays as nestedExternalMaps when the same key exists in both maps, because arrays are objects and!isResolvableis true. This causes arrays to be shallow-copied into plain objects via{ ...existingValue }, losing the Array shape and potentially breaking externals like closure-captured arrays when merged multiple times. Arrays should be treated as atomic values (replace instead of deep-merge).
if (
existingValue !== null &&
typeof existingValue === 'object' &&
value !== null &&
typeof value === 'object' &&
!isResolvable(existingValue) &&
!isResolvable(value)
) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
|
Let's wait for #2425, I kind of already made an assumption that external is either resolvable or external, which is not yet true ( |
Contributor
Author
|
This problem will remain: it('...', () => {
const fn = tgpu.fn([d.u32])`(a) {
let b = d.vec2u(a);
}`.$uses({ d });
expect(tgpu.resolve([fn])).toMatchInlineSnapshot(`
"fn fn_1(a: u32) {
let b = vec2u(a);
}"
`);
});We should probably just autoname just the used items (can we do this lazily?) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Right now, in
mergeExternals, we only name some of the unnamed externals. This PR makes it so we actually traverse all of it.