Skip to content

Integrate mibc mono config extension - #71657

Merged
mdh1418 merged 4 commits into
dotnet:mainfrom
mdh1418:integrate_mibc_mono_config_extension
Jul 6, 2022
Merged

Integrate mibc mono config extension#71657
mdh1418 merged 4 commits into
dotnet:mainfrom
mdh1418:integrate_mibc_mono_config_extension

Conversation

@mdh1418

Copy link
Copy Markdown
Member

Follow up to #71359.

With .mibc profiles now containing a MibcConfig, there are a couple of places that can use some modifications to better integrate this change.

  1. ClrEtwAll.man runtime sku - Only knows DesktopClr and CoreClr. Needs to be extended to know Mono (shows up as 4)
  2. Mono AOT Compiler - Can leverage the MibcConfig to skip .mibc's generated from non-mono runtimes.

This PR makes the following changes:

@ghost

ghost commented Jul 5, 2022

Copy link
Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghostghost assigned mdh1418Jul 5, 2022
@mdh1418
mdh1418force-pushed the integrate_mibc_mono_config_extension branch from 1e4edb1 to bd6e6a0CompareJuly 5, 2022 18:08
@mdh1418

Copy link
Copy Markdown
MemberAuthor

It looks like the Runtime value still shows up as 4 instead of the desired "Mono". Are there more areas I should extend the runtime Sku in dotnet/runtime @brianrob ? Where else needs the Sku extension for Perfview, Visual Studio, etc to recognize the new sku value of 0x4?

@brianrob

Copy link
Copy Markdown
Member

@mdh1418, your change to add 0x4 to the RuntimeSkuMap looks correct, though I don't see anything that emits this value in the current change. Is that elsewhere?

With regard to making the value "Mono" show up properly, you will want to update https://github.com/microsoft/perfview/blob/main/src/TraceEvent/Parsers/ClrTraceEventParser.cs#L12694. This will cover PerfView and TraceEvent, and then any tools that pick-up new versions of TraceEvent.

@mdh1418

Copy link
Copy Markdown
MemberAuthor

@brianrob

emits this value in the current change. Is that elsewhere?

Yes, I believe the Sku value 0x4 makes it to the .nettrace through a rundown event FireEtwRuntimeInformationDCStart

and from there, it makes it to the .mibc through
TraceEventruntimeStart=p.EventsInProcess.Filter(t =>t.EventName=="Runtime/Start").FirstOrDefault();
config.Runtime=runtimeStart?.PayloadByName("Sku")?.ToString();
. These changes were in a previous PR #71359

Thanks, I'll make changes to ClrTraceEventParser and see how it reflects locally!

@brianrob

Copy link
Copy Markdown
Member

@mdh1418 sounds good. What you've described sounds right to me. You will want to make the change to TraceEvent locally and then patch in the updated DLL where dotnet-pgo picks it up. You can do this all locally, and assuming it works, then you'd just need to take a new version of TraceEvent into dotnet-pgo.

@mdh1418

Copy link
Copy Markdown
MemberAuthor

@brianrob I was able to verify that the MibcConfig section of the .mibc profile reflected Mono under Runtime after I built perfview locally with the RuntimeSku extension there.
Screen Shot 2022-07-05 at 5 57 04 PM

I can put up a PR on perfview with the single line change and we can update the TraceEventVersion used in this repo to whichever version that PR makes it to.

@brianrob

Copy link
Copy Markdown
Member

Sounds great!

@lateralusXlateralusX left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Couple of nits.

Comment threadsrc/mono/mono/mini/aot-compiler.c
Comment threadsrc/mono/mono/mini/aot-compiler.c Outdated
Comment threadsrc/mono/mono/mini/aot-compiler.c Outdated
@mdh1418
mdh1418force-pushed the integrate_mibc_mono_config_extension branch from 1cf769f to c9efb02CompareJuly 6, 2022 15:25
@mdh1418
mdh1418 merged commit 3cbcd5b into dotnet:mainJul 6, 2022
@mdh1418
mdh1418 deleted the integrate_mibc_mono_config_extension branch July 6, 2022 19:05
@ghostghost locked as resolved and limited conversation to collaborators Aug 6, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@mdh1418@brianrob@lateralusX