Move EventPipe environment variable parsing logic to native code #32516
Conversation
noahfalk
left a comment
noahfalk
left a comment
There was a problem hiding this comment.
Looks good, a few comments so far : )
josalem
left a comment
josalem
left a comment
There was a problem hiding this comment.
Just a couple comments. I think we should hold off on turning this code on (and the old code off) until we have the mutable EventPipe session work completed. That way it's not multi-phase. I think Noah is right about the ifdef. It will allow us to do local builds/testing until we finish the other work.
josalem
left a comment
josalem
left a comment
There was a problem hiding this comment.
Pending a clean CI, I think this looks good.
noahfalk
left a comment
noahfalk
left a comment
There was a problem hiding this comment.
There are a few memory leaks, otherwise LGTM
…m:sywhang/runtime into dev/suwhang/move-eventpipe-envvar-parsing
| }; | ||
| #endif // FEATURE_PERFTRACING | ||
|
|
||
| #if defined(HOST_UNIX) && (defined(FEATURE_EVENT_TRACE) || defined(FEATURE_EVENTSOURCE_XPLAT)) |
There was a problem hiding this comment.
@sywhang, we updated the build in #32746, so that user can build coreclr without FEATURE_EVENT_TRACE enabled. After rebasing my other PR #32800 branch against master, I am getting errors that XplatEventLoggerConfiguration on line 408 is not defined. The reason is that FEATURE_PERFTRACING is only set when FEATURE_EVENT_TRACE is enabled, so the class definition is omitted from the compilation.
One fix could be to change line 245 to #if defined(FEATURE_PERFTRACING) || defined(FEATURE_EVENTSOURCE_XPLAT) , (along with #32800 current changes), just wanted to confirm if you have any thoughts on such a fix?
Thanks!
There was a problem hiding this comment.
@am11 ah, sorry about that!
One fix could be to change line 245 to #if defined(FEATURE_PERFTRACING) || defined(FEATURE_EVENTSOURCE_XPLAT) , (along with #32800 current changes), just wanted to confirm if you have any thoughts on such a fix?
That sounds fine to me.
I will try to build without FEATURE_EVENT_TRACE before merging my PRs on EventPipe from future to prevent issues like that :)
There was a problem hiding this comment.
Thank you! My coreclr build on Android was failing at 11% and with that (two liner) patch (updated #endif comment as well), it has just succeeded (all green). 👍
I was wondering if we could get to run Android build even on weekly bases (AzDo scheduled jobs), that would help preserving the healthy state for this platform? It will also help validating this feature matrix. :)
ghost
locked as resolved and limited conversation to collaborators
6 participants
This is for #32514.
A spinoff work item from this is enabling SampleProfiler from startup. This is currently not possible because by the time we start an EventPipe session at startup, GCHandleManager has not been set up yet. I will file a separate issue on this (and work on it) once this PR is merged.