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.

Sign up for GitHub

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

Jump to bottom

Tracking: Diagnostic Server and EventPipe C++ to C backport #36820

Closed
lambdageek opened this issue May 21, 2020 · 15 comments
Closed

Tracking: Diagnostic Server and EventPipe C++ to C backport #36820

lambdageek opened this issue May 21, 2020 · 15 comments
Assignees
@lambdageek @lateralusX @josalem
Labels
area-Diagnostics-coreclr enhancement Product code improvement that does NOT require public API changes/additions EventPipe runtime-mono specific to the Mono runtime tracking This issue is tracking the completion of other related issues.
Milestone
6.0.0

Comments

@lambdageek
Copy link
Member

lambdageek commented May 21, 2020 •
edited by lateralusX
Loading

Once #41872 is merged, we will have the original C++ CoreCLR EventPipe + Diagnostic Server implementation, as well as the C shared (currently MonoVM) EventPipe Diagnostic Server code. They should not functionally diverge.

This issue is meant to track changes that land in the CoreCLR version that should be ported over to the shared version.

So far these are:

CoreCLR PR CoreCLR PR Status Shared PR Notes
#36242 Merged - #37756
#36720 Merged - #41152
#36733 Merged - Ported as part of implementing CoreClr EventPipe shim.
#37035 Merged - #37756
#37002 Merged - #41152
#38967 Merged - #41152
#40191 Merged - #41152
#40332 Merged - Ported as part of implementing CoreClr EventPipe shim.
#40499 Merged - #41152
#40918 Merged - Not affecting shared library.
#41052 Merged - Ported as part of implementing CoreClr EventPipe shim.
#41194 Merged - Not affecting shared library. CoreClr EventPipe shim includes change.
#42307 Merged - Fixed in C library when initially detected and reported.
#43711 Merged - #46214
#38225 Merged - #46219
#44068 Merged - C library already handle this scenario.
#45672 Merged - #46220
@lambdageek lambdageek added EventPipe area-Diagnostics-coreclr runtime-mono specific to the Mono runtime tracking This issue is tracking the completion of other related issues. labels May 21, 2020
@ghost
Copy link

ghost commented May 21, 2020

Tagging subscribers to this area: @tommcdon
Notify danmosemsft if you want to be subscribed.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label May 21, 2020
@lambdageek lambdageek removed the untriaged New issue has not been triaged by the area owner label May 21, 2020
@lambdageek
Copy link
Member Author

@lateralusX I think you mentioned that you took the native code changes from #36242 (adding array types) already as part of #34600? Is that accurate?

@lateralusX
Copy link
Member

Just double checked and the changes are applied but in upcoming PR, so not yet merged.

@tommcdon tommcdon added this to the 6.0.0 milestone Jun 16, 2020
@lateralusX
Copy link
Member

Updated table with new PR's affecting EventPipe code that needs to be ported over to Mono.

@lateralusX lateralusX mentioned this issue Aug 21, 2020
Merged
@monojenkins monojenkins mentioned this issue Aug 21, 2020
Merged
monojenkins pushed a commit to monojenkins/mono that referenced this issue Aug 21, 2020
@lateralusX
Port EventPipe CoreCLR changes to Mono.
4713e0b 
Port changes done in the following CoreCLR PR's affecting corresponding Mono runtime EventPipe library sources:

dotnet/runtime#36720
dotnet/runtime#37002
dotnet/runtime#38967
dotnet/runtime#40191
dotnet/runtime#40332
dotnet/runtime#40499

EventPipe native code changes track by dotnet/runtime#36820.
lateralusX added a commit to mono/mono that referenced this issue Aug 25, 2020
@monojenkins @lateralusX
Port EventPipe CoreCLR changes to Mono. (#20278)
45d996b 
Port changes done in the following CoreCLR PR's affecting corresponding Mono runtime EventPipe library sources:

dotnet/runtime#36720
dotnet/runtime#37002
dotnet/runtime#38967
dotnet/runtime#40191
dotnet/runtime#40332
dotnet/runtime#40499

EventPipe native code changes track by dotnet/runtime#36820.

Co-authored-by: lateralusX <[email protected]>
@srxqds
Copy link
Contributor

srxqds commented Aug 25, 2020

could EventPipe work in mono runtime right now, it can use perf view for profiling ?

@lateralusX
Copy link
Member

lateralusX commented Aug 25, 2020 •
edited
Loading

No the complete end to end is WIP, working on diagnostic server port at the moment, once that is in place we should be able to start use tools like dotnet-counters. The complete EventPipe story on Mono is a bigger work item so before we have full perf view profiling in place there are some other work items to implement as well, but once we reached first "milestone" getting dotnet-coutners and several of the tests suites running, we will lay out work items to complete the perf view story.

@lateralusX
Copy link
Member

lateralusX commented Sep 30, 2020 •
edited
Loading

Updated table with new PR, #42307, affecting EventPipe/DiagnosticServer code that needs to be ported over to Mono.

@lateralusX
Copy link
Member

Added #43711 to list of tracked PR's.

@noahfalk noahfalk added the enhancement Product code improvement that does NOT require public API changes/additions label Nov 6, 2020
@am11
Copy link
Member

am11 commented Nov 11, 2020

Has there been a consideration to deduplicate the effort, by drawing out some abstraction? CoreCLR, as is, is building and linking C components (libunwind sources, libSystem.Globalization sources, version.c etc.), so the parts of EventPipe code that is impartial to OOP can be moved to .c files; without too much change in syntax -- if it helps the abstraction and DRY'ing.

@lateralusX
Copy link
Member

The ported library will be shared by both runtimes and once all is done there will only be one version that can be shared between both CoreClr and Mono (the C version of the library). There is also a shim abstracting all runtime specific details into a small set of files implemented by each runtime adapting the library.

@lateralusX
Copy link
Member

Looked over all changes to EventPipe/DiagnosticServer C++ library since last checkpoint (5:th of November), found and added #38225 #44068 #45672 to the list that needs to be evaluated for backport into C library.

@lateralusX
Copy link
Member

Backported all PR detected so far into C library. @josalem are you aware of any PR's currently missing from the list that needs to be backported into C library? I have looked through changes since last checkpoint but couldn't detect any additional PR's than the once already added to the list.

@josalem
Copy link
Contributor

josalem commented Jan 4, 2021

I have a couple brewing, but since we've merged the C version, I can put them in directly. I can't think of any that you haven't already handled.

@josalem
Copy link
Contributor

josalem commented Feb 1, 2021

@lateralusX now that we've switched to the C implementation by default, do we need this issue anymore?

@lateralusX
Copy link
Member

No, since changes will now be done in the shared code, we won't need this. We still might need a way to communicate changes done in runtime specific shim implementation that might make sense to the other runtimes, but that can probably be notified per PR.

@josalem josalem mentioned this issue Feb 19, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Mar 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@lambdageek lambdageek

@lateralusX lateralusX

@josalem josalem

Labels
area-Diagnostics-coreclr enhancement Product code improvement that does NOT require public API changes/additions EventPipe runtime-mono specific to the Mono runtime tracking This issue is tracking the completion of other related issues.
Projects
None yet
Milestone
6.0.0
Development

No branches or pull requests
8 participants
@lambdageek @am11 @srxqds @noahfalk @lateralusX @tommcdon @josalem @Dotnet-GitSync-Bot