I think I now understand the high-level design. Definitely seems workable, and probably pretty fast, too. Just a lot of nits to work out, I think.
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#newcode232 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:232: logger.log(TreeLogger.TRACE, "Invalid units found: " + invalidatedUnits.size()); Okay, after some deep digging, this appears to be a bad interaction between Dependencies and HashMap, see note in Dependencies. 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#newcode324 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:324: * out any old cached files. On 2011/03/14 23:24:26, zundel wrote:
Here's my logic. If you are compiling new units, then it is likely
you'd be
invalidating old units. Plus, I am just frightened of a cache that
never gets
purged. This is currently the only mechanism to purge the cache.
We had issues with the old CacheManager where you'd get into bad states and have to delete the cache directory to get back into a good state. I kind of think it would be better to assume from the start that the cache directory never, ever gets purged, and then prove to ourselves that we'll never do something incorrect, and that it won't grow without bound. 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 " That's exactly what we told people with CacheManager. :D http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/Precompile.java File dev/core/src/com/google/gwt/dev/Precompile.java (left): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/Precompile.java#oldcode133 dev/core/src/com/google/gwt/dev/Precompile.java:133: Serializable { Does this change have anything to do with this CL? http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/Precompile.java File dev/core/src/com/google/gwt/dev/Precompile.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/Precompile.java#newcode664 dev/core/src/com/google/gwt/dev/Precompile.java:664: CompilationStateBuilder.init(logger, options.getWorkDir()); I would have expected this to be war dir. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/PrecompileOnePerm.java File dev/core/src/com/google/gwt/dev/PrecompileOnePerm.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/PrecompileOnePerm.java#newcode186 dev/core/src/com/google/gwt/dev/PrecompileOnePerm.java:186: CompilationStateBuilder.init(logger, options.getWorkDir()); Seems weird that this is workDir sometimes, and war dir other times. 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 (left): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#oldcode394 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:394: ResourceTag tag = resourceContentCache.get(location); I think this cache is actually important. Essentially, this cache says "assume a source file is the same if its lastModified is the same". Without this cache, you have no choice but to read in the contents of ALL source files on every refresh just to compute the content ID, and you can't take advantage of lastModified. I think you either need to roll into UnitCache the concept of a (Location,lastModified) -> ContentId, or else resurrect some of this code and only compute ContentID on the initial load sequence rather than on every refresh. 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#newcode267 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:267: static class ResourceTag { This class is now unused (although, see below for why it was useful). http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode324 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:324: } Calling this twice with a different value should be some kind of error, right? http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode350 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:350: } Suggest auto-initializing this, or else having a specific constructor. If you're concerned about startup performance, CSB.get() could lazy initialize instance instead of static-initializing it. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java File dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java#newcode102 dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java:102: doGetSource(); This needs to be getSource(); otherwise you'll have to read in the source twice. Reading the source twice, in addition to being inefficient, could cause the content ID to appear to change. Although I worry about loading ALL the source into memory at the same time... see comment in CSB. 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#newcode100 dev/core/src/com/google/gwt/dev/javac/Dependencies.java:100: for (String ref : qualified.keySet()) { In other to work around a bug in HashMap (patch outstanding), please iterate over the entrySet() instead and use entry.setValue() to avoid the bug. (Also saves you extra hash computations / index lookups internally). I can verify that this fixes the problem you were seeing where certain units are always turning up invalid. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/Dependencies.java#newcode103 dev/core/src/com/google/gwt/dev/javac/Dependencies.java:103: qualified.put(ref, new Ref(cc.getInternalName(), cc.getSignatureHash())); Suggest a convenience constructor taking a CompiledClass to make this and the other instantiations nicer. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/Dependencies.java#newcode107 dev/core/src/com/google/gwt/dev/javac/Dependencies.java:107: for (String ref : simple.keySet()) { Same as above. 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#newcode59 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:59: protected static enum UnitSource { Confusing name, since "Source" generally means "java source code" around these parts. Perhaps "UnitOrigin"? http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode75 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:75: protected static File cacheDirectory = new File(System.getProperty("java.io.tmpdir")); This should have gone away, right? And definitely shouldn't be static? http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode83 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:83: * forever in {@link ComplationStateBuilder} anyway. The values used to be SOFT so that they could get gc'd under memory pressure. (You'd have to do the same for unitMap too for it to work.) http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode90 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:90: // compareTo() and hashCode(), but it looks like its failing in this case. Need to figure this out, ContentId should definitely work as a key, that's the whole reason for it. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode96 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:96: MemoryUnitCache(TreeLogger logger, File cacheDir) { Seems like MemoryUnitCache shouldn't take any parameters, only PersistentUnitCache. Any way, this whole ctor seems different from the conversational description. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode130 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:130: PersistentUnitCache.deleteOldCacheFiles(logger, null); This seems like it wants to run on a background thread to not block the main thread of execution. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode131 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:131: clearedUnitCache = true; I think I agree with Toby, this should be a no-op. Totally strange that this would do this. And what happens if you're running dev mode with PUC enabled and have open files, and then you run a compile without PUC and it blows away the files out from under you? 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#newcode126 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:126: loadCompleteLatch.countDown(); Reorder these two statements, in case the log() call throws an exception. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode174 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:174: shutDownLatch.countDown(); There's no reason to do this, this is the only thread that's ever waiting on the latch. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode233 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:233: if (!errorLogged) { doesn't seem to do anything http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode237 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:237: .log(level, "Error saving unit to file:" + currentCacheFile.getAbsolutePath(), ex); Checkstyle says, put a space after the colon. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode242 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:242: shutDownLatch.countDown(); Reorder these. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode250 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:250: * If there are more than this many files in the cache, clean up the old files checkstyle wants a period http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode272 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:272: if (!file.equals(currentCacheFilename)) { Using File.equals() would be simpler and more reliable here. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode313 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:313: super(logger, cacheDirectory); "cacheDirectory" is definitely wrong; but I think MemoryUnitCache doesn't want to deal with files at all. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode353 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:353: synchronized (unitMap) { This doing what you want depends heavily on the implementation details of Collections.synchronizedMap(). Either you don't need the synchronized keyword here, or else you need to do something like unitMap.values.toArray() to grab a snapshot and then iterate over your copy. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode401 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:401: UnitCacheEntry oldEntry = unitMap.get(unit.getTypeName()); Seems like you want to double-check timestamp to validate which one is actually the "old" entry. You can't really rely on timestamp of the enclosing file due to two-process concurrency. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode417 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:417: } finally close the input stream 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#newcode28 dev/core/src/com/google/gwt/dev/javac/UnitCache.java:28: public static final String UNIT_CACHE_PREFIX = "gwt-unitCache"; "public static final" is redundant; checkstyle warning http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/UnitCache.java#newcode51 dev/core/src/com/google/gwt/dev/javac/UnitCache.java:51: CompilationUnit find(TreeLogger logger, String typeName); This overload isn't used anywhere (nor are the implementations in the concrete classes). 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#newcode38 dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java:38: public static synchronized UnitCache create(TreeLogger logger) { Does not appear to be used. 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#newcode1 dev/core/src/com/google/gwt/dev/jjs/JJSOptionsImpl.java:1: /* Think you can revert this file from your change. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/test/com/google/gwt/dev/javac/BytecodeSignatureMakerTest.java File dev/core/test/com/google/gwt/dev/javac/BytecodeSignatureMakerTest.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/test/com/google/gwt/dev/javac/BytecodeSignatureMakerTest.java#newcode565 dev/core/test/com/google/gwt/dev/javac/BytecodeSignatureMakerTest.java:565: fail("Signatures match. raw data expected=<" + originalRaw Seemed right the first time? http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/test/com/google/gwt/dev/javac/CompilationStateTestBase.java File dev/core/test/com/google/gwt/dev/javac/CompilationStateTestBase.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/test/com/google/gwt/dev/javac/CompilationStateTestBase.java#newcode1 dev/core/test/com/google/gwt/dev/javac/CompilationStateTestBase.java:1: /* Wants to be reverted. http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
