Skip to content

Conversation

@brianrob
Copy link
Member

@brianrob brianrob commented Jan 4, 2021

Recently, I've seen EventPipe overhead in ETW traces where EventPipe is disabled. Upon drilling down further, this shows up because if either ETW or EventPipe is enabled for an EventSource, then EventSource assumes that both are enabled. This results in pinvokes into native code for each logger to emit the event. When a logger is disabled, this pinvoke wasted work.

Given that we know the state of each logger, it's easy enough to check them individually before the pinvoke. This change does not extend this behavior to self-describing events because this code is quite different, and there isn't easy to consume state that will tell us which events are enabled for each source.

Copy link
Contributor

@josalem josalem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Do we have any data for what the before/after perf impact of this is? Based on your statement, my understanding is that a similar change for self-describing events would require changes in WriteMultiMerge as well as a means of passing in this state. Is that correct?

@brianrob
Copy link
Member Author

brianrob commented Jan 5, 2021

Thanks for the review @josalem. I don't have any before/after data. The improvement is likely to be low. Ultimately, what caused me to actually submit the PR is that it bugs me that we spend any time in EventPipe when it's disabled (or in ETW when it's disabled but EventPipe is enabled).

In order to implement this for self-describing events, you would need to keep track of the set of enabled keywords per EventProvider, such that as soon as the event is fired, you can & the declared keywords with the keywords enabled for each logger to figure out which logger(s) is/are enabled. Then, you could wrap some of the code in if statements, though honestly, a good chunk of the code must be executed even if only one of the loggers is enabled. The only places before you get to WriteEventRaw where you can save perf for self-describing is some of the EventPipe specific stuff like creating the event handle. Most everything else has to be done in either case. Then, when you get to WriteEventRaw, you can avoid the pinvoke by having EventProvider.WriteEventRaw no-op if the event is disabled (it would have to do the keywords check again).

My guess is that the self-describing path isn't nearly as worthwhile to implement as the manifest-based path, simply because the overhead of emitting a self-describing event is much higher, and so this wasted pinvoke is likely to get lost in the noise for self-describing events.

Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks!

@brianrob brianrob merged commit 2ef14c3 into dotnet:master Jan 5, 2021
@brianrob brianrob deleted the etw-eventpipe-overhead branch January 5, 2021 16:55
@ghost ghost locked as resolved and limited conversation to collaborators Feb 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants