Conversation
305ca46 to
24e79bd
Compare
There was a problem hiding this comment.
🟡 Changes recommended
wasm_sdk_cohort detection in run_performance_job.py treats an empty workload source as enabled, which can skip payload setup and then fail inconsistently.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds support for acquiring and using a coherent, SDK-pinned CoreCLR browser-WASM workload cohort (including wasm-tools closure and exact runtime/ref/Crossgen2/ILLink/WebAssembly SDK packs) and propagates that source through performance CI/Helix, with documentation for local usage.
Changes:
- Add
--wasm-workload-sourceoption and forward it into Helix work item execution for CoreCLR WASM ReadyToRun runs. - Implement cohort resolution and installation logic based on the SDK’s
BundledVersions.props, with validation for coherent versions and required packages. - Plumb the new option through pipeline templates and document nightly/VMR CoreCLR WASM R2R usage.
File summaries
| File | Description |
|---|---|
| scripts/tests/test_wasm_coreclr_r2r.py | Adds coverage for forwarding/command generation and cohort version/package validation behavior. |
| scripts/run_performance_job.py | Threads --wasm-workload-source through Helix setup and payload preparation for CoreCLR WASM R2R runs. |
| scripts/dotnet.py | Adds host-RID detection plus coherent WASM workload cohort derivation and installation from an exclusive source. |
| scripts/benchmarks_ci.py | Adds CLI validation and local installation step for coherent CoreCLR WASM workload sources. |
| eng/pipelines/templates/runtime-perf-job.yml | Introduces a wasmWorkloadSource parameter for runtime perf jobs and forwards it downstream. |
| eng/pipelines/templates/run-performance-job.yml | Passes --wasm-workload-source into run_performance_job.py when configured. |
| docs/benchmarking-workflow-dotnet-runtime.md | Documents how to run CoreCLR WASM ReadyToRun using a nightly/VMR SDK + coherent package source. |
| docs/benchmarkdotnet.md | Updates the table-of-contents entry naming for private runtime builds. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
24e79bd to
d83298c
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of correctness/usability issues in the updated docs/CLI error messaging that should be fixed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
d83298c to
6b0c465
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
There are a couple of correctness/documentation issues (empty workload source handling and a duplicate TOC entry) that should be fixed before merging.
Review details
Suppressed comments (3)
docs/benchmarkdotnet.md:31
- The Table of Contents lists “Private Runtime Builds” twice (two bullets with the same #private-runtime-builds anchor), which makes the ToC confusing and adds a duplicate link.
- [Running In Process](#running-in-process)
- [CoreRun](#corerun)
- [dotnet cli](#dotnet-cli)
- [Private Runtime Builds](#private-runtime-builds)
- [Private CoreRT Build](#private-corert-build)
scripts/dotnet.py:278
- This error message is inaccurate: the code allows omitting --dotnet-versions (sdk_versions can be empty) and resolves the SDK from
dotnet --version. The message should only reject multiple SDK versions.
if len(sdk_versions) > 1:
raise ValueError(
'--wasm-workload-source requires exactly one SDK version')
scripts/run_performance_job.py:987
wasm_sdk_cohortbecomes True when--wasm-workload-sourceis an empty string, which then skips the wasm_coreclr payload build and leavesdotnet_pathunset later. Treat empty/whitespace values the same as “not provided”.
wasm_sdk_cohort = (
wasm_coreclr
and args.r2r_run_type == "r2r"
and args.wasm_workload_source is not None)
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 797f00e5-64e5-4d0b-9b62-bf06a1bf9c9d
6b0c465 to
e3f2a6f
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 797f00e5-64e5-4d0b-9b62-bf06a1bf9c9d
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new pre-command insertion for RestoreAdditionalProjectSources uses a POSIX export even when os_group == "windows", which would break prereq setup for any Windows WASM job.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
scripts/run_performance_job.py — RestoreAdditionalProjectSources is always added using a POSIX export command, but… |
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
 |
scripts/dotnet.py — The error message here says “requires exactly one SDK version”, but the code allows zero SDK… View resolved comment |
|
docs/benchmarkdotnet.md — The Table of Contents has a duplicate nested entry for “Private Runtime Builds” that links back to… View resolved comment |
 |
scripts/run_performance_job.py — wasm_sdk_cohort is enabled whenever args.wasm_workload_source is non-None, which treats an… View resolved comment |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 797f00e5-64e5-4d0b-9b62-bf06a1bf9c9d
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The Helix work item command generation for WASM uses a non-Windows environment variable form for --dotnet-path, which makes the Windows path handling inconsistent now that --wasm-workload-source is also being forwarded.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
scripts/run_performance_job.py — In get_work_item_command, the WASM --dotnet-path argument is hard-coded to use… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
scripts/run_performance_job.py — RestoreAdditionalProjectSources is always added using a POSIX export command, but… View resolved comment |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 797f00e5-64e5-4d0b-9b62-bf06a1bf9c9d
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical WASM pipeline and Windows setup issues, plus a cache-isolation issue, remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
eng/pipelines/templates/run-performance-job.yml — This forwards the source whenever it is nonempty, but the existing runtime-wasm job matrix applies… |
|
scripts/run_performance_job.py — Although this helper emits a Windows set assignment, the Windows WASM path still enters the POSIX… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
scripts/run_performance_job.py — In get_work_item_command, the WASM --dotnet-path argument is hard-coded to use… View resolved comment |
Suppressed comments (1)
scripts/dotnet.py:263
--no-http-cachedoes not isolate NuGet's global packages folder, and this workload-install command does not use the temporary--packagesroot from the preceding restore. A stale cached workload-closure package can therefore satisfy installation even when it is absent from the exclusive source, so source completeness and cohort reproducibility are not enforced. Run workload installation with an isolated NuGet package/cache location or otherwise disable the global package cache.
'--skip-manifest-update',
'--configfile',
config_file,
'--no-http-cache',
],
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 797f00e5-64e5-4d0b-9b62-bf06a1bf9c9d
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Workload installation uses an invalid cache option, and source arguments are not safely quoted for Helix serialization.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
scripts/dotnet.py — dotnet workload install does not use the restore-only --no-http-cache option; its cache switch… |
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
scripts/run_performance_job.py — Although this helper emits a Windows set assignment, the Windows WASM path still enters the POSIX… View resolved comment |
|
eng/pipelines/templates/run-performance-job.yml — This forwards the source whenever it is nonempty, but the existing runtime-wasm job matrix applies… View resolved comment |
Suppressed comments (1)
scripts/run_performance_job.py:798
- The source is appended as an unquoted command token, but
send_to_helixserializesWorkItemCommandby joining arguments with spaces (scripts/send_to_helix.py:117). A valid NuGet source containing query parameters such as&, or a local path containing spaces, will therefore be split/interpreted by the Helix shell instead of reachingbenchmarks_ci.pyas one value. Quote/escape this argument with a cross-platform command-line quoting helper before serializing the work-item command.
if wasm_ready_to_run and wasm_workload_source:
work_item_command += [
"--wasm-workload-source",
wasm_workload_source,
]
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 797f00e5-64e5-4d0b-9b62-bf06a1bf9c9d
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Normalize relative custom SDK paths before commands run from the temporary directory.
Review tier: Lite
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
scripts/dotnet.py — dotnet workload install does not use the restore-only --no-http-cache option; its cache switch… View resolved comment |
Summary
--wasm-workload-sourceto acquire the SDK-pinnedwasm-toolsclosure and exact CoreCLR browser-WASM runtime/ref, WebAssembly SDK, host-RID Crossgen2, and ILLink packagesValidation
python3 -m pytest -q scripts/tests/test_wasm_coreclr_r2r.py(21 passed)11.0.100-preview.6.26351.102and its exact WASM cohort from the exclusivedotnet11feed, includingosx-arm64Crossgen2 and native WASM toolinggit diff --checkStack
Depends on #5297. This draft targets
lewing-wasm-coreclr-r2r-perfand should be reviewed/merged after the lower CoreCLR WASM R2R layer.