-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Add a config to MIBC format #71359
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
Merged
Merged
Add a config to MIBC format #71359
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
8319fa8
Add MibcConfig to mibc
EgorBo 3a8f82d
Address feedback
EgorBo 0b68488
Clean up
EgorBo 1714407
Fix build
EgorBo 6ebfbcb
Fix restore
EgorBo 11cab7a
fix crash
EgorBo 776061a
Merge branch 'main' of github.com:dotnet/runtime into extend-mibc
EgorBo 285eab6
Get rid of reflection
EgorBo 03beced
Merge branch 'main' of github.com:dotnet/runtime into extend-mibc
EgorBo 1cf0aef
Address feedback
EgorBo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the Sku work (what does it stand for)? It would be nice to have the mono runtime be emitted, so if we use the Sku for emitting runtime, would it be unproblematic to extend the Sku enum to also contain Mono (not sure if there is a trickle effect)? What is the
Runtime/Start
event/where do i look to find it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure what to pick for runtime so probably it would be easier if you tell me which event to analyze cc @lateralusX
I just opened a random nettrace via PerfView and searched for CoreCLR - this was the first thing to pop up - I assume mono runtime is expected to send this exact event on start with a different value for Sku
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that eventname is the same as
FireEtwRuntimeInformationDCStart
, it seems to be the right event. I think we can try adding mono to the Sku Enum, right now we are emitting CoreCLR as well on Monoruntime/src/mono/mono/eventpipe/ep-rt-mono.c
Line 2804 in 41d9c1c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see
FireEtwRuntimeInformationDCStart
in my sample nettraceThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the events we collect -
Microsoft-Windows-DotNETRuntime:0x1F000080018:5
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I assume the current impl is correct as is, right? it does poll "Runtime/Start" event and extract Sku so once Mono is added we're all set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lateralusX @EgorBo I'm thinking the value should be 0x4 as 0x3 will show up as DesktopClr, CoreClr since it matches those bits (0x1 || 0x2). I obtained a trace with
--providers Microsoft-Windows-DotNETRuntime:0x1F000080018:5
on mono and it indeed shows the sku value within the mibc file. https://gist.github.com/mdh1418/2e9ad2d937dbdd0fb0be063c15eff61aI also verified that we should be able to obtain the actual Sku value from the Mono AOT Compiler side as well. I'll put up a separate PR to integrate the .mibc file validation.
I think we should extend the relevant Sku structs and Mono additions I mentioned earlier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is anything treating the sku as a bit set? Seems like it shouldn't be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakobbotsch I'm not sure what the main rationale is between making something a bitmap vs a valuemap, but this might be why its behaving as such
runtime/src/coreclr/vm/ClrEtwAll.man
Line 611 in 4a63cb2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, seems reasonable then.