Runtime: Add feature flag which controls Event Tracing support

[marek-safar]

On some workloads ETW is not supported at all, on some we'd like to disable it by default due to size impact. There is not "feature flag" to do it right now.

We should introduce something like RuntimeFeature.IsEvenTracingSupported to allow illinker to remove any event tracing code and metadata automatically.

This flag would need to be used to guard any EventSource like calls and attributes but that should be quite straight forward code replace in the framework. The ETW today mostly relies on instance IsEnabled method check which is not suitable for substitution as illinker as it has several overloads and would require all EventSource derived constructors to be kept with their dependencies.

@eerhardt @vitek-karas

[msftbot[bot]]

Tagging subscribers to this area: @vitek-karas, @swaroop-sridhar
Notify danmosemsft if you want to be subscribed.

[stephentoub]

Does that mean that every logging site would now need even more boilerplate:
C# if (RuntimeFeature.IsEventTracingSupported && MyEventSource.Log.IsEnabled) { ... actual logging ... }
? The existing guard is annoying enough, but largely unavoidable (especially if the actual call incurs cost due to computing args); it'd be really good if we could come up with a way to avoid needing the extra clause at every logging site. Some libraries do a ton of logging and it'd be nice to avoid all the extra cruft.

[stephentoub]

For example, maybe we move to a static IsEnabled which does the runtime feature check, along with a lazily-initialized Log? Or maybe there's an attribute-based scheme where we can put some attribute on MyEventSource or Log or even better the base IsEnabled that ties it to this runtime feature from the linker's perspective?

[vitek-karas]

I think we should do this in two layers:

• The implementation - where we would make sure that if event source is turned off, the public API is still there, but inside it doesn't do anything - and none of the code of the internal implementation gets pulled in. This can be done with the static property as suggested above.
• The call sites - once we have the above this is a little less important as the size impact should not be that big. But it would still be nice to have that. If we can solve https://github.com/mono/linker/issues/1243 then it should be possible to do this without modifying the callsites.

Note that we need the implementation guards regardless as we can't rely on all callsites to correctly wrap the call with the If(enabled).

[msftbot[bot]]

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
Notify danmosemsft if you want to be subscribed.

[marek-safar]

The importance of the static property is that gives us the most savings. Relying on instance version of IsEnabled is not great because it means we have to keep whole constructors chain for every custom EventSource and these constructors can bring dependencies and so on.

[eerhardt]

@marek-safar - just so I understand your proposal: are you saying we should change all the callsites to EventSource (both in our dotnet/runtime code, and any external library) and wrap them in a new check for this new static property?

On source.dot.net there are ~550 calls to EventSource.IsEnabled() and EventSource.IsEnabled(EventLevel, EventKeywords). This seems like too many changes, plus the recommendation for external libraries to change their pattern as well.

[tarekgh]

CC @noahfalk

[marek-safar]

Is the expectation that code like
https://github.com/dotnet/runtime/blob/296c82d97d4f146dd4e38973a141965377cf18c0/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/TaskContinuation.cs#L626 with be all "automatically" removed ?

I think we have to review all calls and if we are going to do that we should consider changing them to be fully EventSource "free". One can say that keeping the types, their static and instance ctors and the field is small footprint but it also makes few things simpler.

[omariom]

The new property could be integrated into IsEnabled methods
JIT is smart enough to recognize that.

There are some leftovers but that is JIT's concern.

[stephentoub]

JIT is smart enough to recognize that.

The primary concern driving this discussion isn't actually about the resulting asm after the JIT is done with it: it's IL binary size.

(But, yes, we'll then want whatever we do here to work well when the code isn't eliminated prior to JIT'ing.)

[omariom]

aah.. I now see illinker is mentioned.
Then may be illinker could be taught the tricks JIT can do?

[noahfalk]

I like @vitek-karas split of implementation and callsite.

If or when we get to the callsite portion it was mentioned that inlining an instance method was hard because we'd have to handle nullability checks. One possibility would be to implement an explicit substitution feature rather than inlining. For example a configuration rule for the linker could be "if the IL has a call to EventSource.IsEnabled(), replace that with the expression false."