Skip to content

Commit

Permalink
[generator] emit faster overloads for non-virtual Formatted props (#1101
Browse files Browse the repository at this point in the history
)

Context: dotnet/maui#12130
Context: #994
Context: 79a8e1e
Context: xamarin/monodroid@23f4212

When binding members which have parameter types or return types which
are `java.lang.CharSequence`, the member is "overloaded" to replace
`CharSequence` with `System.String`, and the "original" member has a
`Formatted` *suffix*.

For example, consider [`android.widget.TextView`][1], which has
[`getText()`][1] and [`setText()`][2] methods which have parameter
types and return types which are `java.lang.CharSequence`:

	// Java
	/* partial */ class TextView extends View {
	    public CharSequence getText();
	    public final void setText(CharSequence text);
	}

When bound, this results in *two* properties:

	// C#
	partial class TextView : View {
	    public  Java.Lang.ICharSequence?    TextFormatted   {get => …; set => …; }
	    public  string?                     Text            {get => …; set => …; }
	}

This is also done for methods; see also 79a8e1e.

The "non-`Formatted` overload" works by creating `String` temporaries
to invoke the `Formatted` overload:

	partial class TextView {
	    public string? Text {
	        get => TextFormatted?.ToString ();
	        set {
	            var jls = value == null ? null : new Java.Lang.String (value);
	            TextFormatted = jls;
	            jls?.Dispose ();
	        }
	    }
	}

*Why* was this done?  Because [C# 4.0][3] didn't allow interfaces to
provide conversion operators.  ([C# 8.0][4] would add support for
interfaces to contain operators.)  "Overloading" in this fashion made
it easier to use `System.String` literals with `ICharSequence` members;
compare:

	view.Text           = "string literal";
	// vs.
	view.TextFormatted  = new Java.Lang.String("string literal");
	// …and who would know how to do this?

A problem with the this approach is performance: creating a new
`Java.Lang.String` instance requires:

 1. Creating the managed peer (the `Java.Lang.String` instance),
 2. Creating the native peer (the `java.lang.String` instance),
 3. And *registering the mapping* between (1) and (2)

which feels a bit "silly" when we immediately dispose of the value.

This is particularly noticeable with .NET MAUI apps.  Consider the
[angelru/CvSlowJittering][5] app, which uses XAML to set `Text`
properties, which eventually hit `TextView.Text`.  Profiling shows:

	653.69ms (6.3%) mono.android!Android.Widget.TextView.set_Text(string)
	198.05ms (1.9%) mono.android!Java.Lang.String..ctor(string)
	121.57ms (1.2%) mono.android!Java.Lang.Object.Dispose()

*6.3%* of scrolling time is spent in the `TextView.Text` property
setter!

*Partially optimize* this case: if the `*Formatted` member is
(1) a property, and (2) *not* `virtual`, then we can directly call
the Java setter method.  This avoids the need to create a managed
peer and to register a mapping between the peers:

	// New hotness
	partial class TextView {
	    public string? Text {
	        get => TextFormatted?.ToString (); // unchanged
	        set {
	            const string __id               = "setText.(Ljava/lang/CharSequence;)V";
	            JniObjectReference native_value = JniEnvironment.Strings.NewString (value);
	            try {
	                JniArgumentValue* __args    = stackalloc JniArgumentValue [1];
	                __args [0] = new JniArgumentValue (native_value);
	                _members.InstanceMethods.InvokeNonvirtualVoidMethod (__id, this, __args);
	            } finally {
	                JniObjectReference.Dispose (ref native_value);
	            }
	        }
	    }
	}

[The result][6]?

|              Method |     Mean |     Error |    StdDev | Allocated |
|-------------------- |---------:|----------:|----------:|----------:|
| Before SetFinalText | 6.632 us | 0.0101 us | 0.0079 us |     112 B |
| After SetFinalText  | 1.361 us | 0.0022 us | 0.0019 us |         - |

The `TextView.Text` property setter invocation time is reduced to 20%
of the previous average invocation time.

Note: We *cannot* do this "inlining" if the "`Formatted` overload" is
[`virtual`][7], as that will result in broken semantics that make
sense to *nobody*.

TODO: Consider Optimizing the "`virtual` overload" scenario?  This
could be done by updating `Java.Lang.String`:

	partial interface ICharSequence {
	    public static String FromJniObjectReference (ref JniObjectReference reference, JniObjectReferenceOptions options);
	}

Then updating the "non-`Formatted` overload" to use
`String.FromJniHandle()`:

	partial class TextView {
	    public string? Text {
	        get => TextFormatted?.ToString (); // unchanged
	        set {
	            JniObjectReference native_value = JniEnvironment.Strings.NewString (value);
	            var                java_value   = Java.Lang.ICharSequence.FromJniObjectReference (ref native_value, JniObjectReferenceOptions.CopyAndDoNotRegister);
	            TextFormatted = java_value;
	            java_value?.Dispose ();
	        }
	    }
	}

[0]: https://developer.android.com/reference/android/widget/TextView
[1]: https://developer.android.com/reference/android/widget/TextView#getText()
[2]: https://developer.android.com/reference/android/widget/TextView#setText(java.lang.CharSequence)
[3]: https://learn.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-version-history#c-version-40
[4]: https://learn.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-version-history#c-version-80
[5]: https://github.com/angelru/CvSlowJittering
[6]: https://github.com/jonathanpeppers/BenchmarkDotNet-Android/tree/Android.Widget.TextView
[7]: #994 (comment)
  • Loading branch information
jonathanpeppers authored Apr 25, 2023
1 parent e7d2edb commit 3c2a066
Show file tree
Hide file tree
Showing 6 changed files with 163 additions and 26 deletions.
105 changes: 105 additions & 0 deletions tests/generator-Tests/Unit-Tests/CodeGeneratorTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1152,6 +1152,59 @@ protected override CodeGenerationOptions CreateOptions ()
};
return options;
}

[Test]
public void StringPropertyOverride ([Values("true", "false")] string final)
{
var xml = @$"<api>
<package name='java.lang' jni-name='java/lang'>
<class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' />
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object'
final='true' name='String' static='false' visibility='public'>
</class>
</package>
<package name='android.widget' jni-name='android/widget'>
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' final='false' name='TextView' static='false' visibility='public'>
<method abstract='false' deprecated='not deprecated' final='{final}' name='getText' bridge='false' native='false' return='java.lang.CharSequence' static='false' synchronized='false' synthetic='false' visibility='public'>
</method>
<method abstract='false' deprecated='not deprecated' final='{final}' name='setText' bridge='false' native='false' return='void' static='false' synchronized='false' synthetic='false' visibility='public'>
<parameter name='text' type='java.lang.CharSequence'>
</parameter>
</method>
</class>
</package>
</api>";

var gens = ParseApiDefinition (xml);
var klass = gens.Single (g => g.Name == "TextView");

generator.Context.ContextTypes.Push (klass);
generator.WriteType (klass, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
generator.Context.ContextTypes.Pop ();

if (final == "true") {
Assert.True (writer.ToString ().Contains (
@"set {
const string __id = ""setText.(Ljava/lang/CharSequence;)V"";
global::Java.Interop.JniObjectReference text = global::Java.Interop.JniEnvironment.Strings.NewString (value);
try {
JniArgumentValue* __args = stackalloc JniArgumentValue [1];
__args [0] = new JniArgumentValue (text);
_members.InstanceMethods.InvokeNonvirtualVoidMethod (__id, this, __args);
} finally {
global::Java.Interop.JniObjectReference.Dispose (ref text);
}
}
}"), $"was: `{writer}`");
} else {
Assert.True (writer.ToString ().Contains (
@"set {
var jls = value == null ? null : new global::Java.Lang.String (value);
TextFormatted = jls;
if (jls != null) jls.Dispose ();
}"), $"was: `{writer}`");
}
}
}

[TestFixture]
Expand Down Expand Up @@ -1236,6 +1289,58 @@ public void SupportedOSPlatformConstFields ()

StringAssert.Contains ("[global::System.Runtime.Versioning.SupportedOSPlatformAttribute (\"android30.0\")]", builder.ToString (), "Should contain SupportedOSPlatform!");
}

[Test]
public void StringPropertyOverride ([Values ("true", "false")] string final)
{
var xml = @$"<api>
<package name='java.lang' jni-name='java/lang'>
<class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' />
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object'
final='true' name='String' static='false' visibility='public'>
</class>
</package>
<package name='android.widget' jni-name='android/widget'>
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' final='false' name='TextView' static='false' visibility='public'>
<method abstract='false' deprecated='not deprecated' final='{final}' name='getText' bridge='false' native='false' return='java.lang.CharSequence' static='false' synchronized='false' synthetic='false' visibility='public'>
</method>
<method abstract='false' deprecated='not deprecated' final='{final}' name='setText' bridge='false' native='false' return='void' static='false' synchronized='false' synthetic='false' visibility='public'>
<parameter name='text' type='java.lang.CharSequence'>
</parameter>
</method>
</class>
</package>
</api>";

var gens = ParseApiDefinition (xml);
var klass = gens.Single (g => g.Name == "TextView");

generator.Context.ContextTypes.Push (klass);
generator.WriteType (klass, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
generator.Context.ContextTypes.Pop ();

if (final == "true") {
Assert.True (writer.ToString ().Contains (
@"set {
const string __id = ""setText.(Ljava/lang/CharSequence;)V"";
global::Java.Interop.JniObjectReference native_text = global::Java.Interop.JniEnvironment.Strings.NewString (value);
try {
JniArgumentValue* __args = stackalloc JniArgumentValue [1];
__args [0] = new JniArgumentValue (native_text);
_members.InstanceMethods.InvokeNonvirtualVoidMethod (__id, this, __args);
} finally {
global::Java.Interop.JniObjectReference.Dispose (ref native_text);
}
}"), $"was: `{writer}`");
} else {
Assert.True (writer.ToString ().Contains (
@"set {
var jls = value == null ? null : new global::Java.Lang.String (value);
TextFormatted = jls;
if (jls != null) jls.Dispose ();
}"), $"was: `{writer}`");
}
}
}

[TestFixture]
Expand Down
7 changes: 4 additions & 3 deletions tools/generator/SourceWriters/BoundClass.cs
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ void AddProperty (ClassGen klass, Property property, CodeGenerationOptions opt)
Properties.Add (bound_property);

if (property.Type.StartsWith ("Java.Lang.ICharSequence", StringComparison.Ordinal) && !bound_property.IsOverride)
Properties.Add (new BoundPropertyStringVariant (property, opt));
Properties.Add (new BoundPropertyStringVariant (property, opt, bound_property));
}

void AddAbstractPropertyDeclaration (ClassGen klass, Property property, CodeGenerationOptions opt)
Expand All @@ -361,10 +361,11 @@ void AddAbstractPropertyDeclaration (ClassGen klass, Property property, CodeGene
}
}

Properties.Add (new BoundAbstractProperty (klass, property, opt));
var bound_property = new BoundAbstractProperty (klass, property, opt);
Properties.Add (bound_property);

if (property.Type.StartsWith ("Java.Lang.ICharSequence", StringComparison.Ordinal))
Properties.Add (new BoundPropertyStringVariant (property, opt));
Properties.Add (new BoundPropertyStringVariant (property, opt, bound_property));
}

void AddNestedTypes (ClassGen klass, CodeGenerationOptions opt, CodeGeneratorContext context, GenerationInfo genInfo)
Expand Down
4 changes: 2 additions & 2 deletions tools/generator/SourceWriters/BoundInterface.cs
Original file line number Diff line number Diff line change
Expand Up @@ -215,13 +215,13 @@ void AddProperties (InterfaceGen iface, CodeGenerationOptions opt)
Properties.Add (bound_property);

if (prop.Type.StartsWith ("Java.Lang.ICharSequence", StringComparison.Ordinal) && !bound_property.IsOverride)
Properties.Add (new BoundPropertyStringVariant (prop, opt));
Properties.Add (new BoundPropertyStringVariant (prop, opt, bound_property));
} else {
var bound_property = new BoundProperty (iface, prop, opt, true, false);
Properties.Add (bound_property);

if (prop.Type.StartsWith ("Java.Lang.ICharSequence", StringComparison.Ordinal) && !bound_property.IsOverride)
Properties.Add (new BoundPropertyStringVariant (prop, opt));
Properties.Add (new BoundPropertyStringVariant (prop, opt, bound_property));
}
}
}
Expand Down
34 changes: 30 additions & 4 deletions tools/generator/SourceWriters/BoundPropertyStringVariant.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,15 @@ namespace generator.SourceWriters
// an overload with type 'string' as a convenience for the user.
public class BoundPropertyStringVariant : PropertyWriter
{
public BoundPropertyStringVariant (Property property, CodeGenerationOptions opt)
public BoundPropertyStringVariant (Property property, CodeGenerationOptions opt, BoundAbstractProperty original)
: this (property, opt, original.IsVirtual)
{ }

public BoundPropertyStringVariant (Property property, CodeGenerationOptions opt, BoundProperty original)
: this(property, opt, original.IsVirtual)
{ }

private BoundPropertyStringVariant (Property property, CodeGenerationOptions opt, bool isOriginalVirtual)
{
var is_array = property.Getter.RetVal.IsArray;

Expand Down Expand Up @@ -46,9 +54,27 @@ public BoundPropertyStringVariant (Property property, CodeGenerationOptions opt)
SetBody.Add ($"{property.AdjustedName} = jlsa;");
SetBody.Add ($"foreach (var jls in jlsa) if (jls != null) jls.Dispose ();");
} else {
SetBody.Add ($"var jls = value == null ? null : new global::Java.Lang.String (value);");
SetBody.Add ($"{property.AdjustedName} = jls;");
SetBody.Add ($"if (jls != null) jls.Dispose ();");
if (isOriginalVirtual) {
SetBody.Add ($"var jls = value == null ? null : new global::Java.Lang.String (value);");
SetBody.Add ($"{property.AdjustedName} = jls;");
SetBody.Add ($"if (jls != null) jls.Dispose ();");
} else {
// Emit a "fast" path if the property is non-virtual
IsUnsafe = true;
var method = property.Setter;
var parameter = method.Parameters [0];

SetBody.Add ($"const string __id = \"{method.JavaName}.{method.JniSignature}\";");
SetBody.Add ($"global::Java.Interop.JniObjectReference {parameter.ToNative (opt)} = global::Java.Interop.JniEnvironment.Strings.NewString (value);");

SetBody.Add ("try {");

SourceWriterExtensions.AddMethodBodyTryBlock (SetBody, method, opt);

SetBody.Add ("} finally {");
SetBody.Add ($"\tglobal::Java.Interop.JniObjectReference.Dispose (ref {parameter.ToNative (opt)});");
SetBody.Add ("}");
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion tools/generator/SourceWriters/ClassInvokerClass.cs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ void AddPropertyInvokers (ClassGen klass, IEnumerable<Property> properties, Hash
Properties.Add (bound_property);

if (prop.Type.StartsWith ("Java.Lang.ICharSequence", StringComparison.Ordinal) && !bound_property.IsOverride)
Properties.Add (new BoundPropertyStringVariant (prop, opt));
Properties.Add (new BoundPropertyStringVariant (prop, opt, bound_property));
}
}

Expand Down
37 changes: 21 additions & 16 deletions tools/generator/SourceWriters/Extensions/SourceWriterExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,27 @@ public static void AddMethodBody (List<string> body, Method method, CodeGenerati

body.Add ("try {");

AddMethodBodyTryBlock (body, method, opt);

if (method.IsCompatVirtualMethod) {
body.Add ("}");
body.Add ("catch (Java.Lang.NoSuchMethodError) {");
body.Add (" throw new Java.Lang.AbstractMethodError (__id);");
}

body.Add ("} finally {");

foreach (string cleanup in method.Parameters.GetCallCleanup (opt))
body.Add ("\t" + cleanup);

foreach (var p in method.Parameters.Where (para => para.ShouldGenerateKeepAlive ()))
body.Add ($"\tglobal::System.GC.KeepAlive ({opt.GetSafeIdentifier (p.Name)});");

body.Add ("}");
}

public static void AddMethodBodyTryBlock (List<string> body, Method method, CodeGenerationOptions opt)
{
AddParameterListCallArgs (body, method.Parameters, opt, false);

var invokeType = JavaInteropCodeGenerator.GetInvokeType (method.RetVal.CallMethodPrefix);
Expand Down Expand Up @@ -265,22 +286,6 @@ public static void AddMethodBody (List<string> body, Method method, CodeGenerati
}
body.Add ($"\treturn {method.RetVal.ReturnCast}{method.RetVal.FromNative (opt, r, true) + opt.GetNullForgiveness (method.RetVal)};");
}

if (method.IsCompatVirtualMethod) {
body.Add ("}");
body.Add ("catch (Java.Lang.NoSuchMethodError) {");
body.Add (" throw new Java.Lang.AbstractMethodError (__id);");
}

body.Add ("} finally {");

foreach (string cleanup in method.Parameters.GetCallCleanup (opt))
body.Add ("\t" + cleanup);

foreach (var p in method.Parameters.Where (para => para.ShouldGenerateKeepAlive ()))
body.Add ($"\tglobal::System.GC.KeepAlive ({opt.GetSafeIdentifier (p.Name)});");

body.Add ("}");
}

public static void AddParameterListCallArgs (List<string> body, ParameterList parameters, CodeGenerationOptions opt, bool invoker)
Expand Down

0 comments on commit 3c2a066

Please sign in to comment.