Cleaner optimizaton

build.xml

@@ -1224,6 +1224,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,11 @@
 import java.lang.ref.PhantomReference;
 import java.lang.ref.Reference;
 import java.lang.ref.ReferenceQueue;
+import java.util.Iterator;
+import java.util.Map;
+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 +40,213 @@
  * 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();
-
-    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 handling the
+     * queues of the Cleaners registered with it.
+     * The seconds issue is handled by registering the per-thread Cleaner
+     * instances with the Master's reference queue.
+     */
+
+    public static final long MASTER_CLEANUP_INTERVAL_MS = 5000;
+    public static final long MASTER_MAX_LINGER_MS = 30000;
+
+    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 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);
+                }
+            }
+        }
 
-    private final ReferenceQueue<Object> referenceQueue;
-    private Thread cleanerThread;
-    private CleanerRef firstCleanable;
+        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 Cleaner() {
-        referenceQueue = new ReferenceQueue<>();
-    }
+        protected void put(long n, CleanerRef ref) {
+            cleanables.put(n, ref);
+        }
 
-    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));
+        protected boolean remove(long n) {
+            return cleanables.remove(n) != null;
+        }
     }
 
-    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;
+    private static class MasterCleanerImpl extends CleanerImpl {
+        @Override
+        protected synchronized void put(long n, CleanerRef ref) {
+            super.put(n, 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;
+        @Override
+        protected synchronized boolean remove(long n) {
+            return super.remove(n);
         }
     }
 
-    private static class CleanerRef extends PhantomReference<Object> implements Cleanable {
-        private final Cleaner cleaner;
-        private final Runnable cleanupTask;
-        private CleanerRef previous;
-        private CleanerRef next;
+    private static class MasterCleaner extends Cleaner {
+        private static MasterCleaner INSTANCE;
 
-        public CleanerRef(Cleaner cleaner, Object referent, ReferenceQueue<? super Object> q, Runnable cleanupTask) {
-            super(referent, q);
-            this.cleaner = cleaner;
-            this.cleanupTask = cleanupTask;
+        public static synchronized void add(Cleaner cleaner) {
+            if (INSTANCE == null) {
+                INSTANCE = new MasterCleaner();
+            }
+            final CleanerImpl impl = cleaner.impl;
+            INSTANCE.cleanerImpls.put(impl, true);
+            INSTANCE.register(cleaner, new Runnable() {
+                @Override
+                public void run() {
+                    INSTANCE.cleanerImpls.put(impl, false);
+                }
+            });
         }
 
-        @Override
-        public void clean() {
-            if(cleaner.remove(this)) {
-                cleanupTask.run();
+        private static synchronized boolean deleteIfEmpty() {
+            if (INSTANCE != null && INSTANCE.cleanerImpls.isEmpty()) {
+                INSTANCE = null;
+                return true;
             }
+            return false;
         }
 
-        CleanerRef getPrevious() {
-            return previous;
+        private final Map<CleanerImpl,Boolean> cleanerImpls = new ConcurrentHashMap<CleanerImpl,Boolean>();
+        private long lastNonEmpty = System.currentTimeMillis();
+
+        private MasterCleaner() {
+            super(true);
+            Thread cleanerThread = new Thread() {
+                @Override
+                public void run() {
+                    long now;
+                    long lastMasterRun = 0;
+                    while ((now = System.currentTimeMillis()) < lastNonEmpty + MASTER_MAX_LINGER_MS || !deleteIfEmpty()) {
+                        if (!cleanerImpls.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);
+                        }
+                    }
+                }
+            };
+            cleanerThread.setName("JNA Cleaner");
+            cleanerThread.setDaemon(true);
+            cleanerThread.start();
         }
 
-        void setPrevious(CleanerRef previous) {
-            this.previous = previous;
+        private void masterCleanup() {
+            Iterator<Map.Entry<CleanerImpl,Boolean>> it = cleanerImpls.entrySet().iterator();
+            while (it.hasNext()) {
+                Map.Entry<CleanerImpl,Boolean> entry = it.next();
+                entry.getKey().cleanQueue();
+                if (!entry.getValue() && entry.getKey().cleanables.isEmpty()) {
+                    it.remove();
+                }
+            }
         }
+    }
 
-        CleanerRef getNext() {
-            return next;
+    private static final ThreadLocal<Cleaner> MY_INSTANCE = new ThreadLocal<Cleaner>() {
+        @Override
+        protected Cleaner initialValue() {
+            return new Cleaner(false);
         }
+    };
 
-        void setNext(CleanerRef next) {
-            this.next = next;
+    public static Cleaner getCleaner() {
+        return MY_INSTANCE.get();
+    }
+
+    protected final CleanerImpl impl;
+
+    private Cleaner(boolean master) {
+        if (master) {
+            impl = new MasterCleanerImpl();
+        } else {
+            impl = new CleanerImpl();
+            MasterCleaner.add(this);
         }
     }
 
-    public static interface Cleanable {
-        public void clean();
+    public Cleanable register(Object obj, Runnable cleanupTask) {
+        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);
+            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) {
+                cleanupTask.run();
+                cleanupTask = null;
             }
         }
     }
+
+    public static interface Cleanable {
+        public void clean();
+    }
 }