http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java File dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java (left):
http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#oldcode303 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:303: /** Is this comment no longer relevant? http://gwt-code-reviews.appspot.com/1490801/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/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode100 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:100: */ what's the rationale for this change? Couldn't it lead to a larger backlog of old files that need to be read in at startup? http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode109 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:109: /** Just thinking aloud here (since I am wondering about this too with my caching stuff too)? What happens if multiple processes are using the cache, if an open cache file which is being written to is subsequently read by another process, and then the first process adds more to the file, etc., but meanwhile the second process has decided to compact things, etc.? Is this worth worrying about? Does it make sense to instead write new cache info to a temp file, and then only move it to it's final file name once it's complete and being closed? Or should we have the concept of cache locking, whereby only one process can ever use a cache directory at a time? http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode120 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:120: // This isn't 100% reliable if multiple processes are in contention You could change this so the do-while loop above has while (!newFile.createNewFile()) as it's loop condition, so that you atomically know that you've found the next free file name, and that you've also created it, etc. http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode147 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:147: List<File> cacheFiles = new ArrayList<File>(); need a new line below before the if() http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode164 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:164: I generally assume ALL_CAP_NAMES to refer to static constants. Seems weird for instance classes with anonymous implementations to use that naming? http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode231 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:231: Can we give this a more specific name, like "currentCacheFileStream"? http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode247 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:247: extra blank line here http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode265 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:265: If you only want 1 thread here, consider using Executors.newSingleThreadPool().... I'm guessing it might be a bit more lightweight, etc... http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode276 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:276: } catch (RejectedExecutionException e) { duplicate commas , , http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode318 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:318: super.add(newUnit); don't need final here http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode345 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:345: if (shouldRotate) { Was this done to simplify things? Any ramifications? http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode351 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:351: // Check to see if the previous purge task finished. Unless there's some synchronization, it might not be thread safe to inspect purgeTaskStatus here (and to set it below). Is there any reason not to make this whole cleanup() method synchronized? Or at least the block from here to where it's reset below? http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode367 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:367: */ Yes, agreed...unless you make the whole cleanup method synchronized....(or at least surrounding the larger block including the purgeTaskStatus management....) http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode373 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:373: Note, unless there is synchronization here, purgeTaskStatus could have been reset by another thread before this thread sets it here. http://gwt-code-reviews.appspot.com/1490801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
