-
Notifications
You must be signed in to change notification settings - Fork 5.5k
[browser][wasm] Fix buffered symbol asset loading #127087
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 7 commits
f3053fd
b0dadf3
9950bf4
1e6b860
b03afe1
c1f38f7
fba083c
d021dd5
034bd93
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -186,6 +186,17 @@ export async function mono_download_assets (): Promise<void> { | |
|
|
||
| const instantiate = async (downloadPromise: Promise<AssetEntryInternal>) => { | ||
| const asset = await downloadPromise; | ||
| const headersOnly = skipBufferByAssetTypes[asset.behavior]; | ||
|
|
||
| if (headersOnly) { | ||
| if (asset.behavior === "symbols") { | ||
| await runtimeHelpers.instantiate_symbols_asset(asset); | ||
| cleanupAsset(asset); | ||
| } | ||
| ++loaderHelpers.actual_downloaded_assets_count; | ||
| return; | ||
| } | ||
|
|
||
| if (asset.buffer) { | ||
| if (!skipInstantiateByAssetTypes[asset.behavior]) { | ||
| mono_assert(asset.buffer && typeof asset.buffer === "object", "asset buffer must be array-like or buffer-like or promise of these"); | ||
|
|
@@ -202,24 +213,12 @@ export async function mono_download_assets (): Promise<void> { | |
| runtimeHelpers.instantiate_asset(asset, url, data); | ||
| } | ||
| } else { | ||
| const headersOnly = skipBufferByAssetTypes[asset.behavior]; | ||
| if (!headersOnly) { | ||
| mono_assert(asset.isOptional, "Expected asset to have the downloaded buffer"); | ||
| if (!skipDownloadsByAssetTypes[asset.behavior] && shouldLoadIcuAsset(asset)) { | ||
| loaderHelpers.expected_downloaded_assets_count--; | ||
| } | ||
| if (!skipInstantiateByAssetTypes[asset.behavior] && shouldLoadIcuAsset(asset)) { | ||
| loaderHelpers.expected_instantiated_assets_count--; | ||
| } | ||
| } else { | ||
| if (asset.behavior === "symbols") { | ||
| await runtimeHelpers.instantiate_symbols_asset(asset); | ||
| cleanupAsset(asset); | ||
| } | ||
|
|
||
| if (skipBufferByAssetTypes[asset.behavior]) { | ||
| ++loaderHelpers.actual_downloaded_assets_count; | ||
| } | ||
| mono_assert(asset.isOptional, "Expected asset to have the downloaded buffer"); | ||
| if (!skipDownloadsByAssetTypes[asset.behavior] && shouldLoadIcuAsset(asset)) { | ||
| loaderHelpers.expected_downloaded_assets_count--; | ||
| } | ||
| if (!skipInstantiateByAssetTypes[asset.behavior] && shouldLoadIcuAsset(asset)) { | ||
| loaderHelpers.expected_instantiated_assets_count--; | ||
| } | ||
| } | ||
| }; | ||
|
|
@@ -529,9 +528,7 @@ async function start_asset_download_sources (asset: AssetEntryInternal): Promise | |
| ok: true, | ||
| arrayBuffer: () => buffer, | ||
| json: () => JSON.parse(new TextDecoder("utf-8").decode(buffer)), | ||
| text: () => { | ||
| throw new Error("NotImplementedException"); | ||
| }, | ||
| text: () => new TextDecoder("utf-8").decode(buffer), | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of scope for this PR. |
||
| headers: { | ||
| get: () => undefined, | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -216,6 +216,26 @@ switch (testCase) { | |
| case "MainWithArgs": | ||
| dotnet.withApplicationArgumentsFromQuery(); | ||
| break; | ||
| case "BufferedAssetsTest": | ||
| const originalFetch4 = globalThis.fetch.bind(globalThis); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name is following the established pattern in the file. Changing the pattern is out of scope of this PR. |
||
| dotnet.withModuleConfig({ | ||
| onConfigLoaded: (config) => { | ||
|
elringus marked this conversation as resolved.
|
||
| const bufferedAssets = [ | ||
| ...config.resources.wasmNative, | ||
| ...config.resources.assembly, | ||
| ...config.resources.pdb, | ||
| ...config.resources.wasmSymbols, | ||
|
pavelsavara marked this conversation as resolved.
|
||
| ]; | ||
|
Comment on lines
+220
to
+228
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's controlled by the test fixtures — nothing can be missing here.
Comment on lines
+223
to
+228
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Already commented above.
Comment on lines
+219
to
+228
|
||
| for (const asset of bufferedAssets) { | ||
| const url = new URL(`./_framework/${asset.name}`, location.href); | ||
| asset.buffer = originalFetch4(url).then(r => { | ||
| if (!r.ok) throw new Error(`Failed to fetch buffered asset '${url}': ${r.status} ${r.statusText}`); | ||
| return r.arrayBuffer(); | ||
| }); | ||
| } | ||
|
Comment on lines
+229
to
+235
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Applied. |
||
| } | ||
| }); | ||
| break; | ||
| } | ||
|
|
||
| const { setModuleImports, Module, getAssemblyExports, getConfig, INTERNAL, invokeLibraryInitializers } = await dotnet.create(); | ||
|
|
@@ -403,6 +423,9 @@ try { | |
|
|
||
| exit(foundB && retB == 42 ? 0 : 1); | ||
|
|
||
| break; | ||
| case "BufferedAssetsTest": | ||
| await dotnet.runMainAndExit(); | ||
| break; | ||
| default: | ||
| console.error(`Unknown test case: ${testCase}`); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of scope for this PR.