-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[mono] Extend mono AOT compiler to ingest .mibc profiles #70194
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
Conversation
|
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. |
lambdageek
left a comment
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.
Looking good.
Couple of things i'd like to see changed:
- let's do some cleanup in
image.c - let's make the code that iterates over the mibc group a bit more aware of the format
- instead of copy/pasting the AOT "related methods" logic, extract it into a function and share between the mibc and legacy aot profiler
e636d86 to
15372c2
Compare
15372c2 to
b1fb61e
Compare
lambdageek
left a comment
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.
LGTM. A couple of nits, some portability improvements, and a bit more image_open refactoring suggestions
src/mono/mono/mini/aot-compiler.c
Outdated
| continue; | ||
|
|
||
| g_assert (opcodeIp + 4 < opcodeEnd); | ||
| guint32 token = *(guint32 *)(opcodeIp + 1); |
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.
| guint32 token = *(guint32 *)(opcodeIp + 1); | |
| guint32 token = read32 (opcodeIp + 1); |
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.
Thanks! I thought it would work, but convinced myself it didnt when xcode lldb complained about expr read32 (opcodeIp + 1). Is there a way to have the xcode lldb recognize methods such as read32?
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 think if you compile the runtime with "enough" debug information (maybe -g3) then debuggers ought to recognize macros like read32. not sure if that really works with clang+lldb
|
The failures are unrelated to this PR and have issues created. |
| #include <mono/metadata/loader-internals.h> | ||
|
|
||
| typedef struct { | ||
| int dont_care_about_cli : 1; |
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.
Any specific reason why these parameters where inverted compared to the arguments passed to functions below?
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.
Any particular win to use bit fields for this? I think it would be much clearer to use bool type instead, it would also make the init code more clear.
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.
Any specific reason why these parameters where inverted compared to the arguments passed to functions below?
Turns out most of the time we care about cli and pecoff, so the callers become a bit simpler: they can just initialize the options struct to zero.
Any particular win to use bit fields for this? I think it would be much clearer to use bool type instead, it would also make the init code more clear.
I think I suggested bitfields more out of habit than any rational reason. These could be booleans.
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.
Thanks! I'll modify these to a boolean type in the upcoming PR
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.
(the MonoImage:not_executable we do want to keep in a bitfield - we try to keep the size of that structure down)
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.
This is now in PR #71657 @lateralusX @lambdageek
Part of #69268 and #69512.
In effort to bring back Profiled Ahead-Of-Time compilation on Mono through leveraging EventPipe diagnostics-tracing and CoreCLR's dotnet-pgo tool, this PR enables Mono's AOT Compiler to directly consume a
.mibc(Managed Instrumented Block Counts) Portable Executable file generated through dotnet-pgo for the purpose of AOT compiling methods traced during an app run.This follows the work to emit MethodDetails and BulkType events on the mono runtime, which was imperative to know exactly which methods were encountered during an app run.
Prior Profiled AOT .aotprofile processing
Mono's AOT Compiler (mono-sgen) prior Profiled AOT story involved a
.aotprofilefile which recorded identifying components of methods, embedded types, generic instantiations of methods/types, and the overarching modules https://github.com/mono/mono/tree/main/mcs/class/Mono.Profiler.Log/Mono.Profiler.Aot. The compiler, when compiling particular assemblies, would leverage.aotprofiledata to determine which methods to AOT compile by:X.dll) AOT image (X.dll.so/X.dll.dylib/other native extensions) if they are inherently from that assembly, or related to that assembly. (e.g. methodList<Q>.ToArray()whereListis inSystem.Private.CoreLib.dll, butQis inQ.dll, the method is related to assemblyQ.dll).aotprofile example
Proposed Profiled AOT .mibc processing
The .mibc file format as a Portable Executable already contains comprehensive information of traced methods (as a result of MethodDetails and BulkType events from #68571). As such, there is no need to neither generate nor load ProfileData structs. Instead, we:
mibcGroupMethod-add_mibc_profile_methodsmibcGroupMethod-add_mibc_group_method_methodsadd_single_mibc_profile_method.mibc from HelloWorld mono sample
Breakdown of HelloWorld Sample .mibc file
This PR makes the following changes:
Using the mono-sgen compiler to process
.mibc./build -s mono+libs -os <OS> -arch <arch> -c <config>MONO_LOG_MASK=asm MONO_LOG_LEVEL=debug MONO_PATH=./artifacts/bin/HelloWorld/arm64/Release/osx-arm64/publish/ artifacts/obj/mono/OSX.arm64.Release/out/bin/mono-sgen --aot=profile-only,mibc-profile=/Users/mihw/runtime_two/HelloWorld_arm64.mibc artifacts/bin/HelloWorld/arm64/Release/osx-arm64/publish/System.Console.dll artifacts/bin/HelloWorld/arm64/Release/osx-arm64/publish/System.Private.CoreLib.dll artifacts/bin/HelloWorld/arm64/Release/osx-arm64/publish/HelloWorld.dll