Re: [gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
> Thanks for the review! This looks pretty exciting. Is it ready enough that, if I was comfortable running trunk (which I am for certain projects), I could enable now? Any rough stats on the speed up (particularly for DevMode) for small/large codebases? Thanks, Stephen -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
submitted as r9893 Thanks for the review! http://gwt-code-reviews.appspot.com/1375802/diff/10040/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/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode256 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:256: shutDownLatch.await(5000, TimeUnit.MILLISECONDS); On 2011/03/24 20:24:44, scottb wrote: Do we really need this? I'm thinking maybe we should just wait forever. I thought it would be better to have a timeout, as I don't want to wait forever if there is some kind of filesystem hang trying to delete a file or something. In that case, the program won't shut down properly, leaving DevMode still running with a port open. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode318 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:318: } On 2011/03/24 20:24:44, scottb wrote: I think the other way was better, LinkedBlockingQueue never blocks on put/add. Done. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode365 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:365: } On 2011/03/24 20:24:44, scottb wrote: Same in these two cases, add() is better. Done. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode447 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:447: } On 2011/03/24 20:24:44, scottb wrote: Couldn't much of the above code be replaced with a call to getCacheFiles()? Done. http://gwt-code-reviews.appspot.com/1375802/diff/10040/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/10040/dev/core/src/com/google/gwt/dev/javac/UnitCache.java#newcode52 dev/core/src/com/google/gwt/dev/javac/UnitCache.java:52: void remove(CompilationUnit unit); On 2011/03/24 20:24:44, scottb wrote: Not called from the unit test anywhere. Done. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java File dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java#newcode23 dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java:23: * Unit test for {@link MemoryUnitCache} On 2011/03/24 20:24:44, scottb wrote: Checkstyle wants a period. Done. http://gwt-code-reviews.appspot.com/1375802/diff/10040/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/10040/dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java#newcode25 dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java:25: */ On 2011/03/24 20:24:44, scottb wrote: copy header should go above imports Done. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java#newcode62 dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java:62: return "/mock/" + typeName; On 2011/03/24 20:24:44, scottb wrote: "/mock/" + Shared.toPath(typeName) (Will need to update unit tests.) Done. http://gwt-code-reviews.appspot.com/1375802/diff/10040/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/10040/dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java#newcode26 dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java:26: * Unit test for {@link PersistentUnitCache} On 2011/03/24 20:24:44, scottb wrote: period Done. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java#newcode33 dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java:33: deleteDirRecursive(lastCacheDir); On 2011/03/24 20:24:44, scottb wrote: Reuse Util.recursiveDelete(lastCacheDir, false) Done. http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
http://gwt-code-reviews.appspot.com/1375802/diff/19040/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/19040/dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java#newcode29 dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java:29: import java.io.PrintWriter; You pulled in my extra imports. http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
LGTM, all nits. No need to re-review. 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#newcode70 dev/core/src/com/google/gwt/dev/javac/SourceFileCompilationUnit.java:70: String sourceCode = Shared.readSource(sourceFile); SGTM! Good thinking. http://gwt-code-reviews.appspot.com/1375802/diff/10040/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/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode241 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:241: // These additional closes make the unit tests run reliably. Can remove this comment now (the fix to the reader thread closing makes this not true, but the extra closes are probably a good idea anyway in case any intermediary construction fails). http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode256 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:256: shutDownLatch.await(5000, TimeUnit.MILLISECONDS); Do we really need this? I'm thinking maybe we should just wait forever. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode318 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:318: } I think the other way was better, LinkedBlockingQueue never blocks on put/add. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode365 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:365: } Same in these two cases, add() is better. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode447 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:447: } Couldn't much of the above code be replaced with a call to getCacheFiles()? http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode449 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:449: File cacheFile = new File(cacheDirectory, filename); Per face to face discussion, we refactored to skip if cacheFile is the one we actually opened to write to. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode477 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:477: Utility.close(inputStream); Per face to face, closing FIS also in case constructing the OOS fails. http://gwt-code-reviews.appspot.com/1375802/diff/10040/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/10040/dev/core/src/com/google/gwt/dev/javac/UnitCache.java#newcode52 dev/core/src/com/google/gwt/dev/javac/UnitCache.java:52: void remove(CompilationUnit unit); Not called from the unit test anywhere. http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java File dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java#newcode23 dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java:23: * Unit test for {@link MemoryUnitCache} Checkstyle wants a period. http://gwt-code-reviews.appspot.com/1375802/diff/10040/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/10040/dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java#newcode25 dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java:25: */ copy header should go above imports http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java#newcode62 dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java:62: return "/mock/" + typeName; "/mock/" + Shared.toPath(typeName) (Will need to update unit tests.) http://gwt-code-reviews.appspot.com/1375802/diff/10040/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/10040/dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java#newcode26 dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java:26: * Unit test for {@link
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
While trying to make sense of the flaky failures, we noticed the tests would consistently fail on windows. Scott tracked it down to the current file being opened for read and then left open on an EOF exception by the unitLoader thread. http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
I had some trouble with PersistentUnitCacheTest being flaky, which is why there are some changes in there. The count of files in the directory was not as expected at different points in the test. The solution seems to be to close all 3 output streams. 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; Reverted this file, Precompile, and PrecompileOnePerm. 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()); Reverted file. 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: } On 2011/03/22 21:59:31, scottb wrote: That should be fine as long as the cacheDirectory is the same for all invocations, right? Seems bad to not check. If we change to add persistent caching to unit tests, we will run into problems instantiating the persistent cache over and over, so I updated UnitCacheFactory() to keep a single global instance and return it. 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: } On 2011/03/22 21:59:31, scottb wrote: 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. For GWT unit tests, I agree a persistent cache might be bad, but for tests of code other than GWT core code, it might make sense. anyway, I did as you suggested and auto-initialized unitCache to a MemoryCache instance. 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: * On 2011/03/22 21:59:31, scottb wrote: 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. Done. 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. On 2011/03/22 21:59:31, scottb wrote: 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)? Done. 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; On 2011/03/22 21:59:31, scottb wrote: Unlike SourceFileCompilationUnit, I wouldn't think this one would need this change? GeneratedUnit should handle consistency. reverted 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; On 2011/03/22 21:59:31, scottb wrote: rename field and getter 'source' -> 'origin' too? Done. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCach
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
On Wed, Mar 23, 2011 at 1:02 PM, Eric Ayers wrote: > FYI: I am adding a call to return the resource location to the > CompilationUnit to handle changing the key. That's what CompilationUnitBuilder.getLocation() was used for and in practice, CompilationUnit.getDisplayLocation() returns exactly the same value and serves exactly the same purpose, although you'd need to update the antiquated Javadoc that suggests otherwise. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
On Wed, Mar 23, 2011 at 12:23 PM, Scott Blum wrote: > On Wed, Mar 23, 2011 at 9:57 AM, wrote: >> >> If we only write the cache to the same war dir subdirectory, then I >> think my concerns with purging the cache are solved (the cache will be >> in a well known place). I'm still going to make an exception so that >> setting a system property will allow the cache to be written to a >> specific dir so we can support the caching from GWTShell invocations, >> but I'll remove it from GWTCompiler, Precompile and PrecompileOneShard. > > SGTM. >> >> I'll change the key to lookup by resource name, but that's going to >> cause some indigestion for the "spam error message" reduction patch I've >> been working on. If there is ambiguity caused by super source, then >> that problem needs to be resolved anyway. > > It's not a true ambiguity imagine you're running two apps at once in > dev mode. The first module has a "com.example.Foo" at > 'com/example/Foo.java'. The second module has a totally different > implementation because it uses super source to get the copy at > 'com/example/super/com/example/Foo.java'. These are literally two different > compilation units with different source, mod times, and outputs. > FYI: I am adding a call to return the resource location to the CompilationUnit to handle changing the key. -- Eric Z. Ayers Google Web Toolkit, Atlanta, GA USA -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
On Wed, Mar 23, 2011 at 9:57 AM, wrote: > If we only write the cache to the same war dir subdirectory, then I > think my concerns with purging the cache are solved (the cache will be > in a well known place). I'm still going to make an exception so that > setting a system property will allow the cache to be written to a > specific dir so we can support the caching from GWTShell invocations, > but I'll remove it from GWTCompiler, Precompile and PrecompileOneShard. > SGTM. I'll change the key to lookup by resource name, but that's going to > cause some indigestion for the "spam error message" reduction patch I've > been working on. If there is ambiguity caused by super source, then > that problem needs to be resolved anyway. It's not a true ambiguity imagine you're running two apps at once in dev mode. The first module has a "com.example.Foo" at 'com/example/Foo.java'. The second module has a totally different implementation because it uses super source to get the copy at 'com/example/super/com/example/Foo.java'. These are literally two different compilation units with different source, mod times, and outputs. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
On 2011/03/22 21:59:31, scottb wrote: 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. If we only write the cache to the same war dir subdirectory, then I think my concerns with purging the cache are solved (the cache will be in a well known place). I'm still going to make an exception so that setting a system property will allow the cache to be written to a specific dir so we can support the caching from GWTShell invocations, but I'll remove it from GWTCompiler, Precompile and PrecompileOneShard. I'll change the key to lookup by resource name, but that's going to cause some indigestion for the "spam error message" reduction patch I've been working on. If there is ambiguity caused by super source, then that problem needs to be resolved anyway. http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
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/CompilationUnitBuild
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java File dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java#newcode1 dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java:1: package com.google.gwt.dev.javac; added header locally http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java#newcode8 dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java:8: added class comment locally. 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#newcode1 dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java:1: package com.google.gwt.dev.javac; added header and class comment locally. 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#newcode1 dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java:1: package com.google.gwt.dev.javac; added header and class comment locally. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/impl/MockResource.java File dev/core/test/com/google/gwt/dev/javac/impl/MockResource.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/impl/MockResource.java#newcode3 dev/core/test/com/google/gwt/dev/javac/impl/MockResource.java:3: * reverted file. http://gwt-code-reviews.appspot.com/1375802/diff/10027/eclipse/samples/Showcase/Showcase.launch File eclipse/samples/Showcase/Showcase.launch (left): http://gwt-code-reviews.appspot.com/1375802/diff/10027/eclipse/samples/Showcase/Showcase.launch#oldcode1 eclipse/samples/Showcase/Showcase.launch:1: referted file locally. http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
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. http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
http://gwt-code-reviews.appspot.com/1375802/diff/16004/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/16004/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode91 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:91: .synchronizedMap(new ReferenceMap(AbstractReferenceMap.HARD, AbstractReferenceMap.SOFT)); I thought about these maps while exercising this evening. Better blood flow to the head I guess. I remember one of the caveats of WeakHashMap is that if the (weak) key is referenced by the value, the key will never be garbage collected and the entry will always remain in the map. My thought was that the references for unitMap should be SOFT, SOFT, since we want the cache to be garbage collected and the key references a string in the value. Or, should we leave it as HARD, SOFT and do something to make sure we have a copy of the type name value when we add the key to unitMap? For the unitMapByContentId we probably want both the key and value to be WEAK, since they are just an alternate index on unitMap. Would that also eliminate the need to remove the contentId key when we replace a unit in unitMap? http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
One existing TypeOracle unit test still fails, and I haven't figured out a unit test for PersistentUnitCache yet, but I wanted to get feedback on the other changes I've made. 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) { On 2011/03/08 02:31:25, tobyr wrote: Have we figured out why this would ever be true on a no-op refresh? Yes - fix is in another CL for the lightweight HashMap. I also worked around it in Dependencies by not using 'put' inside an iteration, but instead updating the entry in place. 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; On 2011/03/15 20:56:17, jbrosenberg wrote: What is the default here? Will we have a good persistentCacheDir in the default case, for free? I'd think that would be desirable. there is a mediocre default of the java.io.tmpdir system property. I'm not that fond of it - maybe a 'null' directory parameter should disable the persistent cache. This is an "old" entry point anyway, hopefully it isn't used much. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/GWTShell.java File dev/core/src/com/google/gwt/dev/GWTShell.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/GWTShell.java#newcode200 dev/core/src/com/google/gwt/dev/GWTShell.java:200: protected boolean doStartup() { On 2011/03/15 20:56:17, jbrosenberg wrote: If getWorkDir() is null, what will be the behavior here (shouldn't be the same as in GWTCompiler.java? Yeah, its the java.io.tmpdir system property again. 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 { On 2011/03/15 21:08:13, scottb wrote: Does this change have anything to do with this CL? No, I was just cleaning up a warning. The Serializable is redundant, because PrecompilationResult already implements Serializable. 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()); On 2011/03/15 21:08:13, scottb wrote: I would have expected this to be war dir. That would be nice, but war dir is a linker option. We could change it, but we'd have to modify all the tools that invoke Precompile in this way too. 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()); On 2011/03/15 21:08:13, scottb wrote: Seems weird that this is workDir sometimes, and war dir other times. In the build with multiple entry points, war dir is a link option. Here, I'm not sure war dir would be such a great place, because there are parallel invocations of PrecompileOnePerm that might conflict if we put the cache into a shared dir between all of them. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java File dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java#newcode51 dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java:51: ~(Opcodes.ACC_DEPRECATED | Opcodes.ACC_NATIVE | Opcodes.ACC_STRICT On 2011/03/15 20:56:17, jbrosenberg wrote: line wrap? Bad formatter artifact? This is a consequence of the updated 100 char wrap setting for the Eclipse formatter. Sorry for the churn. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
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#newcode353 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:353: synchronized (unitMap) { Ahh, good call, didn't know that! http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
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#newcode353 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:353: synchronized (unitMap) { According to the javadoc for Collections.synchronizedMap(): "It is imperative that the user manually synchronize on the returned map when iterating over any of its collection views"(I think Eric added the synchronization here in response to an earlier comment of mine). I think this is correct here, and in this case we do want to lock the queue while cleaning up, no? (For ordering/precedence reasons?). http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
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/ja
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
Minor nits, overall looks like a good refactoring. 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; What is the default here? Will we have a good persistentCacheDir in the default case, for free? I'd think that would be desirable. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/GWTShell.java File dev/core/src/com/google/gwt/dev/GWTShell.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/GWTShell.java#newcode200 dev/core/src/com/google/gwt/dev/GWTShell.java:200: protected boolean doStartup() { If getWorkDir() is null, what will be the behavior here (shouldn't be the same as in GWTCompiler.java? http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java File dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java#newcode51 dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java:51: ~(Opcodes.ACC_DEPRECATED | Opcodes.ACC_NATIVE | Opcodes.ACC_STRICT line wrap? Bad formatter artifact? 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#newcode258 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:258: } whitespace 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#newcode182 dev/core/src/com/google/gwt/dev/javac/Dependencies.java:182: } else { all on one line? 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#newcode32 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:32: /** What "weak" hash map are you referring to here? http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode74 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:74: */ Seems this doesn't need to be initialized to a value here, it can be declared final, and set to this value as a default else case in the constructor below. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode93 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:93: This should be AtomicBoolean (or at least volatile). 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#newcode109 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:109: setName("UnitCacheLoader"); Isn't this the default priority anyway? http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode228 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:228: if (unitWriteQueue.isEmpty()) { flush if msg == UnitWriteMessage.SHUTDOWN_THREAD also? http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode231 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:231: } catch (IOException ex) { Why use a level variable here? http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode303 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:303: */ make final http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode307 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:307: // TODO(zundel): do we need to be thread safe? It looks like addCount is only ever used for logging (in TRACE mode), so it's probably not required to use AtomicInteger for program correctness. But if you really want an accurate value, then AtomicInteger is a goo
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
Couple more comments after reading Jason's feedback. 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#newcode188 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:188: ObjectOutputStream stream = null; This never gets closed, either. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode324 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:324: super.add(newUnit); Agreed; although I also wonder if super.add() needs to wait for load to finish, otherwise newly-loaded-from-disk units will tend to clobber built units, unless you handle this in loadUnitMap(). http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
On 2011/03/15 13:59:15, zundel wrote: First, I want to let you know I put a lot of thought into where to put the cache files and after discussions with Toby, we came up with the WEB-INF solution. There is precedent for putting the cache type files in WEB-INF (app engine integration with GWT does this). Also, its a terrific place when you consider that with our GWT ant setup, the cache gets removed when you run 'ant clean'.I've updated the Compiler entry point to also use the war/WEB-INF dir so the cache can be shared with web mode. SGTM, thanks for the explanation. http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
I didn't respond to Scott's initial comments: > - My gut reaction is that it's odd to make it a command-line option. It's not > really a user-facing 'option' in the same way that draftCompile or > style=PRETTY > is, it's just an internal implementation detail. It would seem more like a > jvm > flag, and perhaps later just always on. Toby, Jason and I had planned to put all of these dev mode changes behind a -X command line flag, but I went ahead and took out the command line flag and made it a system property because it simplified the API (enabled boolean flag in static api) > - Same with having to encode the persistence dir into every single entry > point. > Throwing it into WEB-INF in dev mode seems weird, and there's no fundamental > reason dev mode and the compiler shouldn't share a unit cache anyway. How > about > just using / creating a well-known directory under the current directory? If > you fail to create the directory (permission problem) then you don't use the > disk. First, I want to let you know I put a lot of thought into where to put the cache files and after discussions with Toby, we came up with the WEB-INF solution. There is precedent for putting the cache type files in WEB-INF (app engine integration with GWT does this). Also, its a terrific place when you consider that with our GWT ant setup, the cache gets removed when you run 'ant clean'.I've updated the Compiler entry point to also use the war/WEB-INF dir so the cache can be shared with web mode. The problem with using the current working directory is that potentially we are going to splatter cache directories all over the disk, and no user is going to thank us for that. Also, where would we tell users to clean things up? How could they write a reliable clean target? The problem with a 'well known' place, like /tmp/gwt-unitCache is that in a shared environment, that cache is not just going to be shared between compiles (what we want), it is going to be shared between different users on the same machine, and I'm pretty sure we don't want that. -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
Here is a re-working of the cache based on the feedback. I've removed caching inside CompilationStateBuilder, and made all cache lookups by the ContentId. Still TODO on this patch is a unit test, which I will work on tomorrow. 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()); On 2011/03/07 20:14:13, scottb wrote: I would dig on this more. It's perfectly fine and expected that some units will have null dependencies. That should not force those units to recompile... *provided that the dependencies remain null*. I think we need to go deeper... When you say the dependencies remain null, remain from when until when? Are you saying the Dependencies.validate() method needs revamping? or that we add a special case for checking null dependencies? http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode450 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:450: On 2011/03/07 20:14:13, scottb wrote: It does seem kind of strange, though, like the two need to be unified. Maybe PersistentUnitCache should be one of two UnitCache subclasses, or maybe internally it should either use the disk or not. UnitCache is now an interface MemoryUnitCache implements an in-memory cache PersistentUnitCache extends MemoryUnitCache to back up and reload the cache from disk. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/Compiler.java File dev/core/src/com/google/gwt/dev/Compiler.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/Compiler.java#newcode204 dev/core/src/com/google/gwt/dev/Compiler.java:204: // that persists between compilation runs and call PersistentCache.setCacheDir() On 2011/03/08 16:29:31, jbrosenberg wrote: s/call/calls to/ Done. 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"); On 2011/03/08 02:31:25, tobyr wrote: Make "gwt-unitCache" a constant somewhere (and replace all uses)? Done. 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, On 2011/03/08 02:31:25, tobyr wrote: Why not just branch the TreeLogger inside of init? Done. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/DevModeBase.java#newcode1061 dev/core/src/com/google/gwt/dev/DevModeBase.java:1061: } On 2011/03/08 16:29:31, jbrosenberg wrote: white space Done. 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 { On 2011/03/08 02:31:25, tobyr wrote: 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. Done. 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); On 2011/03/08 02:31:25, tobyr wrote: 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. Done. 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-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
I agree with making PUC non-static. This reduces the need to synchronize on getting instance, etc. 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#newcode110 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:110: unitCacheLoaderThread.start(); Fair enough. I guess I was thinking (without actually articulating) that we could have a TPE available as a service for DevMode thread tasks. This would allow control of threading in the app overall, etc. TPE allows scheduling at regular intervals, etc., such for cleaning up directories, or polling a work queue, etc. This would make it very trivial to say, make the adding of units be multi-threaded (can just schedule 10 parallel threads to read off the queue, etc.). http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
One other thing we should talk about is ditching all of the static-ness. The static-ness doesn't gel well with CompilationStateBuilder. CSB is usually a singleton, but you can instantiate an isolated CSB for testing which has a distinct cache that cannot be interfered with. It seems like CSB should auto-initialize itself with a new instance of PersistentUnitCache; but you'd also want a CSB constructor that takes in a DummyUnitCache that doesn't hit the disk at all, for unit tests. 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#newcode110 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:110: unitCacheLoaderThread.start(); TPE is massive overkill when you know you want exactly one thread. I don't see what the big deal is with extending thread when it's a persistent thread with only 1 task. http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
http://gwt-code-reviews.appspot.com/1375802/diff/1/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/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode232 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:232: */ Also need to synchronize isEnabled() http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/Compiler.java File dev/core/src/com/google/gwt/dev/Compiler.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/Compiler.java#newcode204 dev/core/src/com/google/gwt/dev/Compiler.java:204: // that persists between compilation runs and call PersistentCache.setCacheDir() s/call/calls to/ 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#newcode1061 dev/core/src/com/google/gwt/dev/DevModeBase.java:1061: } white space 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#newcode110 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:110: unitCacheLoaderThread.start(); Alternative might be to use a thread pool executor, and execute the runnable, etc. I'm not big fan of extending Thread. 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", cacheDirectory should be declared volatile, if it is assigned under a synchronized block, and referenced outside of one. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode360 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:360: */ This technically needs to be synchronized http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
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 exp
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
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
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
I'm looking at PUC itself now, but I wanted to go ahead and get out some quick comments on everything else. High-level: - My gut reaction is that it's odd to make it a command-line option. It's not really a user-facing 'option' in the same way that draftCompile or style=PRETTY is, it's just an internal implementation detail. It would seem more like a jvm flag, and perhaps later just always on. - Same with having to encode the persistence dir into every single entry point. Throwing it into WEB-INF in dev mode seems weird, and there's no fundamental reason dev mode and the compiler shouldn't share a unit cache anyway. How about just using / creating a well-known directory under the current directory? If you fail to create the directory (permission problem) then you don't use the disk. 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()); I would dig on this more. It's perfectly fine and expected that some units will have null dependencies. That should not force those units to recompile... *provided that the dependencies remain null*. http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode450 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:450: It does seem kind of strange, though, like the two need to be unified. Maybe PersistentUnitCache should be one of two UnitCache subclasses, or maybe internally it should either use the disk or not. 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#newcode47 dev/core/src/com/google/gwt/dev/javac/Dependencies.java:47: public CompiledClass getCompiledClass() { Not used anymore. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/Dependencies.java#newcode202 dev/core/src/com/google/gwt/dev/javac/Dependencies.java:202: return myRef.getSignatureHash().equals(theirs.getSignatureHash()); Given your changes, and that you always use signature hash now, you no longer need to bifurcate SerializedRef / DirectRef at all. You can use SerializedRef always (and just call it Ref). http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
updated patch http://gwt-code-reviews.appspot.com/1375802/diff/1/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/1/dev/core/src/com/google/gwt/dev/DevModeBase.java#newcode1042 dev/core/src/com/google/gwt/dev/DevModeBase.java:1042: try { On 2011/03/06 17:58:14, jbrosenberg wrote: white space Done. 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#newcode450 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:450: On 2011/03/06 17:58:14, jbrosenberg wrote: Is there a reason we prefer persistent cache units over in-memory cached units in the session? Above, in buildFrom, it seems the preference order was the other way around? I swapped the order here to be consistent. If the persistent cache is enabled, the other cache really becomes moot, because the contents should be identical. http://gwt-code-reviews.appspot.com/1375802/diff/1/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/1/dev/core/src/com/google/gwt/dev/javac/Dependencies.java#newcode171 dev/core/src/com/google/gwt/dev/javac/Dependencies.java:171: continue; On 2011/03/05 14:58:25, zundel wrote: Remove me: This was a failed experiment to get rid of invalid units. Done. http://gwt-code-reviews.appspot.com/1375802/diff/1/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/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode57 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:57: * first reference to find() or add(). When the cache is large (1 units), it On 2011/03/06 17:58:14, jbrosenberg wrote: should be "starts this in a background thread" Done. http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode118 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:118: loadUnitMap(logger); On 2011/03/06 17:58:14, jbrosenberg wrote: Just to be safe, move latch countDown() to a finally block, in case an unchecked exception occurs. Done. http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode160 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:160: // wait for shutdown On 2011/03/06 17:58:14, jbrosenberg wrote: Should you wait a maximum amount of time before bailing? In case the write thread is hung trying to write to a stale nfs file, etc. Done. http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode229 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:229: On 2011/03/06 17:58:14, jbrosenberg wrote: I think there needs to be some synchronization for access to the static fields and methods below. Can either make all the static methods synchronized, or I've suggested some object level changes belowAlternatively, remove static instance, and instantiate only if in use (remove need for 'enabled' and 'instance', etc.). The static design is intended to mirror that of CompilationStateBuilder. I didn't want to synchronize all of the work in find() and add() so I moved the work that I think needs be synchronized into getInstance(). http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode232 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:232: */ On 2011/03/06 17:58:14, jbrosenberg wrote: use AtomicBoolean. I chose to isolate this var in synchronized methods in getInstance() and setEnabled(). Sufficient? http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode234 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:234: On 2011/03/06 17:58:14, jbrosenberg wrote: references to instance need to be synchronized. It's referenced and initialized and set to null in several static methods below. Suggest use a Lock before referencing it (e.g. a lock object, or a ReentrantLock, etc.). I chose to isolate this var in synchronized methods, mostly getInstance() http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode236 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:236: On 2011/03/06 17:58:14, jbrosenberg wrote: remove the setCacheDirectory method and make
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
http://gwt-code-reviews.appspot.com/1375802/diff/1/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/1/dev/core/src/com/google/gwt/dev/DevModeBase.java#newcode1042 dev/core/src/com/google/gwt/dev/DevModeBase.java:1042: try { white space 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#newcode450 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:450: Is there a reason we prefer persistent cache units over in-memory cached units in the session? Above, in buildFrom, it seems the preference order was the other way around? http://gwt-code-reviews.appspot.com/1375802/diff/1/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/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode48 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:48: * This was a bit confusing to me. On the one hand, it says there is currently a single global instance (which should imply 1 file). On the other hand it discusses a threshold of multiple files (but isn't there just 1 file for our 1 global instance?) http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode57 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:57: * first reference to find() or add(). When the cache is large (1 units), it should be "starts this in a background thread" http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode118 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:118: loadUnitMap(logger); Just to be safe, move latch countDown() to a finally block, in case an unchecked exception occurs. http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode160 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:160: // wait for shutdown Should you wait a maximum amount of time before bailing? In case the write thread is hung trying to write to a stale nfs file, etc. http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode229 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:229: I think there needs to be some synchronization for access to the static fields and methods below. Can either make all the static methods synchronized, or I've suggested some object level changes belowAlternatively, remove static instance, and instantiate only if in use (remove need for 'enabled' and 'instance', etc.). http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode232 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:232: */ use AtomicBoolean. http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode234 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:234: references to instance need to be synchronized. It's referenced and initialized and set to null in several static methods below. Suggest use a Lock before referencing it (e.g. a lock object, or a ReentrantLock, etc.). http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode236 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:236: remove the setCacheDirectory method and make final, or use an AtomicReference or perhaps can get away with volatile. Complicated by the initial static initializer below + setCacheDirectory static method. http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode239 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:239: private static final String CACHE_PREFIX = "cache-"; How about "gwt-persistent-unit-cache-" or to be shorter "gwt-puc-" http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode306 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:306: if (instance == null) { Do you need to create instance here? Can't you just return null? http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode407 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:407: // Unix) Use System.getProperty("java.io.tmpdir"), which is generally set to a useful default for each OS
[gwt-contrib] Re: Adds a cache (PersistenUnitCache) to store CompilationUnits (issue1375802)
Some comments of my own to kick off the discussion. http://gwt-code-reviews.appspot.com/1375802/diff/1/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/1/dev/core/src/com/google/gwt/dev/DevModeBase.java#newcode1061 dev/core/src/com/google/gwt/dev/DevModeBase.java:1061: "Initializing CompilationUnit cache.")); This message is mainly there to confirm that the -XpersistentUnitCache flag is doing something. The long term intention is just for this to be a behind the scenes mechanism. Maybe I should TODO remove it once it becomes the default. 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 (left): http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#oldcode193 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:193: Map cachedStructurallySame = This mapping for tracking whether the non-private API of a class is changed is replaced by just computing a hash signature with BytecodeSignatureMaker inside of Dependencies.java 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()); We always get this message and a small number of units that have to go through and be re-built. Saved units have some dependencies with non-null values (the classes have been successfully compiled), but the dependencies that come out of the first build have some null dependencies. I think its because some sources that get remapped with rename-to or generate-with have yet to be added to the list of units passed to the compiler when we do the initial type oracle build. http://gwt-code-reviews.appspot.com/1375802/diff/1/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/1/dev/core/src/com/google/gwt/dev/javac/Dependencies.java#newcode171 dev/core/src/com/google/gwt/dev/javac/Dependencies.java:171: continue; Remove me: This was a failed experiment to get rid of invalid units. http://gwt-code-reviews.appspot.com/1375802/diff/1/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/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode72 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:72: * directory with a traditional place for compiled output. If we are happy with the dirs I've chosen, I should add purging the compiled dir to our webappcreator's build.xml clean target and the GWT ant config as well. http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode239 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:239: private static final String CACHE_PREFIX = "cache-"; too general purpose of a name for these files? http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/test/com/google/gwt/dev/javac/CompilationStateTest.java File dev/core/test/com/google/gwt/dev/javac/CompilationStateTest.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/test/com/google/gwt/dev/javac/CompilationStateTest.java#newcode565 dev/core/test/com/google/gwt/dev/javac/CompilationStateTest.java:565: // Refs should be converted into a compact serializable form This is the only change to this file. I'll back out the formatting change. http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors