Hi,
While browsing code in java.lang.reflect.Module (I sometimes do that
just to see how thinks work ;-) I stumbled on the following nested class:
private static class WeakSet<E> {
private final ReadWriteLock lock = new ReentrantReadWriteLock();
private final Lock readLock = lock.readLock();
private final Lock writeLock = lock.writeLock();
private final WeakHashMap<E, Boolean> map = new WeakHashMap<>();
/**
* Adds the specified element to the set.
*/
void add(E e) {
writeLock.lock();
try {
map.put(e, Boolean.TRUE);
} finally {
writeLock.unlock();
}
}
/**
* Returns {@code true} if this set contains the specified element.
*/
boolean contains(E e) {
readLock.lock();
try {
return map.containsKey(e);
} finally {
readLock.unlock();
}
}
}
...while this seems OK from 1st look, it is not. WeakHashMap is not
thread-safe even for seemingly read-only operations. All its operations
can mutate internal state in a non-thread-safe way. The simplest way to
fix this is to use a writeLock for containsKey operation too. But such
structure does not scale well to multiple threads for frequent lookups.
WeakSet is used in Module to keep track of transient read edges, exports
and uses of services added dynamically to the module. Modification
operations are not so performance critical, but lookup operations are.
I propose to add a thread-safe WeakPairMap data structure which
associates a pair of weakly-reachable keys with a strongly-reachable
value based on ConcurrentHashMap. Such data structure is
footprint-friendly, since only a single instance exists for a particular
purpose, totaling 3 instances for the transient structures serving all
Modules in the system:
http://cr.openjdk.java.net/~plevart/jdk9-dev/Module.WeakSet.multithreadUnsafe/webrev.01/
So, what do you think?
Regards, Peter
