From dfce805bcfc56c15a3078b2a853dc139817ffafe Mon Sep 17 00:00:00 2001 From: Konrad `ktoso` Malawski Date: Tue, 29 Oct 2024 09:18:35 +0900 Subject: [PATCH 1/2] SwiftArena.ofAuto() which uses GC to manage swift instances (and destroy them) --- .../swiftkit/AutoSwiftMemorySession.java | 56 ++++++++++ .../swift/swiftkit/ClosableSwiftArena.java | 28 +++++ .../swiftkit/ConfinedSwiftMemorySession.java | 72 ++++++++++++ .../java/org/swift/swiftkit/SwiftArena.java | 105 +++--------------- .../swift/swiftkit/SwiftInstanceCleanup.java | 34 ++++-- .../org/swift/swiftkit/AutoArenaTest.java | 80 +++++++++++++ 6 files changed, 281 insertions(+), 94 deletions(-) create mode 100644 SwiftKit/src/main/java/org/swift/swiftkit/AutoSwiftMemorySession.java create mode 100644 SwiftKit/src/main/java/org/swift/swiftkit/ClosableSwiftArena.java create mode 100644 SwiftKit/src/main/java/org/swift/swiftkit/ConfinedSwiftMemorySession.java create mode 100644 SwiftKit/src/test/java/org/swift/swiftkit/AutoArenaTest.java diff --git a/SwiftKit/src/main/java/org/swift/swiftkit/AutoSwiftMemorySession.java b/SwiftKit/src/main/java/org/swift/swiftkit/AutoSwiftMemorySession.java new file mode 100644 index 00000000..57e2c978 --- /dev/null +++ b/SwiftKit/src/main/java/org/swift/swiftkit/AutoSwiftMemorySession.java @@ -0,0 +1,56 @@ +package org.swift.swiftkit; + +import java.lang.foreign.MemorySegment; +import java.lang.ref.Cleaner; +import java.util.Objects; +import java.util.concurrent.ThreadFactory; + +/** + * A memory session which manages registered objects via the Garbage Collector. + * + *

When registered Java wrapper classes around native Swift instances {@link SwiftInstance}, + * are eligible for collection, this will trigger the cleanup of the native resources as well. + * + *

This memory session is LESS reliable than using a {@link ConfinedSwiftMemorySession} because + * the timing of when the native resources are cleaned up is somewhat undefined, and rely on the + * system GC. Meaning, that if an object nas been promoted to an old generation, there may be a + * long time between the resource no longer being referenced "in Java" and its native memory being released, + * and also the deinit of the Swift type being run. + * + *

This can be problematic for Swift applications which rely on quick release of resources, and may expect + * the deinits to run in expected and "quick" succession. + * + *

Whenever possible, prefer using an explicitly managed {@link SwiftArena}, such as {@link SwiftArena#ofConfined()}. + */ +final class AutoSwiftMemorySession implements SwiftArena { + + private final Cleaner cleaner; + + public AutoSwiftMemorySession(ThreadFactory cleanerThreadFactory) { + this.cleaner = Cleaner.create(cleanerThreadFactory); + } + + @Override + public void register(SwiftHeapObject object) { + SwiftHeapObjectCleanup cleanupAction = new SwiftHeapObjectCleanup(object.$memorySegment(), object.$swiftType()); + 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"); + MemorySegment resource = value.$memorySegment(); + var cleanupAction = new SwiftValueCleanup(resource); + cleaner.register(value, cleanupAction); + } + +} diff --git a/SwiftKit/src/main/java/org/swift/swiftkit/ClosableSwiftArena.java b/SwiftKit/src/main/java/org/swift/swiftkit/ClosableSwiftArena.java new file mode 100644 index 00000000..c257ae57 --- /dev/null +++ b/SwiftKit/src/main/java/org/swift/swiftkit/ClosableSwiftArena.java @@ -0,0 +1,28 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2024 Apple Inc. and the Swift.org project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of Swift.org project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + +package org.swift.swiftkit; + +/** + * Auto-closable version of {@link SwiftArena}. + */ +public interface ClosableSwiftArena extends SwiftArena, AutoCloseable { + + /** + * Close the arena and make sure all objects it managed are released. + * Throws if unable to verify all resources have been release (e.g. over retained Swift classes) + */ + void close(); + +} diff --git a/SwiftKit/src/main/java/org/swift/swiftkit/ConfinedSwiftMemorySession.java b/SwiftKit/src/main/java/org/swift/swiftkit/ConfinedSwiftMemorySession.java new file mode 100644 index 00000000..e06a07f3 --- /dev/null +++ b/SwiftKit/src/main/java/org/swift/swiftkit/ConfinedSwiftMemorySession.java @@ -0,0 +1,72 @@ +package org.swift.swiftkit; + +import java.util.LinkedList; +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; + +final class ConfinedSwiftMemorySession implements ClosableSwiftArena { + + final static int CLOSED = 0; + final static int ACTIVE = 1; + + final Thread owner; + final AtomicInteger state; + + final ConfinedResourceList resources; + + public ConfinedSwiftMemorySession(Thread owner) { + this.owner = owner; + this.state = new AtomicInteger(ACTIVE); + this.resources = new ConfinedResourceList(); + } + + public void checkValid() throws RuntimeException { + if (this.owner != null && this.owner != Thread.currentThread()) { + throw new WrongThreadException("ConfinedSwift arena is confined to %s but was closed from %s!".formatted(this.owner, Thread.currentThread())); + } else if (this.state.get() < ACTIVE) { + throw new RuntimeException("SwiftArena is already closed!"); + } + } + + @Override + public void close() { + checkValid(); + + // Cleanup all resources + if (this.state.compareAndExchange(ACTIVE, CLOSED) == ACTIVE) { + this.resources.runCleanup(); + } // else, was already closed; do nothing + } + + @Override + public void register(SwiftHeapObject object) { + checkValid(); + + var cleanup = new SwiftHeapObjectCleanup(object.$memorySegment(), object.$swiftType()); + this.resources.add(cleanup); + } + + @Override + public void register(SwiftValue value) { + checkValid(); + + var cleanup = new SwiftValueCleanup(value.$memorySegment()); + this.resources.add(cleanup); + } + + static final class ConfinedResourceList implements SwiftResourceList { + // TODO: Could use intrusive linked list to avoid one indirection here + final List resourceCleanups = new LinkedList<>(); + + void add(SwiftInstanceCleanup cleanup) { + resourceCleanups.add(cleanup); + } + + @Override + public void runCleanup() { + for (SwiftInstanceCleanup cleanup : resourceCleanups) { + cleanup.run(); + } + } + } +} diff --git a/SwiftKit/src/main/java/org/swift/swiftkit/SwiftArena.java b/SwiftKit/src/main/java/org/swift/swiftkit/SwiftArena.java index 98bfd5ef..02c1b599 100644 --- a/SwiftKit/src/main/java/org/swift/swiftkit/SwiftArena.java +++ b/SwiftKit/src/main/java/org/swift/swiftkit/SwiftArena.java @@ -14,23 +14,26 @@ package org.swift.swiftkit; -import java.util.LinkedList; -import java.util.List; -import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.ThreadFactory; /** * A Swift arena manages Swift allocated memory for classes, structs, enums etc. * When an arena is closed, it will destroy all managed swift objects in a way appropriate to their type. - *

- * A confined arena has an associated owner thread that confines some operations to - * associated owner thread such as {@link #close()}. + * + *

A confined arena has an associated owner thread that confines some operations to + * associated owner thread such as {@link ClosableSwiftArena#close()}. */ -public interface SwiftArena extends AutoCloseable { +public interface SwiftArena { - static SwiftArena ofConfined() { + static ClosableSwiftArena ofConfined() { return new ConfinedSwiftMemorySession(Thread.currentThread()); } + static SwiftArena ofAuto() { + ThreadFactory cleanerThreadFactory = r -> new Thread(r, "AutoSwiftArenaCleanerThread"); + return new AutoSwiftMemorySession(cleanerThreadFactory); + } + /** * Register a Swift reference counted heap object with this arena (such as a {@code class} or {@code actor}). * Its memory should be considered managed by this arena, and be destroyed when the arena is closed. @@ -43,91 +46,18 @@ static SwiftArena ofConfined() { */ void register(SwiftValue value); - /** - * Close the arena and make sure all objects it managed are released. - * Throws if unable to verify all resources have been release (e.g. over retained Swift classes) - */ - void close(); - } -final class ConfinedSwiftMemorySession implements SwiftArena { - -// final Arena underlying; - final Thread owner; - final SwiftResourceList resources; - - final int CLOSED = 0; - final int ACTIVE = 1; - final AtomicInteger state; - - public ConfinedSwiftMemorySession(Thread owner) { - this.owner = owner; - resources = new ConfinedResourceList(); - state = new AtomicInteger(ACTIVE); - } - - public void checkValid() throws RuntimeException { - if (this.owner != null && this.owner != Thread.currentThread()) { - throw new WrongThreadException("ConfinedSwift arena is confined to %s but was closed from %s!".formatted(this.owner, Thread.currentThread())); - } else if (this.state.get() < ACTIVE) { - throw new RuntimeException("Arena is already closed!"); - } - } - - @Override - public void register(SwiftHeapObject object) { - this.resources.add(new SwiftHeapObjectCleanup(object)); - } - - @Override - public void register(SwiftValue value) { - this.resources.add(new SwiftValueCleanup(value.$memorySegment())); - } - - @Override - public void close() { - checkValid(); - - // Cleanup all resources - if (this.state.compareAndExchange(ACTIVE, CLOSED) == ACTIVE) { - this.resources.cleanup(); - } // else, was already closed; do nothing - } - - /** - * Represents a list of resources that need a cleanup, e.g. allocated classes/structs. - */ - static abstract class SwiftResourceList implements Runnable { - // TODO: Could use intrusive linked list to avoid one indirection here - final List resourceCleanups = new LinkedList<>(); - - abstract void add(SwiftInstanceCleanup cleanup); - - public abstract void cleanup(); - - public final void run() { - cleanup(); // cleaner interop - } - } - - static final class ConfinedResourceList extends SwiftResourceList { - @Override - void add(SwiftInstanceCleanup cleanup) { - resourceCleanups.add(cleanup); - } - - @Override - public void cleanup() { - for (SwiftInstanceCleanup cleanup : resourceCleanups) { - cleanup.run(); - } - } - } +/** + * Represents a list of resources that need a cleanup, e.g. allocated classes/structs. + */ +interface SwiftResourceList { + void runCleanup(); } + final class UnexpectedRetainCountException extends RuntimeException { public UnexpectedRetainCountException(Object resource, long retainCount, int expectedRetainCount) { super(("Attempting to cleanup managed memory segment %s, but it's retain count was different than [%d] (was %d)! " + @@ -135,3 +65,4 @@ public UnexpectedRetainCountException(Object resource, long retainCount, int exp ).formatted(resource, expectedRetainCount, retainCount)); } } + diff --git a/SwiftKit/src/main/java/org/swift/swiftkit/SwiftInstanceCleanup.java b/SwiftKit/src/main/java/org/swift/swiftkit/SwiftInstanceCleanup.java index a20a7599..8ac62793 100644 --- a/SwiftKit/src/main/java/org/swift/swiftkit/SwiftInstanceCleanup.java +++ b/SwiftKit/src/main/java/org/swift/swiftkit/SwiftInstanceCleanup.java @@ -19,23 +19,43 @@ /** * A Swift memory instance cleanup, e.g. count-down a reference count and destroy a class, or destroy struct/enum etc. */ -sealed interface SwiftInstanceCleanup extends Runnable { +interface SwiftInstanceCleanup extends Runnable { } -record SwiftHeapObjectCleanup(SwiftHeapObject instance) implements SwiftInstanceCleanup { +/** + * Implements cleaning up a Swift {@link SwiftHeapObject}. + *

+ * This class does not store references to the Java wrapper class, and therefore the wrapper may be subject to GC, + * which may trigger a cleanup (using this class), which will clean up its underlying native memory resource. + */ +// non-final for testing +class SwiftHeapObjectCleanup implements SwiftInstanceCleanup { + + final MemorySegment selfPointer; + final SwiftAnyType selfType; + + /** + * This constructor on purpose does not just take a {@link SwiftHeapObject} in order to make it very + * clear that it does not take ownership of it, but we ONLY manage the native resource here. + * + * This is important for {@link AutoSwiftMemorySession} which relies on the wrapper type to be GC-able, + * when no longer "in use" on the Java side. + */ + SwiftHeapObjectCleanup(MemorySegment selfPointer, SwiftAnyType selfType) { + this.selfPointer = selfPointer; + this.selfType = selfType; + } @Override public void run() throws UnexpectedRetainCountException { // Verify we're only destroying an object that's indeed not retained by anyone else: - long retainedCount = SwiftKit.retainCount(this.instance); + long retainedCount = SwiftKit.retainCount(selfPointer); if (retainedCount > 1) { - throw new UnexpectedRetainCountException(this.instance, retainedCount, 1); + throw new UnexpectedRetainCountException(selfPointer, retainedCount, 1); } // Destroy (and deinit) the object: - var ty = this.instance.$swiftType(); - - SwiftValueWitnessTable.destroy(ty, this.instance.$memorySegment()); + SwiftValueWitnessTable.destroy(selfType, selfPointer); // Invalidate the Java wrapper class, in order to prevent effectively use-after-free issues. // FIXME: some trouble with setting the pointer to null, need to figure out an appropriate way to do this diff --git a/SwiftKit/src/test/java/org/swift/swiftkit/AutoArenaTest.java b/SwiftKit/src/test/java/org/swift/swiftkit/AutoArenaTest.java new file mode 100644 index 00000000..7ba215ab --- /dev/null +++ b/SwiftKit/src/test/java/org/swift/swiftkit/AutoArenaTest.java @@ -0,0 +1,80 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2024 Apple Inc. and the Swift.org project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of Swift.org project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + +package org.swift.swiftkit; + +import org.junit.jupiter.api.Test; + +import java.lang.foreign.GroupLayout; +import java.lang.foreign.MemorySegment; +import java.lang.ref.Cleaner; +import java.util.concurrent.CountDownLatch; + +public class AutoArenaTest { + + + @Test + @SuppressWarnings("removal") // System.runFinalization() will be removed + public void cleaner_releases_native_resource() { + SwiftHeapObject object = new FakeSwiftHeapObject(); + + // Latch waiting for the cleanup of the object + var cleanupLatch = new CountDownLatch(1); + + // we're retaining the `object`, register it with the arena: + AutoSwiftMemorySession arena = (AutoSwiftMemorySession) SwiftArena.ofAuto(); + arena.register(object, new SwiftHeapObjectCleanup(object.$memorySegment(), object.$swiftType()) { + @Override + public void run() throws UnexpectedRetainCountException { + cleanupLatch.countDown(); + } + }); + + // Release the object and hope it gets GC-ed soon + + //noinspection UnusedAssignment + object = null; + + var i = 1_000; + while (cleanupLatch.getCount() != 0) { + System.runFinalization(); + System.gc(); + + if (i-- < 1) { + throw new RuntimeException("Reference was not cleaned up! Did Cleaner not pick up the release?"); + } + } + + } + + private static class FakeSwiftHeapObject implements SwiftHeapObject { + public FakeSwiftHeapObject() { + } + + @Override + public MemorySegment $memorySegment() { + return MemorySegment.NULL; + } + + @Override + public GroupLayout $layout() { + return null; + } + + @Override + public SwiftAnyType $swiftType() { + return null; + } + } +} From 9b54dd5216a56ab554af50f4c12bc17eaf138cbd Mon Sep 17 00:00:00 2001 From: Konrad `ktoso` Malawski Date: Tue, 29 Oct 2024 10:14:28 +0900 Subject: [PATCH 2/2] Add missing license headers --- .../org/swift/swiftkit/AutoSwiftMemorySession.java | 14 ++++++++++++++ .../swift/swiftkit/ConfinedSwiftMemorySession.java | 14 ++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/SwiftKit/src/main/java/org/swift/swiftkit/AutoSwiftMemorySession.java b/SwiftKit/src/main/java/org/swift/swiftkit/AutoSwiftMemorySession.java index 57e2c978..9d25ad12 100644 --- a/SwiftKit/src/main/java/org/swift/swiftkit/AutoSwiftMemorySession.java +++ b/SwiftKit/src/main/java/org/swift/swiftkit/AutoSwiftMemorySession.java @@ -1,3 +1,17 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2024 Apple Inc. and the Swift.org project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of Swift.org project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + package org.swift.swiftkit; import java.lang.foreign.MemorySegment; diff --git a/SwiftKit/src/main/java/org/swift/swiftkit/ConfinedSwiftMemorySession.java b/SwiftKit/src/main/java/org/swift/swiftkit/ConfinedSwiftMemorySession.java index e06a07f3..e727f5db 100644 --- a/SwiftKit/src/main/java/org/swift/swiftkit/ConfinedSwiftMemorySession.java +++ b/SwiftKit/src/main/java/org/swift/swiftkit/ConfinedSwiftMemorySession.java @@ -1,3 +1,17 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2024 Apple Inc. and the Swift.org project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of Swift.org project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + package org.swift.swiftkit; import java.util.LinkedList;