Avoid embedding PGO data for Wasm targets - #134093
Conversation
Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/crossgen-contrib |
Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved issues remain in compiler enforcement, WASI handling, regression coverage, and documentation.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates Wasm ReadyToRun compilation to consume MIBC profiles without embedding PGO data.
Changes:
- Gates embedded PGO data for Wasm targets.
- Re-enables MIBC inputs for browser builds.
- Adjusts CoreCLR, test, and Crossgen2 invocation paths.
File summaries
| File | Summary |
|---|---|
src/tests/Common/CLRTest.CrossGen.targets |
Adjusts test PGO arguments; the WASI guard still requires correction. |
src/installer/pkg/sfx/Microsoft.NETCore.App/Microsoft.NETCore.App.Runtime.CoreCLR.sfxproj |
Removes the browser MIBC exclusion. |
src/coreclr/tools/r2rtest/Crossgen2Runner.cs |
Preserves MIBC while avoiding embedded PGO for Wasm. |
src/coreclr/crossgen-corelib.proj |
Separates MIBC input from PGO embedding; compiler-level enforcement remains needed. |
eng/codeOptimization.targets |
Disables Wasm PGO embedding; regression coverage and documentation remain needed. |
Review details
Suppressed comments (3)
eng/codeOptimization.targets:39
- The PR description lists documenting the Wasm-specific behavior, but this change adds no documentation and leaves
EmbedPgoDataOptionas the generic “Embed instrumentation data in generated file” help text. Add the promised Crossgen2/help or design documentation stating that--embed-pgo-datais ignored for Wasm, or remove that claimed change.
<PropertyGroup Condition="'$(IncludeMibcFilesInReadyToRun)' == 'true' and '$(TargetArchitecture)' != 'wasm'">
src/coreclr/crossgen-corelib.proj:166
- This only stops this MSBuild invocation from passing
--embed-pgo-data; Crossgen2 itself still passesGet(_command.EmbedPgoData)directly intoProfileDataManager(src/coreclr/tools/aot/crossgen2/Program.cs:578-593), and the code generator emitsPgoInstrumentationDatawhenever that value is true. A direct or user-supplied--embed-pgo-dataon a Wasm target can therefore still embed the section and reproduce the failure, contrary to the stated Wasm-specific switch behavior. Make Crossgen2 ignore the option when the resolved target architecture isTargetArchitecture.Wasm32(or otherwise enforce that invariant in the compiler), rather than relying only on its callers.
<CrossGenDllCmd Condition="'$(UsingToolIbcOptimization)' != 'true' and '$(EnableNgenOptimization)' == 'true' and '$(PublishReadyToRunContainerFormat)' != 'wasm'">$(CrossGenDllCmd) --embed-pgo-data</CrossGenDllCmd>
src/tests/Common/CLRTest.CrossGen.targets:379
- The Windows branch has the same gap as the Unix branch:
CrossGen2OutputFormatis not set for WASI, soCrossGen2SynthesizePgostill emits--embed-pgo-datawhenTargetArchitectureiswasm. Keep both script variants keyed off the target architecture.
if not "$(CrossGen2OutputFormat)"=="wasm" (
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Direct Crossgen2 invocations can still embed PGO for Wasm, and regression coverage is missing.
Review details
Suppressed comments (5)
Previously missed (1) — in code that hasn't changed since the last review.
src/coreclr/tools/r2rtest/Crossgen2Runner.cs:160
- This only suppresses the flag in the r2rtest command-line builder; Crossgen2 itself still honors
--embed-pgo-datafor Wasm.Program.cspasses the option directly toProfileDataManager, andReadyToRunCodegenNodeFactoryemits the instrumentation section whenever it is true, with no Wasm guard. Thus a directcrossgen2 --targetarch:wasm --embed-pgo-data -m:...invocation can still produce embedded PGO, contrary to the stated Wasm-specific behavior. Force the option off in Crossgen2 for Wasm targets (or narrow the documented contract).
eng/codeOptimization.targets:40
- This changes ReadyToRun Wasm builds to consume MIBC files without embedding their synthesized PGO data, but the PR adds no regression test or output assertion for that behavior. Add coverage that compiles a Wasm R2R image with MIBC input and verifies both that the profile guides compilation and that the generated image has no embedded PGO section; otherwise this build-wide change can regress silently.
<PropertyGroup Condition="'$(IncludeMibcFilesInReadyToRun)' == 'true' and '$(TargetArchitecture)' != 'wasm'">
<PublishReadyToRunCrossgen2ExtraArgs>$(PublishReadyToRunCrossgen2ExtraArgs);--embed-pgo-data</PublishReadyToRunCrossgen2ExtraArgs>
eng/codeOptimization.targets:40
- These guards only stop this MSBuild path from emitting the option; Crossgen2 itself still honors
--embed-pgo-datafor Wasm.Program.cspassesGet(_command.EmbedPgoData)directly intoProfileDataManager, andReadyToRunCodegenNodeFactoryaddsPgoInstrumentationDatawhen it is true, without checking the target architecture. Therefore a directcrossgen2 --targetarch:wasm --embed-pgo-data -m:...invocation still embeds PGO, which contradicts the stated switch behavior. Enforce the Wasm check inside Crossgen2 (and test it), or narrow the documented contract to these callers.
<PropertyGroup Condition="'$(IncludeMibcFilesInReadyToRun)' == 'true' and '$(TargetArchitecture)' != 'wasm'">
<PublishReadyToRunCrossgen2ExtraArgs>$(PublishReadyToRunCrossgen2ExtraArgs);--embed-pgo-data</PublishReadyToRunCrossgen2ExtraArgs>
src/coreclr/crossgen-corelib.proj:166
- This guard is keyed on the output container format, but the Wasm decision is based on the target architecture elsewhere in this change. Crossgen2 also normalizes a PE format to Wasm when
--targetarch:wasmis used (src/coreclr/tools/aot/crossgen2/Program.cs:450-454), so a Wasm corelib build with an empty or overridden non-Wasm format can still append--embed-pgo-dataand emit the PGO section. Key this condition on$(TargetArchitecture) != 'wasm'instead.
<CrossGenDllCmd Condition="'$(UsingToolIbcOptimization)' != 'true' and '$(EnableNgenOptimization)' == 'true' and '$(PublishReadyToRunContainerFormat)' != 'wasm'">$(CrossGenDllCmd) --embed-pgo-data</CrossGenDllCmd>
src/coreclr/crossgen-corelib.proj:166
- The PR description lists documenting the Wasm-specific
--embed-pgo-databehavior, but this diff contains no documentation or Crossgen2 help update explaining that MIBC inputs remain enabled while embedding is disabled. Please add the promised documentation or correct the description so the user-facing contract is not left implicit.
<CrossGenDllCmd Condition="'$(UsingToolIbcOptimization)' != 'true' and '$(EnableNgenOptimization)' == 'true'">$(CrossGenDllCmd) -m:$(MergedMibcPath)</CrossGenDllCmd>
<CrossGenDllCmd Condition="'$(UsingToolIbcOptimization)' != 'true' and '$(EnableNgenOptimization)' == 'true' and '$(PublishReadyToRunContainerFormat)' != 'wasm'">$(CrossGenDllCmd) --embed-pgo-data</CrossGenDllCmd>
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Crossgen2 now ignores
--embed-pgo-datafor Wasm targets while continuing to use MIBC profile data to guide compilation.Changes
Note
This description was generated with GitHub Copilot.