-
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
Java enumerations should *cache* the field values #1243
Labels
enhancement
Proposed change to current functionality
generator
Issues binding a Java library (generator, class-parse, etc.)
Milestone
Comments
Updated description to mention that we should do caching for all |
Here is an example trace: Search for either I'm doing this for now, which would also help .NET 8: |
jpobst
added
enhancement
Proposed change to current functionality
generator
Issues binding a Java library (generator, class-parse, etc.)
labels
Aug 15, 2024
jonpryor
added a commit
that referenced
this issue
Sep 4, 2024
Context: #1243 Context: #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<T>`, 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.0029275 | 2927 | | "nullable" caching (not thread-safe; #1248 approach) | 65 | 0.0000823 | 82 | | Inlined thread-safe caching | 48 | 0.0000656 | 65 | | `ReadOnlyProperty<T>` caching | 24+17 = 41 | 0.0001644 | 164 | 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<T>` caching is nearly 2x slower than "nullable". Can `ReadOnlyProperty<T>` be made faster? [0]: #1248 (comment) [^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.
jonpryor
added a commit
that referenced
this issue
Sep 4, 2024
Context: #1243 Context: #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<T>`, 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<T>` 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<T>` caching is nearly 2x slower than "nullable". Can `ReadOnlyProperty<T>` be made faster? [0]: #1248 (comment) [^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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
enhancement
Proposed change to current functionality
generator
Issues binding a Java library (generator, class-parse, etc.)
Consider the
Thread.State
enum:This "actually" is a class with fields for each enum value:
When we bind it, we bind the fields as properties:
java-interop/src/Java.Base-ref.cs
Lines 5490 to 5506 in fcad336
However, each of those properties involves a ValueManager lookup:
This is JavaInterop1, not XAJavaInterop1, but .NET for Android isn't much different:
The problem is that value lookup is not fast -- identity hash code needs to be obtained, locks obtained, etc. -- to the point that repeated enum lookups can actually show up in profiles.
TODO: @jonathanpeppers, please provide your profile data. :-D
Suggestion: could we update property generation for all
final
fields to cache the return value? This means we'd only need to callStaticFields.GetObjectValue()
and "GetValue" once, instead of once per-access:The text was updated successfully, but these errors were encountered: