-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[mono] Extend mono AOT compiler to ingest .mibc profiles #70194
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. |
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
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. |
@@ -9,6 +9,17 @@ | |||
#include <mono/metadata/image.h> | |||
#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
.aotprofile
file 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.aotprofile
data 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()
whereList
is inSystem.Private.CoreLib.dll
, butQ
is 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_methods
mibcGroupMethod
-add_mibc_group_method_methods
add_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