Skip to content

[JExtract] Unify mechanisms between value types and reference types #240

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jun 2, 2025
Merged

[JExtract] Unify mechanisms between value types and reference types #240

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7700181
[JExtract] Unify mechanisms between value types and reference types
rintaro Jun 1, 2025
971bb92
[JExtract] Don't emit constructor without SwiftArena
rintaro Jun 1, 2025
68f8049
[JExtract] Move the 'arena' parameter to the end
rintaro Jun 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 0 additions & 22 deletions Samples/SwiftAndJavaJarSampleLib/src/test/java/org/swift/swiftkit/SwiftArenaTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,26 +50,4 @@ public void arena_releaseClassOnClose_class_ok() {

// TODO: should we zero out the $memorySegment perhaps?
}

@Test
public void arena_releaseClassOnClose_class_leaked() {
String memorySegmentDescription = "<none>";

try {
try (var arena = SwiftArena.ofConfined()) {
var obj = new MySwiftClass(arena,1, 2);
memorySegmentDescription = obj.$memorySegment().toString();

// Pretend that we "leaked" the class, something still holds a reference to it while we try to destroy it
retain(obj.$memorySegment());
assertEquals(2, retainCount(obj.$memorySegment()));
}

fail("Expected exception to be thrown while the arena is closed!");
} catch (Exception ex) {
// The message should point out which objects "leaked":
assertTrue(ex.getMessage().contains(memorySegmentDescription));
}

}
}
21 changes: 0 additions & 21 deletions Samples/SwiftKitSampleApp/src/test/java/org/swift/swiftkit/SwiftArenaTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,27 +88,6 @@ public void arena_markAsDestroyed_preventUseAfterFree_struct() {
}
}

@Test
public void arena_releaseClassOnClose_class_leaked() {
String memorySegmentDescription = "<none>";

try {
try (var arena = SwiftArena.ofConfined()) {
var obj = new MySwiftClass(arena,1, 2);
memorySegmentDescription = obj.$memorySegment().toString();

// Pretend that we "leaked" the class, something still holds a reference to it while we try to destroy it
retain(obj.$memorySegment());
assertEquals(2, retainCount(obj.$memorySegment()));
}

fail("Expected exception to be thrown while the arena is closed!");
} catch (Exception ex) {
// The message should point out which objects "leaked":
assertTrue(ex.getMessage().contains(memorySegmentDescription));
}
}

@Test
public void arena_initializeWithCopy_struct() {

Expand Down
8 changes: 6 additions & 2 deletions Sources/JExtractSwift/ImportedDecls.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,8 +267,12 @@ public struct ImportedFunc: ImportedDecl, CustomStringConvertible {
public var isInit: Bool = false

public var isIndirectReturn: Bool {
returnType.isValueType ||
(isInit && (parent?.isValueType ?? false))
switch returnType.originalSwiftTypeKind {
case .actor, .class, .struct, .enum:
return true
default:
return false
}
}

public init(
Expand Down
84 changes: 17 additions & 67 deletions Sources/JExtractSwift/Swift2JavaTranslator+Printing.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,11 +290,8 @@ extension Swift2JavaTranslator {
parentProtocol = "SwiftValue"
}

printer.printTypeDecl("public final class \(decl.javaClassName) implements \(parentProtocol)") {
printer.printTypeDecl("public final class \(decl.javaClassName) extends SwiftInstance implements \(parentProtocol)") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok 👍

printer in
// ==== Storage of the class
printClassSelfProperty(&printer, decl)
printStatusFlagsField(&printer, decl)

// Constants
printClassConstants(printer: &printer)
Expand Down Expand Up @@ -400,35 +397,6 @@ extension Swift2JavaTranslator {
)
}

/// Print a property where we can store the "self" pointer of a class.
private func printClassSelfProperty(_ printer: inout CodePrinter, _ decl: ImportedNominalType) {
printer.print(
"""
// Pointer to the referred to class instance's "self".
private final MemorySegment selfMemorySegment;

public final MemorySegment $memorySegment() {
return this.selfMemorySegment;
}
"""
)
}

private func printStatusFlagsField(_ printer: inout CodePrinter, _ decl: ImportedNominalType) {
printer.print(
"""
// TODO: make this a flagset integer and/or use a field updater
/** Used to track additional state of the underlying object, e.g. if it was explicitly destroyed. */
private final AtomicBoolean $state$destroyed = new AtomicBoolean(false);

@Override
public final AtomicBoolean $statusDestroyedFlag() {
return this.$state$destroyed;
}
"""
)
}

private func printClassMemoryLayout(_ printer: inout CodePrinter, _ decl: ImportedNominalType) {
printer.print(
"""
Expand Down Expand Up @@ -472,30 +440,11 @@ extension Swift2JavaTranslator {
\(decl.renderCommentSnippet ?? " *")
*/
public \(parentName.unqualifiedJavaTypeName)(\(renderJavaParamDecls(decl, paramPassingStyle: .wrapper))) {
this(/*arena=*/null, \(renderForwardJavaParams(decl, paramPassingStyle: .wrapper)));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should remove the constructors without an arena passed in actually… fine to keep in this pr, wdyt tho?

Copy link
Member Author

@rintaro rintaro Jun 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with removing this constructor as we don't want users to use ofAuto() arena as much as possible.
Also mixing different arena in a session sounds scary 😅 E.g.

try(var arena = SwiftArena.ofConfined()) {
  var obj = new MyClass(arena);
  var value = new MyStruct(); // Accidental use of "auto" arena.
  obj.process(value); // Probably fine, but...
}

This is actually probably fine, but still it's not ideal.
Removing this convenience constructor prevents this kind of issues.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it’ll be more consistent to just always pass arenas, might save us weird headaches down the line.

you can remove here or we’ll do later, your call

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do it in this PR 👍

this(SwiftArena.ofAuto(), \(renderForwardJavaParams(decl, paramPassingStyle: .wrapper)));
}
"""
)

let initializeMemorySegment =
if let parent = decl.parent,
parent.isReferenceType
{
"""
this.selfMemorySegment = (MemorySegment) mh$.invokeExact(
\(renderForwardJavaParams(decl, paramPassingStyle: nil))
);
"""
} else {
"""
this.selfMemorySegment = arena.allocate($layout());
mh$.invokeExact(
\(renderForwardJavaParams(decl, paramPassingStyle: nil)),
/* indirect return buffer */this.selfMemorySegment
);
"""
}

printer.print(
"""
/**
Expand All @@ -505,20 +454,23 @@ extension Swift2JavaTranslator {
\(decl.renderCommentSnippet ?? " *")
*/
public \(parentName.unqualifiedJavaTypeName)(SwiftArena arena, \(renderJavaParamDecls(decl, paramPassingStyle: .wrapper))) {
var mh$ = \(descClassIdentifier).HANDLE;
try {
super(() -> {
var mh$ = \(descClassIdentifier).HANDLE;
try {
MemorySegment _result = arena.allocate($LAYOUT);

if (SwiftKit.TRACE_DOWNCALLS) {
SwiftKit.traceDowncall(\(renderForwardJavaParams(decl, paramPassingStyle: nil)));
}

\(initializeMemorySegment)

if (arena != null) {
arena.register(this);
}
} catch (Throwable ex$) {
throw new AssertionError("should not reach here", ex$);
}
mh$.invokeExact(
\(renderForwardJavaParams(decl, paramPassingStyle: nil)),
/* indirect return buffer */_result
);
return _result;
} catch (Throwable ex$) {
throw new AssertionError("should not reach here", ex$);
}
}, arena);
}
"""
)
Expand Down Expand Up @@ -714,9 +666,7 @@ extension Swift2JavaTranslator {
let guardFromDestroyedObjectCalls: String =
if decl.hasParent {
"""
if (this.$state$destroyed.get()) {
throw new IllegalStateException("Attempted to call method on already destroyed instance of " + getClass().getSimpleName() + "!");
}
$ensureAlive();
"""
} else { "" }

Expand Down
44 changes: 13 additions & 31 deletions Sources/JExtractSwift/SwiftThunkTranslator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,32 +93,18 @@ struct SwiftThunkTranslator {
"""
let typeName = "\(parent.swiftTypeName)"

if parent.isReferenceType {
return [
"""
\(raw: cDecl)
public func \(raw: thunkName)(\(raw: st.renderSwiftParamDecls(function, paramPassingStyle: nil))) -> UnsafeMutableRawPointer /* \(raw: typeName) */ {
var _self = \(raw: typeName)(\(raw: st.renderForwardSwiftParams(function, paramPassingStyle: nil)))
let self$ = unsafeBitCast(_self, to: UnsafeMutableRawPointer.self)
_swiftjava_swift_retain(object: self$)
return self$
}
"""
]
} else {
return [
"""
\(raw: cDecl)
public func \(raw: thunkName)(
\(raw: st.renderSwiftParamDecls(function, paramPassingStyle: nil)),
resultBuffer: /* \(raw: typeName) */ UnsafeMutableRawPointer
) {
var _self = \(raw: typeName)(\(raw: st.renderForwardSwiftParams(function, paramPassingStyle: nil)))
resultBuffer.assumingMemoryBound(to: \(raw: typeName).self).initialize(to: _self)
}
"""
]
}
return [
"""
\(raw: cDecl)
public func \(raw: thunkName)(
\(raw: st.renderSwiftParamDecls(function, paramPassingStyle: nil)),
resultBuffer: /* \(raw: typeName) */ UnsafeMutableRawPointer
) {
var _self = \(raw: typeName)(\(raw: st.renderForwardSwiftParams(function, paramPassingStyle: nil)))
resultBuffer.assumingMemoryBound(to: \(raw: typeName).self).initialize(to: _self)
}
"""
]
}

func render(forFunc decl: ImportedFunc) -> [DeclSyntax] {
Expand All @@ -136,11 +122,7 @@ struct SwiftThunkTranslator {
let paramPassingStyle: SelfParameterVariant?
let callBase: String
let callBaseDot: String
if let parent = decl.parent, parent.isReferenceType {
paramPassingStyle = .swiftThunkSelf
callBase = "let self$ = unsafeBitCast(_self, to: \(parent.originalSwiftType).self)"
callBaseDot = "self$."
} else if let parent = decl.parent, !parent.isReferenceType {
if let parent = decl.parent {
paramPassingStyle = .swiftThunkSelf
callBase =
"var self$ = _self.assumingMemoryBound(to: \(parent.originalSwiftType).self).pointee"
Expand Down
34 changes: 6 additions & 28 deletions SwiftKit/src/main/java/org/swift/swiftkit/AutoSwiftMemorySession.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,38 +48,16 @@ public AutoSwiftMemorySession(ThreadFactory cleanerThreadFactory) {
}

@Override
public void register(SwiftHeapObject object) {
var statusDestroyedFlag = object.$statusDestroyedFlag();
Runnable markAsDestroyed = () -> statusDestroyedFlag.set(true);

SwiftHeapObjectCleanup cleanupAction = new SwiftHeapObjectCleanup(
object.$memorySegment(),
object.$swiftType(),
markAsDestroyed
);
register(object, cleanupAction);
}

// visible for testing
void register(SwiftHeapObject object, SwiftHeapObjectCleanup cleanupAction) {
Objects.requireNonNull(object, "obj");
Objects.requireNonNull(cleanupAction, "cleanupAction");


cleaner.register(object, cleanupAction);
}

@Override
public void register(SwiftValue value) {
Objects.requireNonNull(value, "value");
public void register(SwiftInstance instance) {
Objects.requireNonNull(instance, "value");

// We're doing this dance to avoid keeping a strong reference to the value itself
var statusDestroyedFlag = value.$statusDestroyedFlag();
var statusDestroyedFlag = instance.$statusDestroyedFlag();
Runnable markAsDestroyed = () -> statusDestroyedFlag.set(true);

MemorySegment resource = value.$memorySegment();
var cleanupAction = new SwiftValueCleanup(resource, value.$swiftType(), markAsDestroyed);
cleaner.register(value, cleanupAction);
MemorySegment resource = instance.$memorySegment();
var cleanupAction = new SwiftInstanceCleanup(resource, instance.$swiftType(), markAsDestroyed);
cleaner.register(instance, cleanupAction);
}

@Override
Expand Down
23 changes: 5 additions & 18 deletions SwiftKit/src/main/java/org/swift/swiftkit/ConfinedSwiftMemorySession.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,28 +61,15 @@ public void close() {
}

@Override
public void register(SwiftHeapObject object) {
public void register(SwiftInstance instance) {
checkValid();

var statusDestroyedFlag = object.$statusDestroyedFlag();
var statusDestroyedFlag = instance.$statusDestroyedFlag();
Runnable markAsDestroyed = () -> statusDestroyedFlag.set(true);

var cleanup = new SwiftHeapObjectCleanup(
object.$memorySegment(), object.$swiftType(),
markAsDestroyed);
this.resources.add(cleanup);
}

@Override
public void register(SwiftValue value) {
checkValid();

var statusDestroyedFlag = value.$statusDestroyedFlag();
Runnable markAsDestroyed = () -> statusDestroyedFlag.set(true);

var cleanup = new SwiftValueCleanup(
value.$memorySegment(),
value.$swiftType(),
var cleanup = new SwiftInstanceCleanup(
instance.$memorySegment(),
instance.$swiftType(),
markAsDestroyed);
this.resources.add(cleanup);
}
Expand Down
24 changes: 12 additions & 12 deletions SwiftKit/src/main/java/org/swift/swiftkit/SwiftAnyType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,18 +34,18 @@ public SwiftAnyType(MemorySegment memorySegment) {
this.memorySegment = memorySegment.asReadOnly();
}

public SwiftAnyType(SwiftHeapObject object) {
if (object.$layout().name().isEmpty()) {
throw new IllegalArgumentException("SwiftHeapObject must have a mangled name in order to obtain its SwiftType.");
}

String mangledName = object.$layout().name().get();
var type = SwiftKit.getTypeByMangledNameInEnvironment(mangledName);
if (type.isEmpty()) {
throw new IllegalArgumentException("A Swift Any.Type cannot be null!");
}
this.memorySegment = type.get().memorySegment;
}
// public SwiftAnyType(SwiftHeapObject object) {
// if (object.$layout().name().isEmpty()) {
// throw new IllegalArgumentException("SwiftHeapObject must have a mangled name in order to obtain its SwiftType.");
// }
//
// String mangledName = object.$layout().name().get();
// var type = SwiftKit.getTypeByMangledNameInEnvironment(mangledName);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel we need this function anymore. Every instance has $swiftType(). I don't even think object.$layout().name() is a mangled name (i.e. it's initialized with .withName(SwiftKit.nameOfSwiftType(typeMetadata, true));)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah let's remove, it is from the time when we were still calling directly into initializers from the java side I think

// if (type.isEmpty()) {
// throw new IllegalArgumentException("A Swift Any.Type cannot be null!");
// }
// this.memorySegment = type.get().memorySegment;
// }


public MemorySegment $memorySegment() {
Expand Down
10 changes: 2 additions & 8 deletions SwiftKit/src/main/java/org/swift/swiftkit/SwiftArena.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,10 @@ static SwiftArena ofAuto() {
}

/**
* Register a Swift reference counted heap object with this arena (such as a {@code class} or {@code actor}).
* Register a Swift object.
* Its memory should be considered managed by this arena, and be destroyed when the arena is closed.
*/
void register(SwiftHeapObject object);

/**
* Register a struct, enum or other non-reference counted Swift object.
* Its memory should be considered managed by this arena, and be destroyed when the arena is closed.
*/
void register(SwiftValue value);
void register(SwiftInstance instance);

}

Expand Down
Loading