Remove special handling of EventSource

[vitek-karas]

Linker has some special handling of EventSource types:
https://github.com/mono/linker/blob/55bd0ebd00ddbf2cd817314b1cf6824003cfb463/src/linker/Linker.Steps/MarkStep.cs#L1701-L1705

With the changes to support EventSource fully with annotations, this should be removed once the necessary framework changes are made.

[vitek-karas]

/cc @LakshanF

[vitek-karas]

Part of #1349

[LakshanF]

PR #2063 Reverted the earlier checkin and we need to revisit this

[LakshanF]

We cannot remove the special handling of EventSource nested classes easily due to the scenarios like the following

• A non-public EventSource is defined in a library
• No references to nested classes of the EventSource is made inside the library
• There is a reference to the EventSource externally that requires the EventSource manifest to be generated. (the nested types like the Keywords class are needed to generate the manifest).

Specific example for the above scenario is the internal NetEventSource in System.Net.Mail.dll. This test case shows the below error path if we don't have this special handling and remove the nested class like "Keywords",

System.ArgumentException : Use of undefined keyword value 0x1 for event Info.

 at System.Diagnostics.Tracing.ManifestBuilder.ManifestError(String msg, Boolean runtimeCritical) in /_/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs:line 5529
   at System.Diagnostics.Tracing.ManifestBuilder.AppendKeywords(StringBuilder sb, UInt64 keywords, String eventName) in /_/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs:line 5932
   at System.Diagnostics.Tracing.ManifestBuilder.StartEvent(String eventName, EventAttribute eventAttribute) in /_/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs:line 5383
   at System.Diagnostics.Tracing.EventSource.CreateManifestAndDescriptors(Type eventSourceType, String eventSourceDllName, EventSource source, EventManifestOptions flags) in /_/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs:line 3369
   at System.Diagnostics.Tracing.EventSource.GenerateManifest(Type eventSourceType, String assemblyPathToIncludeInManifest, EventManifestOptions flags) in /_/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs:line 401
   at System.Diagnostics.Tracing.EventSource.GenerateManifest(Type eventSourceType, String assemblyPathToIncludeInManifest) in /_/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs:line 371
   at System.Net.Mail.Tests.LoggingTest.EventSource_ExistsWithCorrectId() in /_/src/libraries/System.Net.Mail/tests/Functional/LoggingTest.cs:line 25
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture) in /_/src/mono/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs:line 378

[MichalStrehovsky]

Is this being stripped during library build?

This sounds like an analysis hole with any DAM placed on classes when illink is running in the special "library mode" (and doesn't have whole program view).

If illink is running in the library mode, it shouldn't wait to see if GetType is ever called on something that was annotated. It should mark things eagerly whenever a subclass of an annotated class is seen.

[LakshanF]

Yes, specifically for the System.Net.Mail.dll with the -a FILE library option,

-a "C:\Work\core\runtime\artifacts\obj\System.Net.Mail\net6.0-windows-Release\PreTrim/System.Net.Mail.dll" library

[vitek-karas]

Can I just repeat that I hate the library mode ...

I think I agree with Michal. The easiest way to implement this would be to add a new CodeOptimization which will enable/disable the "wait for GetType call".

[marek-safar]

We cannot remove the special handling of EventSource nested classes easily due to the scenarios like the following

I think we should move it to the special code path we have for other such cases in library mode (e.g. AssemblyRootMode.Library). We don't need any new CodeOptimization just move the logic there.

[LakshanF]

Just handling this on library mode alone might have a larger scope impact than just on type hierarchy (e.g. the EventSource special casing as impacted here). Specifically, it looks like we need to relax TypePreserveMembers.Visible to include nested types more than what is currently covered. @vitek-karas's new CodeOptimization value can limit to the Type hierarchy feature

[marek-safar]

That's not what I meant. You keep the same logic that does the selection today but instead of the run on every Mark it runs only for types that have Library mode visibility.

[LakshanF]

Sorry for misunderstanding. To be specific, do you mean to move the EventSource special handling code to be under the library mode?

[marek-safar]

You will probably have to refactor the code a bit but yes. Essentially adding something like the code below to https://github.com/mono/linker/blob/main/src/linker/Linker.Steps/MarkStep.cs#L2537

if ((members & TypePreserveMembers.Library) != 0 && f.IsStatic && BCL.EventTracingForWindows.IsProviderName (f.DeclaringType.Name) (f)) {
	MarkField (f, di);
	continue;
}

[vitek-karas]

I don't like that approach. I would much rather implement what Michal suggested. Basically in library mode, the type hierarchy marking will happen regardless of us finding the "usage" for it.
In a way it can be seen as another public API - and so library mode should "root" it just like it roots other things.

I don't think this should be in any way specific to event source.

[marek-safar]

Basically in library mode, the type hierarchy marking will happen regardless of us finding the "usage" for it.

I didn't measure it but I expect this to make the mode useless because most libraries build savings comes from removing internal infrastructure types which are needed only for a single platform.

It won't also help with the case of private or internal events though not sure how common they are in libraries build.

[vitek-karas]

Note that the type still has to be referenced by something in the library. By "usage" I meant that linker sees variable.GetType() where variable is statically of the annotated type (EventSource in this case).

Private or internal events are actually the case where this should help - they are referenced by the library code, but some of the members on them are not references (since they're only accessed via reflection from the EventSource implementation). In order to keep them correctly working, we need to preserve these additional members. The way that works in app-mode is that we apply the annotation (since we see the GetType call inside EventSource). In the case of library-mode we don't apply the annotation because we don't see the GetType call - which is the problem here - and also the fix.

[LakshanF]

The fix seems reasonable enough with the library optimization mode and only applied if a base type has annotations.

[LakshanF]

Some results from runtime build.cmd, specifically from target ILLinkTrimSharedFramework since ILLinkTrimOOBAssemblies does not trim its assemblies in the library mode in build.cmd.

• Debug mode (around 67K or so difference)

With Fix: 
165 File(s)     22,114,304 bytes

With existing EventSource special handling: 
165 File(s)     22,046,720 bytes

The differences are because we apply the DAM for internal derived types. For example,

• In EventSource, its DynamicallyAccessedMemberTypes.All and the size differences are due to internal types like NetEventSource
• In DataSet, its more restricted.