-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[generator] emit faster/better overloads for String #1101
Conversation
See 15671db if you want to see the diff of generator output |
tools/generator/SourceWriters/Extensions/SourceWriterExtensions.cs
Outdated
Show resolved
Hide resolved
Context: #994 Testing a .NET MAUI application, I found this while scrolling: 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() This appears to be a high-impact area for .NET MAUI apps. Previously, for `TextView.Text` we emitted something like: public partial class TextView { public string? Text { get => TextFormatted == null ? null : TextFormatted.ToString (), set { var jls = value == null ? null : new global::Java.Lang.String (value); TextFormatted = jls; if (jls != null) jls.Dispose (); } } } Where instead we could emit: 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); } } I added a test for these `String` overloads, as there wasn't exactly one that tested this exact scenario. Testing on a Pixel 5 in a .NET 8 BenchmarkDotNet project: public class TextViewBenchmark { readonly TextView textView = new TextView(Application.Context); [Benchmark] public void SetText() => textView.Text = "Hello World!"; } https://github.com/jonathanpeppers/BenchmarkDotNet-Android/blob/Android.Widget.TextView/DotNetRunner/TextViewBenchmark.cs | Method | Mean | Error | StdDev | Allocated | |--------------- |---------:|----------:|----------:|----------:| | Before SetText | 7.588 us | 0.0088 us | 0.0078 us | 112 B | | After SetText | 1.814 us | 0.0009 us | 0.0009 us | - | Note that I don't think this completely solves #994, as there are other ideas mentioned there.
7192798
to
15671db
Compare
A problem occurred to me with this optimization: it's a possible semantic break. Consider: class MyTextClock : Android.Widget.TextClock {
public override Java.Lang.ICharSequence? Format12HourFormatted {
get => …;
set => …;
}
}
// …
TextClock myTextClock = new MyTextClock ();
myTextClock.Format12Hour = "whatever"; The current expectation is that when The approaches proposed in Issue #994 break that semantic. That's Not Good™. The only time the proposals in #994 are valid is when the "underlying" Fortunately that restriction is (partially) the case for It's not fine for every possible scenario. For reference, the namespace Android.Widget {
public partial class TextClock : Android.Widget.TextView {
public virtual unsafe Java.Lang.ICharSequence? Format12HourFormatted {
// Metadata.xml XPath method reference: path="/api/package[@name='android.widget']/class[@name='TextClock']/method[@name='getFormat12Hour' and count(parameter)=0]"
[Register ("getFormat12Hour", "()Ljava/lang/CharSequence;", "GetGetFormat12HourHandler")]
get {
const string __id = "getFormat12Hour.()Ljava/lang/CharSequence;";
try {
var __rm = _members.InstanceMethods.InvokeVirtualObjectMethod (__id, this, null);
return global::Java.Lang.Object.GetObject<Java.Lang.ICharSequence> (__rm.Handle, JniHandleOwnership.TransferLocalRef);
} finally {
}
}
// Metadata.xml XPath method reference: path="/api/package[@name='android.widget']/class[@name='TextClock']/method[@name='setFormat12Hour' and count(parameter)=1 and parameter[1][@type='java.lang.CharSequence']]"
[Register ("setFormat12Hour", "(Ljava/lang/CharSequence;)V", "GetSetFormat12Hour_Ljava_lang_CharSequence_Handler")]
set {
const string __id = "setFormat12Hour.(Ljava/lang/CharSequence;)V";
IntPtr native_value = CharSequence.ToLocalJniHandle (value);
try {
JniArgumentValue* __args = stackalloc JniArgumentValue [1];
__args [0] = new JniArgumentValue (native_value);
_members.InstanceMethods.InvokeVirtualVoidMethod (__id, this, __args);
} finally {
JNIEnv.DeleteLocalRef (native_value);
global::System.GC.KeepAlive (value);
}
}
}
public string? Format12Hour {
get { return Format12HourFormatted == null ? null : Format12HourFormatted.ToString (); }
set {
var jls = value == null ? null : new global::Java.Lang.String (value);
Format12HourFormatted = jls;
if (jls != null) jls.Dispose ();
}
}
}
} |
Thus, based on above analysis, we can only make this optimization if the We may want to rethink things for the "general" case, if we care about the "general" case right now. For example, would there be benefit in adding support for non-registered partial class TextClock {
public string? Format12Hour {
get { return Format12HourFormatted == null ? null : Format12HourFormatted.ToString (); }
set {
JniObjectReference native_text = global::Java.Interop.JniEnvironment.Strings.NewString (value);
using var jls = value == null ? null : new Java.Lang.String (native_text.Handle, JniHandleOwnership.TransferLocalRef | JniHandleOwnership.DoNotRegister);
Format12HourFormatted = jls;
}
}
} Using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve that the "how" looks correct. That is, the generator
changes look good.
I will defer to jonp on the "what". That is, is what this generates sane. 😁
Context: https://github.com/dotnet/maui/issues/12130
Context: https://github.com/xamarin/java.interop/issues/994
Context: 79a8e1ee93673aa2f8ef9f2ea8cb9bebdc1394df
Context: https://github.com/xamarin/monodroid/commit/23f421202b94afd95f3b62de20b09a2ed7a13429
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 79a8e1ee.
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]: https://github.com/xamarin/java.interop/issues/994#issuecomment-1520781139 |
Context: #994
Testing a .NET MAUI application, I found this while scrolling:
This appears to be a high-impact area for .NET MAUI apps.
Previously, for
TextView.Text
we emitted something like:Where instead we could emit:
We would only do this for cases where
setText()
isfinal
.I added a test for these
String
overloads, as there wasn't exactlyone that tested this exact scenario.
Testing on a Pixel 5 in a .NET 8 BenchmarkDotNet project, Java
code:
C# benchmark:
https://github.com/jonathanpeppers/BenchmarkDotNet-Android/tree/Android.Widget.TextView
Note that
After SetText
is probably just the same and there issome variance between runs.
Note that I don't think this completely solves #994, as there are other
ideas mentioned there.