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

Enable deduplication of generics in Xamarin.iOS build #17766

Merged
merged 12 commits into from
Mar 30, 2023
Merged

Enable deduplication of generics in Xamarin.iOS build #17766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
bd340ca
Enable deduplication of generics in Xamarin.iOS build
kotlarmilos Mar 10, 2023
39f2d8a
Auto-format source code
Mar 10, 2023
b80f9ea
Calculate dedup arguments in ComputeAOTArguments step
kotlarmilos Mar 14, 2023
3398ef4
Merge branch 'net8.0' into improvement/ios-dedup
kotlarmilos Mar 23, 2023
3818279
Remove dedup assembly from bundle
kotlarmilos Mar 23, 2023
902ebcc
Add IsDedupAssembly to assembly metadata
kotlarmilos Mar 27, 2023
6b31644
Add a dedup switch
kotlarmilos Mar 27, 2023
2940cf4
Add note on isDedupAssembly variable
kotlarmilos Mar 27, 2023
f2d1cc5
Allow overriding _IsDedupEnabled on project level
kotlarmilos Mar 28, 2023
16e2f09
[CI] Fix GitHub ssh host key (#17900)
dalexsoto Mar 27, 2023
93d34e9
Auto-format source code
Mar 28, 2023
3760822
Merge remote-tracking branch 'origin/net8.0' into improvement/ios-dedup
rolfbjarne Mar 29, 2023
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
27 changes: 27 additions & 0 deletions dotnet/targets/Xamarin.Shared.Sdk.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,7 @@
@(_AotArguments -> 'AOTArgument=%(Identity)')
AOTCompiler=$(_XamarinAOTCompiler)
AOTOutputDirectory=$(_AOTOutputDirectory)
DedupAssembly=$(_DedupAssembly)
AppBundleManifestPath=$(_AppBundleManifestPath)
CacheDirectory=$(_LinkerCacheDirectory)
@(_BundlerDlsym -> 'Dlsym=%(Identity)')
Expand Down Expand Up @@ -918,6 +919,8 @@

<_AOTInputDirectory>$(_IntermediateNativeLibraryDir)aot-input/</_AOTInputDirectory>
<_AOTOutputDirectory>$(_IntermediateNativeLibraryDir)aot-output/</_AOTOutputDirectory>
<_IsDedupEnabled Condition="'$(_IsDedupEnabled)' == ''">true</_IsDedupEnabled>
<_DedupAssembly Condition="'$(_RunAotCompiler)' == 'true' And '$(IsMacEnabled)' == 'true' And '$(_IsDedupEnabled)' == 'true'">$(IntermediateOutputPath)aot-instances.dll</_DedupAssembly>

<_LibMonoLinkMode Condition="'$(_LibMonoLinkMode)' == '' And ('$(ComputedPlatform)' != 'iPhone' Or '$(_PlatformName)' == 'macOS')">dylib</_LibMonoLinkMode>
<_LibMonoLinkMode Condition="'$(_LibMonoLinkMode)' == ''">static</_LibMonoLinkMode>
Expand Down Expand Up @@ -1022,6 +1025,29 @@
</ItemGroup>
</Target>

<Target Name="_CreateAOTDedupAssembly"
Condition="'$(_RunAotCompiler)' == 'true' And '$(IsMacEnabled)' == 'true'"
DependsOnTargets="_ComputeManagedAssemblyToLink"
BeforeTargets="PrepareForILLink"
Inputs="$(MSBuildThisFileFullPath)"
Outputs="$(_DedupAssembly)">

<WriteLinesToFile File="$(_IntermediateNativeLibraryDir)aot-instances.cs" Overwrite="true" Lines="" WriteOnlyWhenDifferent="true" />
<Csc Sources="$(_IntermediateNativeLibraryDir)aot-instances.cs"
OutputAssembly="$(_DedupAssembly)"
TargetType="library"
Deterministic="true"
References="@(ReferencePath)"
ToolExe="$(CscToolExe)"
ToolPath="$(CscToolPath)" />
<Delete Files="$(_IntermediateNativeLibraryDir)aot-instances.cs" />

<ItemGroup>
<ManagedAssemblyToLink Include="$(_DedupAssembly)" />
<TrimmerRootAssembly Include="$(_DedupAssembly)" />
</ItemGroup>
</Target>

<PropertyGroup>
<_AOTCompileDependsOn>
$(_AOTCompileDependsOn);
Expand All @@ -1030,6 +1056,7 @@
_DetectAppManifest;
_ReadAppManifest;
_WriteAppManifest;
_CreateAOTDedupAssembly;
</_AOTCompileDependsOn>
</PropertyGroup>
<Target Name="_AOTCompile"
Expand Down
8 changes: 6 additions & 2 deletions msbuild/Xamarin.MacDev.Tasks/Tasks/AOTCompileTaskBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ public override bool Execute ()
var processArguments = Assemblies [i].GetMetadata ("ProcessArguments");
var aotData = Assemblies [i].GetMetadata ("AOTData");
var aotAssembly = Assemblies [i].GetMetadata ("AOTAssembly");
Boolean.TryParse (Assemblies [i].GetMetadata ("IsDedupAssembly"), out var isDedupAssembly);

var aotAssemblyItem = new TaskItem (aotAssembly);
aotAssemblyItem.SetMetadata ("Arguments", "-Xlinker -rpath -Xlinker @executable_path/ -Qunused-arguments -x assembler -D DEBUG");
Expand All @@ -98,10 +99,13 @@ public override bool Execute ()
return false;
}
arguments.Add ($"{string.Join (",", parsedArguments)}");
if (globalAotArguments is not null)
if (globalAotArguments?.Any () == true)
arguments.Add ($"--aot={string.Join (",", globalAotArguments)}");
arguments.AddRange (parsedProcessArguments);
arguments.Add (input);
if (isDedupAssembly)
arguments.AddRange (inputs);
else
arguments.Add (input);

processes [i] = ExecuteAsync (AOTCompilerPath, arguments, environment: environment, sdkDevPath: SdkDevPath, showErrorIfFailure: false /* we show our own error below */)
.ContinueWith ((v) => {
Expand Down
16 changes: 14 additions & 2 deletions tools/common/Application.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1495,13 +1495,13 @@ public bool IsAOTCompiled (string assembly)

public IList<string> GetAotArguments (string filename, Abi abi, string outputDir, string outputFile, string llvmOutputFile, string dataFile)
{
GetAotArguments (filename, abi, outputDir, outputFile, llvmOutputFile, dataFile, out var processArguments, out var aotArguments);
GetAotArguments (filename, abi, outputDir, outputFile, llvmOutputFile, dataFile, null, out var processArguments, out var aotArguments);
processArguments.Add (string.Join (",", aotArguments));
processArguments.Add (filename);
return processArguments;
}

public void GetAotArguments (string filename, Abi abi, string outputDir, string outputFile, string llvmOutputFile, string dataFile, out List<string> processArguments, out List<string> aotArguments, string llvm_path = null)
public void GetAotArguments (string filename, Abi abi, string outputDir, string outputFile, string llvmOutputFile, string dataFile, bool? isDedupAssembly, out List<string> processArguments, out List<string> aotArguments, string llvm_path = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I thinkisDedupAssembly can be a nonnullable boolean type and it would simplify comparisons further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GetAotArguments is used in legacy build as well, where dedup is not supported. Variable isDedupAssembly could have the following values:

  • NULL means that dedup is not enabled
  • FALSE means that dedup-skip flag should be passed for all assemblies
  • TRUE means that dedup-include flag should be passed for a container assembly

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right, I forgot about that case. Thanks for the explanation.
Could you please add this to the description of the PR?

{
string fname = Path.GetFileName (filename);
processArguments = new List<string> ();
Expand Down Expand Up @@ -1536,6 +1536,18 @@ public void GetAotArguments (string filename, Abi abi, string outputDir, string
aotArguments.Add ($"data-outfile={dataFile}");
aotArguments.Add ("static");
aotArguments.Add ("asmonly");
// This method is used in legacy build as well, where dedup is not supported.
// Variable isDedupAssembly could have the following values:
// - NULL means that dedup is not enabled
// - FALSE means that dedup-skip flag should be passed for all assemblies except a container assemblt
// - TRUE means that dedup-include flag should be passed for the container assembly
if (isDedupAssembly.HasValue) {
if (isDedupAssembly.Value) {
aotArguments.Add ($"dedup-include={fname}");
} else {
aotArguments.Add ($"dedup-skip");
}
}
if (app.LibMonoLinkMode == AssemblyBuildTarget.StaticObject || !Driver.IsDotNet)
aotArguments.Add ("direct-icalls");
aotArguments.AddRange (app.AotArguments);
Expand Down
5 changes: 5 additions & 0 deletions tools/dotnet-linker/LinkerConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ public class LinkerConfiguration {
public List<Abi> Abis;
public string AOTCompiler;
public string AOTOutputDirectory;
public string DedupAssembly;
public string CacheDirectory { get; private set; }
public Version DeploymentTarget { get; private set; }
public HashSet<string> FrameworkAssemblies { get; private set; } = new HashSet<string> ();
Expand Down Expand Up @@ -125,6 +126,9 @@ public static LinkerConfiguration GetInstance (LinkContext context, bool createI
case "AOTOutputDirectory":
AOTOutputDirectory = value;
break;
case "DedupAssembly":
DedupAssembly = value;
break;
case "CacheDirectory":
CacheDirectory = value;
break;
Expand Down Expand Up @@ -386,6 +390,7 @@ public void Write ()
Console.WriteLine ($" ABIs: {string.Join (", ", Abis.Select (v => v.AsArchString ()))}");
Console.WriteLine ($" AOTArguments: {string.Join (", ", Application.AotArguments)}");
Console.WriteLine ($" AOTOutputDirectory: {AOTOutputDirectory}");
Console.WriteLine ($" DedupAssembly: {DedupAssembly}");
Console.WriteLine ($" AppBundleManifestPath: {Application.InfoPListPath}");
Console.WriteLine ($" AreAnyAssembliesTrimmed: {Application.AreAnyAssembliesTrimmed}");
Console.WriteLine ($" AssemblyName: {Application.AssemblyName}");
Expand Down
10 changes: 9 additions & 1 deletion tools/dotnet-linker/Steps/ComputeAOTArguments.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ protected override void TryEndProcess ()

var app = Configuration.Application;
var outputDirectory = Configuration.AOTOutputDirectory;
var dedupFileName = Path.GetFileName (Configuration.DedupAssembly);
var isDedupEnabled = Configuration.Target.Assemblies.Any (asm => Path.GetFileName (asm.FullPath) == dedupFileName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above. Using item metadata instead to prevent comparing against a filename.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ComputeAOTArguments computes assemblies metadata, so it can't use the metadata. Is it possible to provide a dedup flag in Xamarin.Shared.Sdk.targets?

Anyway, we plan to simplify the dedup build process, so such assembly shouldn't be required soon.

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, we plan to simplify the dedup build process, so such assembly shouldn't be required soon.

That's great, it would make most of this PR unnecessary!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, it was allowed by dotnet/runtime#83711. As it would require runtime changes and additional testing, I suggest doing it as a follow-up effort on both runtime and xamarin repositories, as outlined in dotnet/runtime#83973 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering whether we could remove the dependency on the assembly filename and that we could do something like:

<ItemGroup>
    <ManagedAssemblyToLink Include="$(_DedupAssembly)" />
	    <IsDedupAssembly>true</IsDedupAssembly>
    <ManagedAssemblyToLink>
</ItemGroup>

but I missed the fact that the custom linker step does not have access to MSBuild item metadata at this point. My bad, and thanks for checking this out.


foreach (var asm in Configuration.Target.Assemblies) {
var isAOTCompiled = asm.IsAOTCompiled;
Expand All @@ -30,6 +32,10 @@ protected override void TryEndProcess ()
};

var input = asm.FullPath;
bool? isDedupAssembly = null;
if (isDedupEnabled) {
isDedupAssembly = Path.GetFileName (input) == dedupFileName;
}
var abis = app.Abis.Select (v => v.AsString ()).ToArray ();
foreach (var abi in app.Abis) {
var abiString = abi.AsString ();
Expand All @@ -38,7 +44,7 @@ protected override void TryEndProcess ()
var aotData = Path.Combine (outputDirectory, arch, Path.GetFileNameWithoutExtension (input) + ".aotdata");
var llvmFile = Configuration.Application.IsLLVM ? Path.Combine (outputDirectory, arch, Path.GetFileName (input) + ".llvm.o") : string.Empty;
var objectFile = Path.Combine (outputDirectory, arch, Path.GetFileName (input) + ".o");
app.GetAotArguments (asm.FullPath, abi, outputDirectory, aotAssembly, llvmFile, aotData, out var processArguments, out var aotArguments, Path.GetDirectoryName (Configuration.AOTCompiler));
app.GetAotArguments (asm.FullPath, abi, outputDirectory, aotAssembly, llvmFile, aotData, isDedupAssembly, out var processArguments, out var aotArguments, Path.GetDirectoryName (Configuration.AOTCompiler));
item.Metadata.Add ("Arguments", StringUtils.FormatArguments (aotArguments));
item.Metadata.Add ("ProcessArguments", StringUtils.FormatArguments (processArguments));
item.Metadata.Add ("Abi", abiString);
Expand All @@ -47,6 +53,8 @@ protected override void TryEndProcess ()
item.Metadata.Add ("AOTAssembly", aotAssembly);
item.Metadata.Add ("LLVMFile", llvmFile);
item.Metadata.Add ("ObjectFile", objectFile);
if (isDedupAssembly.HasValue && isDedupAssembly.Value)
item.Metadata.Add ("IsDedupAssembly", isDedupAssembly.Value.ToString ());
}

assembliesToAOT.Add (item);
Expand Down