[marshal methods] Part 2 of ? #7123

grendello · 2022-06-23T17:38:11Z

Implement a marshal method candidate classifier which is passed to JavaCallableWrapperGenerator
in order to identify methods that can be registered "statically" and for which we can generate
native code instead of using System.Reflection.Emit.

The selected methods are marked with the [UnmanagedCallersOnly] attribute and their support
code in the form of the connector method (which returns a delegate to the native callback) as
well as its backing field which stores the delegate, are removed from the assemblies.

#998) Context: dotnet/android#7123 Context: fb94d59 Instead of having `JavaCallableWrapperGenerator` care about and collect marshal methods override information, introduce a new `JavaCallableMethodClassifier` type, and use that as the integration point: public abstract partial class JavaCallableMethodClassifier { public abstract bool ShouldBeDynamicallyRegistered TypeDefinition topType, MethodDefinition registeredMethod, MethodDefinition implementedMethod, CustomAttribute registerAttribute ); } An instance of `JavaCallableMethodClassifier` can now be provided to the `JavaCallableWrapperGenerator` constructor. If provided, then `ShouldBeDynamicallyRegistered()` will be used to control which methods are added to `__md_methods` and `Runtime.register()`. If a type `JavaCallableWrapperGenerator` is processing doesn't have any dynamically registered methods, then the code to register them is no longer generated.

src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs

jonpryor · 2022-07-13T23:11:20Z

src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs


 				res.Load (assembly.ItemSpec);
+#if ENABLE_MARSHAL_METHODS
+				if (!Debug) {


Why not skip these if (!Debug) StoreMarshalAssemblyPath() checks and just call StoreMarshalAssemblyPath())? StoreMarshalAssemblyPath() checks Debug anyway, so this is just adding extra levels of complexity & indentation, for no observable benefit.

In the first case we save one string allocation, in the second instance yes, it's not necessary

jonpryor · 2022-07-13T23:18:21Z

src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs


-			// Step 2 - Generate type maps
-			//   Type mappings need to use all the assemblies, always.
-			WriteTypeMappings (allJavaTypes, cache);


Why move this? Type mappings should be unrelated to the contents of the .java files, and allJavaTypes appears to be unchanged, so…. What "hidden"/"non-obvious" bits am I not seeing?

Generating marshal methods also potentially rewrites assemblies, thus changing token ids and type maps must absolutely see the final ones.

jonpryor · 2022-07-13T23:20:48Z

src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs

 						jti.Generate (writer);
 #if ENABLE_MARSHAL_METHODS
-						overriddenMethodDescriptors.AddRange (jti.OverriddenMethodDescriptors);
+						if (!Debug) {


We could remove this if + indentation if we just did if (classifier?.FoundDynamicallyRegisteredMethods), thus implicitly relying on the fact that classifier is null in Debug builds.

But that means relying on "implicit context".

Not sure which tradeoff is "better": minimizing indentation or keeping things explicit.

I much prefer if the code is explicit, makes it easier to understand the flow for someone not familiar with it

src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsAssemblyRewriter.cs

jonpryor · 2022-07-13T23:34:08Z

src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsAssemblyRewriter.cs

+				method.CallbackField?.DeclaringType.Fields.Remove (method.CallbackField);
+			}
+
+			Console.WriteLine ();


I think we'll be wanting Log.LogDebugMessage(), not Console.WriteLine()…

The CWLs will be gone eventually, I will remove them in the last PR in the series

jonpryor · 2022-07-13T23:35:07Z

src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsAssemblyRewriter.cs

+			// versions around.
+			foreach (string path in newAssemblyPaths) {
+				string target = Path.Combine (Path.GetDirectoryName (path), Path.GetFileNameWithoutExtension (path));
+				MoveFile (path, target);


We may want/need to rework the build system so that we write files into new/separate directories instead of modifying files in-place, otherwise we'll provoke the incremental build + file sharing gods.

I asked @jonathanpeppers about this and he thought it was better to write them in place. We can optimize this in the last PR in the series though

jonpryor · 2022-07-13T23:37:05Z

src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsAssemblyRewriter.cs

+		}
+	}
+}
+#endif


At this point I suspect I'm the only one who remembers this convention, but please place a comment on #endif with the #if expression:

#endif // ENABLE_MARSHAL_METHODS

I apply the convention when it's there to stay :) This macro is going to go away in the last PR of the series, it's just a measure to not "poison" current code until marshal methods are finished

src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsClassifier.cs

jonpryor · 2022-07-13T23:41:01Z

src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsClassifier.cs

+			RegisteredMethod = registeredMethod ?? throw new ArgumentNullException (nameof (registeredMethod));
+			ImplementedMethod = implementedMethod ?? throw new ArgumentNullException (nameof (implementedMethod));
+			CallbackField = callbackField;
+			JniTypeName = jniTypeName;


Should we also verify that these are not null?

CallbackField is not required, I'll add checks for the Jni* arguments

jonpryor · 2022-07-13T23:42:58Z

src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsClassifier.cs

+		public IDictionary<string, IList<MarshalMethodEntry>> MarshalMethods => marshalMethods;
+		public ICollection<AssemblyDefinition> Assemblies => assemblies;
+		public bool FoundDynamicallyRegisteredMethods => haveDynamicMethods;
+#endif


Ditto #endif comment…

jonpryor · 2022-07-13T23:44:08Z

src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsClassifier.cs

+
+		TypeDefinitionCache tdCache;
+		DirectoryAssemblyResolver resolver;
+		Dictionary<string, IList<MarshalMethodEntry>> marshalMethods;


Given that this is a private field, why IList<MarshalMethodEntry> and not just List<MarshalMethodEntry>? Interface method invocation is (slightly) slower than class method invocation.

The field is exported by the MarshalMethods property

src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs

src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsAssemblyRewriter.cs

Implement a marshal method candidate classifier which is passed to JavaCallableWrapperGenerator in order to identify methods that can be registered "statically" and for which we can generate native code instead of using System.Reflection.Emit. The selected methods are marked with the `[UnmanagedCallersOnly]` attribute and their support code in the form of the connector method (which returns a delegate to the native callback) as well as its backing field which stores the delegate, are removed from the assemblies.

jonpryor · 2022-07-18T16:48:39Z

[Xamarin.Android.Build.Tasks, monodroid] Marshal Method Classifier

Context: e1af9587bb98d4c249bbc392ceccc2b53ffff155
Context: https://github.com/xamarin/xamarin-android/pull/7163

Changes: https://github.com/xamarin/java.interop/compare/c942ab683c12e88e0527ed584a13b411e005ea57...fadbb82c3b8ab7979c19e9f139bdf2589e47549e

  * xamarin/java.interop@fadbb82c: [generator] Add support for @explicitInterface metadata (#1006)
  * xamarin/java.interop@3e4a3c4f: [Java.Interop.Tools.JavaCallableWrappers] JavaCallableMethodClassifier (#998)


xamarin/java.interop@3e4a3c4f reworked how
`Java.Interop.Tools.JavaCallableWrappers.JavaCallableWrapperGenerator`
can be used to find JNI marshal methods.

Update `Xamarin.Android.Build.Tasks.dll` to provide a
`JavaCallableMethodClassifier` to `JavaCallableWrapperGenerator`,
collecting Cecil `MethodDefinition` instances for JNI marshal methods
which can be created at build time via LLVM Marshal Methods.

Methods which cannot be created at build time include methods with
`[Export]`.

Once candidate LLVM marshal methods are found, the marshal method is
updated to have the [`UnmanagedCallersOnlyAttribute` attribute][0],
and related System.Reflection.Emit-based infrastructure such as the
`Get…Handler()` methods and `cb_…` fields are removed.

As with e1af9587, this feature is *not* enabled by default, and
remains a xamarin-android Build time configuration option.

[0]:https://docs.microsoft.com/dotnet/api/system.runtime.interopservices.unmanagedcallersonlyattribute?view=net-6.0

grendello force-pushed the marshal-methods-generator branch 5 times, most recently from b069552 to 8f142d8 Compare June 30, 2022 20:51

grendello force-pushed the marshal-methods-generator branch 6 times, most recently from cc9e57a to e18d523 Compare July 8, 2022 10:13

grendello force-pushed the marshal-methods-generator branch from e18d523 to d088e1f Compare July 8, 2022 14:47

grendello force-pushed the marshal-methods-generator branch from d088e1f to b0e8cd8 Compare July 11, 2022 08:14

grendello marked this pull request as ready for review July 11, 2022 08:14

grendello requested review from dellis1972, jonathanpeppers and jonpryor as code owners July 11, 2022 08:14

grendello mentioned this pull request Jul 11, 2022

[marshal methods] Part 3 of ? #7163

Merged

grendello force-pushed the marshal-methods-generator branch from b0e8cd8 to 1a1ffca Compare July 13, 2022 08:30