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/MemoryUnitCache.java#newcode90 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:90: protected final Map<String, UnitCacheEntry> unitMap = Collections On 2011/03/22 21:59:31, scottb wrote:
Document the semantic meaning of the key? e.g. unitMapByXXX?
Done. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode95 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:95: // compareTo() and hashCode(), but it looks like its failing in this case. On 2011/03/22 21:59:31, scottb wrote:
This comment still true?
removed http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode108 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:108: * from the system property <code>java.io.tmpdir</code>. On 2011/03/22 21:59:31, scottb wrote:
This still seems way unintuitive to me. Is there no better way to
handle this?
MemoryUnitCache shouldn't import java.io.File at all, IMHO. Why
should every
unit test that uses MemoryUnitCache affect the disk?
removed. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode111 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:111: String propertyCacheDir = System.getProperty("gwt.persistentunitcachedir"); On 2011/03/22 21:59:31, scottb wrote:
How does this relate to UnitCacheFactory.isPersistent? Are these
supposed to be
two different properties, or is this a left-over from a previous patch
version? all of this logic moved to the PersistentUnitCache constructor and the java.io.tmpdir fallback has been removed. The gwt.persistentunitcachedir is used to set a cache directory for GWTShell. The the main class options in that case don't support the '-war' option. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode182 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:182: void deleteOldCacheFiles(TreeLogger logger, File current) { moved to PersistentUnitCache http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode203 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:203: File[] getCacheFiles() { moved to PersistentUnitCache http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java File dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode237 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:237: } On 2011/03/22 21:59:31, scottb wrote:
This block is typically rendered Utility.close(stream).
Done. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode255 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:255: Runtime.getRuntime().removeShutdownHook(this); On 2011/03/22 21:59:31, scottb wrote:
"this" is UnitWriteMessageThread, you'd need to save a reference to
the original
shutdown hook thread.
Done. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode323 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:323: if (unitCacheEntry.getSource() == UnitOrigin.PERSISTENT) { On 2011/03/22 21:59:31, scottb wrote:
Once the unit is copied from an 'old' cache file into the current one,
shouldn't
its state get updated? Otherwise, multiple identical copies could get
written
into the current cache.
Now that I think about it, it seems like UnitOrigin is really a flag
between
whether or not the unit has been written to the current cache file.
I only intended this logic to run once per cache instantiation, (only one file is created per cache instantiation, so this should hold unless another process gets in the way). To make this more explicit, I added a boolean. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode401 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:401: } On 2011/03/22 21:59:31, scottb wrote:
Another Utility.close()
Done. 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); On 2011/03/22 21:59:31, scottb wrote:
Initialize lastModified right before reading the source, to ensure the lastModified most closely matches the content. (Doing it before means
you see
newer content with an older mod time, which worst case means duplicate
work
later; reading first then reading the mod time means you potentially
miss an
update and the user gets stale code).
This is an important detail to get right, because we use resource times to compare staleness, and getting this wrong will give the cache a reputation of unreliability. I don't think moving the time setting to getSource() is what we want. If getSource() were first called before compiling the classes, and that source is then used in the compile, then yes, but I don't think that's what happening. The unit is constructed after the compiler calls process(). I don't know how long the windows is after the source is read to when this unit is created. We use the unit last modified time to compare with the source file resource time. The compiled classes (what we really care about being stale) and contentId are both set in the constructor, so if the source changes before someone calls 'getSource()', you'd be marking a stale unit as more current than it really is, right? The time we really want is the resource modified time when the source file was read for the JDT compile. I changed the constructor so that the last modified value is passed down from when the last modified time is set in from the buidler. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/UnitCache.java File dev/core/src/com/google/gwt/dev/javac/UnitCache.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/UnitCache.java#newcode28 dev/core/src/com/google/gwt/dev/javac/UnitCache.java:28: static final String UNIT_CACHE_PREFIX = "gwt-unitCache"; On 2011/03/22 21:59:31, scottb wrote:
'static' and 'final' are also redundant
moved them to PersistentUnitCache. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java File dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java#newcode42 dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java:42: return new PersistentUnitCache(logger, cacheDir); On 2011/03/22 21:59:31, scottb wrote:
It occurs to me that every client that uses the same directory ought
to share
the same PUC. A weak map of File -> PUC seems correct here to get
singleton
behavior by File.
Yeah, that would be great, but we only really need to support a single unit cache system wide anyway, so I made it a single static instance. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java File dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java#newcode12 dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java:12: private static AtomicInteger nextTimestamp = new AtomicInteger(1); On 2011/03/22 21:59:31, scottb wrote:
final
Done. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java#newcode28 dev/core/test/com/google/gwt/dev/javac/MockCompilationUnit.java:28: return "Mock: " + typeName; On 2011/03/22 21:59:31, scottb wrote:
In other places, we always use "/mock/" + typeName
Done. http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java File dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java#newcode17 dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java:17: cacheDir.deleteOnExit(); On 2011/03/22 21:59:31, scottb wrote:
This works? The doc for File.delete() says it only works for empty
directories,
I would have though this directory would not be empty at the end of
the test
run.
I added code in tearDown() http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java#newcode18 dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java:18: } catch (IOException ex) { On 2011/03/22 21:59:31, scottb wrote:
Just add the throws to the unit test method, the original exception is
generally
better in test failure logs.
Done. http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
