jni: Unify common methods using generic implementations

[sidepelican]

Motivation

As mentioned in the 'Others' section of PR #572, we currently have branching logic in the generator for common functions like destroy, depending on whether the target type is generic or not.

This branching is undesirable as it increases maintenance costs.
While PR #567 partially addressed this by introducing .synthesizedFunction, I have found a better approach, I propose it and solve the problem.

Solution

Instead of generating type-specific code, providing a single generic implementation within the SwiftJava package for functions like toString and destroy.

Since JNISwiftInstance always including type metadata, I realized that we can utilize the technique proposed in this comment to call a generic implementation.

@_cdecl("Java_org_swift_swiftkit_core_SwiftObjects_toString__JJ")
public func Java_org_swift_swiftkit_core_SwiftObjects_toString__JJ(environment: UnsafeMutablePointer<JNIEnv?>!, thisClass: jclass, selfPointer: jlong, selfTypePointer: jlong) -> jstring? {
  let selfTypeBits$ = Int(Int64(fromJNI: selfTypePointer, in: environment))
  guard let selfType$ = UnsafeRawPointer(bitPattern: selfTypeBits$) else {
    fatalError("selfType metadata address was null")
  }
  let typeMetadata = unsafeBitCast(selfType$, to: Any.Type.self)

  func perform<T>(as type: T.Type) -> jstring? {
    guard let self$ = UnsafeMutablePointer<T>(bitPattern: selfPointer) else {
      fatalError("self memory address was null")
    }
    return String(describing: self$.pointee).getJNIValue(in: environment)
  }
  return perform(as: typeMetadata)
}

Pros:

• Significantly simplifies the code generator implementation.
• Greatly reduces the amount of generated code, leading to smaller binary sizes.
• Aligns the implementation more closely with the FFM side.

Cons:

• Increase in runtime overhead:
  • Add calling to runtime functions (SIL's open_existential_metatype).
  • Loss of certain optimizations (e.g., inlining that might skip VWT lookups when individual implementations are generated).

Regarding the downsides, I believe the overhead is negligible compared to the inherent cost of JNI calls.

Changes

Following this new direction, I have replaced the generated code for the following functions with generic implementations:

• toString
• toDebugString
• destroy
• getDiscriminator

Additionally, I have removed .synthesizedFunction as it is no longer needed.

[ktoso]

👍

[ktoso]

Overall I really like this shape, it's true as you say we take some performance hit here for the opening but perhaps it is worth it anyway.

Would you be able to do a quick benchmark before/after? We have benchmarks set up in the repo, should be quick to add and try.

Overall though I'm supportive of this change, it's a nice pattern we'll likely follow in many more places!

[sidepelican]

@ktoso After rewriting the implementation to use SwiftJavaMacros, I added and ran some benchmarks.

The results showed a performance degradation than I had expected.
Curious about this, I investigated the cause and found that the current implementation of SwiftJavaMacros introduces significant overhead.
When I temporarily reverted the implementation and re-measured, the results returned to the expected range.

Before (main branch), sha: 0b01d82

Benchmark                                          (dataSize)  (taskCount)  Mode  Cnt     Score     Error  Units
GenericBenchmark.basicStruct_basicGetter                  N/A          N/A  avgt   30     9.189 ±   0.028  ns/op
GenericBenchmark.basicStruct_toString                     N/A          N/A  avgt   30  1837.659 ±  15.291  ns/op
GenericBenchmark.genericStruct_basicGetter                N/A          N/A  avgt   30    40.452 ±   0.409  ns/op
GenericBenchmark.genericStruct_toString                   N/A          N/A  avgt   30  1949.575 ±  13.998  ns/op

After (Using SwiftJavaMacros), sha: be833a4

Benchmark                                          (dataSize)  (taskCount)  Mode  Cnt     Score      Error  Units
GenericBenchmark.basicStruct_basicGetter                  N/A          N/A  avgt   30     9.365 ±    0.151  ns/op
GenericBenchmark.basicStruct_toString                     N/A          N/A  avgt   30  2599.917 ±   31.407  ns/op
GenericBenchmark.genericStruct_basicGetter                N/A          N/A  avgt   30    42.743 ±    1.196  ns/op
GenericBenchmark.genericStruct_toString                   N/A          N/A  avgt   30  2687.621 ±  192.211  ns/op

After (without SwiftJavaMacros), sha: c87e390

Benchmark                                          (dataSize)  (taskCount)  Mode  Cnt     Score     Error  Units
GenericBenchmark.basicStruct_basicGetter                  N/A          N/A  avgt   30     9.303 ±   0.048  ns/op
GenericBenchmark.basicStruct_toString                     N/A          N/A  avgt   30  1911.742 ±  19.403  ns/op
GenericBenchmark.genericStruct_basicGetter                N/A          N/A  avgt   30    44.299 ±   2.601  ns/op
GenericBenchmark.genericStruct_toString                   N/A          N/A  avgt   30  1920.005 ±  17.951  ns/op

While we might be able to resolve this by improving the SwiftJavaMacros implementation, I have reverted the changes for now to prioritize performance. I'm open to keeping the SwiftJavaMacros version and improving it later, but what are your thoughts?

[sidepelican]

Sorry, I realized I was misusing SwiftJavaMacros.
I mistakenly implemented a static function as an instance function.
After fixing that, it performed at an ideal speed.

After (with SwiftJavaMacro, rev2), sha: ff4e8e7

Benchmark                                          (dataSize)  (taskCount)  Mode  Cnt     Score     Error  Units
GenericBenchmark.basicStruct_basicGetter                  N/A          N/A  avgt   30     9.321 ±   0.034  ns/op
GenericBenchmark.basicStruct_toString                     N/A          N/A  avgt   30  1909.003 ±  10.223  ns/op
GenericBenchmark.genericStruct_basicGetter                N/A          N/A  avgt   30    43.091 ±   1.101  ns/op
GenericBenchmark.genericStruct_toString                   N/A          N/A  avgt   30  1924.357 ±   9.108  ns/op

[ktoso]

Sweet, yes they should perform as well as manually writing these for the most part. Thank you! Giving it a final look then

[ktoso]

Very nice, thank you!

[ktoso]

Seems we have a real runtime crash sadly

[sidepelican]

Currently, many warnings like the following are being displayed when running tests:

objc[75138]: Class _TtC9SwiftJava6Writer is implemented in both /Users/iceman/github/swift-java/.build/arm64-apple-macosx/debug/libSwiftJava.dylib (0x105901658) and /Users/iceman/github/swift-java/Samples/SwiftJavaExtractJNISampleApp/.build/arm64-apple-macosx/debug/libMySwiftLibrary.dylib (0x125040ba8). This may cause spurious casting failures and mysterious crashes. One of the duplicates must be removed or renamed.

I believe this is caused by the coexistence of the dynamic SwiftJava package and MySwiftLibrary, which has a static dependency (via SwiftJavaRuntimeSupport) on the same SwiftJava package.

As a result, EnumWithValueCases might be failing to cast because it conforms to the static version of _RawDiscriminatorRepresentable but not the dynamic one.

[ktoso]

Hm, worth a try as a quick fix. We can follow up with refactoring the modules perhaps...

[sidepelican]

As a simple fix, I merged SwiftJavaRuntimeSupport into libSwiftJava .
I considered several other options, but they didn't really work out for various reasons.

[ktoso]

Hm, I'm not sure about this... @madsodgaard wdyt? The runtime funcs split was to be able to depend only on those I guess, idk if we truly do so anywhere

[madsodgaard]

I don't think we are using the separate product for anything. I think its a legacy thing from when we were experiementing with dynamic products, but now that SwiftJava is dynamic, I think its fine to move the runtime support product into SwiftJava.

Though it is a breaking change, so should probably highlight it.

[ktoso]

We're not guaranteeing stability yet so I'd rather do changes like this aggressively right now rather than after we tag and release.

Let's go with this then, thanks for the great work here @sidepelican !