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

[mono] Extend MonoAOTCompiler Task #70851

Merged
merged 3 commits into from
Jun 22, 2022

Conversation

mdh1418
Copy link
Member

@mdh1418 mdh1418 commented Jun 16, 2022

Part of #69512

In effort to make the dotnet-pgo based Profiled AOT story on mono more seamless (i.e. less steps for users), the MonoAOTCompiler itself can call a dotnet-pgo executable and inherently process a .nettrace, and ultimately feeding into #70194.


This PR makes the following changes:

  1. Extends the mono HelloWorld desktop sample to use AOT Compilation
  2. Extends the MonoAOTCompiler Task to invoke dotnet-pgo and process a .nettrace
    a) Allows for cache usage to avoid generating the same .mibc from the same source .nettrace

Example workflow with Desktop mono HelloWorld sample

Generating a .nettrace

make run
DOTNET_DiagnosticPorts="~/myport,suspend" <path-to-HelloWorld-executable> (e.g. ../../../../artifacts/bin/HelloWorld/arm64/Release/osx-arm64/publish/HelloWorld)
in a separate tab
dotnet-trace collect --providers Microsoft-Windows-DotNETRuntime:0x1F000080018:5 --diagnostic-port ~/myport --output <output .nettrace file>

Utilizing the .nettrace

In the makefile
Set AOT?=true
Set NET_TRACE_PATH= to the output .nettrace file
Set PGO_BINARY_PATH= to the dotnet-pgo executable
make run

src/tasks/AotCompilerTask/MonoAOTCompiler.cs Outdated Show resolved Hide resolved
src/tasks/AotCompilerTask/MonoAOTCompiler.cs Outdated Show resolved Hide resolved
src/tasks/AotCompilerTask/MonoAOTCompiler.cs Outdated Show resolved Hide resolved
src/tasks/AotCompilerTask/MonoAOTCompiler.cs Outdated Show resolved Hide resolved
src/tasks/AotCompilerTask/MonoAOTCompiler.cs Outdated Show resolved Hide resolved
src/tasks/AotCompilerTask/MonoAOTCompiler.cs Outdated Show resolved Hide resolved
@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 16, 2022
@mdh1418 mdh1418 force-pushed the mono_aot_compiler_handle_nettrace branch from 1acab5c to e6b41f2 Compare June 16, 2022 20:13
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 16, 2022
@mdh1418 mdh1418 force-pushed the mono_aot_compiler_handle_nettrace branch from 0ae941b to c142358 Compare June 16, 2022 21:12
@mdh1418 mdh1418 force-pushed the mono_aot_compiler_handle_nettrace branch from c142358 to 765f3aa Compare June 17, 2022 16:55
@radical
Copy link
Member

radical commented Jun 21, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

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

Thanks for your patience with the feedback, LGTM! 👍

@mdh1418 mdh1418 force-pushed the mono_aot_compiler_handle_nettrace branch from 69e7953 to e6c6176 Compare June 21, 2022 17:20
@mdh1418
Copy link
Member Author

mdh1418 commented Jun 21, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mdh1418
Copy link
Member Author

mdh1418 commented Jun 21, 2022

This failure is unrelated #67509

The Browser wasm lane failures are also unrelated #71096, the other lanes have been failing on other builds.

The timeout seems to be timing out on other builds as well.

@mdh1418 mdh1418 merged commit 46f53e3 into dotnet:main Jun 22, 2022
@mdh1418 mdh1418 deleted the mono_aot_compiler_handle_nettrace branch June 22, 2022 01:06
@@ -878,6 +936,12 @@ private bool PrecompileLibrary(PrecompileArguments args)
bool copied = false;
foreach (var proxyFile in args.ProxyFiles)
{
if (!File.Exists(proxyFile.TempFile))
{
Log.LogError($"Precompile command succeeded, but can't find the expected temporary output file - {proxyFile.TempFile} for {assembly}.{Environment.NewLine}{output}");
Copy link
Member

Choose a reason for hiding this comment

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

This check is causing some Android AOT builds to fail with:

C:\a\_work\1\s\bin\Release\dotnet\packs\Microsoft.Android.Sdk.Windows\33.0.0-ci.darc-main-c081764c-bbdd-4561-82bd-a59b76df22f3.71\targets\Microsoft.Android.Sdk.Aot.targets(91,5): Precompile command succeeded, but can't find the expected temporary output file - obj\Release\android-arm\aot\UnnamedProject.dll-llvm.o for C:\a\_work\1\a\TestRelease\06-28_05.09.49\temp\BuildBasicApplicationReleaseProfiledAotTrue\obj\Release\android-arm\linked\UnnamedProject.dll.

This started happening here: dotnet/android#7127

Here is the project with msbuild.binlog inside:

BuildBasicApplicationReleaseProfiledAotTrue.zip

I think the file it's looking for doesn't actually exist:

obj\Release\android-arm\aot\UnnamedProject.dll-llvm.o

And this file is at:

obj\Release\android-arm\aot\armeabi-v7a\UnnamedProject\temp-llvm.o

Let me know if I need to change something in the Android workload, thanks!

Copy link
Member

@radical radical Jun 28, 2022

Choose a reason for hiding this comment

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

In the mono-aot-cross command line you have ,llvm-outfile=obj\Release\android-arm\aot\UnnamedProject.dll-llvm.o. Is this file not expected to be an output? I don't see it in the obj directory.

Copy link
Member

Choose a reason for hiding this comment

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

Full mono-aot-cross command:

[UnnamedProject.dll] Exec (with response file contents expanded) in C:\a\_work\1\a\TestRelease\06-28_05.16.51\temp\BuildBasicApplicationReleaseProfiledAotTrue: MONO_PATH=C:\a\_work\1\a\TestRelease\06-28_05.16.51\temp\BuildBasicApplicationReleaseProfiledAotTrue\obj\Release\android-arm\linked; MONO_ENV_OPTIONS= C:\a\_work\1\s\xamarin-android\bin\Release\dotnet\packs\Microsoft.NETCore.App.Runtime.AOT.win-x64.Cross.android-arm\7.0.0-preview.6.22322.7\Sdk\..\tools\mono-aot-cross.exe --verbose --debug
--llvm "
    --aot=asmwriter,
    temp-path=obj\Release\android-arm\aot\armeabi-v7a\UnnamedProject,
    nodebug,
    llvm-path=C:\a\_work\1\s\xamarin-android\bin\Release\dotnet\packs\Microsoft.NETCore.App.Runtime.AOT.win-x64.Cross.android-arm\7.0.0-preview.6.22322.7\Sdk\..\tools,
    mtriple=armv7-linux-gnueabi,
    tool-prefix=C:\a\_work\1\s\xamarin-android\bin\Release\dotnet\packs\Microsoft.Android.Sdk.Windows\33.0.0-ci.darc-main-c081764c-bbdd-4561-82bd-a59b76df22f3.71\tools\binutils\bin\arm-linux-androideabi-,
    outfile=obj\Release\android-arm\aot\UnnamedProject.dll.so,
    llvm-outfile=obj\Release\android-arm\aot\UnnamedProject.dll-llvm.o,
    ld-name=ld.CMD,
    ld-flags=\"-LC:\a\_work\1\a\TestRelease\06-28_05.16.51\temp\SDK Ümläüts\sdk\ndk\24.0.8215888\toolchains\llvm\prebuilt\windows-x86_64\sysroot\usr\lib\arm-linux-androideabi\21\";\"-LC:\a\_work\1\s\xamarin-android\bin\Release\dotnet\packs\Microsoft.Android.Sdk.Windows\33.0.0-ci.darc-main-c081764c-bbdd-4561-82bd-a59b76df22f3.71\tools\binutils\bin\..\sysroot\usr\lib\arm-linux-androideabi\";\"C:\a\_work\1\a\TestRelease\06-28_05.16.51\temp\SDK Ümläüts\sdk\ndk\24.0.8215888\toolchains\llvm\prebuilt\windows-x86_64\sysroot\usr\lib\arm-linux-androideabi\21\libc.so\";\"C:\a\_work\1\a\TestRelease\06-28_05.16.51\temp\SDK Ümläüts\sdk\ndk\24.0.8215888\toolchains\llvm\prebuilt\windows-x86_64\sysroot\usr\lib\arm-linux-androideabi\21\libm.so\"
    -s"
"obj\Release\android-arm\linked\UnnamedProject.dll"
  • In that we have:
,outfile=obj\Release\android-arm\aot\UnnamedProject.dll.so
,llvm-outfile=obj\Release\android-arm\aot\UnnamedProject.dll-llvm.o
  • Should obj\Release\android-arm\aot\UnnamedProject.dll-llvm.o not be expected to exist?

  • It gets added to the list of files at

    string llvmObjectFile = Path.Combine(OutputDir, Path.ChangeExtension(assemblyFilename, ".dll-llvm.o"));

  • because no cache file is being used, proxy.TempFile will be same as proxy.TargetFile.

  • Also,

    aotAssembly.SetMetadata("LlvmObjectFile", proxyFile.TargetFile);

    • this suggests that the file is expected to exist
    • if that's true, then I think maybe mono-aot-cross failed but didn't return a non-zero error code?
  • in the output I see many LLVM failed messages, are those expected?

  • We do delete the temp file (in this case == target file):

    • but this shouldn't be getting hit.

(I'll anyway prep a PR for fixing some temp file stuff)

Copy link
Member

Choose a reason for hiding this comment

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

#71411 should remove the error. But IIUC, in this case if the dll-llvm.o file doesn't exist, then it will fail outside this task.

Maybe we should add a check to ensure that all the "expected" output files actually exist after running mono-aot-cross. Unless you think there is some reason not to.

Copy link
Member

@jonathanpeppers jonathanpeppers Jun 29, 2022

Choose a reason for hiding this comment

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

I'm honestly not sure about any of the intermediate files that are created here. We only really use the .so files that exist at the end, and those appear to exist? (Back when this task completed successfully)

We also don't use EnableLLVM=true by default, so there could be an issue or two when that is enabled.

Copy link
Member

Choose a reason for hiding this comment

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

@jonathanpeppers aotAssembly.SetMetadata("LlvmObjectFile", proxyFile.TargetFile); - this isn't used?

Copy link
Member

Choose a reason for hiding this comment

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

No, we don't set this metadata at all. I think it's because we use the .so files as-is -- and don't currently do anything extra.

There were some ideas about using a native linker to do extra things (which would need the .o files) -- but we haven't implemented that yet.

Copy link
Member

Choose a reason for hiding this comment

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

@akoeplinger please help out with this if ankit's fix doesn't solve the issue

Copy link
Member

@radical radical Jun 29, 2022

Choose a reason for hiding this comment

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

The code indicates that it is expecting an output file by creating this ProxyFile object:

string llvmObjectFile = Path.Combine(OutputDir, Path.ChangeExtension(assemblyFilename, ".dll-llvm.o"));
ProxyFile proxyFile = _cache.NewFile(llvmObjectFile);

So, from correctness pov it would be completely fair to check that the expected output file exists after mono-aot-cross has run. But in this case it seems that:

  1. the file is never generated
  2. even though we have aotAssembly.SetMetadata("LlvmObjectFile", proxyFile.TargetFile); , it is not being used else I would expect the task user/caller to break since the file is not generated.

Copy link
Member

Choose a reason for hiding this comment

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

I see it being used at:

// use AOT files if available
var obj = file.GetMetadata("AssemblerFile");
var llvmObj = file.GetMetadata("LlvmObjectFile");

// use AOT files if available
var obj = file.GetMetadata("AssemblerFile");
var llvmObj = file.GetMetadata("LlvmObjectFile");

I'll make this file optional (for the existence check), and check for all other files.

radical added a commit to radical/runtime that referenced this pull request Jun 28, 2022
radical added a commit that referenced this pull request Jun 29, 2022
…71411)

* MonoAOTCompilerTask: Don't check for temp file if cache is disabled

Prompted by #70851 (comment)

* Emit a message when deleting tmp files
@ghost ghost locked as resolved and limited conversation to collaborators Jul 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants