Skip to content
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.Interop] ReadOnlyProperty<T>? #1249

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jonpryor
Copy link
Member

@jonpryor jonpryor commented 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, 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?

Footnotes

  1. Not strictly true; instance fields can change within the
    object constructor, and static fields change change within
    the static constructor. As [generator] Cache static final field values. #1248 is about static fields of
    bound types, there should be no way for us to observe this.
    Things become trickier with instance fields.

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.
@jonpryor jonpryor force-pushed the dev/jonp/jonp-ReadOnlyProperty branch from 088fb93 to 998f0b6 Compare September 4, 2024 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant