Fix DiagnosticSource to work with NativeAOT by eerhardt · Pull Request #76109 · dotnet/runtime

eerhardt · 2022-09-23T23:13:25Z

There were 2 problems:

The use of MakeGenericType doesn't work when a property is a ValueType. An app will crash when a listener is enabled and DiagnosticSourceEventSource tries writing values.
The properties on KeyValuePair were not being preserved correctly, so the Arguments of the DiagnosticSourceEventSource methods were not being serialized correctly.

Add test (and infrastructure) to ensure DiagnosticSource works in a NativeAOT app

Remaining Tasks:

Hook the new NativeAotTests up to a CI leg. @MichalStrehovsky @joperezr - thoughts on which leg to hook them up to?

There were 2 problems: 1. The use of MakeGenericType doesn't work when a property is a ValueType. An app will crash when a listener is enabled and DiagnosticSourceEventSource tries writing values. 2. The properties on KeyValuePair were not being preserved correctly, so the Arguments of the DiagnosticSourceEventSource methods were not being serialized correctly. Add test (and infrastructure) to ensure DiagnosticSource works in a NativeAOT app Fixdotnet#75945

ghost · 2022-09-23T23:13:30Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2022-09-23T23:13:52Z

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

There were 2 problems:

The use of MakeGenericType doesn't work when a property is a ValueType. An app will crash when a listener is enabled and DiagnosticSourceEventSource tries writing values.
The properties on KeyValuePair were not being preserved correctly, so the Arguments of the DiagnosticSourceEventSource methods were not being serialized correctly.

Add test (and infrastructure) to ensure DiagnosticSource works in a NativeAOT app

Fix #75945

Remaining Tasks:

Hook the new NativeAotTests up to a CI leg. @MichalStrehovsky @joperezr - thoughts on which leg to hook them up to?

Author:	eerhardt
Assignees:	-
Labels:	`area-System.Diagnostics.Tracing`, `new-api-needs-documentation`
Milestone:	-

tarekgh

Added a question and test suggestion. LGTM!

MichalStrehovsky · 2022-09-26T06:28:26Z

Cc @LakshanF

MichalStrehovsky

Thank you!

MichalStrehovsky · 2022-09-26T06:29:35Z

@MichalStrehovsky @joperezr - thoughts on which leg to hook them up to?

Maybe we could hook it up here:

https://github.com/dotnet/runtime/blob/main/eng/pipelines/coreclr/nativeaot-post-build-steps.yml

marek-safar · 2022-09-26T08:37:52Z

+ {
+ Type elemType = enumerableOfTType.GetGenericArguments()[0];
+#if NETCOREAPP
+ if (!RuntimeFeature.IsDynamicCodeSupported && elemType.IsValueType)


Would be better to have only RuntimeFeature.IsDynamicCodeSupported check and assert on elemType.IsValueType ?

I don't believe there is a valid assert we could put on elemType.IsValueType. It is valid for it to be either true or false. (Note my test that I added is IEnumerable<int>.)
The reason I'm checking for IsValueType is so the behavior/implementation remains the same for IEnumerable<RefType> - since it is possible to still call MakeGenericType if the Type being passed is a ref type.

eerhardt · 2022-09-26T22:35:47Z

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


@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?)

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.

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.

FYI - @noahfalk @tommcdon

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.

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

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.

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.

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.

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,

- Only run them in Release configuration - Suppress IL2026 warning

noahfalk

LGTM, thanks @eerhardt!

eerhardt · 2022-09-27T15:02:58Z

@MichalStrehovsky @jkotas - are the test failures an existing issue? I tried searching and I can't find any open issues for them.

runtime (Build Linux arm64 Release NativeAOT) failures are all System.IO.FileSystem.Tests, which just got enabled in #76146 2 days ago.

System.PlatformNotSupportedException : Operation is not supported on this platform.
at System.IO.Tests.FileSystemTest.mkfifo(String path, Int32 mode) + 0x28
at System.IO.Tests.File_ExistsAsDirectory.FalseForNonRegularFile() + 0x40
at System.IO.FileSystem!<BaseAddress>+0x11c8aa4
at System.Reflection.DynamicInvokeInfo.Invoke(Object, IntPtr, Object[], BinderBundle, Boolean) + 0xd4

runtime (Build windows arm64 Release NativeAOT) failures seem transient: The process cannot access the file 'D:\a\_work\1\s\artifacts\helix\tests\windows.AnyCPU.Release\System.Runtime.Tests.zip' because it is being used by another process.

jkotas · 2022-09-27T15:12:58Z

I have not seen failures like this.

"The process cannot access the file 'D:\a_work\1\s\artifacts\helix\tests\windows.AnyCPU.Release\System.Runtime.Tests.zip' because it is being used by another process." suggests that there is a build race condition.

Also, one of the test failures is:
System.DllNotFoundException : Unable to load shared library 'kernel32.dll' or one of its dependencies. In order to help diagnose loading problems, consider using a tool like strace.. It looks like a build race condition that mixes of Windows and Linux bits.`

Any chance that the changes in the test infrastructure you have made introduced this build race condition?

eerhardt · 2022-09-27T15:18:32Z

Ah, I see it now. The MSBuild property I added "TestNativeAot" conflicts with an existing MSBuild property with the same name, but to mean different things. I'll rename my new one.

Also, one of the test failures is:
System.DllNotFoundException : Unable to load shared library 'kernel32.dll' or one of its dependencies. In order to help diagnose loading problems, consider using a tool like strace..

Where did you see that error? I hadn't seen that one.

jkotas · 2022-09-27T15:21:43Z

Where did you see that error?

In this log: https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-76109-merge-fe6babe92f654ea086/System.IO.FileSystem.Tests/1/console.51c56558.log?helixlogtype=result

MichalStrehovsky

Looks great otherwise, thanks!

Set EventSourceSupport only on the projects that need it.

eerhardt · 2022-09-28T22:07:28Z

@noahfalk @tommcdon @MichalStrehovsky @jkotas - any thoughts on backporting this to net7? We can leave EventSourceSupport off by default. But if someone turns it on, any app that sends an Http request would be broken when a trace was running.

MichalStrehovsky · 2022-09-28T23:25:37Z

The failure mode is pretty bad, but it's mitigated by the fact that it's off by default. Someone needs to opt in and presumably would run into it immediately after opting in. The current theory is that opting into it wouldn't be a mainstream scenario for the appmodels we currently support with Native AOT.

Since it's not a mainstream scenario, I don't know if that meets the bar. It will more likely meet the bar if we have a customer report and then we can service it.

MichalStrehovsky · 2022-11-15T08:41:57Z

/backport to release/7.0

akoeplinger · 2022-11-16T11:42:09Z

@MichalStrehovsky can you try the backport again?

MichalStrehovsky · 2022-11-18T01:27:15Z

/backport to release/7.0

github-actions · 2022-11-18T01:27:27Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3493408951

eerhardt requested review from LakshanF, MichalStrehovsky, joperezr, noahfalk and vitek-karas September 23, 2022 23:13

ghost added area-System.Diagnostics.Tracing new-api-needs-documentation labels Sep 23, 2022

ghost assigned eerhardt Sep 23, 2022

eerhardt removed the new-api-needs-documentation label Sep 23, 2022

tarekgh reviewed Sep 23, 2022
View reviewed changes

Comment thread...es/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSourceEventSource.cs

tarekgh reviewed Sep 23, 2022
View reviewed changes

Comment thread...System.Diagnostics.DiagnosticSource/tests/NativeAotTests/DiagnosticSourceEventSourceTests.cs

tarekgh approved these changes Sep 23, 2022
View reviewed changes

MichalStrehovsky approved these changes Sep 26, 2022
View reviewed changes

marek-safar reviewed Sep 26, 2022
View reviewed changes

Enable new NativeAotTests in CI
ba624bf

eerhardt commented Sep 26, 2022
View reviewed changes

Fix NativeAOT tests
b633e7f
- Only run them in Release configuration - Suppress IL2026 warning

noahfalk approved these changes Sep 27, 2022
View reviewed changes

Fix test CI leg
3b4f5bd

vitek-karas approved these changes Sep 27, 2022
View reviewed changes

Set IlcFrameworkNativePath correctly
ccca518

Don't run NativeAot published app tests on OSX since it isn't supported
9aeca47

MichalStrehovsky reviewed Sep 28, 2022
View reviewed changes

Comment threadeng/testing/linker/SupportFiles/Directory.Build.props Outdated

MichalStrehovsky approved these changes Sep 28, 2022
View reviewed changes

Respond to PR feedback
15824c8
Set EventSourceSupport only on the projects that need it.

eerhardt merged commit 70b33f8 into dotnet:mainSep 28, 2022

eerhardt deleted the FixDiagnosticSourceNativeAot-main branch September 28, 2022 21:30

eerhardt mentioned this pull request Oct 1, 2022
System.Private.Xml AOT compatibility fixes #76473
Merged

ghost locked as resolved and limited conversation to collaborators Nov 4, 2022

MichalStrehovsky mentioned this pull request Nov 15, 2022
.NET Native AOT warnings from System.Net.Http #78367
Closed

github-actionsBot unlocked this conversation Nov 18, 2022

github-actionsBot mentioned this pull request Nov 18, 2022
[release/7.0] Fix DiagnosticSource to work with NativeAOT #78532
Merged

github-actionsBot locked as resolved and limited conversation to collaborators Nov 18, 2022

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

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

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

Uh oh!

Conversation

eerhardt commented Sep 23, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 23, 2022

Uh oh!

ghost commented Sep 23, 2022

Uh oh!

Uh oh!

Uh oh!

tarekgh left a comment

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky commented Sep 26, 2022

Uh oh!

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky commented Sep 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotasSep 26, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Size of code per namespace with EventSource enabled

Size of code per namespace with EventSource disabled

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noahfalk left a comment

Choose a reason for hiding this comment

Uh oh!

eerhardt commented Sep 27, 2022

Uh oh!

jkotas commented Sep 27, 2022

Uh oh!

eerhardt commented Sep 27, 2022

Uh oh!

jkotas commented Sep 27, 2022

Uh oh!

Uh oh!

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Uh oh!

eerhardt commented Sep 28, 2022

Uh oh!

MichalStrehovsky commented Sep 28, 2022

Uh oh!

MichalStrehovsky commented Nov 15, 2022

Uh oh!

akoeplinger commented Nov 16, 2022

Uh oh!

MichalStrehovsky commented Nov 18, 2022

Uh oh!

github-actionsBot commented Nov 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

eerhardt commented Sep 23, 2022•
edited
Loading

jkotasSep 26, 2022•
edited
Loading