http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/DevMode.java File dev/core/src/com/google/gwt/dev/DevMode.java (right):
http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/DevMode.java#newcode417 dev/core/src/com/google/gwt/dev/DevMode.java:417: File persistentCacheDir = new File(options.getWarDir(), "/WEB-INF/gwt-unitCache"); Make "gwt-unitCache" a constant somewhere (and replace all uses)? http://gwt-code-reviews.appspot.com/1375802/diff/5002/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/5002/dev/core/src/com/google/gwt/dev/DevModeBase.java#newcode889 dev/core/src/com/google/gwt/dev/DevModeBase.java:889: PersistentUnitCache.init(getTopLogger().branch(TreeLogger.INFO, Why not just branch the TreeLogger inside of init? http://gwt-code-reviews.appspot.com/1375802/diff/5002/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/5002/dev/core/src/com/google/gwt/dev/GWTCompiler.java#newcode180 dev/core/src/com/google/gwt/dev/GWTCompiler.java:180: } else { This logic was a bit confusing to follow. Mind setting persistentCacheDir to null in the if block instead of initializing it to null? A comment might help too. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java (left): http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#oldcode422 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:422: CompilationUnit existingUnit = unitCache.get(contentId); Put the check of the unitCache and PUC together (for example, in a method) so you don't duplicate the the cachedUnits.put/compileMoreLater logic. Or... replace unitCache with PUC altogether. http://gwt-code-reviews.appspot.com/1375802/diff/5002/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/5002/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode231 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:231: if (invalidatedUnits.size() > 0) { Have we figured out why this would ever be true on a no-op refresh? http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode252 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:252: PersistentUnitCache.cleanup(logger); Is there some reason this can't just happen on a thread on a periodic timer in PUC? http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode393 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:393: // run in this process. Is it possible to just replace unitCache with PUC at this point? Under what circumstances would we want something in the unitCache, but not in the PersistentUnitCache? http://gwt-code-reviews.appspot.com/1375802/diff/5002/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/5002/dev/core/src/com/google/gwt/dev/javac/Dependencies.java#newcode142 dev/core/src/com/google/gwt/dev/javac/Dependencies.java:142: * classes. Must be called before {@link #validate(String, Map, Map)}. Update this link tag. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/Dependencies.java#newcode215 dev/core/src/com/google/gwt/dev/javac/Dependencies.java:215: logger.log(TreeLogger.DEBUG, "Invalid ref: " + entry.getKey() + " mine: " Wrap in isLoggable? http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/Dependencies.java#newcode228 dev/core/src/com/google/gwt/dev/javac/Dependencies.java:228: logger Wrap in isLoggable? 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#newcode49 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:49: * Each run of the compiler should call {@link cleanup()} when finished adding s/cleanup/#cleanup. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode55 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:55: * single file. This reads really strange. I think more explanation is needed. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode61 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:61: * uses lots of heap and takes 5-10 seconds ({@link #init(TreeLogger)} starts Missing a period? http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode62 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:62: * eagerly loading the cache a background thread). s/cache a background/cache in a background http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode64 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:64: * - Although duplicate units with the same type name are eventually cleaned up, It'd be nice for this doc to show up correctly in quick view for javadoc. I.E. use list, paragraph, etc... tags. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode77 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:77: * What happens if you run 2 devmodes or a devmode + web compile concurrently with the same cachdir set? Boom? I think that's a case where we need to make sure we don't explode, even if it's something along disabling cache writes or the cache entirely. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode117 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:117: logger.log(logger.ERROR, "Interrupted waiting for PersistentUnitCache to load.", ex); s/logger.ERROR/TreeLogger.ERROR http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode250 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:250: private static File cacheDirectory = new File(System.getProperty("java.io.tmpdir", What happens when multiple GWT processes (devmode + compiler) share this folder? http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode253 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:253: private static final String CACHE_PREFIX = "gwt-unitCache-"; I think this constant wants to be public and referenced from the other places it's already being used. 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) { Nobody seems to call this, except for init() on cases of enabled == false which should happen... never? http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode346 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:346: logger.log(TreeLogger.TRACE, "Persistent unit cache dir set to: " Shouldn't this be cacheDirectory instead of cacheDir? http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode348 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:348: Should there be some test around here that ensures the cacheDirectory is actually available and accessible? http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode352 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:352: PersistentUnitCache.clearPersistentCache(logger); Under what conditions would you call init() and enabled not be true? http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode374 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:374: * {@link #add(CompilationUnit)} and {@link #findUnit(String)} methods will be Bad link for add, but agree with scottb that it seems this method should just disappear. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode397 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:397: if (current != null) { I don't understand the treatment of a null current. In the case that current is null, won't all of the equal tests in the for loop below fail? In other words, isn't this entire method a no-op with a null current? http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode423 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:423: files = cacheFiles.toArray(new String[0]); s/new String[0]/new String[cacheFiles.size()] 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. Bad link to CompilationStateBuilder. I think scottb's comment resonates with mine about dropping CSB.unitCache in favor of PUC. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerPersistentUnitCache.java File dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerPersistentUnitCache.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerPersistentUnitCache.java#newcode33 dev/core/src/com/google/gwt/dev/util/arg/ArgHandlerPersistentUnitCache.java:33: return "Enable persistent CompliationUnit caching. If this argument " This seems like strange behavior to me. http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
