Cleaner optimizaton

build.xml

@@ -1232,6 +1232,7 @@ cd ..
     </propertyset>
     <junit fork="${test.fork}" forkmode="${test.forkmode}" failureproperty="testfailure" tempdir="${build}">
       <jvmarg if:set="test.jdwp" value="${test.jdwp}" />
+      <jvmarg value="-Xmx64m" />
       <!-- optionally run headless -->
       <syspropertyset refid="headless"/>
       <!-- avoid VM conflicts with JNA protected mode -->

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.*;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -35,154 +39,226 @@
  * objects. It replaces the {@code Object#finalize} based resource deallocation
  * that is deprecated for removal from the JDK.
  *
- * <p><strong>This class is intented to be used only be JNA itself.</strong></p>
+ * <p><strong>This class is intended to be used only be JNA itself.</strong></p>
  */
 public class Cleaner {
-    private static final Cleaner INSTANCE = new Cleaner();
+    private static final Logger LOG = Logger.getLogger(Cleaner.class.getName());
 
-    public static Cleaner getCleaner() {
-        return INSTANCE;
-    }
+    /* General idea:
+     *
+     * There's one Cleaner per thread, kept in a ThreadLocal static variable.
+     * This instance handles all to-be-cleaned objects registered by this
+     * thread. Whenever the thread registers another object, it first checks
+     * if there are references in the queue and cleans them up, then continues
+     * with the registration.
+     *
+     * This leaves two cases open, for which we employ a "Master Cleaner" and
+     * a separate cleaning thread.
+     * 1. If a long-lived thread registers some objects in the beginning, but
+     *    then stops registering more objects, the previously registered
+     *    objects will never be cleared.
+     * 2. When a thread exits before all its registered objects have been
+     *    cleared, the ThreadLocal instance is lost, and so are the pending
+     *    objects.
+     *
+     * The Master Cleaner handles the first issue by regularly checking the
+     * activity of the Cleaners registered with it, and taking over the queues
+     * of any cleaners appearing to be idle.
+     * Similarly, the second issue is handled by taking over the queues of threads
+     * that have terminated.
+     */
 
-    private final ReferenceQueue<Object> referenceQueue;
-    private Thread cleanerThread;
-    private CleanerRef firstCleanable;
-
-    private Cleaner() {
-        referenceQueue = new ReferenceQueue<>();
-    }
+    public static final long MASTER_CLEANUP_INTERVAL_MS = 5000;
+    public static final long MASTER_MAX_LINGER_MS = 30000;
 
-    public synchronized 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));
-    }
+    private static class CleanerImpl {
+        protected final ReferenceQueue<Object> referenceQueue = new ReferenceQueue<Object>();
+        protected final Map<Long,CleanerRef> cleanables = new ConcurrentHashMap<Long,CleanerRef>();
+        private final AtomicBoolean lock = new AtomicBoolean(false);
 
-    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();
+        private void cleanQueue() {
+            if (lock.compareAndSet(false, true)) {
+                try {
+                    Reference<?> ref;
+                    while ((ref = referenceQueue.poll()) != null) {
+                        try {
+                            if (ref instanceof Cleanable) {
+                                ((Cleanable) ref).clean();
+                            }
+                        } catch (RuntimeException ex) {
+                            Logger.getLogger(Cleaner.class.getName()).log(Level.SEVERE, null, ex);
+                        }
+                    }
+                } finally {
+                    lock.set(false);
+                }
             }
-            return ref;
         }
-    }
 
-    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;
+        public Cleanable register(Object obj, Runnable cleanupTask) {
+            cleanQueue();
+            // The important side effect is the PhantomReference, that is yielded
+            // after the referent is GCed
+            return new CleanerRef(this, obj, referenceQueue, cleanupTask);
         }
-    }
 
-    private static class CleanerRef extends PhantomReference<Object> implements Cleanable {
-        private final Cleaner cleaner;
-        private final Runnable cleanupTask;
-        private CleanerRef previous;
-        private CleanerRef next;
+        protected void put(long n, CleanerRef ref) {
+            cleanables.put(n, ref);
+        }
 
-        public CleanerRef(Cleaner cleaner, Object referent, ReferenceQueue<? super Object> q, Runnable cleanupTask) {
-            super(referent, q);
-            this.cleaner = cleaner;
-            this.cleanupTask = cleanupTask;
+        protected boolean remove(long n) {
+            return cleanables.remove(n) != null;
         }
+    }
 
-        @Override
-        public void clean() {
-            if(cleaner.remove(this)) {
-                cleanupTask.run();
+    static class MasterCleaner extends Cleaner {
+        static MasterCleaner INSTANCE;
+
+        public static synchronized void add(Cleaner cleaner) {
+            if (INSTANCE == null) {
+                INSTANCE = new MasterCleaner();
             }
+            INSTANCE.cleaners.add(cleaner);
         }
 
-        CleanerRef getPrevious() {
-            return previous;
+        /** @return true if the caller thread can terminate */
+        private static synchronized boolean deleteIfEmpty(MasterCleaner caller) {
+            if (INSTANCE == caller && INSTANCE.cleaners.isEmpty()) {
+                INSTANCE = null;
+            }
+            return caller.cleaners.isEmpty();
         }
 
-        void setPrevious(CleanerRef previous) {
-            this.previous = previous;
+        final Set<Cleaner> cleaners = Collections.synchronizedSet(new HashSet<>());
+        final Set<CleanerImpl> referencedCleaners = new HashSet<>();
+        final Set<CleanerImpl> watchedCleaners = new HashSet<>();
+
+        private MasterCleaner() {
+            Thread cleanerThread = new Thread(() -> {
+                    long lastNonEmpty = System.currentTimeMillis();
+                    long now;
+                    long lastMasterRun = 0;
+                    while ((now = System.currentTimeMillis()) < lastNonEmpty + MASTER_MAX_LINGER_MS || !deleteIfEmpty(MasterCleaner.this)) {
+                        if (!cleaners.isEmpty()) { lastNonEmpty = now; }
+                        try {
+                            Reference<?> ref = impl.referenceQueue.remove(MASTER_CLEANUP_INTERVAL_MS);
+                            if(ref instanceof CleanerRef) {
+                                ((CleanerRef) ref).clean();
+                            }
+                            // "now" is not really *now* at this point, but off by no more than MASTER_CLEANUP_INTERVAL_MS
+                            if (lastMasterRun + MASTER_CLEANUP_INTERVAL_MS <= now) {
+                                masterCleanup();
+                                lastMasterRun = now;
+                            }
+                        } catch (InterruptedException ex) {
+                            // Can be raised on shutdown. If anyone else messes with
+                            // our reference queue, well, there is no way to separate
+                            // the two cases.
+                            // https://groups.google.com/g/jna-users/c/j0fw96PlOpM/m/vbwNIb2pBQAJ
+                            break;
+                        } catch (Exception ex) {
+                            Logger.getLogger(Cleaner.class.getName()).log(Level.SEVERE, null, ex);
+                        }
+                    }
+                    LOG.log(Level.FINE, "MasterCleaner thread {0} exiting", Thread.currentThread());
+                }, "JNA Cleaner");
+            LOG.log(Level.FINE, "Starting new MasterCleaner thread {0}", cleanerThread);
+            cleanerThread.setDaemon(true);
+            cleanerThread.start();
         }
 
-        CleanerRef getNext() {
-            return next;
+        private void masterCleanup() {
+            for (Iterator<Map.Entry<Thread,Cleaner>> it = Cleaner.INSTANCES.entrySet().iterator(); it.hasNext(); ) {
+                Map.Entry<Thread,Cleaner> entry = it.next();
+                if (!cleaners.contains(entry.getValue())) { continue; }
+                Cleaner cleaner = entry.getValue();
+                long currentCount = cleaner.counter.get();
+                if (currentCount == cleaner.lastCount // no new cleanables registered since last master cleanup interval -> assume it is no longer in use
+                        || !entry.getKey().isAlive()) { // owning thread died -> assume it is no longer in use
+                    it.remove();
+                    CleanerImpl impl = cleaner.impl;
+                    LOG.log(Level.FINE, () -> "MasterCleaner stealing cleaner " + impl + " from thread " + entry.getKey());
+                    referencedCleaners.add(impl);
+                    watchedCleaners.add(impl);
+                    register(cleaner, () -> {
+                        referencedCleaners.remove(impl);
+                        LOG.log(Level.FINE, "Cleaner {0} no longer referenced", impl);
+                    });
+                    cleaners.remove(cleaner);
+                } else {
+                    cleaner.lastCount = currentCount;
+                }
+            }
+
+            for (Iterator<CleanerImpl> it = watchedCleaners.iterator(); it.hasNext(); ) {
+                CleanerImpl impl = it.next();
+                impl.cleanQueue();
+                if (!referencedCleaners.contains(impl)) {
+                    if (impl.cleanables.isEmpty()) {
+                        it.remove();
+                        LOG.log(Level.FINE, "Discarding empty Cleaner {0}", impl);
+                    }
+                }
+            }
         }
+    }
+
+    private static final Map<Thread,Cleaner> INSTANCES = new ConcurrentHashMap<>();
+
+    public static Cleaner getCleaner() {
+        return INSTANCES.computeIfAbsent(Thread.currentThread(), Cleaner::new);
+    }
+
+    protected final CleanerImpl impl;
+    protected final Thread owner;
+    protected final AtomicLong counter = new AtomicLong(Long.MIN_VALUE);
+    protected long lastCount; // used by MasterCleaner only
 
-        void setNext(CleanerRef next) {
-            this.next = next;
+    private Cleaner() {
+        this(null);
+    }
+
+    private Cleaner(Thread owner) {
+        impl = new CleanerImpl();
+        this.owner = owner;
+        if (owner != null) {
+            MasterCleaner.add(this);
         }
+        LOG.log(Level.FINE, () -> owner == null ? "Created new MasterCleaner"
+                                                : "Created new Cleaner " + impl + " for thread " + owner);
     }
 
-    public static interface Cleanable {
-        public void clean();
+    public Cleanable register(Object obj, Runnable cleanupTask) {
+        counter.incrementAndGet();
+        return impl.register(obj, cleanupTask);
     }
 
-    private class CleanerThread extends Thread {
+    private static class CleanerRef extends PhantomReference<Object> implements Cleanable {
+        private static final AtomicLong COUNTER = new AtomicLong(Long.MIN_VALUE);
 
-        private static final long CLEANER_LINGER_TIME = 30000;
+        private final CleanerImpl cleaner;
+        private final long number = COUNTER.incrementAndGet();
+        private Runnable cleanupTask;
 
-        public CleanerThread() {
-            super("JNA Cleaner");
-            setDaemon(true);
+        public CleanerRef(CleanerImpl impl, Object referent, ReferenceQueue<Object> q, Runnable cleanupTask) {
+            super(referent, q);
+            LOG.log(Level.FINER, () -> "Registering " + referent + " with " + impl + " as " + this);
+            this.cleaner = impl;
+            this.cleanupTask = cleanupTask;
+            cleaner.put(number, this);
         }
 
         @Override
-        public void run() {
-            while (true) {
-                try {
-                    Reference<? extends Object> 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.log(Level.FINER, "Registered Cleaners: {0}", registeredCleaners.toString());
-                            }
-                        }
-                    }
-                } catch (InterruptedException ex) {
-                    // Can be raised on shutdown. If anyone else messes with
-                    // our reference queue, well, there is no way to separate
-                    // the two cases.
-                    // https://groups.google.com/g/jna-users/c/j0fw96PlOpM/m/vbwNIb2pBQAJ
-                    break;
-                } catch (Exception ex) {
-                    Logger.getLogger(Cleaner.class.getName()).log(Level.SEVERE, null, ex);
-                }
+        public void clean() {
+            if(cleaner.remove(this.number) && cleanupTask != null) {
+                LOG.log(Level.FINER, "Cleaning up {0}", this);
+                cleanupTask.run();
+                cleanupTask = null;
             }
         }
     }
+
+    public interface Cleanable {
+        void clean();
+    }
 }