Skip to content

Fix DiagnosticSource to work with NativeAOT #76109

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.

Sign up for GitHub

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

Merged
eerhardt merged 7 commits into from
Sep 28, 2022
Merged

Fix DiagnosticSource to work with NativeAOT #76109

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1227d64
Fix DiagnosticSource to work with NativeAOT
eerhardt Sep 23, 2022
ba624bf
Enable new NativeAotTests in CI
eerhardt Sep 26, 2022
b633e7f
Fix NativeAOT tests
eerhardt Sep 27, 2022
3b4f5bd
Fix test CI leg
eerhardt Sep 27, 2022
ccca518
Set IlcFrameworkNativePath correctly
eerhardt Sep 27, 2022
9aeca47
Don't run NativeAot published app tests on OSX since it isn't supported
eerhardt Sep 27, 2022
15824c8
Respond to PR feedback
eerhardt Sep 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Enable new NativeAotTests in CI
  • Loading branch information
@eerhardt
eerhardt committed Sep 26, 2022
commit ba624bf2563c2a65475bb78eb44286bb3e4458e9
3 changes: 3 additions & 0 deletions eng/pipelines/coreclr/nativeaot-post-build-steps.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,6 @@ steps:
- ${{ if ne(parameters.osGroup, 'windows') }}:
- script: $(Build.SourcesDirectory)/src/tests/run.sh --runnativeaottests $(buildConfigUpper) ${{ parameters.archType }}
displayName: Run tests in single file mode

- script: $(Build.SourcesDirectory)$(dir)build$(scriptExt) -ci -arch ${{ parameters.archType }} $(_osParameter) -s libs.tests -c $(_BuildConfig) /p:TestAssemblies=false /p:TestNativeAot=true $(_officialBuildParameter) $(_crossBuildPropertyArg) /bl:$(Build.SourcesDirectory)/artifacts/log/$(buildConfigUpper)/NativeAotTests.binlog ${{ parameters.extraTestArgs }}
displayName: Run NativeAot Library Tests
3 changes: 3 additions & 0 deletions eng/testing/linker/SupportFiles/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,8 @@
<EnableTrimAnalyzer>false</EnableTrimAnalyzer>
<SuppressTrimAnalysisWarnings>true</SuppressTrimAnalysisWarnings>
<NoWarn>$(NoWarn);IL2121</NoWarn>

<!-- EventSourceSupport is disabled by default with PublishAot=true, so need to enable it here. -->
<EventSourceSupport>true</EventSourceSupport>
Copy link
Member Author

@eerhardt eerhardt Sep 26, 2022

Choose a reason for hiding this comment

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

@MichalStrehovsky @jkotas - with this change we now have tests ensuring EventSource works (at least in proc) with PublishAot. Do you think in .NET 8 we should re-enable EventSourceSupport=true by default? Or do you think we should keep it disabled by default going forward (for size savings?)

Copy link
Member

@jkotas jkotas Sep 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I would wait for feedback on the current .NET 7 default before doing more changes.

I think it makes sense to leave it off by default and turn it on only for server app-models in their SDKs.

Copy link
Member

@MichalStrehovsky MichalStrehovsky Sep 27, 2022

Choose a reason for hiding this comment

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

Agree with Jan. I kind of like the size savings. I would like to see feedback. Re-enabling it is an instant 30% size regression from 7.0 for hello world.

Copy link
Member Author

@eerhardt eerhardt Sep 27, 2022

Choose a reason for hiding this comment

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

FYI - @noahfalk @tommcdon

Copy link
Member

@noahfalk noahfalk Sep 27, 2022

Choose a reason for hiding this comment

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

Just curious, how big (in bytes) is 30%? I wouldn't have guessed the savings were such a high percentage but I don't know if that is because I overestimated the size of HelloWorld or underestimated the size of EventSource.

Copy link
Member

@MichalStrehovsky MichalStrehovsky Sep 27, 2022

Choose a reason for hiding this comment

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

Yes, we have a log and I'm hoping to do some offcycle work to visualize/diff them.

Copy link
Member

@brianrob brianrob Oct 3, 2022

Choose a reason for hiding this comment

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

I've love to see the results of this. Agreed with @noahfalk that this is more than I would have expected. It feels important to describe the diagnostic expectations for native AOT in .NET 8 so that we can determine how this decision fits in.

Copy link
Member

@MichalStrehovsky MichalStrehovsky Oct 4, 2022

Choose a reason for hiding this comment

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

There's a similar difference for PublishTrimmed as well - the size of managed code goes from 2,718,848 bytes to 3,003,008. So the cost is about 300 kB in IL terms. (The repro steps for both is basically just run dotnet publish -r win-x64 -c Release with -p:PublishTrimmed=true for trimming or -p:PublishAot=true for NativeAOT. Use -p:EventSourceSupport=false to enable/disable EventSource.)

For the NativeAOT case a quick breakdown is following.

Size of code per namespace with EventSource enabled

Namespace Size of code in bytes
System 414,414
System.Collections.Generic 246,832
System.Globalization 128,834
Internal.Runtime.TypeLoader 128,583
System.Diagnostics.Tracing 116,975
System.Text 92,958
Internal.TypeSystem 85,341
System.Reflection 40,905
System.Reflection.Runtime.TypeInfos 36,802
System.Collections.Concurrent 34,363
System.Reflection.Runtime.General 31,092
System.Numerics 30,384
System.Threading 27,273
Internal.Reflection.Execution 27,180
System.IO 26,659
System.Resources 25,769
System.Reflection.Runtime.MethodInfos 22,513
System.Runtime 21,400
System.Runtime.CompilerServices 20,287
System.Buffers 20,055

Size of code per namespace with EventSource disabled

Namespace Size of code in bytes
System 306,356
System.Collections.Generic 156,429
Internal.Runtime.TypeLoader 127,630
System.Globalization 107,047
System.Text 86,065
Internal.TypeSystem 85,341
System.Reflection 36,931
System.Reflection.Runtime.TypeInfos 35,576
System.Collections.Concurrent 34,363
System.Numerics 30,384
System.Reflection.Runtime.General 27,776
System.IO 26,299
Internal.Reflection.Execution 25,920
System.Resources 25,445
System.Runtime 21,336

(This is just namespaces with more than 20 kB of code)

Most of the cost is in transitive dependencies. EventSource has a big transitive closure and depends on a lot of framework code.

I can file an issue if there's interest in reducing the transtive closure.

Copy link
Member Author

@eerhardt eerhardt Oct 4, 2022

Choose a reason for hiding this comment

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

Most of the cost is in transitive dependencies. EventSource has a big transitive closure and depends on a lot of framework code.

This tells me that this is the upper bound of the size savings for having EventSource disabled. In a real world app that uses these transitive dependencies, they won't be able to be trimmed. So the size savings of disabling EventSource will be less. For example, 90 kB of size savings is coming from System.Collections.Generic which many apps will use. When the app uses those collections, they won't be able to be trimmed anymore, even with EventSource disabled.

Copy link
Member

@MichalStrehovsky MichalStrehovsky Oct 5, 2022

Choose a reason for hiding this comment

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

So the size savings of disabling EventSource will be less. For example, 90 kB of size savings is coming from System.Collections.Generic which many apps will use. When the app uses those collections, they won't be able to be trimmed anymore, even with EventSource disabled

That depends - when thinking about native code, one needs to think about instantiated generic code. For example the costs associated with System.Collections.Generic.Dictionary2<System.Type,System.Diagnostics.Tracing.TraceLoggingTypeInfo>` are exclusive to EventSource not matter what the app does.

It's easy to test out theories - just pick a bigger app and see the impact of EventSource. For apps the size of current ASP.NET, an extra 600 kB overhead is negligible either way,

</PropertyGroup>
</Project>
4 changes: 4 additions & 0 deletions eng/testing/linker/SupportFiles/Directory.Build.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
DependsOnTargets="BundleTestWasmApp"
Condition="'$(TargetArchitecture)' == 'wasm' And '$(TargetOS)' == 'browser'" />

<PropertyGroup Condition="'$(PublishAot)' == 'true'">
<ILCompilerTargetsPath>$(CoreCLRBuildIntegrationDir)Microsoft.DotNet.ILCompiler.SingleEntry.targets</ILCompilerTargetsPath>
</PropertyGroup>

<!-- Overriding these targets as these projects won't need to binplace -->
<Target Name="PublishTestAsSelfContained" />

Expand Down
1 change: 1 addition & 0 deletions eng/testing/linker/project.csproj.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
<IlcBuildTasksPath>{IlcBuildTasksPath}</IlcBuildTasksPath>
<IlcSdkPath>{IlcSdkPath}</IlcSdkPath>
<IlcFrameworkPath>{IlcFrameworkPath}</IlcFrameworkPath>
<CoreCLRBuildIntegrationDir>{CoreCLRBuildIntegrationDir}</CoreCLRBuildIntegrationDir>
</PropertyGroup>

<ItemGroup>
Expand Down
1 change: 1 addition & 0 deletions eng/testing/linker/trimmingTests.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@
.Replace('{IlcBuildTasksPath}', '$(CoreCLRILCompilerDir)netstandard/ILCompiler.Build.Tasks.dll')
.Replace('{IlcSdkPath}', '$(CoreCLRAotSdkDir)')
.Replace('{IlcFrameworkPath}', '$(MicrosoftNetCoreAppRuntimePackRidLibTfmDir)')
.Replace('{CoreCLRBuildIntegrationDir}', '$(CoreCLRBuildIntegrationDir)')
.Replace('{RuntimeHostConfigurationOptions}', '$(_runtimeHostConfigurationOptionsString)')
.Replace('{AdditionalProjectReferences}', '$(_additionalProjectReferencesString)')
.Replace('{RepositoryEngineeringDir}', '$(RepositoryEngineeringDir)')
Expand Down
20 changes: 13 additions & 7 deletions src/libraries/tests.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<!-- Build for NetCoreAppCurrent by default if no BuildTargetFramework is supplied or if not all configurations are built. -->
<TargetFramework>$([MSBuild]::ValueOrDefault('$(BuildTargetFramework)', '$(NetCoreAppCurrent)'))-$(TargetOS)</TargetFramework>
<!-- Filter ProjectReferences to build the best matching target framework only. -->
<FilterTraversalProjectReferences Condition="'$(TestTrimming)' != 'true'">true</FilterTraversalProjectReferences>
<FilterTraversalProjectReferences Condition="'$(TestTrimming)' != 'true' and '$(TestNativeAot)' != 'true'">true</FilterTraversalProjectReferences>
</PropertyGroup>

<PropertyGroup>
Expand All @@ -18,6 +18,7 @@
<TestAssemblies Condition="'$(TestAssemblies)' == ''">true</TestAssemblies>
<TestPackages Condition="'$(TestPackages)' == ''">false</TestPackages>
<TestTrimming Condition="'$(TestTrimming)' == ''">false</TestTrimming>
<TestNativeAot Condition="'$(TestNativeAot)' == ''">false</TestNativeAot>

<RunHighAOTResourceRequiringTestsOnly Condition="'$(RunHighAOTResourceRequiringTestsOnly)' == ''">false</RunHighAOTResourceRequiringTestsOnly>

Expand Down Expand Up @@ -590,6 +591,11 @@
Condition="'$(TestTrimming)' == 'true'"
AdditionalProperties="%(AdditionalProperties);SkipTrimmingProjectsRestore=true" />
<ProjectReference Include="@(TrimmingTestProjects)" />
<NativeAotTestProjects Include="$(MSBuildThisFileDirectory)*\tests\**\*.NativeAotTests.proj"
Exclude="@(ProjectExclusions)"
Condition="'$(TestNativeAot)' == 'true'"
AdditionalProperties="%(AdditionalProperties);SkipTrimmingProjectsRestore=true" />
<ProjectReference Include="@(NativeAotTestProjects)" />

<!-- wasm.build.tests are run on _WasmBuildTests job on CI, and with library tests locally. -->
<ProjectReference Include="$(RepoRoot)\src\mono\wasm\Wasm.Build.Tests\*.Tests.csproj"
Expand Down Expand Up @@ -760,17 +766,17 @@
Directories="$(AppBundleRoot)" />
</Target>

<!-- Restoring all trimming test projects upfront in one single call to RestoreTrimmingProjects
<!-- Restoring all trimming/NativeAot test projects upfront in one single call to RestorePublishedAppTestProjects
so as to avoid possible race conditions that could happen if we restore each individually. -->
<Target Name="RestoreTrimmingProjects"
<Target Name="RestorePublishedAppTestProjects"
BeforeTargets="Build"
Condition="'$(TestTrimming)' == 'true'">
<MSBuild Projects="@(TrimmingTestProjects)"
Condition="'$(TestTrimming)' == 'true' or '$(TestNativeAot)' == 'true'">
<MSBuild Projects="@(TrimmingTestProjects);@(NativeAotTestProjects)"
Targets="GetTrimmingProjectsToRestore">
<Output TaskParameter="TargetOutputs" ItemName="_TrimmingProjectsToRestore" />
<Output TaskParameter="TargetOutputs" ItemName="_PublishedAppTestProjectsToRestore" />
</MSBuild>

<MSBuild Projects="@(_TrimmingProjectsToRestore)"
<MSBuild Projects="@(_PublishedAppTestProjectsToRestore)"
Targets="Restore"
Properties="MSBuildRestoreSessionId=$([System.Guid]::NewGuid());Configuration=$(Configuration)" />
</Target>
Expand Down