Profiler attach over the diagnostics pipe#24670
Conversation
|
Diagnostics repo change is here: dotnet/diagnostics#274 I am working on testing on linux, I have tested on Windows already. |
davmason
changed the title
|
@davmason am I reading this correctly. In 3.0 both Linux and Windows will use this IPC mechanism to do attach? That will be very nice. Would this open doors to the EventPipe to Profiler API work as well? |
noahfalk
left a comment
noahfalk
left a comment
There was a problem hiding this comment.
A couple small followups, but looks good overall. Deleting lots of code is satisfying : )
| pClientData = nullptr; | ||
| } | ||
|
|
||
| hr = ProfilingAPIUtility::LoadProfilerForAttach(&profilerGuid, |
There was a problem hiding this comment.
As a second issue, its safe for now but @josalem will need to coordinate with you if he makes the diagnostic server multi-threaded. LoadProfilerForAttach doesn't appear safe to run in a multi-threaded environment without adding some additional synchronization that would let an attach request cleanly win/lose the attach race.
There was a problem hiding this comment.
Since this is the only place we would be loading a profiler from, I think all that would be necessary is to add a Crst around the call.
Now that I'm thinking about synchronization, it may race with a startup profiler too. I'll have to look in to it more tomorrow.
You are correct that it will enable attach for linux in 3.0, and replace the existing Windows attach mechanism as well. This will be the attach mechanism going forward and the old Windows only one is removed. It also has the benefit that any future platforms we decide to support will hopefully be a lot less costly to bring up. This work doesn't have any impact on the event pipe to profiler API bridge though. The issue there is not getting access to the EventPipe data (as your PR proved it is not a ton of work to expose the data from EventPipe to profilers), but there are issues with how to consume it from the profiler's point of view. We don't currently have a mechanism to have two different EventPipe sessions going, so the profiler and any diagnostic tools wouldn't be able to coexist. There is also the issue of decoding the events, if we just dump the raw encoding over the wire to the profiler then it becomes a contract and any change to the encoding breaks people. All that is to say there is still a fair amount of design work left and it will be post 3.0. |
davmason
force-pushed
the
profiler_attach
branch
4 times, most recently
from
257dae6 to
e7b9896
Compare
|
Kicked this off privately as https://dev.azure.com/dnceng/public/_build/results?buildId=197378 |
Remove the old windows only profiler attach mechanism and replace it with a cross plat implementation over the diagnostics pipe Commit migrated from dotnet/coreclr@bbde6ae
6 participants
CC @dotnet/dotnet-diag