Minor nits, overall looks like a good refactoring.
http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/GWTCompiler.java File dev/core/src/com/google/gwt/dev/GWTCompiler.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/GWTCompiler.java#newcode179 dev/core/src/com/google/gwt/dev/GWTCompiler.java:179: tempWorkDir = true; What is the default here? Will we have a good persistentCacheDir in the default case, for free? I'd think that would be desirable. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/GWTShell.java File dev/core/src/com/google/gwt/dev/GWTShell.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/GWTShell.java#newcode200 dev/core/src/com/google/gwt/dev/GWTShell.java:200: protected boolean doStartup() { If getWorkDir() is null, what will be the behavior here (shouldn't be the same as in GWTCompiler.java? http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java File dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java#newcode51 dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java:51: ~(Opcodes.ACC_DEPRECATED | Opcodes.ACC_NATIVE | Opcodes.ACC_STRICT line wrap? Bad formatter artifact? http://gwt-code-reviews.appspot.com/1375802/diff/9001/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/9001/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode258 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:258: } whitespace http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/Dependencies.java File dev/core/src/com/google/gwt/dev/javac/Dependencies.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/Dependencies.java#newcode182 dev/core/src/com/google/gwt/dev/javac/Dependencies.java:182: } else { all on one line? http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java File dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode32 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:32: /** What "weak" hash map are you referring to here? http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode74 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:74: */ Seems this doesn't need to be initialized to a value here, it can be declared final, and set to this value as a default else case in the constructor below. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode93 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:93: This should be AtomicBoolean (or at least volatile). http://gwt-code-reviews.appspot.com/1375802/diff/9001/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/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode109 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:109: setName("UnitCacheLoader"); Isn't this the default priority anyway? http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode228 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:228: if (unitWriteQueue.isEmpty()) { flush if msg == UnitWriteMessage.SHUTDOWN_THREAD also? http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode231 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:231: } catch (IOException ex) { Why use a level variable here? http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode303 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:303: */ make final http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode307 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:307: // TODO(zundel): do we need to be thread safe? It looks like addCount is only ever used for logging (in TRACE mode), so it's probably not required to use AtomicInteger for program correctness. But if you really want an accurate value, then AtomicInteger is a good choice (no locking, little contention). http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode324 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:324: super.add(newUnit); Seems this wait should happen in UnitWriter.run, so that this thread doesn't need to wait for unitCacheMapLoader(). http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode350 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:350: * Re send all units read in from the cache to the writer thread. They will typo? http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode386 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:386: Arrays.sort(sortedDirnames); loop on sortedDirNames? http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/UnitCache.java File dev/core/src/com/google/gwt/dev/javac/UnitCache.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/UnitCache.java#newcode20 dev/core/src/com/google/gwt/dev/javac/UnitCache.java:20: /** Is this correct? Seems the persistent version is a sub-class of the memory version, and uses both memory and persistent storage. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java File dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java#newcode27 dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java:27: /** Need to update comments here and below, it seems what's enabled is whether to use persistent caching or not (and it falls back to a MemoryUnitCache otherwise). e.g. it instantiates a cache regardlessly. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/jjs/JJSOptionsImpl.java File dev/core/src/com/google/gwt/dev/jjs/JJSOptionsImpl.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/jjs/JJSOptionsImpl.java#newcode27 dev/core/src/com/google/gwt/dev/jjs/JJSOptionsImpl.java:27: private boolean aggressivelyOptimize = true; Seems these changes are formatting only, so should be removed from the patch. http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
