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

Enable dedup optimization in FullAOT mode only #20687

Merged
merged 12 commits into from
Jun 7, 2024
Merged

Enable dedup optimization in FullAOT mode only #20687

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3f0c6cf
Enable dedup optimization only in FullAOT mode
kotlarmilos Jun 5, 2024
936c053
Add unit tests for partial AOT compilation
kotlarmilos Jun 5, 2024
f07f29b
Search bundle for aot-instances.dll
kotlarmilos Jun 5, 2024
c1750d9
Remove unused variable
kotlarmilos Jun 5, 2024
40672df
Auto-format source code
Jun 5, 2024
c0c99b5
Remove try-catch block
kotlarmilos Jun 5, 2024
f5fc50d
Parametrize tests and add maccatalyst-arm64
kotlarmilos Jun 6, 2024
ce4d456
Auto-format source code
Jun 6, 2024
0d42329
Merge branch 'main' into bugfix/dedup-partial-aot-compilation
rolfbjarne Jun 6, 2024
2d8cb06
Fix tests
kotlarmilos Jun 6, 2024
8d6f002
Merge branch 'bugfix/dedup-partial-aot-compilation' of github.com:xam…
kotlarmilos Jun 6, 2024
9863d03
Remove osx test cases since the dedup is mobile-only
kotlarmilos Jun 7, 2024
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
7 changes: 4 additions & 3 deletions dotnet/targets/Xamarin.Shared.Sdk.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1096,8 +1096,9 @@

<_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>
<!-- Enable dedup optimization only in FullAOT mode -->
<_IsDedupEnabled Condition="'$(_IsDedupEnabled)' == '' And '$(_RunAotCompiler)' == 'true' And '$(MtouchInterpreter)' == '' And '$(IsMacEnabled)' == 'true'">true</_IsDedupEnabled>
<_DedupAssembly Condition="'$(_IsDedupEnabled)' == 'true'">$(IntermediateOutputPath)aot-instances.dll</_DedupAssembly>

<!-- default to 'static' for Mac Catalyst to work around https://github.com/xamarin/xamarin-macios/issues/14686 -->
<_LibMonoLinkMode Condition="'$(_LibMonoLinkMode)' == '' And '$(_PlatformName)' == 'MacCatalyst'">static</_LibMonoLinkMode>
Expand Down Expand Up @@ -1219,7 +1220,7 @@
</Target>

<Target Name="_CreateAOTDedupAssembly"
Condition="'$(_RunAotCompiler)' == 'true' And '$(IsMacEnabled)' == 'true'"
Condition="'$(_IsDedupEnabled)' == 'true'"
DependsOnTargets="_ComputeManagedAssemblyToLink"
BeforeTargets="PrepareForILLink"
Inputs="$(MSBuildThisFileFullPath)"
Expand Down
74 changes: 74 additions & 0 deletions tests/dotnet/UnitTests/ProjectTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1917,5 +1917,79 @@ public void RaisesAppDomainUnhandledExceptionEvent (ApplePlatform platform)
var output = ExecuteWithMagicWordAndAssert (appExecutable, env);
}
}

bool FindAssembly(string path, string dllName)
{
try
{
foreach (string file in Directory.GetFiles(path, "*.dll", SearchOption.AllDirectories))
{
if (Path.GetFileName(file).Equals(dllName, StringComparison.OrdinalIgnoreCase))
{
return true;
}
}
}
catch (Exception ex)
{
Console.WriteLine($"An error occurred: {ex.Message}");
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to ignore any exceptions? Wouldn't it be better to just let exceptions flow, which would eventually fail the test? This seems like it could end up hiding problems at some point.

}

return false;
}

[Test]
[TestCase (ApplePlatform.iOS, "ios-arm64;")]
[TestCase (ApplePlatform.MacOSX, "osx-arm64;osx-x64")]
[TestCase (ApplePlatform.MacCatalyst, "maccatalyst-x64;")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we test maccatalyst-arm64 as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. I didn't add simulators since we already test this on physical devices.

[TestCase (ApplePlatform.TVOS, "tvos-arm64;")]
public void PartialAOTExceptCoreLib (ApplePlatform platform, string runtimeIdentifiers)
{
var project = "MySimpleApp";
Configuration.IgnoreIfIgnoredPlatform (platform);
Configuration.AssertRuntimeIdentifiersAvailable (platform, runtimeIdentifiers);

var project_path = GetProjectPath (project, runtimeIdentifiers: runtimeIdentifiers, platform: platform, out var appPath);
Clean (project_path);
var properties = GetDefaultProperties (runtimeIdentifiers);
properties ["MtouchInterpreter"] = "-all,System.Private.CoreLib.dll";
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Just a thought. As far as I can see the added two test methods only differ by this setting, maybe it could have been an additional parameter to a single test method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, thanks.


DotNet.AssertBuild (project_path, properties);

Assert.True (!FindAssembly (appPath, "aot-instances.dll"), "Dedup optimization shouldn't been enabled for partial AOT compilation");

var appExecutable = GetNativeExecutable (platform, appPath);

if (CanExecute (platform, runtimeIdentifiers)) {
var output = ExecuteWithMagicWordAndAssert (appExecutable);
}
}

[Test]
[TestCase (ApplePlatform.iOS, "ios-arm64;")]
[TestCase (ApplePlatform.MacOSX, "osx-arm64;osx-x64")]
[TestCase (ApplePlatform.MacCatalyst, "maccatalyst-x64;")]
[TestCase (ApplePlatform.TVOS, "tvos-arm64;")]
public void PartialAOTOnlyCoreLib (ApplePlatform platform, string runtimeIdentifiers)
{
var project = "MySimpleApp";
Configuration.IgnoreIfIgnoredPlatform (platform);
Configuration.AssertRuntimeIdentifiersAvailable (platform, runtimeIdentifiers);

var project_path = GetProjectPath (project, runtimeIdentifiers: runtimeIdentifiers, platform: platform, out var appPath);
Clean (project_path);
var properties = GetDefaultProperties (runtimeIdentifiers);
properties ["MtouchInterpreter"] = "all,-System.Private.CoreLib.dll";

DotNet.AssertBuild (project_path, properties);

Assert.True (!FindAssembly (appPath, "aot-instances.dll"), "Dedup optimization shouldn't been enabled for partial AOT compilation");

var appExecutable = GetNativeExecutable (platform, appPath);

if (CanExecute (platform, runtimeIdentifiers)) {
var output = ExecuteWithMagicWordAndAssert (appExecutable);
}
}
}
}
Loading