From f0da2f8466a946fd111eb78c81717a5588f8be4f Mon Sep 17 00:00:00 2001 From: Brett Wooldridge Date: Tue, 23 Sep 2025 03:32:28 +0900 Subject: [PATCH 1/3] low-contention refactored reference Cleaner --- src/com/sun/jna/internal/Cleaner.java | 126 +++++++++----------------- 1 file changed, 45 insertions(+), 81 deletions(-) diff --git a/src/com/sun/jna/internal/Cleaner.java b/src/com/sun/jna/internal/Cleaner.java index a2095937f..5d895a57d 100644 --- a/src/com/sun/jna/internal/Cleaner.java +++ b/src/com/sun/jna/internal/Cleaner.java @@ -27,6 +27,10 @@ import java.lang.ref.PhantomReference; import java.lang.ref.Reference; import java.lang.ref.ReferenceQueue; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.logging.Level; import java.util.logging.Logger; @@ -45,102 +49,65 @@ public static Cleaner getCleaner() { } private final ReferenceQueue referenceQueue; - private Thread cleanerThread; - private CleanerRef firstCleanable; + private final Set uncleaned; + private final AtomicBoolean cleanerRunning; private Cleaner() { referenceQueue = new ReferenceQueue<>(); + uncleaned = ConcurrentHashMap.newKeySet(); + cleanerRunning = new AtomicBoolean(false); } - public synchronized Cleanable register(Object obj, Runnable cleanupTask) { + public Cleanable register(Object obj, Runnable cleanupTask) { // The important side effect is the PhantomReference, that is yielded // after the referent is GCed - return add(new CleanerRef(this, obj, referenceQueue, cleanupTask)); - } + Cleanable cleanable = add(new CleanerRef(obj, referenceQueue, cleanupTask)); - private synchronized CleanerRef add(CleanerRef ref) { - synchronized (referenceQueue) { - if (firstCleanable == null) { - firstCleanable = ref; - } else { - ref.setNext(firstCleanable); - firstCleanable.setPrevious(ref); - firstCleanable = ref; - } - if (cleanerThread == null) { - Logger.getLogger(Cleaner.class.getName()).log(Level.FINE, "Starting CleanerThread"); - cleanerThread = new CleanerThread(); - cleanerThread.start(); - } - return ref; + if (cleanerRunning.compareAndSet(false, true)) { + Logger.getLogger(Cleaner.class.getName()).log(Level.FINE, "Starting CleanerThread"); + Thread cleanerThread = new CleanerThread(); + cleanerThread.start(); } + + return cleanable; } - private synchronized boolean remove(CleanerRef ref) { - synchronized (referenceQueue) { - boolean inChain = false; - if (ref == firstCleanable) { - firstCleanable = ref.getNext(); - inChain = true; - } - if (ref.getPrevious() != null) { - ref.getPrevious().setNext(ref.getNext()); - } - if (ref.getNext() != null) { - ref.getNext().setPrevious(ref.getPrevious()); - } - if (ref.getPrevious() != null || ref.getNext() != null) { - inChain = true; - } - ref.setNext(null); - ref.setPrevious(null); - return inChain; - } + private CleanerRef add(final CleanerRef toAdd) { + uncleaned.add(toAdd); + return toAdd; + } + + // Remove by node reference + private void remove(final CleanerRef node) { + uncleaned.remove(node); } private static class CleanerRef extends PhantomReference implements Cleanable { - private final Cleaner cleaner; private final Runnable cleanupTask; - private CleanerRef previous; - private CleanerRef next; + private final AtomicBoolean cleaned; - public CleanerRef(Cleaner cleaner, Object referent, ReferenceQueue q, Runnable cleanupTask) { + CleanerRef(Object referent, ReferenceQueue q, Runnable cleanupTask) { super(referent, q); - this.cleaner = cleaner; this.cleanupTask = cleanupTask; + this.cleaned = new AtomicBoolean(false); } @Override public void clean() { - if(cleaner.remove(this)) { + if (cleaned.compareAndSet(false, true)) { + INSTANCE.remove(this); cleanupTask.run(); } } - - CleanerRef getPrevious() { - return previous; - } - - void setPrevious(CleanerRef previous) { - this.previous = previous; - } - - CleanerRef getNext() { - return next; - } - - void setNext(CleanerRef next) { - this.next = next; - } } - public static interface Cleanable { - public void clean(); + public interface Cleanable { + void clean(); } private class CleanerThread extends Thread { - private static final long CLEANER_LINGER_TIME = 30000; + private final long CLEANER_LINGER_TIME = TimeUnit.SECONDS.toMillis(30L); public CleanerThread() { super("JNA Cleaner"); @@ -151,26 +118,23 @@ public CleanerThread() { public void run() { while (true) { try { - Reference ref = referenceQueue.remove(CLEANER_LINGER_TIME); + Reference ref = referenceQueue.remove(CLEANER_LINGER_TIME); if (ref instanceof CleanerRef) { ((CleanerRef) ref).clean(); } else if (ref == null) { - synchronized (referenceQueue) { - Logger logger = Logger.getLogger(Cleaner.class.getName()); - if (firstCleanable == null) { - cleanerThread = null; - logger.log(Level.FINE, "Shutting down CleanerThread"); - break; - } else if (logger.isLoggable(Level.FINER)) { - StringBuilder registeredCleaners = new StringBuilder(); - for(CleanerRef cleanerRef = firstCleanable; cleanerRef != null; cleanerRef = cleanerRef.next) { - if(registeredCleaners.length() != 0) { - registeredCleaners.append(", "); - } - registeredCleaners.append(cleanerRef.cleanupTask.toString()); + Logger logger = Logger.getLogger(Cleaner.class.getName()); + if (cleanerRunning.compareAndSet(uncleaned.isEmpty(), false)) { + logger.log(Level.FINE, "Shutting down CleanerThread"); + break; + } else if (logger.isLoggable(Level.FINER)) { + StringBuilder registeredCleaners = new StringBuilder(); + uncleaned.forEach((cleanerRef) -> { + if (registeredCleaners.length() != 0) { + registeredCleaners.append(", "); } - logger.log(Level.FINER, "Registered Cleaners: {0}", registeredCleaners.toString()); - } + registeredCleaners.append(cleanerRef.cleanupTask.toString()); + }); + logger.log(Level.FINER, "Registered Cleaners: {0}", registeredCleaners.toString()); } } } catch (InterruptedException ex) { From d5dc284f5881e2e2e2a1e15bb42e09ac46006597 Mon Sep 17 00:00:00 2001 From: Brett Wooldridge Date: Wed, 24 Sep 2025 04:02:36 +0900 Subject: [PATCH 2/3] change to ensure strong reference is held throughout register() method --- src/com/sun/jna/internal/Cleaner.java | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/com/sun/jna/internal/Cleaner.java b/src/com/sun/jna/internal/Cleaner.java index 5d895a57d..110428653 100644 --- a/src/com/sun/jna/internal/Cleaner.java +++ b/src/com/sun/jna/internal/Cleaner.java @@ -58,10 +58,10 @@ private Cleaner() { cleanerRunning = new AtomicBoolean(false); } - public Cleanable register(Object obj, Runnable cleanupTask) { + public Cleanable register(Object referent, Runnable cleanupTask) { // The important side effect is the PhantomReference, that is yielded // after the referent is GCed - Cleanable cleanable = add(new CleanerRef(obj, referenceQueue, cleanupTask)); + Cleanable cleanable = add(new CleanerRef(referent, referenceQueue, cleanupTask)); if (cleanerRunning.compareAndSet(false, true)) { Logger.getLogger(Cleaner.class.getName()).log(Level.FINE, "Starting CleanerThread"); @@ -69,6 +69,16 @@ public Cleanable register(Object obj, Runnable cleanupTask) { cleanerThread.start(); } + // NOTE: This is a "pointless" check in the conventional sense, however it serves to guarantee that the + // referent is not garbage collected before the CleanerRef is fully constructed which can happen due + // to reordering of instructions by the compiler or the CPU. In Java 9+ Reference.reachabilityFence() was + // introduced to provide this guarantee, but we want to stay compatible with Java 8, so this is the common + // idiom to achieve the same effect, by ensuring that the referent is still strongly reachable at + // this point. + if (referent == null) { + throw new IllegalArgumentException("The referent object must not be null"); + } + return cleanable; } @@ -83,13 +93,12 @@ private void remove(final CleanerRef node) { } private static class CleanerRef extends PhantomReference implements Cleanable { - private final Runnable cleanupTask; - private final AtomicBoolean cleaned; + private volatile Runnable cleanupTask; + private AtomicBoolean cleaned = new AtomicBoolean(false); CleanerRef(Object referent, ReferenceQueue q, Runnable cleanupTask) { super(referent, q); this.cleanupTask = cleanupTask; - this.cleaned = new AtomicBoolean(false); } @Override From 6e4890f3d91981fcd8a832b96a5b062075946754 Mon Sep 17 00:00:00 2001 From: Brett Wooldridge Date: Wed, 24 Sep 2025 06:51:52 +0900 Subject: [PATCH 3/3] seemingly simple change to remove AtomicBoolean relieves GC pressure substantially --- src/com/sun/jna/internal/Cleaner.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/com/sun/jna/internal/Cleaner.java b/src/com/sun/jna/internal/Cleaner.java index 110428653..460cceb47 100644 --- a/src/com/sun/jna/internal/Cleaner.java +++ b/src/com/sun/jna/internal/Cleaner.java @@ -88,13 +88,12 @@ private CleanerRef add(final CleanerRef toAdd) { } // Remove by node reference - private void remove(final CleanerRef node) { - uncleaned.remove(node); + private boolean remove(final CleanerRef node) { + return uncleaned.remove(node); } private static class CleanerRef extends PhantomReference implements Cleanable { private volatile Runnable cleanupTask; - private AtomicBoolean cleaned = new AtomicBoolean(false); CleanerRef(Object referent, ReferenceQueue q, Runnable cleanupTask) { super(referent, q); @@ -103,8 +102,7 @@ private static class CleanerRef extends PhantomReference implements Clea @Override public void clean() { - if (cleaned.compareAndSet(false, true)) { - INSTANCE.remove(this); + if (INSTANCE.remove(this)) { cleanupTask.run(); } }