[coreclr] Throttling of parallel downloads is supported#124964
[coreclr] Throttling of parallel downloads is supported#124964ilonatommy wants to merge 4 commits intodotnet:mainfrom
Conversation
ilonatommy
added
arch-wasm
|
Tagging subscribers to this area: @agocke, @dotnet/runtime-infrastructure |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #124900 where max parallel downloads exceed the configured maximum in CoreCLR WASM. The issue stems from differences in how CoreCLR and Mono handle download throttling and WASM module loading.
Changes:
- Removed "webcil10" from the noThrottleNoRetry exclusion list in CoreCLR, allowing webcil assemblies to be properly throttled
- Added parseInt conversion for maxParallelDownloads in test harness to ensure CoreCLR receives a number type (not string)
- Added CoreCLR-specific logic to exclude dotnet.native.wasm from fetch counting, matching Mono's behavior where it's loaded via import()
- Updated test to pass runtime flavor information to differentiate between Mono and CoreCLR test expectations
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/native/libs/Common/JavaScript/loader/assets.ts | Removes "webcil10" from noThrottleNoRetry map to enable proper throttling of webcil assemblies |
| src/mono/wasm/testassets/WasmBasicTestApp/App/wwwroot/main.js | Adds parseInt conversion for maxParallelDownloads config and CoreCLR-specific fetch wrapper to exclude dotnet.native.wasm from download counting |
| src/mono/wasm/Wasm.Build.Tests/MaxParallelDownloadsTests.cs | Passes runtime flavor to test to enable different behavior verification for Mono vs CoreCLR |
| BrowserQueryString: new NameValueCollection { {"maxParallelDownloads", maxParallelDownloads } } | ||
| BrowserQueryString: new NameValueCollection { | ||
| {"maxParallelDownloads", maxParallelDownloads }, | ||
| {"runtimeFlavor", s_buildEnv.IsMonoRuntime ? "Mono" : "CoreCLR" } |
There was a problem hiding this comment.
The property s_buildEnv.IsMonoRuntime does not exist. The correct way to check the runtime flavor is to use EnvironmentVariables.RuntimeFlavor != "CoreCLR" (as seen in BuildEnvironment.cs line 114 and WasmTemplateTestsBase.cs line 127). Alternatively, consider adding a public property IsMonoRuntime to BuildEnvironment class that returns EnvironmentVariables.RuntimeFlavor != "CoreCLR" for better consistency and readability.
| {"runtimeFlavor", s_buildEnv.IsMonoRuntime ? "Mono" : "CoreCLR" } | |
| {"runtimeFlavor", EnvironmentVariables.RuntimeFlavor != "CoreCLR" ? "Mono" : "CoreCLR" } |
| } | ||
| }; | ||
| dotnet.withConfig({ maxParallelDownloads: maxParallelDownloads }); | ||
| dotnet.withConfig({ maxParallelDownloads: parseInt(maxParallelDownloads) }); |
There was a problem hiding this comment.
The parseInt call should include a radix parameter (10) and handle potential NaN results. If maxParallelDownloads is null/undefined/invalid, parseInt returns NaN, which could cause unexpected behavior in the runtime throttling logic. Consider adding validation or using parseInt(maxParallelDownloads, 10) || 16 to provide a fallback to the default value.
| dotnet.withConfig({ maxParallelDownloads: parseInt(maxParallelDownloads) }); | |
| dotnet.withConfig({ maxParallelDownloads: parseInt(maxParallelDownloads, 10) || 16 }); |
| const isCoreCLR = params.get("runtimeFlavor") === "CoreCLR"; | ||
| globalThis.fetch = async (...args) => { | ||
| if (isCoreCLR) { | ||
| const url = typeof args[0] === "string" ? args[0] : args[0]?.url ?? ""; |
There was a problem hiding this comment.
All the changes are described in the PR, this is point 3.
There was a problem hiding this comment.
I see. I think we do not use import() for .wasm files.
| BrowserQueryString: new NameValueCollection { | ||
| {"maxParallelDownloads", maxParallelDownloads }, | ||
| {"runtimeFlavor", s_buildEnv.IsMonoRuntime ? "Mono" : "CoreCLR" } | ||
| } |
There was a problem hiding this comment.
The PR description lists removing the webcil10 exemption from throttling in src/native/libs/Common/JavaScript/loader/assets.ts (the noThrottleNoRetry map), but in the current code that entry still appears to be present. If that change was intentionally deferred (e.g., due to Webcil loader concerns), please update the PR description accordingly; otherwise include the intended code change so the description matches what this PR actually does.
|
In favor of #124969. |
Fixes #124900.
Changes
runtime/src/mono/browser/runtime/loader/assets.ts
Line 485 in e1c62a5
runtime/src/native/libs/Common/JavaScript/loader/assets.ts
Line 350 in e1c62a5
We have to add conversion to integer in wbt main.js.
2) All webcil assemblies are "webcil10" type that is excluded from throttling in coreclr, see:runtime/src/native/libs/Common/JavaScript/loader/assets.ts
Line 466 in e1c62a5
We have to remove this entry.dotnetwasmthroughglobalThis.fetch, it's loaded viaimport(). In CoreCLRdotnetwasmis fetched and the test counts it towards parallel downloads count, which increases it. We have to update the WBT expectations as well.Has to wait for #124850 to be merged.