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

JavaKit: Java maintaining references to Swift values without Panama #168

Open
lhoward opened this issue Nov 8, 2024 · 20 comments
Open

JavaKit: Java maintaining references to Swift values without Panama #168

lhoward opened this issue Nov 8, 2024 · 20 comments
Labels
enhancement New feature or request

Comments

@lhoward
Copy link
Contributor

lhoward commented Nov 8, 2024

Edit: something I have found useful. Refactored to not use finalize(), and added links. Essentially:

import java.lang.ref.Cleaner;

class SwiftHeapObjectHolder implements AutoCloseable {
  private static final Cleaner cleaner = Cleaner.create();

  private final Cleaner.Cleanable _cleanable;
  public long _swiftHeapObject;

  public SwiftHeapObjectHolder(long heapObjectInt64Ptr) {
    final Runnable F = () -> SwiftHeapObjectHolder._releaseSwiftHeapObject(heapObjectInt64Ptr);
    SwiftHeapObjectHolder._retainSwiftHeapObject(heapObjectInt64Ptr);
    _cleanable = cleaner.register(this, F);
    _swiftHeapObject = heapObjectInt64Ptr;
  }

  @Override
  public void close() throws Exception {
    _swiftHeapObject = 0;
    _cleanable.clean();
  }

  static native void _retainSwiftHeapObject(long heapObjectInt64Ptr);
  static native void _releaseSwiftHeapObject(long heapObjectInt64Ptr);
}

https://github.com/PADL/FlutterSwift/blob/main/Sources/FlutterAndroid/com/padl/FlutterAndroid/SwiftHeapObjectHolder.java
https://github.com/PADL/FlutterSwift/blob/main/Sources/FlutterAndroid/SwiftHeapObjectHolderNativeMethods.swift

@ktoso
Copy link
Collaborator

ktoso commented Nov 8, 2024

No doubt you don't want to ship JARs.

? We'll absolutely be shipping some libraries or plugins as jars eventually.

@ktoso
Copy link
Collaborator

ktoso commented Nov 8, 2024

We should not be using finalizers if we can help it.

Please have a look at SwiftKit and the SwiftArena, this is the way to manage swift resources in Java, it's a more predictable modern way of doing so. There is an AutoArena which uses phantom references to release when references are dropped in a more efficient way.

@lhoward
Copy link
Contributor Author

lhoward commented Nov 8, 2024

Ah, just thought that would make the build process even more of a pain the proverbial!

@ktoso
Copy link
Collaborator

ktoso commented Nov 8, 2024

Can we rephrase this ticket in terms of the problem to solve rather than propose the solution?

The "modern way" is SwiftArena and we should look into using it first if this is about any plain old "keep object alive while i have references to it in java".

@lhoward
Copy link
Contributor Author

lhoward commented Nov 9, 2024

Sure. I wasn't aware of SwiftArena. But it looks like it requires Panorama, whereas Android I believe is still at JDK 17. Will confirm shortly.

@lhoward lhoward changed the title JavaKit: SwiftObjectHolder JavaKit: Java maintaining references to Swift values without Panorama Nov 9, 2024
@ktoso
Copy link
Collaborator

ktoso commented Nov 9, 2024

So funnily enough SwiftArena is not a JDK Arena type but its usage is very similar (but we never use it to "allocate"), the AutoSwiftArena just uses phantom references which have been around in the JDK for long.

I think we should pursue making that lib have some parts which are usable on older platforms as well, and that may include the AutoSwiftArena perhaps - then we don't have to invent another new way to solve the same issue 🤔 Maybe we'd do some swiftkit-core that's compatible with older platforms

@ktoso
Copy link
Collaborator

ktoso commented Nov 9, 2024

Thanks for the rename! :)

@lhoward
Copy link
Contributor Author

lhoward commented Nov 9, 2024

Sounds good. I may not have the cycles to help on this but I'll have a think!

@lhoward
Copy link
Contributor Author

lhoward commented Nov 9, 2024

Pleasure. Here I was thinking I had thought of something you hadn't.

@ktoso
Copy link
Collaborator

ktoso commented Nov 9, 2024

I'm sure you'll bump into a lot of things we've not thought about yet, this one I had a sneaky plan for though :)

@lhoward
Copy link
Contributor Author

lhoward commented Nov 9, 2024

The biggest remaining issue for me is object/JNIEnv affinity, but there's an open issue for that 😉

@ktoso ktoso added the enhancement New feature or request label Nov 9, 2024
@lhoward
Copy link
Contributor Author

lhoward commented Nov 9, 2024

We should not be using finalizers if we can help it.

Please have a look at SwiftKit and the SwiftArena, this is the way to manage swift resources in Java, it's a more predictable modern way of doing so. There is an AutoArena which uses phantom references to release when references are dropped in a more efficient way.

Amusingly SwiftKit uses _typeByName() which I proposed nearly ten years ago. Must be getting old.

@ktoso
Copy link
Collaborator

ktoso commented Nov 9, 2024

We will actually stop using that as soon as I finish my “extract build plugin” PR, but yeah it’s still hidden api…

@lhoward lhoward changed the title JavaKit: Java maintaining references to Swift values without Panorama JavaKit: Java maintaining references to Swift values without Panama Nov 9, 2024
@lhoward
Copy link
Contributor Author

lhoward commented Nov 9, 2024

Of potential note: https://github.com/vova7878/PanamaPort

@ktoso
Copy link
Collaborator

ktoso commented Nov 9, 2024

Oh that's very interesting, perhaps a route we could attempt huh 🤔 Thanks for sharing!

@lhoward
Copy link
Contributor Author

lhoward commented Nov 10, 2024

I think I will just migrate my code to use Cleanable instead of finalize(). I don't have the time right now to unpick the back-portable bits of SwiftArena.

@ktoso
Copy link
Collaborator

ktoso commented Nov 10, 2024

Yeah that would already be a nice step! And that’ll then be easy to hook into the arena :) I can look at these later on

@lhoward
Copy link
Contributor Author

lhoward commented Nov 10, 2024

Yeah that would already be a nice step! And that’ll then be easy to hook into the arena :) I can look at these later on

Pasted in code/links to (edited) original post in case you feel like taking a look. Perhaps I should add C JNI wrappers for swift_retain and swift_release rather than using Unmanaged.

@ktoso
Copy link
Collaborator

ktoso commented Nov 14, 2024

Yeah that's right.

This is done by https://github.com/swiftlang/swift-java/blob/main/SwiftKit/src/main/java/org/swift/swiftkit/AutoSwiftMemorySession.java#L47-L51 already, no need to reinvent -- just reorganize the code so this part of it lives in a non-FFM dependent part of the project. We can make some SwiftKitBasic maybe, and the destroy Runnables actually doing FFM invocations into native we'd then have in a separate module that does use FFM. but we can have others -- like this one just reference counting or using JNI for the destroy somehow?

I think we should look into sharing the infrastructure basically

@lhoward
Copy link
Contributor Author

lhoward commented Nov 14, 2024

Sounds good. Won't be able to look at this again for a few weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants