High-level, my major issues at this point are: 1) MemoryUnitCache having anything to do with the on-disk cache.
2) Using PUC from entry points other than DevMode and Compiler (although we should seriously consider the JUnitShell use case). 3) Caching by type name instead of resource location. On 2011/03/22 17:33:02, zundel wrote:
Uploaded a new patch that adds a unit test for the persistent cache
and fixes
the problem unit testing the type oracle. After discussion with
tobyr,
modified the unitMap and unitMapByContentId to be HARD key, SOFT
value. +1 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; I'm coming around to thinking that only DevMode and Compiler (both of which have a war dir) are the only entry points that should use PUC at all. I don't see any real use for it from any other entry point, it just makes things more complicated. 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()); Or we just disable PUC from those entry points, which kind of makes sense anyway. If you're doing something sophisticated with Precompile, you're ultimately going to want to use CompileModule anyway instead of PUC. "war or nothing" seems like a pretty simple and effective strategy to me. 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); Glad you fixed this. NFS, virus scanners, disk encryption, can all make an impact here. (I'm particularly aware of this angle since one of my machines is a Windows with virus scanning & disk encryption, and disk access is much, much slower than on a similar Linux system for me.) 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#newcode324 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:324: } That should be fine as long as the cacheDirectory is the same for all invocations, right? Seems bad to not check. 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: } Sorry, I didn't very clearly make the point that you just made, which is that it's so cheap to instantiate an in-memory cache, you might as well just auto-initialize the in-memory cache and replace it later if necessary. Persistent unit cache doesn't make sense for unit tests. For directly testing PUC itself you'd set it up manually, and for anything else you'd rather have the hermeticism and decoupling. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode339 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:339: * My one issue here is that this used to lookup by location, not type name. Doing it by location means that different modules with different super source paths can coexist peacefully. Doing it by type name means that two different resources with the same logical type name end up stepping on each other. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode340 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:340: * TODO: maybe use a finer brush than to synchronize the whole thing. Yay, nice! If it's stale, should we go ahead and evict the stale unit to free up memory (since we're about to compile, which will need a lot of memory)? http://gwt-code-reviews.appspot.com/1375802/diff/10027/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/10027/dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java#newcode184 dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java:184: return lastModified; Unlike SourceFileCompilationUnit, I wouldn't think this one would need this change? GeneratedUnit should handle consistency. http://gwt-code-reviews.appspot.com/1375802/diff/10027/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/10027/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode44 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:44: private final UnitOrigin source; rename field and getter 'source' -> 'origin' too? http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode90 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:90: protected final Map<String, UnitCacheEntry> unitMap = Collections Document the semantic meaning of the key? e.g. unitMapByXXX? http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode95 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:95: // compareTo() and hashCode(), but it looks like its failing in this case. This comment still true? http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode108 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:108: * from the system property <code>java.io.tmpdir</code>. This still seems way unintuitive to me. Is there no better way to handle this? MemoryUnitCache shouldn't import java.io.File at all, IMHO. Why should every unit test that uses MemoryUnitCache affect the disk? http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode111 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:111: String propertyCacheDir = System.getProperty("gwt.persistentunitcachedir"); How does this relate to UnitCacheFactory.isPersistent? Are these supposed to be two different properties, or is this a left-over from a previous patch version? http://gwt-code-reviews.appspot.com/1375802/diff/10027/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/10027/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode237 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:237: } This block is typically rendered Utility.close(stream). http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode255 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:255: Runtime.getRuntime().removeShutdownHook(this); "this" is UnitWriteMessageThread, you'd need to save a reference to the original shutdown hook thread. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode323 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:323: if (unitCacheEntry.getSource() == UnitOrigin.PERSISTENT) { Once the unit is copied from an 'old' cache file into the current one, shouldn't its state get updated? Otherwise, multiple identical copies could get written into the current cache. Now that I think about it, it seems like UnitOrigin is really a flag between whether or not the unit has been written to the current cache file. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode401 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:401: } Another Utility.close() http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/SourceFileCompilationUnit.java File dev/core/src/com/google/gwt/dev/javac/SourceFileCompilationUnit.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/SourceFileCompilationUnit.java#newcode52 dev/core/src/com/google/gwt/dev/javac/SourceFileCompilationUnit.java:52: this.lastModified = sourceFile.getLastModified(); Yes, this was the exact reason ResourceTag/resourceContentCache existed. But you should move the initialization into getSource(). http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/SourceFileCompilationUnit.java#newcode70 dev/core/src/com/google/gwt/dev/javac/SourceFileCompilationUnit.java:70: String sourceCode = Shared.readSource(sourceFile); Initialize lastModified right before reading the source, to ensure the lastModified most closely matches the content. (Doing it before means you see newer content with an older mod time, which worst case means duplicate work later; reading first then reading the mod time means you potentially miss an update and the user gets stale code). http://gwt-code-reviews.appspot.com/1375802/diff/10027/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/10027/dev/core/src/com/google/gwt/dev/javac/UnitCache.java#newcode28 dev/core/src/com/google/gwt/dev/javac/UnitCache.java:28: static final String UNIT_CACHE_PREFIX = "gwt-unitCache"; 'static' and 'final' are also redundant http://gwt-code-reviews.appspot.com/1375802/diff/10027/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/10027/dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java#newcode42 dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java:42: return new PersistentUnitCache(logger, cacheDir); It occurs to me that every client that uses the same directory ought to share the same PUC. A weak map of File -> PUC seems correct here to get singleton behavior by File. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java File dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java#newcode12 dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java:12: private static AtomicInteger nextTimestamp = new AtomicInteger(1); final http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java#newcode28 dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java:28: return "Mock: " + typeName; In other places, we always use "/mock/" + typeName http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java File dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java#newcode17 dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java:17: cacheDir.deleteOnExit(); This works? The doc for File.delete() says it only works for empty directories, I would have though this directory would not be empty at the end of the test run. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java#newcode18 dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java:18: } catch (IOException ex) { Just add the throws to the unit test method, the original exception is generally better in test failure logs. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java#newcode37 dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java:37: assertNotNull(result); Total opinion, but I tend to find that this level of pedantic checking makes the test code harder to read. The next line would throw an NPE anyway, so why clutter the test code? http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
