From 998f0b6bc152be5302690728c66f699e6fd583a9 Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Wed, 4 Sep 2024 13:29:19 -0400 Subject: [PATCH] [Java.Interop] ReadOnlyProperty? Context: https://github.com/dotnet/java-interop/issues/1243 Context: https://github.com/dotnet/java-interop/pull/1248 Java's `final` keyword is contextual, and maps to (at least?) three separate keywords in C#: * `const` on fields * `readonly` on fields * `sealed` on types and methods When binding fields, we only support "const" `final` fields: fields for which the value is known at compile-time. Non-`const` fields are bound as properties, requiring a lookup for every property access. This can be problematic, performance-wise, as `final` fields without a compile-time value only need to be looked up once; afterward, their value cannot change [^1]. As such, we should consider altering our binding of "readonly" static properties to *cache* the value. PR #1248 implemented a "nullable"-based approach to caching the field value. While this approach works for reference types, it is likely not thread safe for `int?` and other value types. [There is a comment on #1248 to make the approach thread-safe][0], but @jonpryor isn't entirely sure if it's correct. The "straightfoward" approach would be to use a C# `lock` statement, but that requires a GC-allocated lock object, which would increase memory use. Furthermore, if this code is wrong, the only way to fix it is by regenerating the bindings. @jonpryor considered moving the thread-safety logic into a separate type, moving it outside of the generated code. This is implemented as `ReadOnlyProperty`, in this commit. To help figure this out, along with the performance implications, add a `ReadOnlyPropertyTiming` test fixture to `Java.Interop-PerformanceTests.dll` to measure performance, and update `JavaTiming` to have the various proposed binding ideas so that we can determine the resulting code size. Results are as follows: | Approach | Code Size (bytes) | Total (s) | Amortized (ticks) | | ----------------------------------------------------- | ----------------: | --------: | ----------------: | | No caching (current) | 21 | 0.0029098 | 2909 | | "nullable" caching (not thread-safe; #1248 approach) | 65 | 0.0000827 | 82 | | Inlined thread-safe caching | 48 | 0.0000664 | 66 | | `ReadOnlyProperty` caching | 19+21 = 40 | 0.0001548 | 154 | Worst performance is to not cache anything. At least the expected behavior is verified. "Nullable" caching is quite performant. Pity it isn't thread-safe. "Inlined thread-safe caching" is ~20% faster than "nullable" caching. `ReadOnlyProperty` caching is nearly 2x slower than "nullable". Can `ReadOnlyProperty` be made faster? [0]: https://github.com/dotnet/java-interop/pull/1248#issuecomment-2327185096 [^1]: Not strictly true; *instance* fields can change within the object constructor, and *static* fields change change within the static constructor. As #1248 is about static fields of *bound* types, there should be no way for us to observe this. Things become trickier with instance fields. --- .../Java.Interop/JavaTiming.cs | 44 ++++++++++++ .../Java.Interop/ReadOnlyPropertyTiming.cs | 71 +++++++++++++++++++ .../interop/performance/JavaTiming.java | 7 ++ 3 files changed, 122 insertions(+) create mode 100644 tests/Java.Interop-PerformanceTests/Java.Interop/ReadOnlyPropertyTiming.cs diff --git a/tests/Java.Interop-PerformanceTests/Java.Interop/JavaTiming.cs b/tests/Java.Interop-PerformanceTests/Java.Interop/JavaTiming.cs index a4104ac70..738f062d0 100644 --- a/tests/Java.Interop-PerformanceTests/Java.Interop/JavaTiming.cs +++ b/tests/Java.Interop-PerformanceTests/Java.Interop/JavaTiming.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Collections.Concurrent; +using System.Threading; using Java.Interop; using Java.Interop.GenericMarshaler; @@ -36,6 +37,49 @@ public unsafe JavaTiming () Construct (ref peer, JniObjectReferenceOptions.CopyAndDispose); } + public static int StaticReadonlyField_NoCache { + get { + const string __id = "STATIC_READONLY_FIELD.I"; + var __v = _members.StaticFields.GetInt32Value (__id); + return __v; + } + } + + static int? _StaticReadonlyField_Cache; + public static int StaticReadonlyField_ThreadUnsafeCache { + get { + if (_StaticReadonlyField_Cache.HasValue) + return _StaticReadonlyField_Cache.Value; + const string __id = "STATIC_READONLY_FIELD.I"; + var __v = _members.StaticFields.GetInt32Value (__id); + return (int) (_StaticReadonlyField_Cache = __v); + } + } + static int _StaticReadonlyField_haveValue; + static int _StaticReadonlyField_value; + + public static int StaticReadonlyField_ThreadSafeCache { + get { + if (1 == Interlocked.CompareExchange (ref _StaticReadonlyField_haveValue, 1, 0)) + return _StaticReadonlyField_value; + const string __id = "STATIC_READONLY_FIELD.I"; + var __v = _members.StaticFields.GetInt32Value (__id); + return _StaticReadonlyField_value = __v; + } + } + + static ReadOnlyProperty _rop_StaticReadonlyField = new ReadOnlyProperty (); + public static unsafe int StaticReadonlyField_Rop { + get { + static int _GetInt32Value () + { + return _members.StaticFields.GetInt32Value ("STATIC_READONLY_FIELD.I"); + } + delegate *managed c = &_GetInt32Value; + return _rop_StaticReadonlyField.GetValue (c); + } + } + static JniMethodInfo svm; public static void StaticVoidMethod () { diff --git a/tests/Java.Interop-PerformanceTests/Java.Interop/ReadOnlyPropertyTiming.cs b/tests/Java.Interop-PerformanceTests/Java.Interop/ReadOnlyPropertyTiming.cs new file mode 100644 index 000000000..cb257f9d0 --- /dev/null +++ b/tests/Java.Interop-PerformanceTests/Java.Interop/ReadOnlyPropertyTiming.cs @@ -0,0 +1,71 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.Globalization; +using System.Linq; +using System.Reflection; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Threading; + +using Java.Interop; + +using NUnit.Framework; + +namespace Java.Interop.PerformanceTests { + + + [TestFixture] + class ReadOnlyPropertyTiming : Java.InteropTests.JavaVMFixture { + [Test] + public void StaticReadOnlyPropertyTiming () + { + const int count = 1000; + + var noCache = Stopwatch.StartNew (); + for (int i = 0; i < count; ++i) { + _ = JavaTiming.StaticReadonlyField_NoCache; + } + noCache.Stop (); + + var badCache = Stopwatch.StartNew (); + for (int i = 0; i < count; ++i) { + _ = JavaTiming.StaticReadonlyField_ThreadUnsafeCache; + } + badCache.Stop (); + + var goodCache = Stopwatch.StartNew (); + for (int i = 0; i < count; ++i) { + _ = JavaTiming.StaticReadonlyField_ThreadSafeCache; + } + goodCache.Stop (); + + var ropCache = Stopwatch.StartNew (); + for (int i = 0; i < count; ++i) { + _ = JavaTiming.StaticReadonlyField_Rop; + } + ropCache.Stop (); + + Console.WriteLine ("Static ReadOnly Property Lookup + Invoke Timing:"); + Console.WriteLine ("\t No caching: {0}, {1} ticks", noCache.Elapsed, noCache.ElapsedTicks / count); + Console.WriteLine ("\t Thread Unsafe Cache: {0}, {1} ticks", badCache.Elapsed, badCache.ElapsedTicks / count); + Console.WriteLine ("\t Thread-Safe Cache: {0}, {1} ticks", goodCache.Elapsed, goodCache.ElapsedTicks / count); + Console.WriteLine ("\tReadOnlyProperty Cache: {0}, {1} ticks", ropCache.Elapsed, ropCache.ElapsedTicks / count); + } + } + + struct ReadOnlyProperty { + int have_value; + T value; + + [MethodImpl (MethodImplOptions.AggressiveInlining)] + public unsafe T GetValue (delegate * c) + { + if (1 == Interlocked.CompareExchange (ref have_value, 1, 0)) + return value; + var __v = c (); + value = __v; + return __v; + } + } +} \ No newline at end of file diff --git a/tests/Java.Interop-PerformanceTests/java/com/xamarin/interop/performance/JavaTiming.java b/tests/Java.Interop-PerformanceTests/java/com/xamarin/interop/performance/JavaTiming.java index ca45b787d..60671befb 100644 --- a/tests/Java.Interop-PerformanceTests/java/com/xamarin/interop/performance/JavaTiming.java +++ b/tests/Java.Interop-PerformanceTests/java/com/xamarin/interop/performance/JavaTiming.java @@ -2,6 +2,13 @@ public class JavaTiming { + public static final int STATIC_READONLY_FIELD = getStaticReadonlyFieldValue (); + + static int getStaticReadonlyFieldValue () + { + return 42; + } + public static void StaticVoidMethod () { }