[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
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)
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.
[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)
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)
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)
On Wed, Mar 23, 2011 at 9:57 AM, zun...@google.com 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 Wed, Mar 23, 2011 at 12:23 PM, Scott Blum sco...@google.com wrote: On Wed, Mar 23, 2011 at 9:57 AM, zun...@google.com 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 1:02 PM, Eric Ayers zun...@google.com 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)
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: ?xml version=1.0 encoding=UTF-8 standalone=no? 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)
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):
[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/ -- 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)
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.
[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 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)
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)
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)
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 good
[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):
[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)
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):
[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)
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)
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)
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)
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)
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?
[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 explanation
[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, etc.
[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: MapCompiledClass, CompiledClass 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
