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.

Sign up for GitHub

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

Jump to bottom

[mono] Extend MonoAOTCompiler Task #70851

Merged
merged 3 commits into from
Jun 22, 2022
Merged

[mono] Extend MonoAOTCompiler Task #70851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1338ca1
[mono] Extend HelloWorld sample to use MonoAotCompiler Task
mdh1418 Jun 13, 2022
1fffa3b
[mono] Extend MonoAotCompiler Task to invoke dotnet-pgo
mdh1418 Jun 13, 2022
e6c6176
Use cache for incremental builds
mdh1418 Jun 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/mono/mono.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
<BuildMonoAOTCrossCompiler Condition="'$(TargetsiOS)' == 'true'">true</BuildMonoAOTCrossCompiler>
<BuildMonoAOTCrossCompiler Condition="'$(TargetstvOS)' == 'true'">true</BuildMonoAOTCrossCompiler>
<BuildMonoAOTCrossCompiler Condition="'$(TargetsMacCatalyst)' == 'true'">true</BuildMonoAOTCrossCompiler>
<BuildMonoAOTCrossCompiler Condition="'$(TargetsOSX)' == 'true'">true</BuildMonoAOTCrossCompiler>
<BuildMonoAOTCrossCompiler Condition="'$(TargetsBrowser)' == 'true'">true</BuildMonoAOTCrossCompiler>
<BuildMonoAOTCrossCompiler Condition="'$(TargetsAndroid)' == 'true'">true</BuildMonoAOTCrossCompiler>
<MonoObjCrossDir>$([MSBuild]::NormalizeDirectory('$(MonoObjDir)', 'cross'))</MonoObjCrossDir>
Expand Down
30 changes: 30 additions & 0 deletions src/mono/sample/HelloWorld/HelloWorld.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,34 @@
<OutputType>Exe</OutputType>
<TargetFramework>$(NetCoreAppCurrent)</TargetFramework>
</PropertyGroup>

<UsingTask TaskName="MonoAOTCompiler"
AssemblyFile="$(MonoAOTCompilerTasksAssemblyPath)" />

<Target Name="AOTCompileApp" Condition="'$(RunAOTCompilation)' == 'true'" AfterTargets="CopyFilesToPublishDirectory">
<PropertyGroup>
<_AotOutputType>Library</_AotOutputType>
<_AotLibraryFormat>Dylib</_AotLibraryFormat>
<UseAotDataFile>false</UseAotDataFile>
</PropertyGroup>

<ItemGroup>
<AotInputAssemblies Include="$(PublishDir)\*.dll" />
</ItemGroup>

<MonoAOTCompiler
CompilerBinaryPath="@(MonoAotCrossCompiler->WithMetadataValue('RuntimeIdentifier','$(TargetOS.ToLowerInvariant())-$(TargetArchitecture.ToLowerInvariant())'))"
OutputType="$(_AotOutputType)"
LibraryFormat="$(_AotLibraryFormat)"
Assemblies="@(AotInputAssemblies)"
OutputDir="$(PublishDir)"
IntermediateOutputPath="$(IntermediateOutputPath)"
UseAotDataFile="$(UseAotDataFile)"
CacheFilePath="$(IntermediateOutputPath)aot_compiler_cache.json"
NetTracePath="$(NetTracePath)"
PgoBinaryPath="$(PgoBinaryPath)"
MibcProfilePath="$(MibcProfilePath)">
<Output TaskParameter="CompiledAssemblies" ItemName="BundleAssemblies" />
</MonoAOTCompiler>
</Target>
</Project>
15 changes: 13 additions & 2 deletions src/mono/sample/HelloWorld/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ DOTNET_Q_ARGS=--nologo -v:q -consoleloggerparameters:NoSummary

MONO_CONFIG ?=Debug
MONO_ARCH=x64
AOT?=false

#NET_TRACE_PATH=<path-to-trace-of-sample>
#PGO_BINARY_PATH=<path-to-dotnet-pgo-executable>
#MIBC_PROFILE_PATH=<path-to-mibc-for-sample>

OS := $(shell uname -s)
ifeq ($(OS),Darwin)
Expand All @@ -12,10 +17,16 @@ else
TARGET_OS=linux
endif

MONO_ENV_OPTIONS ?=--llvm
MONO_ENV_OPTIONS ?=

publish:
$(DOTNET) publish -c $(MONO_CONFIG) -r $(TARGET_OS)-$(MONO_ARCH)
$(DOTNET) publish \
-c $(MONO_CONFIG) \
-r $(TARGET_OS)-$(MONO_ARCH) \
/p:RunAOTCompilation=$(AOT) \
'/p:NetTracePath="$(NET_TRACE_PATH)"' \
'/p:PgoBinaryPath="$(PGO_BINARY_PATH)"' \
'/p:MibcProfilePath="$(MIBC_PROFILE_PATH)"'

run: publish
COMPlus_DebugWriteToStdErr=1 \
Expand Down
99 changes: 89 additions & 10 deletions src/tasks/AotCompilerTask/MonoAOTCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,16 @@ public class MonoAOTCompiler : Microsoft.Build.Utilities.Task
/// </summary>
public bool UseDwarfDebug { get; set; }

/// <summary>
/// Path to Dotnet PGO binary (dotnet-pgo)
/// </summary>
public string? PgoBinaryPath { get; set; }

/// <summary>
/// NetTrace file to use when invoking dotnet-pgo for
/// </summary>
public string? NetTracePath { get; set; }

/// <summary>
/// File to use for profile-guided optimization, *only* the methods described in the file will be AOT compiled.
/// </summary>
Expand All @@ -119,7 +129,7 @@ public class MonoAOTCompiler : Microsoft.Build.Utilities.Task
/// <summary>
/// Mibc file to use for profile-guided optimization, *only* the methods described in the file will be AOT compiled.
/// </summary>
public string[]? MibcProfilePath { get; set; }
public string[] MibcProfilePath { get; set; } = Array.Empty<string>();

/// <summary>
/// List of profilers to use.
Expand Down Expand Up @@ -271,6 +281,20 @@ private bool ProcessAndValidateArguments()
if (!Directory.Exists(IntermediateOutputPath))
Directory.CreateDirectory(IntermediateOutputPath);

if (!string.IsNullOrEmpty(NetTracePath))
{
if (!File.Exists(NetTracePath))
{
Log.LogError($"NetTracePath={nameof(NetTracePath)} doesn't exist");
return false;
}
if (!File.Exists(PgoBinaryPath))
{
Log.LogError($"NetTracePath was provided, but {nameof(PgoBinaryPath)}='{PgoBinaryPath}' doesn't exist");
return false;
}
}

if (AotProfilePath != null)
{
foreach (var path in AotProfilePath)
Expand All @@ -283,15 +307,12 @@ private bool ProcessAndValidateArguments()
}
}

if (MibcProfilePath != null)
foreach (var path in MibcProfilePath)
{
foreach (var path in MibcProfilePath)
if (!File.Exists(path))
{
if (!File.Exists(path))
{
Log.LogError($"MibcProfilePath '{path}' doesn't exist.");
return false;
}
Log.LogError($"MibcProfilePath '{path}' doesn't exist.");
return false;
}
}

Expand Down Expand Up @@ -400,6 +421,39 @@ public override bool Execute()
}
}

private bool ProcessNettrace(string netTraceFile)
{
var outputMibcPath = Path.Combine(OutputDir, Path.ChangeExtension(Path.GetFileName(netTraceFile), ".mibc"));

if (_cache!.Enabled)
{
string hash = Utils.ComputeHash(netTraceFile);
mdh1418 marked this conversation as resolved.
Show resolved Hide resolved
if (!_cache!.UpdateAndCheckHasFileChanged($"-mibc-source-file-{Path.GetFileName(netTraceFile)}", hash))
{
Log.LogMessage(MessageImportance.Low, $"Skipping generating {outputMibcPath} from {netTraceFile} because source file hasn't changed");
return true;
}
else
{
Log.LogMessage(MessageImportance.Low, $"Generating {outputMibcPath} from {netTraceFile} because the source file's hash has changed.");
}
}

(int exitCode, string output) = Utils.TryRunProcess(Log,
PgoBinaryPath!,
$"create-mibc --trace {netTraceFile} --output {outputMibcPath}");

if (exitCode != 0)
{
Log.LogError($"dotnet-pgo({PgoBinaryPath}) failed for {netTraceFile}:{output}");
return false;
}

MibcProfilePath = MibcProfilePath.Append(outputMibcPath).ToArray();
Log.LogMessage(MessageImportance.Low, $"Generated {outputMibcPath} from {PgoBinaryPath}");
return true;
}

private bool ExecuteInternal()
{
if (!ProcessAndValidateArguments())
Expand All @@ -417,6 +471,9 @@ private bool ExecuteInternal()

_cache = new FileCache(CacheFilePath, Log);

if (!string.IsNullOrEmpty(NetTracePath) && !ProcessNettrace(NetTracePath))
return false;

List<PrecompileArguments> argsList = new();
foreach (var assemblyItem in _assembliesToCompile)
argsList.Add(GetPrecompileArgumentsFor(assemblyItem, monoPaths));
Expand Down Expand Up @@ -756,7 +813,7 @@ private PrecompileArguments GetPrecompileArgumentsFor(ITaskItem assemblyItem, st
}
}

if (MibcProfilePath?.Length > 0)
if (MibcProfilePath.Length > 0)
{
aotArgs.Add("profile-only");
foreach (var path in MibcProfilePath)
Expand Down Expand Up @@ -833,12 +890,13 @@ private PrecompileArguments GetPrecompileArgumentsFor(ITaskItem assemblyItem, st
private bool PrecompileLibrary(PrecompileArguments args)
{
string assembly = args.AOTAssembly.GetMetadata("FullPath");
string output;
try
{
string msgPrefix = $"[{Path.GetFileName(assembly)}] ";

// run the AOT compiler
(int exitCode, string output) = Utils.TryRunProcess(Log,
(int exitCode, output) = Utils.TryRunProcess(Log,
CompilerBinaryPath,
$"--response=\"{args.ResponseFilePath}\"",
args.EnvironmentVariables,
Expand Down Expand Up @@ -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 •
edited
Loading

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

    runtime/src/tasks/AotCompilerTask/MonoAOTCompiler.cs

    Line 784 in cc3ac1b

    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,

    runtime/src/tasks/AotCompilerTask/MonoAOTCompiler.cs

    Line 788 in cc3ac1b

    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):

    runtime/src/tasks/AotCompilerTask/MonoAOTCompiler.cs

    Line 1249 in cc3ac1b

    File.Delete(TempFile);

    • 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 •
edited
Loading

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 •
edited
Loading

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:

runtime/src/tasks/AotCompilerTask/MonoAOTCompiler.cs

Lines 784 to 785 in cc3ac1b

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:

runtime/src/tasks/AppleAppBuilder/AppleAppBuilder.cs

Lines 203 to 205 in 5877e8b

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

runtime/src/tasks/AndroidAppBuilder/ApkBuilder.cs

Lines 141 to 143 in 5877e8b

// 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.

return false;
}

copied |= proxyFile.CopyOutputFileIfChanged();
_fileWrites.Add(proxyFile.TargetFile);
}
Expand Down Expand Up @@ -1092,8 +1156,20 @@ public FileCache(string? cacheFilePath, TaskLoggingHelper log)
_newCache = new(_oldCache.FileHashes);
}

public bool UpdateAndCheckHasFileChanged(string filePath, string newHash)
{
if (!Enabled)
throw new InvalidOperationException("Cache is not enabled. Make sure the cache file path is set");

_newCache!.FileHashes[filePath] = newHash;
return !_oldCache!.FileHashes.TryGetValue(filePath, out string? oldHash) || oldHash != newHash;
}

public bool ShouldCopy(ProxyFile proxyFile, [NotNullWhen(true)] out string? cause)
{
if (!Enabled)
throw new InvalidOperationException("Cache is not enabled. Make sure the cache file path is set");

cause = null;

string newHash = Utils.ComputeHash(proxyFile.TempFile);
Expand Down Expand Up @@ -1151,6 +1227,9 @@ public bool CopyOutputFileIfChanged()

try
{
if (!File.Exists(TempFile))
throw new LogAsErrorException($"Could not find the temporary file {TempFile} for target file {TargetFile}. Look for any errors/warnings generated earlier in the build.");

if (!_cache.ShouldCopy(this, out string? cause))
{
_cache.Log.LogMessage(MessageImportance.Low, $"Skipping copying over {TargetFile} as the contents are unchanged");
Expand Down