1. The implementation is not a "true" CopyOnWrite. It is more of a "concurrent iteration safe wrapper". Implying that it is CopyOnWrite will confuse users.
2. Does the code pass the JSR-166 Unit tests?
3. For speed: Why are the getters synchronized? Could you use atomics?
4. What are you trying to solve, why not use a (some other class)?
I have updated the code, at http://flyingspaniel.wikidot.com/cow to address those comments:
1. To better indicate that these are not true CopyOnWrite, they are now named CISListWrapper and CISMapWrapper, where CIS stands for "Concurrent iteration safe". There are improved comments. Users who are familiar with CopyOnWrite behavior should not be confused or disappointed.
2. In addition to my own unit test (CISWrapperTest.java) I modified some of the JSR 166 unit tests, renaming them, making them more generic, and taking out a few tests (serialization) that didn't really apply (or work for me). This required a few changes in my code - mainly adding equals(), hashCode() and toString() methods that I had neglected.
3. As implemented, the reads do need to be synchronized. (also here). I wasn't concerned about ultimate speed, and this has the tremendous advantage of being more "idiot-proof". The wrapped Collection need not be synchronized. Removing the synchronization might require a user to add a Collections.synchronizedXXX() wrapper around the underlying collection, adding one more layer and eliminating any speed benefit.
I did consider changing the synchronization from the wraper to the wrapee. That is, instead of the current
public synchronized boolean containsKey(Object key) {
return wrapee.containsKey(key);
}
use instead:
public boolean containsKey(Object key) {
synchronized(wrapee) {
return wrapee.containsKey(key);
}
}
This is "left as an exercise for the reader" and might offer modest speed increases if the underlying collection were itself synchronized. If you really want the utmost in speed on simple reads, extend or use something like ConcurrentHashMap, or use one of the "true" CopyOnWriteMaps:
org.apache.mina.util.CopyOnWriteMap
and the Atlassian Concurrency Utilities.
4. So why use the CIS code? The "true" CopyOnWrite wrappers make a copy of your Map, and, for the most part, they copy the data into a basic HashMap or TreeMap. If that behavior is suitable, you are better off using their wrappers. Of course, if that basic behavior is suitable and you are seeking ultimate speed, you could consider rewriting your code to use the "standard" java.util.concurrent.ConcurrentHashMap.
If you have an existing List or Map that is not thread-safe or iteration-safe, and it has special or complex behavior that is not a simple HashMap or TreeMap, then this pure wrapper is useful. For example, you have a class com.mycompany.FunkyMap and it:
- validates inputs and throws Exceptions
- has special behavior for null keys or values
- has unusual rules for sorting
- implements some security rules on puts and gets. (and throws Exceptions)
- logs stuff
- encrypts values and stores them on a database
- is some facade or proxy (say, for an ORM)
- is buggy and you have set some breakpoints in your IDE or added printlns.
- is a singleton
In order to keep this behavior in a pure CopyOnWrite, the copy would have to itself be a FunkyMap, not a TreeMap. One could use reflection, but there goes all the speed bonus. To their credit, the Atlassian Utilities do provide for the possibility of subclasses. It's a small bit of work and may not be suitable for all cases.
No comments:
Post a Comment