Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial work porting native diagnostic server library to C library potentially shared between runtimes. #41872

Merged

Conversation

lateralusX
Copy link
Member

@lateralusX lateralusX commented Sep 4, 2020

PR handling port of Diagnostic Server C++ library from CoreCLR into a C library that can be shared between Mono as well as CoreCLR runtime. Port follows same guidelines setup for event pipe port #34600. Diagnostic server library is currently hosted as part of event pipe library but hosting its own runtime shim as well as source files (so could be split into separate library if ever make sense). Diagnostic Server have dependencies on event pipe library (and reuse part of event pipe runtime shim from its own shim).

This is the first PR getting the code from diagnostic server codebase over to C library. Once that is done there will be follow up PR's starting to enabling more of the CoreCLR tests suites over event pipe currently depending on diagnostic server and connection between diagnostic server and event pipe library.

Diagnostic server processinfo, https://github.com/dotnet/runtime/tree/master/src/tests/tracing/eventpipe/processinfo, test pass running on Windows/Linux Mono.

eventpipe buffersize, https://github.com/dotnet/runtime/tree/master/src/tests/tracing/eventpipe/buffersize, test pass running on Windows/Linux Mono, connecting diagnostic server with eventpipe and IPC test clients and parsers.

TODO's before this PR can be merged:

  • Fix all builds (currently only validated on Windows).
  • Port POSIX PAL.
  • Get pass on process info on Linux/Mac enabling the test as part of PR.
  • Implement WriteEvent icall as well as starting IPC streaming thread in event pipe library.
  • Get pass on one test consuming event pipe data over IPC.

@MattGal
Copy link
Member

MattGal commented Sep 23, 2020

@lateralusX please investigate your tests locally before submitting more changes.

This change caused hundreds of 15 minute timeouts and also seems like it may have taken down machines during the run (logs with only Attempt.2 or higher mean that attempt 1 may have done something tragic to the box)

Example log seems to show the timeout comes from a hang during test discovery; this should be reproducible locally using a Centos.7 docker container if you don't have a local setup.

===========================================================================================================
~/work/B6A309CE/w/BEEC0A79/e ~/work/B6A309CE/w/BEEC0A79/e
  Discovering: System.ObjectModel.Tests (method display = ClassAndMethod, method display options = None)

@lateralusX
Copy link
Member Author

lateralusX commented Sep 23, 2020

Will run complete test suite on at least local Linux install before pushing more commits towards this branch. I need to get hold of a local OSX to run the OSX test lanes locally as well, but I believe the timeout issue looks related to the once on Linux, so solving it there might solve the OSX timeouts as well.

@lateralusX lateralusX removed the NO-REVIEW Experimental/testing PR, do NOT review it label Sep 29, 2020
@lateralusX lateralusX changed the title [WIP] Initial work porting native diagnostic server library to C library potentially shared between runtimes. Initial work porting native diagnostic server library to C library potentially shared between runtimes. Sep 29, 2020
@lateralusX lateralusX force-pushed the lateralusX/diagnostic-server-master branch from 6db3988 to 5e53049 Compare September 30, 2020 09:55
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

LGTM, though i primarily focused on the overall structure, and the Mono bits

The DS protocol bits look plausible, too, but i'm not qualified to spot discrepancies from the coreclr implementation.

src/mono/mono/eventpipe/ep-rt-mono.c Outdated Show resolved Hide resolved
src/mono/mono/eventpipe/ep-rt-mono.c Show resolved Hide resolved
src/mono/mono/eventpipe/ds-ipc-posix.c Outdated Show resolved Hide resolved
src/mono/mono/eventpipe/ds-rt-mono.c Outdated Show resolved Hide resolved
src/mono/mono/eventpipe/ds-rt-mono.h Show resolved Hide resolved
src/mono/mono/eventpipe/ds-server.c Show resolved Hide resolved
library that can be shared between Mono as well as CoreCLR runtime.

Port follows same guidelines setup for event pipe port dotnet#34600.
Diagnostic server library is currently hosted as part of event pipe
library but hosting its own runtime shim as well as source files
(so could be split into separate library if ever make sense).
Diagnostic Server have dependencies on event pipe library
(and reuse part of event pipe runtime shim from its how shim).

This is the first PR getting the code from diagnostic server codebase
over to C library.
@lateralusX lateralusX force-pushed the lateralusX/diagnostic-server-master branch from 013915f to 00955f4 Compare October 2, 2020 12:34
@lateralusX lateralusX marked this pull request as ready for review October 5, 2020 07:23
@lateralusX lateralusX merged commit f63b6f0 into dotnet:master Oct 5, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
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