http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/DevModeBase.java File dev/core/src/com/google/gwt/dev/DevModeBase.java (right):
http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/DevModeBase.java#newcode1042 dev/core/src/com/google/gwt/dev/DevModeBase.java:1042: try { white space http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode450 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:450: Is there a reason we prefer persistent cache units over in-memory cached units in the session? Above, in buildFrom, it seems the preference order was the other way around? http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java File dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode48 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:48: * This was a bit confusing to me. On the one hand, it says there is currently a single global instance (which should imply 1 file). On the other hand it discusses a threshold of multiple files (but isn't there just 1 file for our 1 global instance?) http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode57 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:57: * first reference to find() or add(). When the cache is large (10000 units), it should be "....starts this in a background thread...." http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode118 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:118: loadUnitMap(logger); Just to be safe, move latch countDown() to a finally block, in case an unchecked exception occurs. http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode160 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:160: // wait for shutdown Should you wait a maximum amount of time before bailing? In case the write thread is hung trying to write to a stale nfs file, etc. http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode229 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:229: I think there needs to be some synchronization for access to the static fields and methods below. Can either make all the static methods synchronized, or I've suggested some object level changes below....Alternatively, remove static instance, and instantiate only if in use (remove need for 'enabled' and 'instance', etc.). http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode232 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:232: */ use AtomicBoolean. http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode234 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:234: references to instance need to be synchronized. It's referenced and initialized and set to null in several static methods below. Suggest use a Lock before referencing it (e.g. a lock object, or a ReentrantLock, etc.). http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode236 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:236: remove the setCacheDirectory method and make final, or use an AtomicReference or perhaps can get away with volatile. Complicated by the initial static initializer below + setCacheDirectory static method. http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode239 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:239: private static final String CACHE_PREFIX = "cache-"; How about "gwt-persistent-unit-cache-" or to be shorter "gwt-puc-" http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode306 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:306: if (instance == null) { Do you need to create instance here? Can't you just return null? http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode407 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:407: // Unix) Use System.getProperty("java.io.tmpdir"), which is generally set to a useful default for each OS, etc. http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode425 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:425: */ Declare this final, initialize here (or in constructor) http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode427 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:427: make this a final AtomicInteger if you need it to be accurate in multithreaded environment. http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode461 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:461: // Send all units in the unit map to the writer thread. If unitMap is a synchronizedMap, need to synchronize on unitMap while iterating (see suggestion to use ConcurrentHashMap below). http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode464 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:464: } Update comment? http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode484 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:484: SpeedTracerLogger.start(DevModeEventType.LOAD_PERSISTENT_UNIT_CACHE); Perhaps use a ConcurrentHashMap here? It looks like you create an iterator over unitMap's entries above, so you either need to synchronize access while iterating, or use a concurrent map. http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode520 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:520: } put in finally clause? http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
