Set `DynamicCodeSupport=false` to enable trimming in full AOT mode #18555

ivanpovazan · 2023-07-13T14:54:02Z

This PR sets the feature switch DynamicCodeSupport to false for platforms:

where it's not possible to publish without the AOT compiler
when interpreter is not enabled

This enables ILLink to remove code paths depending on RuntimeFeature.IsDynamicCodeSupported for example:
https://github.com/dotnet/runtime/blob/dedaf46154a8938848fc9a7e0bd6e5ccebe7809c/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Interpreter/CallInstruction.cs#L19

To further clarify which platforms are included for this size optimizations according to comment:

Include iOS and tvOS

Include Mac Catalyst when building for arm64 (x64 has the JIT). This is slightly problematic though, since by default we build for arm64 only for release builds (i.e. the debug build doesn't necessarily know that the release build uses a different runtime identifier). As such, I think it would make sense to handle Mac Catalyst as if arm64 is required and disable dynamic code support even when only building for x64. If someone wants to publish a Mac Catalyst app on x64 only, then can force DynamicCodeSupport=true themselves.

Estimated size savings are shown bellow:

dotnet new maui (ios)	old SLE.dll	new SLE.dll + DynamicCodeSupported=false	diff (%)
Size on disk (Mb)	40,53	38,78	-4,31%
.pkg (Mb)	14,83	14,20	-4,21%

Fixes #18340

ivanpovazan · 2023-07-14T09:20:36Z

The failures seem to be related.
Seems like as we treat maccatalyst-x64 and maccatalyst-arm64 differently now, there is an issue with that in multiple places.
Will look into it.

#88723) - Ship a single assembly for all platforms, and have better compatibility between our AOT engines - Size of the shipped assembly for iOS-like platforms is increased by ~113Kb - The fullAOT + UseInterpreter=true configurations with Mono are now interpreting Ref.Emit generated code instead of using Reflection - To prevent size regressions with Mono xamarin/xamarin-macios#18555 needs to be merged

ivanpovazan · 2023-07-20T09:02:35Z

The failures seem to be related. Seems like as we treat maccatalyst-x64 and maccatalyst-arm64 differently now, there is an issue with that in multiple places. Will look into it.

After an offline discussion with @mandel-macaque it seems this is related to building universal apps.

During the build, the app bundles created for maccatalyst-x64 and maccatalyst-arm64 need to be merged via MergeAppBundlesTaskBase. However, as this PR introduced a change where maccatalyst-arm64 is configured differently -> now has a feature switch enabled for DynamicCodeSupport, while maccatalyst-x64 does not, the build throws at:

xamarin-macios/msbuild/Xamarin.MacDev.Tasks/Tasks/MergeAppBundlesTaskBase.cs

Line 403 in fb147a1

    
           Log.LogError (MSBStrings.E7077 /* Unable to merge the file '{0}', it's different between the input app bundles. */, entries [0].RelativePath);

as it is not possible to merge runtimeconfig.bin as they are different (I am posting the difference between runtimeconfig.jsons which is essentially the same thing):

diff /Users/ivan/repos/xamarin/xamarin-nativeaot/tests/dotnet/AppWithXCAssets/MacCatalyst/bin/Debug/net8.0-maccatalyst/maccatalyst-x64/AppWithXCAssets.runtimeconfig.json /Users/ivan/repos/xamarin/xamarin-nativeaot/tests/dotnet/AppWithXCAssets/MacCatalyst/bin/Debug/net8.0-maccatalyst/maccatalyst-arm64/AppWithXCAssets.runtimeconfig.json
26a27
>       "System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported": false,

Question

How much are we sensitive to maccatalyst app size?

Proposal

As this is purely a size-saving (trimming) optimization, a potential fix to this problem is to exclude maccatalyst-arm64 as a platform that could benefit from it. It is also important to note that if we choose this approach the app size would actually be worse than before due to changes introduced by: dotnet/runtime#88723 because the library now carries Ref.Emit backend (which will not be trimmed if DynamicCodeSupported is not set to false). Before, this was not an issue, because we had library-build-time trimming enabled which was removing Ref.Emit backend from it.

SamMonoRT · 2023-07-20T17:21:57Z

Your proposed solution seems fine to me.

ivanpovazan · 2023-07-21T13:20:38Z

Your proposed solution seems fine to me.

Thanks, I will try to get some app size numbers for the proposed solution regarding maccatalyst-arm64.
But until, then still waiting on some feedback here.

ivanpovazan · 2023-07-25T10:10:32Z

I have excluded maccatalyst-arm64 for now, to see if we can get a green CI with the proposed changes on other platforms.

ivanpovazan · 2023-07-25T16:16:05Z

The failed tests seem unrelated as they are timing out.
I will do another run when we bump the net8.0 branch

dalexsoto · 2023-07-26T14:05:54Z

/azp run

azure-pipelines · 2023-07-26T14:06:16Z

Azure Pipelines successfully started running 1 pipeline(s).

dotnet/targets/Xamarin.Shared.Sdk.targets

… watchOS as it is not supported on dotnet Co-authored-by: Rolf Bjarne Kvinge <[email protected]>

…sable-dynamic-code

vs-mobiletools-engineering-service2 · 2023-08-04T09:33:14Z

💻 [CI Build] Windows Integration Tests passed 💻

✅ All Windows Integration Tests passed.

Pipeline on Agent
Hash: 29187818122138e1a265cd51662a360272a75d78 [PR build]

vs-mobiletools-engineering-service2 · 2023-08-04T09:46:47Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2023-08-04T09:46:52Z

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

✅ All tests on macOS M1 - Mac Big Sur (11.5) passed.

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2023-08-04T09:49:45Z

💻 [PR Build] Tests on macOS M1 - Mac Ventura (13.0) passed 💻

✅ All tests on macOS M1 - Mac Ventura (13.0) passed.

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2023-08-04T09:59:42Z

✅ API diff for current PR / commit

NET (empty diffs)

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

❗ API diff vs stable (Breaking changes)

.NET (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)

iOS: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
tvOS: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
MacCatalyst: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
macOS: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
Microsoft.iOS vs Microsoft.MacCatalyst: vsdrops gist

ℹ️ Generator diff

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

Pipeline on Agent
Hash: 29187818122138e1a265cd51662a360272a75d78 [PR build]

ivanpovazan · 2023-08-04T12:06:28Z

The previous build failures are now gone, but the tests are still failing on CI.
All failures seem to be caused by tests timing out, example:

MonoTouchFixtures.CoreServices.FSEventStreamTest
[05:36:27.7072430] 	[FAIL] TestExtendedFileEvents : System.AggregateException : One or more errors occurred. (test has timed out at 10s; increase the timeout or reduce the number of files created. Created directories: 0 Created files: 0 Removed files: 169 Created then removed files: 256)
[05:36:27.7073610]   ----> System.TimeoutException : test has timed out at 10s; increase the timeout or reduce the number of files created. Created directories: 0 Created files: 0 Removed files: 169 Created then removed files: 256
[05:36:27.7074290] 		   at MonoTouchFixtures.CoreServices.FSEventStreamTest.TestFSMonitor.Run() in /Users/builder/azdo/_work/4/s/xamarin-macios/tests/monotouch-test/CoreServices/FSEventStreamTest.cs:line 150
[05:36:27.7074560] 		   at MonoTouchFixtures.CoreServices.FSEventStreamTest.RunTest(FSEventStreamCreateFlags createFlags) in /Users/builder/azdo/_work/4/s/xamarin-macios/tests/monotouch-test/CoreServices/FSEventStreamTest.cs:line 77
[05:36:27.7074770] 		   at MonoTouchFixtures.CoreServices.FSEventStreamTest.TestExtendedFileEvents() in /Users/builder/azdo/_work/4/s/xamarin-macios/tests/monotouch-test/CoreServices/FSEventStreamTest.cs:line 74
[05:36:27.7074970] 		   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
[05:36:27.7075190] 		   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
[05:36:27.7075390] 		--TimeoutException
[05:36:27.7075930] 		   at MonoTouchFixtures.CoreServices.FSEventStreamTest.TestFSMonitor.CreateFilesAndWaitForFSEventsThread() in /Users/builder/azdo/_work/4/s/xamarin-macios/tests/monotouch-test/CoreServices/FSEventStreamTest.cs:line 203
[05:36:27.7076180] 		   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
[05:36:27.7076420] 		--- End of stack trace from previous location ---
[05:36:27.7076610] 		   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
[05:36:27.7076800] 		   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)

@rolfbjarne should we just try to rerun them?

rolfbjarne · 2023-08-04T15:53:04Z

The previous build failures are now gone, but the tests are still failing on CI. All failures seem to be caused by tests timing out, example:

MonoTouchFixtures.CoreServices.FSEventStreamTest
[05:36:27.7072430] 	[FAIL] TestExtendedFileEvents : System.AggregateException : One or more errors occurred. (test has timed out at 10s; increase the timeout or reduce the number of files created. Created directories: 0 Created files: 0 Removed files: 169 Created then removed files: 256)
[05:36:27.7073610]   ----> System.TimeoutException : test has timed out at 10s; increase the timeout or reduce the number of files created. Created directories: 0 Created files: 0 Removed files: 169 Created then removed files: 256
[05:36:27.7074290] 		   at MonoTouchFixtures.CoreServices.FSEventStreamTest.TestFSMonitor.Run() in /Users/builder/azdo/_work/4/s/xamarin-macios/tests/monotouch-test/CoreServices/FSEventStreamTest.cs:line 150
[05:36:27.7074560] 		   at MonoTouchFixtures.CoreServices.FSEventStreamTest.RunTest(FSEventStreamCreateFlags createFlags) in /Users/builder/azdo/_work/4/s/xamarin-macios/tests/monotouch-test/CoreServices/FSEventStreamTest.cs:line 77
[05:36:27.7074770] 		   at MonoTouchFixtures.CoreServices.FSEventStreamTest.TestExtendedFileEvents() in /Users/builder/azdo/_work/4/s/xamarin-macios/tests/monotouch-test/CoreServices/FSEventStreamTest.cs:line 74
[05:36:27.7074970] 		   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
[05:36:27.7075190] 		   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
[05:36:27.7075390] 		--TimeoutException
[05:36:27.7075930] 		   at MonoTouchFixtures.CoreServices.FSEventStreamTest.TestFSMonitor.CreateFilesAndWaitForFSEventsThread() in /Users/builder/azdo/_work/4/s/xamarin-macios/tests/monotouch-test/CoreServices/FSEventStreamTest.cs:line 203
[05:36:27.7076180] 		   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
[05:36:27.7076420] 		--- End of stack trace from previous location ---
[05:36:27.7076610] 		   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
[05:36:27.7076800] 		   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)

@rolfbjarne should we just try to rerun them?

Yup, done

vs-mobiletools-engineering-service2 · 2023-08-04T18:08:10Z

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: simulator tests.

🎉 All 92 tests passed 🎉

Tests counts

⚠️ bcl: No tests selected. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 4 tests passed. Html Report (VSDrops) Download
✅ framework: All 4 tests passed. Html Report (VSDrops) Download
✅ generator: All 1 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 4 tests passed. Html Report (VSDrops) Download
⚠️ install_source: No tests selected. Html Report (VSDrops) Download
✅ introspection: All 4 tests passed. Html Report (VSDrops) Download
✅ linker: All 40 tests passed. Html Report (VSDrops) Download
⚠️ mac_binding_project: No tests selected. Html Report (VSDrops) Download
⚠️ mmp: No tests selected. Html Report (VSDrops) Download
⚠️ mononative: No tests selected. Html Report (VSDrops) Download
✅ monotouch: All 26 tests passed. [attempt 2] Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
⚠️ mtouch: No tests selected. Html Report (VSDrops) Download
⚠️ xammac: No tests selected. Html Report (VSDrops) Download
✅ xcframework: All 4 tests passed. Html Report (VSDrops) Download
✅ xtro: All 1 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: 29187818122138e1a265cd51662a360272a75d78 [PR build]

ivanpovazan requested review from rolfbjarne and mauroa July 13, 2023 14:54

dalexsoto approved these changes Jul 13, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

ivanpovazan mentioned this pull request Jul 14, 2023

[libs][iOS] Unify System.Linq.Expression.dll build for all platforms dotnet/runtime#88723

Merged

This comment has been minimized.

Sign in to view

ivanpovazan requested review from dalexsoto and mandel-macaque July 28, 2023 12:23

rolfbjarne requested changes Aug 3, 2023

View reviewed changes

dotnet/targets/Xamarin.Shared.Sdk.targets Outdated Show resolved Hide resolved

Inlcude maccatalyst when specifying DynamicCodeSupport=false, exclude…

15c35b4

… watchOS as it is not supported on dotnet Co-authored-by: Rolf Bjarne Kvinge <[email protected]>

ivanpovazan requested a review from rolfbjarne August 3, 2023 08:56

rolfbjarne approved these changes Aug 3, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

ivanpovazan mentioned this pull request Aug 3, 2023

MAUI iOS sample app size regression compared to .NET7 dotnet/runtime#79825

Closed

Merge remote-tracking branch 'upstream/net8.0' into feature-switch-di…

2918781

…sable-dynamic-code

This comment has been minimized.

Sign in to view

ivanpovazan merged commit 0c0e5d7 into xamarin:net8.0 Aug 7, 2023

Set DynamicCodeSupport=false to enable trimming in full AOT mode #18555

Set DynamicCodeSupport=false to enable trimming in full AOT mode #18555

Conversation

ivanpovazan commented Jul 13, 2023 • edited Loading

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 commented Jul 14, 2023 • edited Loading

ivanpovazan commented Jul 20, 2023 • edited Loading

Question

Proposal

SamMonoRT commented Jul 20, 2023

ivanpovazan commented Jul 21, 2023

ivanpovazan commented Jul 25, 2023

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 commented Jul 25, 2023

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.

dalexsoto commented Jul 26, 2023

azure-pipelines bot commented Jul 26, 2023

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 Aug 4, 2023

💻 [CI Build] Windows Integration Tests passed 💻

vs-mobiletools-engineering-service2 commented Aug 4, 2023

📚 [PR Build] Artifacts 📚

Packages generated

vs-mobiletools-engineering-service2 commented Aug 4, 2023

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

vs-mobiletools-engineering-service2 commented Aug 4, 2023

💻 [PR Build] Tests on macOS M1 - Mac Ventura (13.0) passed 💻

vs-mobiletools-engineering-service2 commented Aug 4, 2023

✅ API diff for current PR / commit

❗ API diff vs stable (Breaking changes)

ℹ️ Generator diff

This comment has been minimized.

ivanpovazan commented Aug 4, 2023

rolfbjarne commented Aug 4, 2023

vs-mobiletools-engineering-service2 commented Aug 4, 2023

🚀 [CI Build] Test results 🚀

Test results

Tests counts

Set `DynamicCodeSupport=false` to enable trimming in full AOT mode #18555

Set `DynamicCodeSupport=false` to enable trimming in full AOT mode #18555

ivanpovazan commented Jul 13, 2023 •

edited

Loading

ivanpovazan commented Jul 14, 2023 •

edited

Loading

ivanpovazan commented Jul 20, 2023 •

edited

Loading