Correctly apply EventSourceSupport feature switch to NativeRuntimeEventSource#43602
Conversation
…ntSource `NativeRuntimeEventSource.Process` 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.
vitek-karas
added
the
linkable-framework
|
Tagging subscribers to this area: @vitek-karas, @agocke |
| [NonEvent] | ||
| internal unsafe void ProcessEvent(uint eventID, uint osThreadID, DateTime timeStamp, Guid activityId, Guid childActivityId, ReadOnlySpan<byte> payload) | ||
| { | ||
| if (!IsSupported) |
There was a problem hiding this comment.
(nit) since this is an instance method, could this use IsEnabled() instead?
There was a problem hiding this comment.
I actually don't know - the runtime event source is weird in many ways, so using IsSupported felt like the safe option here.
There was a problem hiding this comment.
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.
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. |
|
This is probably more complicated then that. There's also this: Which roots all of the I also verified that simple hello world console app still works with all of the 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. |
|
I created #43657 to track the somewhat wider problem of EventPipe behavior if we start removing parts of it. |
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! |
ghost
locked as resolved and limited conversation to collaborators
5 participants
NativeRuntimeEventSource.ProcessEventis 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.