[XM] Prototype of AOT support in Xamarin.Mac #1340

chamons · 2016-12-12T19:31:13Z

No description provided.

chamons · 2016-12-12T19:31:22Z

Let's get this out here for testing.

spouliot

beside the name, the rest is fine :)

chamons · 2016-12-12T20:13:34Z

@spouliot I'll %s/experimental-xm-aot/xm-aot/gc after QA approves it and we're going to master. I just want a scary sounding name for testing.

monojenkins · 2016-12-12T20:55:11Z

Build success

rolfbjarne · 2016-12-13T09:19:28Z

tools/mmp/driver.cs

@@ -358,6 +359,7 @@ static void Main2 (string [] args)
 				{ "xamarin-framework-directory=", "The framework directory", v => { xm_framework_dir = v; }, true },
 				{ "xamarin-full-framework", "Used with --target-framework=4.5 to select XM 4.5 Target Framework", v => { IsUnifiedFullXamMacFramework = true; } },
 				{ "xamarin-system-framework", "Used with --target-framework=4.5 to select XM 4.5 Target Framework", v => { IsUnifiedFullSystemFramework = true; } },
+				{ "experimental-xm-aot", "Enable experimental AOT Xamarin.Mac support. Careful, the pieces might be sharp.", v => { experimental_xm_aot = true; }, true }, // Experimental - If it breaks for now you get to keep the pieces 


I would name this experimental-aot (imho the xm part is redundant).

rolfbjarne · 2016-12-13T09:20:15Z

tools/mmp/driver.cs

@@ -771,12 +773,35 @@ static void Pack (IList<string> unprocessed)
 			if (App.LinkMode != LinkMode.All && App.RuntimeOptions != null)
 				App.RuntimeOptions.Write (App.AppDirectory);

+			if (experimental_xm_aot.HasValue && experimental_xm_aot.Value && IsUnified) {


Maybe show an error if aot is requested for Classic instead of silently ignoring it.

rolfbjarne · 2016-12-13T09:22:40Z

tools/mmp/driver.cs

 			if (!string.IsNullOrEmpty (certificate_name)) {
 				CodeSign ();
 				Watch ("Code Sign", 1);
 			}
 		}

+		static void AOTBundle ()
+		{
+			string monoExe = Is64Bit ? "/usr/local/bin/mono64" : "/usr/local/bin/mono";


I think /Library/Frameworks/Mono.framework/Commands is the supported location of the mono binaries.

Also I'd use mono32 and mono64, since I think the mono binary one day will switch to be 64-bit.

hmm... not sure mono32 can AOT 64bits code.

I think that XM needs to ship it's own AOT compilers since the system mono might not match with the BCL version XM ships.

IOW the above code should be fine when using the system mono (as long as both 32 and 64 bits are shipping) but it might fail for the bundle 4.5 support coming out of XM.

For a first preview it might be good enough to specify which mono version(s) is/are compatible.

rolfbjarne · 2016-12-13T09:25:44Z

tools/mmp/driver.cs

+			string quotedFileList = string.Join (" ", dirInfo.EnumerateFiles ().Where (aotableFile).Select (x => Quote(x.ToString ())));
+			foreach (var file in dirInfo.EnumerateFiles ().Where (aotableFile))
+			{
+				if (RunCommand (monoExe, String.Format ("--aot=hybrid {0}", Quote (file.ToString ())), new string [] {"MONO_PATH", mmp_dir }) > 0)


This can be parallelized to speed it up significantly.

mtouch currently uses Parallel.ForEach to AOT in parallel [1] (the default is to set MaxDegreeOfParallelism to the number of processors, but this can be overridden by passing -jX /--jobs X (like for make) to motuch).

monojenkins · 2016-12-16T23:10:36Z

Build failure

monojenkins · 2016-12-21T00:23:59Z

Build failure

rolfbjarne · 2016-12-21T09:58:53Z

tools/mmp/driver.cs

+						case "all":
+							aot = AotType.All;
+							break;
+						case "platform":


Another possibility here is sdk: every assembly in the XM SDK (that we ship).

Another syntax idea (but yours is fine if you like it better):

--aot:+System.dll,-MyCustom.dll

which would set AotType.Explicit, enable AOT for System.dll and disable it for MyCustom.dll

This is how mtouch' --dlsym: option works.

@rolfbjarne Lining up with how mtouch does things is good enough for me. :) I'll rework

rolfbjarne · 2016-12-21T10:01:47Z

tools/mmp/driver.cs

+				case AotType.Explicit:
+					return aot_explicit_reference.Contains (x.Name);
+				default:
+					throw new InvalidOperationException ($"AOTBundle with aot: {aot}?");


This should have a better error number, maybe something like this: https://github.com/rolfbjarne/xamarin-macios/blob/framework-sdk/tools/mtouch/error.cs#L108

rolfbjarne · 2016-12-21T10:03:52Z

tools/mmp/driver.cs

+
+			Parallel.ForEach (dirInfo.EnumerateFiles ().Where (aotableFile), new ParallelOptions () { MaxDegreeOfParallelism = Driver.Concurrency }, file => {
+				if (RunCommand (monoExe, String.Format ("--aot=hybrid {0}", Quote (file.ToString ())), new string [] {"MONO_PATH", mmp_dir }) > 0)
+					ErrorHelper.Warning (1337, string.Format ("AOT failed on {0}", file));


Maybe make this an error? Users can now ignore specific assemblies if they're causing trouble, while still AOT-ing the rest, so making it an error won't prevent people from enabling AOT.

Yeah, it was a warning from back before ignore was an option. 👍

monojenkins · 2016-12-21T17:45:20Z

Build failure

monojenkins · 2016-12-21T19:07:38Z

Build success

spouliot · 2016-12-22T13:30:15Z

tools/mmp/error.cs

 	//					MM5103	Failed to compile. Error code - {0}. Please file a bug report at http://bugzilla.xamarin.com
+	//					MM5104  ** reserved mtouch **
+	//					MM5105	Failed to AOT compile. Error code - {0}. Please file a bug report at http://bugzilla.xamarin.com


MT3xxx segment is reserved for AOT compiler errors

monojenkins · 2016-12-29T18:28:26Z

Build failure

rolfbjarne · 2017-01-02T07:14:56Z

tools/mmp/aot.cs

+					aotFiles.Add (file);
+					break;
+				case AotType.SDK:
+					if (fileName == "Xamarin.Mac.dll" || fileName == "System.dll" || fileName == "mscorlib.dll")


Any particular reason for limiting "SDK" to these three assemblies? We already have a Profile.IsSdkAssembly method you can use: https://github.com/xamarin/xamarin-macios/blob/master/tools/mmp/driver.cs#L687

No good reason. Let me use that.

So @rolfbjarne there is apparently a good reason:

https://github.com/xamarin/xamarin-macios/blob/master/tools/mmp/linker/MonoMac.Tuner/MonoMacProfile.cs#L106

The list of SDK assemblies turns out to be the entire BCL pretty much + the Xamarin.Mac bits.

The idea behind the SDK option was to be a middle ground of the stuff pulled in always while being fast.

I'm open to changing this list, or renaming it if you can think of a better name. Swapping to Profile.IsSdkAssembly would make it almost the same as All.

Let's not overload (and confuse) the meaning of SDK in the tooling. Rename it to something else, e.g. core ?

FWIW sdk might be an valid (we lack data to see if it's a good one) option, it's definitively a good one for the linker (and we should avoid confusion between them).

I like core as well, for the same reasons Sebastien mentions.

rolfbjarne · 2017-01-02T07:17:24Z

tools/mmp/aot.cs

+			if (!IsAOT)
+				throw ErrorHelper.CreateError (0099, "Internal error \"AOTBundle with aot: {0}\" Please file a bug report with a test case (http://bugzilla.xamarin.com).", aotType);
+
+			const string MonoExePath = "/Library/Frameworks/Xamarin.Mac.framework/Commands/bmac-mobile-mono";


This doesn't take XAMMAC_FRAMEWORK_PATH into account, you already have a method to get it: https://github.com/xamarin/xamarin-macios/blob/master/tools/mmp/driver.cs#L963

Good catch.

- Stop hard coded bmac-mobile-mono path - Rename sdk to core - Add real sdk option to AOT

monojenkins · 2017-01-03T22:52:21Z

Build failure

chamons · 2017-01-04T14:50:21Z

Build error is my fault. Fixing.

monojenkins · 2017-01-04T16:45:04Z

Build success

chamons · 2017-01-06T17:20:10Z

@spouliot / @rolfbjarne Any more changes for this PR?

chamons · 2017-01-06T17:21:15Z

The only outstanding thing on my end is to add the 32-bit mono to what we ship and remove the "only allow AOT in 64-bit" check.

chamons · 2017-01-09T22:52:03Z

Ok. 32-bit changes are in.

QA got back to us and said it will be awhile until they can test things manually.

I'd like to get this in master now. We can rip out the option late in C10 if things unlikely don't work out.

@rolfbjarne / @spouliot - Ready to go? Can I get an approve changes if 👍

monojenkins · 2017-01-10T00:11:26Z

Build failure

spouliot · 2017-01-10T01:25:59Z

@chamons 👍 please update the release notes in the wiki

rolfbjarne · 2017-01-10T09:25:37Z

tools/mmp/aot.cs

+			if (!IsAOT)
+				throw ErrorHelper.CreateError (0099, "Internal error \"AOTBundle with aot: {0}\" Please file a bug report with a test case (http://bugzilla.xamarin.com).", aotType);
+
+			string monoExePath = Path.Combine (XamarinMacPrefix, "bin/bmac-mobile-mono" + (Is64Bit ? "" : "-32"));


Does this work for all the three profiles (mobile, xm 4.5 and system)? I believe we'll have to use the system mono when using the system profile.

Bah. You are right. I incorrectly assumed that as long as we got the assemblies in place by the rest of the system, AOT could not care about which mono (system or built in) was used.

Let me rework.

spouliot

👍

monojenkins · 2017-01-10T17:59:09Z

Build failure

monojenkins · 2017-01-10T21:16:00Z

Build failure

chamons · 2017-01-10T22:23:33Z

build

monojenkins · 2017-01-11T00:22:46Z

Build failure

chamons · 2017-01-11T17:27:48Z

build

monojenkins · 2017-01-11T18:50:59Z

Build failure

chamons · 2017-01-11T20:08:44Z

Test failure is a known issue in the mac bindings tests and is unrelated I believe.

[XM] Prototype of AOT support in Xamarin.Mac

601750c

chamons self-assigned this Dec 12, 2016

dnfclas added the cla-already-signed label Dec 12, 2016

spouliot approved these changes Dec 12, 2016

View reviewed changes

rolfbjarne requested changes Dec 13, 2016

View reviewed changes

chamons added 4 commits December 16, 2016 14:26

Remove unnecessary allocations of Version in Static Registrar

54d6bef

Merge branch 'master' into xm_aot

4dccc16

Add AOT test

30798b0

Code review changes

16c4c35

rolfbjarne approved these changes Dec 19, 2016

View reviewed changes

Add various options to control AOT process

ee82602

rolfbjarne reviewed Dec 21, 2016

View reviewed changes

chamons added 3 commits December 21, 2016 10:16

Move from InvalidOperationException to error 0099 for clarity

43f1ba0

Code review changes

cdc21e4

Fix test error and missing file

e7c0ca8

spouliot suggested changes Dec 22, 2016

View reviewed changes

chamons added 3 commits December 28, 2016 12:48

Refactor all of AOT code into seperate tested assembly

be3c3e3

Fix unnecessary diff

8bc9968

Merge branch 'master' into xm_aot

6aab530

rolfbjarne reviewed Jan 2, 2017

View reviewed changes

rolfbjarne requested changes Jan 2, 2017

View reviewed changes

Code review changes

6b8e65e

- Stop hard coded bmac-mobile-mono path - Rename sdk to core - Add real sdk option to AOT

Fix msbuild test by removing unnecesary MMP_TEST define

e2d2f0c

Add 32-bit AOT support

1c73430

spouliot approved these changes Jan 10, 2017

View reviewed changes

rolfbjarne reviewed Jan 10, 2017

View reviewed changes

Re-add 0098 error on just classic now

74f0c65

spouliot reviewed Jan 10, 2017

View reviewed changes

Fix AOT to use system mono when requested instead of built in mono

65a95bf

rolfbjarne approved these changes Jan 10, 2017

View reviewed changes

chamons removed the requires-qa-before-merge The pull request requires QA to approve it before it can be merged label Jan 11, 2017

chamons merged commit ffe142d into master Jan 11, 2017

chamons deleted the xm_aot branch January 11, 2017 20:10

[XM] Prototype of AOT support in Xamarin.Mac #1340

[XM] Prototype of AOT support in Xamarin.Mac #1340

Conversation

chamons commented Dec 12, 2016

chamons commented Dec 12, 2016

spouliot left a comment

Choose a reason for hiding this comment

chamons commented Dec 12, 2016

monojenkins commented Dec 12, 2016

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

monojenkins commented Dec 16, 2016

monojenkins commented Dec 21, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Dec 21, 2016

monojenkins commented Dec 21, 2016

Choose a reason for hiding this comment

monojenkins commented Dec 29, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Jan 3, 2017

chamons commented Jan 4, 2017

monojenkins commented Jan 4, 2017

chamons commented Jan 6, 2017

chamons commented Jan 6, 2017

chamons commented Jan 9, 2017

monojenkins commented Jan 10, 2017

spouliot commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spouliot left a comment

Choose a reason for hiding this comment

monojenkins commented Jan 10, 2017

monojenkins commented Jan 10, 2017

chamons commented Jan 10, 2017

monojenkins commented Jan 11, 2017

chamons commented Jan 11, 2017

monojenkins commented Jan 11, 2017

chamons commented Jan 11, 2017