Skip to content
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

[wasm] Re-enable source generator tests failing due to OOM #60701

Merged
merged 9 commits into from
Nov 2, 2021

Conversation

radical
Copy link
Member

@radical radical commented Oct 20, 2021

This adds support for setting per-project optimization flags, and sets them for these projects so they don't OOM.

  • Microsoft.Extensions.Logging.Generators.Roslyn3.11.Tests
  • Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests
  • System.Text.Json.SourceGeneration.Roslyn3.11.Unit.Tests
  • System.Text.Json.SourceGeneration.Roslyn4.0.Unit.Tests
  • System.Text.RegularExpressions.Generators.Tests

Some individual ones are disabled due to
#58226, and #60899 .

Additionally, if a AOT build fails, then on linux it dumps the last few lines from dmesg, to help identify it was an oom-kill.

Fixes #51961 .

@radical radical force-pushed the misc-wasm-fixes branch 2 times, most recently from d1b85d7 to 9cbdbf3 Compare October 26, 2021 05:35
@radical radical added the arch-wasm WebAssembly architecture label Oct 26, 2021
@ghost
Copy link

ghost commented Oct 26, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

WIP!

Fixes #51961, and #60048

Author: radical
Assignees: -
Labels:

arch-wasm, area-Build-mono

Milestone: -

.. path. This shows up at runtime as a cryptic `FS Error`, because it
fails to create the parent directory.
…options for avoiding OOM

.. and use a separate script template for AOT builds.
- `Microsoft.Extensions.Logging.Generators.Roslyn3.11.Tests`
- `Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests`
- `System.Text.Json.SourceGeneration.Roslyn3.11.Unit.Tests`
- `System.Text.Json.SourceGeneration.Roslyn4.0.Unit.Tests`
- `System.Text.RegularExpressions.Generators.Tests`

Some individual ones are disabled due to
dotnet#58226, and
dotnet#60899 .

Fixes dotnet#51961 .
@radical radical changed the title [wasm] Misc build improvements [wasm] Re-enable source generator tests failing due to OOM Oct 27, 2021
@radical radical marked this pull request as ready for review October 27, 2021 23:21
eng/Versions.props Outdated Show resolved Hide resolved
@@ -38,12 +38,12 @@
</PropertyGroup>

<PropertyGroup Condition="'$(BuildAOTTestsOnHelix)' == 'true'">
<_AOTBuildCommand>dotnet msbuild publish/ProxyProjectForAOTOnHelix.proj /bl:$XHARNESS_OUT/AOTBuild.binlog</_AOTBuildCommand>
<_AOTBuildCommand>BuildAOT publish/ProxyProjectForAOTOnHelix.proj $XHARNESS_OUT/AOTBuild.binlog</_AOTBuildCommand>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to clearly communicate that this is a function defined in a script, and not a command? (Will it work on Windows?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm how about naming it _buildAOTFunc? or just _buildAOT even.
re:windows, we are not running these on windows yet. And I have a separate PR for that, where I can add the cmd/ps1 equivalent of the new template script.

Copy link
Contributor

@kg kg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but it probably needs another review from someone more familiar with the nuances here

<PropertyGroup>
<_MainAssemblyPath Condition="'%(WasmAssembliesToBundle.FileName)' == $(AssemblyName) and '%(WasmAssembliesToBundle.Extension)' == '.dll'">%(WasmAssembliesToBundle.Identity)</_MainAssemblyPath>
<RuntimeConfigFilePath>$([System.IO.Path]::ChangeExtension($(_MainAssemblyPath), '.runtimeconfig.json'))</RuntimeConfigFilePath>
<EmccLinkOptimizationFlag Condition="'$(EmccLinkOptimizationFlag)' == ''">-Oz -Wl%252C-O0 -Wl%252C-lto-O0</EmccLinkOptimizationFlag>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An xml comment (doesn't need to be long) explaining why these specific flags were selected would be great here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave this one for @radekdoulik to add, since he set these originally.

@@ -371,7 +375,7 @@
DependsOnTargets="_WasmSelectRuntimeComponentsForLinking;_WasmCompileAssemblyBitCodeFilesForAOT;_WasmWriteRspFilesForLinking"
Returns="@(FileWrites)" >

<Message Text="Linking with emcc. This may take a while ..." Importance="High" />
<Message Text="Linking with emcc with $(EmccLinkOptimizationFlag). This may take a while ..." Importance="High" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Optional suggestion) Maybe put a prefix on these 3 stages like '2/3' to indicate how many more emcc steps are left in the build since each step is so slow

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, a AOT build looks like:

  AOT'ing 31 assemblies
  [1/30] System.ComponentModel.dll -> System.ComponentModel.dll.bc
  [2/30] System.Net.Primitives.dll -> System.Net.Primitives.dll.bc
...

  Compiling native assets with emcc with -Oz. This may take a while ...
  [1/3] pinvoke.c -> pinvoke.o [took 0.331s]
  [2/3] corebindings.c -> corebindings.o [took 0.353s]
...

  Compiling assembly bitcode files with -Oz ...
  [1/30] System.ComponentModel.dll.bc -> System.ComponentModel.dll.o [took 0.498s]
  [2/30] System.Net.Primitives.dll.bc -> System.Net.Primitives.dll.o [took 0.517s]
...

  <linking>

Adding another level of numbering might be confusing. Maybe some other way of communicating the same idea?

@radical radical merged commit 5a078b5 into dotnet:main Nov 2, 2021
@radical radical deleted the misc-wasm-fixes branch November 2, 2021 20:16
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
3 participants