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

Integrate mibc mono config extension #71657

Merged
merged 4 commits into from
Jul 6, 2022

Conversation

mdh1418
Copy link
Member

@mdh1418 mdh1418 commented Jul 5, 2022

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:

@dotnet-issue-labeler
Copy link

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.

@ghost ghost assigned mdh1418 Jul 5, 2022
@mdh1418 mdh1418 force-pushed the integrate_mibc_mono_config_extension branch from 1e4edb1 to bd6e6a0 Compare July 5, 2022 18:08
@mdh1418
Copy link
Member Author

mdh1418 commented Jul 5, 2022

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
Member

brianrob commented Jul 5, 2022

@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
Member Author

mdh1418 commented Jul 5, 2022

@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

RUNTIME_SKU_MONO,
and from there, it makes it to the .mibc through
TraceEvent runtimeStart = 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
Member

brianrob commented Jul 5, 2022

@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
Member Author

mdh1418 commented Jul 5, 2022

@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
Member

brianrob commented Jul 5, 2022

Sounds great!

Copy link
Member

@lateralusX lateralusX left a comment

Choose a reason for hiding this comment

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

LGTM! Couple of nits.

src/mono/mono/mini/aot-compiler.c Show resolved Hide resolved
src/mono/mono/mini/aot-compiler.c Outdated Show resolved Hide resolved
src/mono/mono/mini/aot-compiler.c Outdated Show resolved Hide resolved
@mdh1418 mdh1418 force-pushed the integrate_mibc_mono_config_extension branch from 1cf769f to c9efb02 Compare July 6, 2022 15:25
@mdh1418 mdh1418 merged commit 3cbcd5b into dotnet:main Jul 6, 2022
@mdh1418 mdh1418 deleted the integrate_mibc_mono_config_extension branch July 6, 2022 19:05
@ghost ghost locked as resolved and limited conversation to collaborators Aug 6, 2022
Sign up for free to 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