Reading the code, I think what I'm missing is some kind of theory of operation. I don't have a good mental model of the life cycle of individual files as they relate to GWT processes, refreshes, builds, etc. I can't easily answer questions like:
- Does PUC use a different cache file each refresh? Each process? - How exactly do CUs from a previous run 'roll forward' into the next iteration of the cache? - How does a unit get evicted? Is there any relationship to CU invalidation? I basically need to form the big picture. http://gwt-code-reviews.appspot.com/1375802/diff/5002/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/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode85 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:85: private static class UnitCacheEntry { Is this class an artifact of a previous design? I don't think it does anything anymore. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode109 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:109: unitCacheLoaderThread.setName("UnitCacheLoader"); Also, setPriority()? http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode110 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:110: unitCacheLoaderThread.start(); I think this is Discouraged. It might make more sense to make simply make this class UnitCacheLoaderThread extends Thread, and have the caller call 'start' on it, instead of decoupling the thread from the runnable. In this case, the two are so strongly bound that there's no simplicity in decoupling them. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode151 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:151: private class UnitWriteThread implements Runnable { Again, you might as well just extend Thread the two are so tightly bound. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode161 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:161: unitWriteThread.start(); And same comment on auto-starting. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode174 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:174: shutDownLatch.await(); Use the await(long, TimeUnit) overload instead of the TimerTask stuff, and check the return value to decide if you should interrupt() the writer thread. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode205 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:205: // ignore You'd want to bail here if you allow the Shutdown thread to interrupt this one. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode233 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:233: ex.printStackTrace(); Logger? http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode246 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:246: private static boolean enabled = false; It seems like the state of this variable has such radical implications as to warrant splitting a dummy vs. real cache class. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode282 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:282: public static synchronized void cleanup(TreeLogger logger) { I don't really understand why CSB needs to care, or why this needs to be explicitly called. Won't on-disk caches be cleaned up eventually in subsequent runs? http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode293 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:293: public static synchronized void clearPersistentCache(TreeLogger logger) { This doesn't feel like public API either. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode295 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:295: // delete the current PersistentUnitCache instance I don't think this code path is currently possible. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode324 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:324: * out any old cached files. Why would running without a cache cause a clear? http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode379 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:379: public static synchronized void setEnabled(boolean value) { This seems weird, since you already have init(). Presumbly, if you call init(), you want it. If you don't want it, don't call init(). http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode442 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:442: * forever in {@link ComplationStateBuilder} anyway. It seems like, in this design, CompilationStateBuilder just wants to delegate ALL of its caching logic to this class. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode471 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:471: private void cleanupCacheFiles(TreeLogger logger) { I'm not sure I understand what this is doing, really. Shouldn't everything have already been written out to the current cache file? Why do we have to do it again? http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
