Correctly apply EventSourceSupport feature switch to NativeRuntimeEventSource

[vitek-karas]

NativeRuntimeEventSource.ProcessEvent is called from native code in CoreCLR and as such is rooted in the descriptor. But if event source is disabled via feature switch it should not do anything.

[ghost]

Tagging subscribers to this area: @vitek-karas, @agocke
See info in area-owners.md if you want to be subscribed.

[ghost]

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

[eerhardt]

LGTM

[eerhardt]

(nit) since this is an instance method, could this use IsEnabled() instead?

[vitek-karas]

I actually don't know - the runtime event source is weird in many ways, so using IsSupported felt like the safe option here.

[tarekgh]

@noahfalk may advise better here. using IsEnabled looks the right way to check but I'll let @noahfalk comment on that.

[noahfalk]

I think both IsSupported or IsEnabled() would work fine, but in the interest of simplicity (and performance) I'd say leave it as IsSupported. I'd suggest adding a comment to pull this out when its time to do #43657. This change feels like the quick and dirty version, #43657 would be the good version that prunes far more code and eliminates the need for this change.

[noahfalk]

NativeRuntimeEventSource.ProcessEvent is called from native code in CoreCLR

Its been a while since I last looked at this code but that wasn't what I recalled? Looking at the code I see EventPipeEventDispatcher start a task that spins in a loop p/invoking to GetNextEvent. Is there another calling path I missed or is it this path that leads to the EventSource being rooted?

Ideally I'd recommend linking out the EventPipeEventDispatcher type entirely if your goal is to shrink code size when EventSource/EventListener isn't supported.

[vitek-karas]

This is probably more complicated then that. There's also this:

runtime/src/coreclr/src/System.Private.CoreLib/src/ILLink/ILLinkTrim.xml

Lines 8 to 11 in d701804

	<!-- Accessed via private reflection by an external tracing controller. -->
	<type fullname="System.Diagnostics.Tracing.EventPipe*" />
	<!-- The private Event methods are accessed by private reflection in the base EventSource class. -->
	<type fullname="System.Diagnostics.Tracing.NativeRuntimeEventSource" />

Which roots all of the EventPipe* types fully - which in turn will bring in the NativeRuntimeEventSource, but that is also rooted explicitly above. I don't understand this system well enough to be able to tell if it's OK to remove all of these types if EventSource is disabled. It would be really nice to be able to do that though - I tried trimming it away and it saves 60KB - that's actually very interesting size win.

I also verified that simple hello world console app still works with all of the EventPipe* trimmed away. There's also no direct dependency on anything EventPipe* from managed code (linker removed all of it). Most of NativeRuntimeEventSource also went away (there are some direct dependencies on its instance, but nothing else).

That said I don't know what native code dependencies there are and how the system will behave if the managed counterparts are not present.

[vitek-karas]

I created #43657 to track the somewhat wider problem of EventPipe behavior if we start removing parts of it.

[noahfalk]

That said I don't know what native code dependencies there are and how the system will behave if the managed counterparts are not present.

The design is intended to behave fine, in practice its possible we'd find some dependency slipped in unintentionally and we need to snip it. If you want help understanding anything when you are ready to work on #43657 just let me know and we can dig in. Thanks!