Skip to content
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

Improve performance for Cleaner implementation #1617

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Improve performance for Cleaner implementation #1617

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b901c18
Improve performance for Cleaner implementation
matthiasblaesing Jul 13, 2024
f9d7bbf
Move break to correct location
matthiasblaesing Jul 17, 2024
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
149 changes: 80 additions & 69 deletions src/com/sun/jna/internal/Cleaner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@
import java.lang.ref.PhantomReference;
import java.lang.ref.Reference;
import java.lang.ref.ReferenceQueue;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.logging.Level;
import java.util.logging.Logger;

Expand All @@ -44,65 +50,79 @@ public static Cleaner getCleaner() {
return INSTANCE;
}

// Guard for trackedObjects and cleanerThread. The readlock is utilized when
// the trackedObjects are manipulated, the writelock protectes starting and
// stopping the CleanerThread
private final ReadWriteLock cleanerThreadLock = new ReentrantReadWriteLock();
Copy link

Choose a reason for hiding this comment

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

I believe the readlock is not necessary at all. trackedObjects is an AtomicLong and doesn't need any protection.
The important step is to protect the starting and stopping of the CleanerThread. In particular, it must be ensured that the thread doesn't terminate while a new reference is added without starting a new CleanerThread..

Copy link
Member Author

Choose a reason for hiding this comment

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

The cleanerThreadLock protects the interaction of trackedObjects and cleanerThread. The critical section is the starting and stopping of the Thead if and only iff the number of tracked objects reaches zero or is above 1.

This is the sequence I want to prevent:

  1. T1 using the cleaner calls register and runs before incrementAndGet is invoked
  2. CleanerThread reaches the trackedObjects.get() == 0 check and enters the if block, but does not execute further yet.
  3. T1 executes incrementAndGet and receives 1, checks the value of cleanerThread and finds a Thread there.
  4. CleanerThread continues, clears cleanerThread and finishes execution

There is a subtle issue there, the break needs to move into the inner if in line 173 (https://github.com/java-native-access/jna/pull/1617/files/b901c180f4a1026675a2859bbf24727574e19e6a#diff-11b3b0981223743529d972f317f26dfd9488b4a9b80a34a2d6f76cc2c58dcc00R173) into the inner if.

Copy link

Choose a reason for hiding this comment

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

So, I attempted to write some pseudocode to show how to do it with a single lock and found a race in it. The cause of the race is the double-check locking that you employ in order to achieve better parallelism.
Without the double-check a single lock is sufficient, but then you're back to a mutex block that is always executed, hence no more real parallelism.

I still wonder if a tiny mutex block would be faster than the read/write lock, in particular because the ReadWriteLock JavaDoc says just that:

Further, if the read operations are too short the overhead of the read-write lock implementation (which is inherently more complex than a mutual exclusion lock) can dominate the execution cost, particularly as many read-write lock implementations still serialize all threads through a small section of code.

private final ReferenceQueue<Object> referenceQueue;
private Thread cleanerThread;
private CleanerRef firstCleanable;
// Count of objects tracked by the cleaner. When >0 it means objects are
// being tracked by the cleaner and the cleanerThread must be running
private final AtomicLong trackedObjects = new AtomicLong();
// Map only serves as holder, so that the CleanerRefs stay hard referenced
// and quickly accessible for removal
@SuppressWarnings("MismatchedQueryAndUpdateOfCollection")
private final Map<CleanerRef,CleanerRef> map = new ConcurrentHashMap<>();
// Thread to handle the actual cleaning
private volatile Thread cleanerThread;

private Cleaner() {
referenceQueue = new ReferenceQueue<>();
}

public synchronized Cleanable register(Object obj, Runnable cleanupTask) {
@SuppressWarnings("EmptySynchronizedStatement")
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));
try {
return add(new CleanerRef(this, obj, referenceQueue, cleanupTask));
} finally {
synchronized (obj) {
Copy link

Choose a reason for hiding this comment

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

This is potentially dangerous. Some other thread could be holding a lock on obj already.
This may be a non-issue because Cleaner is for internal use only.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you please elaborate, why you think we can deadlock here? For a deadlock we need two different locks, but Cleaner#register itself only locks the monitor of obj here. Sure, we can block at this position, but the other thread currently holding the monitor of obj can finish the critical section, release the monitor and then we run to completion.

Copy link

Choose a reason for hiding this comment

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

Think of two threads. One locks object A and registers object B, the other one locks B and registers A.
Like I said, this is an unlikely constellation in Cleaner's internal use.

// Used as a reachability fence for obj
// Ensure, that add completes before obj can be collected and
// the cleaner is run
}
}
}

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 CleanerRef add(CleanerRef ref) {
map.put(ref, ref);
cleanerThreadLock.readLock().lock();
try {
long count = trackedObjects.incrementAndGet();
if (cleanerThread == null && count > 0) {
Copy link

Choose a reason for hiding this comment

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

Because register() ensures that ref cannot be cleaned before add() has completed, we know that count > 0 is always true here.

cleanerThreadLock.readLock().unlock();
cleanerThreadLock.writeLock().lock();
try {
if (cleanerThread == null && trackedObjects.get() > 0) {
Logger.getLogger(Cleaner.class.getName()).log(Level.FINE, "Starting CleanerThread");
cleanerThread = new CleanerThread();
cleanerThread.start();
}
} finally {
cleanerThreadLock.readLock().lock();
Copy link

Choose a reason for hiding this comment

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

This lock is unlocked again immediately. That can be solved better without re-locking.

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 think you are mistaken. The readLock is locked and the writeLock is unlocked. This downgrades the ReadWriteLock from write to read. This is needed, as the readLock is unlocked in the finally clause. My assumption here is, that while a thread holds the writeLock now other thread can enter it, thus taking the readLock can be done on a fast path.

Copy link

Choose a reason for hiding this comment

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

This is needed, as the readLock is unlocked in the finally clause.

What I meant is that you could set a flag instead and skip the unlock in finally if the flag is set.

cleanerThreadLock.writeLock().unlock();
}
}
return ref;
} finally {
cleanerThreadLock.readLock().unlock();
}
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;
private void remove(CleanerRef ref) {
map.remove(ref);
cleanerThreadLock.readLock().lock();
Copy link

Choose a reason for hiding this comment

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

I think the lock here is not required. Like you said, the lock protects the interaction of cleanerThread and trackedObjects, but there is no interaction here.

try {
trackedObjects.decrementAndGet();
} finally {
cleanerThreadLock.readLock().unlock();
}
}

private static class CleanerRef extends PhantomReference<Object> implements Cleanable {
private final Cleaner cleaner;
private final Runnable cleanupTask;
private CleanerRef previous;
private CleanerRef next;
private final AtomicBoolean cleaned = new AtomicBoolean(false);

public CleanerRef(Cleaner cleaner, Object referent, ReferenceQueue<? super Object> q, Runnable cleanupTask) {
super(referent, q);
Expand All @@ -112,26 +132,11 @@ public CleanerRef(Cleaner cleaner, Object referent, ReferenceQueue<? super Objec

@Override
public void clean() {
if(cleaner.remove(this)) {
if (!cleaned.getAndSet(true)) {
cleaner.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 {
Expand All @@ -155,22 +160,28 @@ public void run() {
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(", ");
cleanerThreadLock.readLock().lock();
try {
if (trackedObjects.get() == 0) {
cleanerThreadLock.readLock().unlock();
cleanerThreadLock.writeLock().lock();
try {
if (trackedObjects.get() == 0) {
Logger.getLogger(Cleaner.class.getName()).log(Level.FINE, "Shutting down CleanerThread");
cleanerThread = null;
break;
}
registeredCleaners.append(cleanerRef.cleanupTask.toString());
} finally {
cleanerThreadLock.readLock().lock();
Copy link

Choose a reason for hiding this comment

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

See above. Re-locking can be avoided.

Copy link
Member Author

Choose a reason for hiding this comment

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

See above.

cleanerThreadLock.writeLock().unlock();
}
logger.log(Level.FINER, "Registered Cleaners: {0}", registeredCleaners.toString());
}
} finally {
cleanerThreadLock.readLock().unlock();
}
Logger logger = Logger.getLogger(Cleaner.class.getName());
if (logger.isLoggable(Level.FINER)) {
logger.log(Level.FINER, "Registered Cleaners: {0}", trackedObjects.get());
}
}
} catch (InterruptedException ex) {
Expand Down
Loading