Enable dedup optimization in FullAOT mode only #20687

kotlarmilos · 2024-06-05T14:34:08Z

Description

This PR enables the dedup optimization in FullAOT mode only. The optimization can only run in FullAOT mode with complete application context. Without it, the AOT compiler may fail to collect all generic instances, and the runtime can't find them as they are searched in the dedup assembly.

Changes

This PR updates the SDK targets to enable dedup optimization in FullAOT mode only. This change doesn't depend on any runtime changes.

Verification

This PR also introduces partial AOT tests. They inspect the bundle for aot-instances.dll, which shouldn't be generated in a partial AOT compilation setup. Additionally, basic functionality is tested by asserting at app startup.

Additional notes

This change should be backported to .NET 8 as well.

Fixes dotnet/runtime#99248

rolfbjarne · 2024-06-05T15:04:58Z

tests/dotnet/UnitTests/ProjectTest.cs

+			var dllPath = Path.Combine (appPath, "aot-instances.dll");
+			Assert.That (dllPath, Does.Not.Exist, "Dedup optimization shouldn't been enabled for partial AOT compilation");


This will always pass on macOS and Mac Catalyst, because assemblies are in a subdirectory of the app bundle (Contents/MonoBundle/*.dll).

I think I would just list all the files in the app bundle, and assert that none of them are named 'aot-instances.dll'

Thank you. I wasn't sure if there was the existing function for .dll search, so I added a new one.

rolfbjarne · 2024-06-05T15:05:17Z

tests/dotnet/UnitTests/ProjectTest.cs

+			var dllPath = Path.Combine (appPath, "aot-instances.dll");
+			Assert.That (dllPath, Does.Not.Exist, "Dedup optimization shouldn't been enabled for partial AOT compilation");


Same as above.

rolfbjarne · 2024-06-05T15:06:59Z

tests/dotnet/UnitTests/ProjectTest.cs

+			var appExecutable = GetNativeExecutable (platform, appPath);
+			ExecuteWithMagicWordAndAssert (platform, runtimeIdentifiers, appExecutable);


This needs to check if we can execute first, like this:

xamarin-macios/tests/dotnet/UnitTests/ProjectTest.cs

Lines 1601 to 1604 in 5566f49

if (CanExecute (platform, runtimeIdentifiers)) {

var output = ExecuteWithMagicWordAndAssert (appExecutable);

Assert.That (output, Does.Contain ("42"), "Execution");

}

Otherwise ExecuteWithMagicWordAndAssert will try to run iOS and tvOS executables on macOS.

github-actions · 2024-06-05T15:37:06Z

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

rolfbjarne · 2024-06-05T16:03:19Z

tests/dotnet/UnitTests/ProjectTest.cs

+			}
+			catch (Exception ex)
+			{
+				Console.WriteLine($"An error occurred: {ex.Message}");


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.

ivanpovazan

LGTM, I only left two minor comments.

ivanpovazan · 2024-06-06T07:34:17Z

tests/dotnet/UnitTests/ProjectTest.cs

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


Shouldn't we test maccatalyst-arm64 as well?

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

ivanpovazan · 2024-06-06T07:36:01Z

tests/dotnet/UnitTests/ProjectTest.cs

+			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";


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.

Good point, thanks.

github-actions · 2024-06-06T08:10:28Z

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

…arin/xamarin-macios into bugfix/dedup-partial-aot-compilation

vs-mobiletools-engineering-service2 · 2024-06-07T09:38:31Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2024-06-07T10:20:38Z

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)

~~iOS~~ (no change detected)
~~tvOS~~ (no change detected)
~~watchOS~~ (no change detected)
~~macOS~~ (no change detected)

NET (empty diffs)

~~iOS~~: (empty diff detected)
~~tvOS~~: (empty diff detected)
~~MacCatalyst~~: (empty diff detected)
~~macOS~~: (empty diff detected)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
watchOS: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)

.NET (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
MacCatalyst: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)
Microsoft.iOS vs Microsoft.MacCatalyst: vsdrops gist

Legacy Xamarin (stable) vs .NET

iOS: vsdrops gist
tvOS: vsdrops gist
macOS: vsdrops gist

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent
Hash: 9863d03b6a6e88af5aa15255fba28b92c79a5584 [PR build]

rolfbjarne · 2024-06-07T11:21:52Z

/sudo backport release/8.0.1xx-xcode15.1

vs-mobiletools-engineering-service2 · 2024-06-07T11:22:20Z

Backport Job to branch release/8.0.1xx-xcode15.1 Created! The magic is happening here

vs-mobiletools-engineering-service2 · 2024-06-07T11:25:08Z

Oh no! Backport failed! Please see https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=9694077 for more details.

… only (#20701) ## Description This PR enables the dedup optimization in FullAOT mode only. The optimization can only run in FullAOT mode with complete application context. Without it, the AOT compiler may fail to collect all generic instances, and the runtime can't find them as they are searched in the dedup assembly. ## Changes This PR updates the SDK targets to enable dedup optimization in FullAOT mode only. This change doesn't depend on any runtime changes. ## Verification This PR also introduces partial AOT tests. They inspect the bundle for `aot-instances.dll`, which shouldn't be generated in a partial AOT compilation setup. Additionally, basic functionality is tested by asserting at app startup. ## Additional notes This change should be backported to .NET 8 as well. Fixes dotnet/runtime#99248 Backport of #20687 --------- Co-authored-by: Milos Kotlar <kotlarmilos@gmail.com>

kotlarmilos added 2 commits June 5, 2024 15:52

Enable dedup optimization only in FullAOT mode

3f0c6cf

Add unit tests for partial AOT compilation

936c053

kotlarmilos added the bug If an issue is a bug or a pull request a bug fix label Jun 5, 2024

kotlarmilos added this to the .NET 9 milestone Jun 5, 2024

kotlarmilos self-assigned this Jun 5, 2024

kotlarmilos requested review from rolfbjarne and ivanpovazan June 5, 2024 14:35

rolfbjarne requested changes Jun 5, 2024

View reviewed changes

kotlarmilos and others added 3 commits June 5, 2024 17:33

Search bundle for aot-instances.dll

f07f29b

Remove unused variable

c1750d9

Auto-format source code

40672df

rolfbjarne reviewed Jun 5, 2024

View reviewed changes

Remove try-catch block

c0c99b5

This comment has been minimized.

Sign in to view

ivanpovazan approved these changes Jun 6, 2024

View reviewed changes

kotlarmilos and others added 2 commits June 6, 2024 10:04

Parametrize tests and add maccatalyst-arm64

f5fc50d

Auto-format source code

ce4d456

This comment has been minimized.

Sign in to view

kotlarmilos added 2 commits June 6, 2024 16:59

Fix tests

2d8cb06

Merge branch 'bugfix/dedup-partial-aot-compilation' of github.com:xam…

8d6f002

…arin/xamarin-macios into bugfix/dedup-partial-aot-compilation

This comment has been minimized.

Sign in to view

dalexsoto approved these changes Jun 7, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

Remove osx test cases since the dedup is mobile-only

9863d03

rolfbjarne approved these changes Jun 7, 2024

View reviewed changes

rolfbjarne merged commit 603781b into main Jun 7, 2024
88 checks passed

vs-mobiletools-engineering-service2 mentioned this pull request Jun 7, 2024

[release/8.0.1xx-xcode15.1] Enable dedup optimization in FullAOT mode only #20701

Merged

ivanpovazan mentioned this pull request Jul 10, 2024

Slow/exploding build times with new workload version 8078 #20848

Closed

dalexsoto mentioned this pull request Jul 16, 2024

Workload 8.0.61 breaks Mono interpreter on MacCatalyst. dotnet/maui#23577

Closed

rolfbjarne deleted the bugfix/dedup-partial-aot-compilation branch October 9, 2024 06:23

		var dllPath = Path.Combine (appPath, "aot-instances.dll");
		Assert.That (dllPath, Does.Not.Exist, "Dedup optimization shouldn't been enabled for partial AOT compilation");

		var appExecutable = GetNativeExecutable (platform, appPath);
		ExecuteWithMagicWordAndAssert (platform, runtimeIdentifiers, appExecutable);

	if (CanExecute (platform, runtimeIdentifiers)) {
	var output = ExecuteWithMagicWordAndAssert (appExecutable);
	Assert.That (output, Does.Contain ("42"), "Execution");
	}

Enable dedup optimization in FullAOT mode only #20687

Enable dedup optimization in FullAOT mode only #20687

Conversation

kotlarmilos commented Jun 5, 2024 • edited Loading

Description

Changes

Verification

Additional notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 5, 2024

⚠️ Your code has been reformatted. ⚠️

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ivanpovazan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 6, 2024

⚠️ Your code has been reformatted. ⚠️

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vs-mobiletools-engineering-service2 commented Jun 7, 2024

📚 [PR Build] Artifacts 📚

Packages generated

vs-mobiletools-engineering-service2 commented Jun 7, 2024

✅ API diff for current PR / commit

✅ API diff vs stable

ℹ️ Generator diff

rolfbjarne commented Jun 7, 2024

vs-mobiletools-engineering-service2 commented Jun 7, 2024

vs-mobiletools-engineering-service2 commented Jun 7, 2024

kotlarmilos commented Jun 5, 2024 •

edited

Loading