Uh oh!
There was an error while loading. Please reload this page.
Update the BDN version to use 0.13.1.1819. - #2532
Merged
Merged
Conversation
LoopedBard3
commented
Jul 19, 2022
MemberAuthor
This should be merged if tests pass and centos tests don't regress. |
caaavik-msft
approved these changes
Jul 19, 2022
adamsitnik
commented
Jul 19, 2022
Member
@LoopedBard3 thank you for updating BDN! I can see that the NativeAOT leg is still failing: DetailsIt looks like ilc failed with OOM (exited with code 137)? cc @MichalStrehovsky |
MichalStrehovsky
commented
Jul 19, 2022
Member
Yup, ILC was killed by the oom killer. How much RAM do the Helix machines where this runs have? Compilation is known to require a lot of RAM. We also run with server GC because it makes things compile 10% faster than workstation GC, but the GC heuristics in server GC are tuned for eating as much RAM as possible. I don't know how that works in systems that overpromise the amount of memory available and then randomly kill processes when they want to use the promised RAM. BenchmarkDotNet has a rather large dependency closure. We could try to tweak it - it should bring the memory use down. dotnet/BenchmarkDotNet#2020 was just me throwing up hands and not wanting to deal with specifying what exactly needs to be kept. I also just merged dotnet/runtime#72430 that might help a little bit. |
adamsitnik
commented
Jul 19, 2022
Member
This compilation worked fine for a while, and I wonder what changed that? In the past we got a huge compilation perf boost by setting: In the log I can see the following warning:
@MichalStrehovsky does it mean that it's already being ignored, and we are now rooting the application assembly again? |
MichalStrehovsky
commented
Jul 19, 2022
Member
I made BenchmarkDotNet assembly fully rooted in my BDN pull request - so it might be from that. We were trimming it previously, but this broke once ILC started trimming fields (we would never trim fields previously).
The |
Member
The contents of Details
That is correct: @MichalStrehovsky is it possible to root specific types using attribute annotations? I took a look at BDN code and making it fully trimmable would take some time + we don't need it in most of the cases (host process is JIT, benchmark process is AOT and it uses very few of all our APIs) Edit: by rooting type I mean keeping all it's fields and properties metadata |
MichalStrehovsky added a commit
to MichalStrehovsky/BenchmarkDotNet
that referenced
this pull request
Jul 20, 2022
Another attempt at fixing the problem I tried to fix in dotnet#2020. In that pull request, I reflection-rooted all of BenchmarkDotNet, but that looks like it increases the size of the closure too much (dotnet/performance#2532). So here I'm rolling that part back and annotating enough of the `Characteristic` APIs to make it so that we shouldn't run into the original problem. I basically added `<EnableTrimAnalyzer>true</EnableTrimAnalyzer>` to the BechmarkDotNet project, looked at the trimming warning in https://github.com/dotnet/BenchmarkDotNet/blob/63e28c100a42a6492d09a0b93a8a4c141061bb0d/src/BenchmarkDotNet/Characteristics/CharacteristicHelper.cs#L49-L68 where we were getting the crash and kept getting annotations until it stopped generating new warnings. We now have an annotation in a spot that should make sure the `Job` class has all fields and properties kept (it's used with one of the annotated `Create` methods). There are more trimming warnings that I didn't try to solve because they're unrelated to the problem at hand.
MichalStrehovsky
commented
Jul 20, 2022
Member
Took a stab at it in dotnet/BenchmarkDotNet#2046. |
adamsitnik pushed a commit
to dotnet/BenchmarkDotNet
that referenced
this pull request
Jul 20, 2022
* Make a bit of BenchmarkDotNet trimmable Another attempt at fixing the problem I tried to fix in #2020. In that pull request, I reflection-rooted all of BenchmarkDotNet, but that looks like it increases the size of the closure too much (dotnet/performance#2532). So here I'm rolling that part back and annotating enough of the `Characteristic` APIs to make it so that we shouldn't run into the original problem. I basically added `<EnableTrimAnalyzer>true</EnableTrimAnalyzer>` to the BechmarkDotNet project, looked at the trimming warning in https://github.com/dotnet/BenchmarkDotNet/blob/63e28c100a42a6492d09a0b93a8a4c141061bb0d/src/BenchmarkDotNet/Characteristics/CharacteristicHelper.cs#L49-L68 where we were getting the crash and kept getting annotations until it stopped generating new warnings. We now have an annotation in a spot that should make sure the `Job` class has all fields and properties kept (it's used with one of the annotated `Create` methods). There are more trimming warnings that I didn't try to solve because they're unrelated to the problem at hand.
| <_BenchmarkDotNetSourcesN>$([MSBuild]::NormalizeDirectory('$(BenchmarkDotNetSources)'))</_BenchmarkDotNetSourcesN> | ||
| </PropertyGroup> | ||
| <ItemGroup Condition="'$(BenchmarkDotNetSources)' == ''"> | ||
| <PackageReference Include="BenchmarkDotNet" Version="0.13.1.1786" /> |
Member
There was a problem hiding this comment.
@LoopedBard3 this are the only changes we need, please revert the changes to other files.
Full historical background:
- Precisely track reflected on fields runtime#70546 introduced a change to NativeAOT which removed metadata for fields that were not reflected using annotations. This broke BDN which was missing the annotations. Opt out of metadata trimming BenchmarkDotNet#2020 solved the problem by enforcing the compiler to keep all metadata for BDN, but at a cost of doing more work. This resulted in OOM (
OutOfMemoryException) for our CI as the build agents have simply too little memory. Make a bit of BenchmarkDotNet trimmable BenchmarkDotNet#2046 introduced a proper fix and made this particular problem go away by applying the right annotations. - Add array initialization optimization roslyn#62392 introduced a regression in Roslyn, which started using
GC.AllocateUninitializedArrayfor the new array operator (new byte[]). This API is not supported by NativeAOT and started causing new failures that we have observed after you updated the version. This was reported in Major performance regression of the array initialization pattern roslyn#62864 and reverted in Revert "Add array initialization optimization" roslyn#62868. In theory all we need to do now is to bump BDN version and wait until Roslyn changes flow into SDK.
Contributor
There was a problem hiding this comment.
I reverted the other files, hope that's OK @LoopedBard3 .
| runCategories: 'runtime libraries' | ||
| channels: | ||
| - nativeaot7.0 | ||
| variables: |
Member
There was a problem hiding this comment.
Applying a fix to yml file solves the problem for the CI, but not for the users who run the benchmarks locally using the python script (this includes our monthly runs).
In case you need to disable some AOT benchmarks in the future you can use AotFilter attribute like here:
This works for all scenarios.
danmoseleyforce-pushed
the
UpdateBDNto13.1.1819
branch
from
July 27, 2022 21:04
05a19d4 to
5191aeaComparedanmoseley
commented
Jul 27, 2022
Contributor
failures now are consistent with note above that we are waiting on a new Roslyn |
danmoseley
commented
Jul 27, 2022
Contributor
@LoopedBard3 How are dependencies (the SDK) ingested? I dont see automated PR's for this. |
adamsitnik
commented
Jul 28, 2022
Member
The python scripts ask dotnet install scripts to provide latest version. |
adamsitnik
approved these changes
Jul 28, 2022
adamsitnik
left a comment
Member
There was a problem hiding this comment.
LGTM, thank you for working on this @LoopedBard3 !
I am going to merge it now so as soon as Roslyn fix flows into the installer the NativeAOT CI leg becomes green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updates BDN version to 0.13.1.1819, per #2496