Here is a re-working of the cache based on the feedback. I've removed caching inside CompilationStateBuilder, and made all cache lookups by the ContentId.
Still TODO on this patch is a unit test, which I will work on tomorrow. http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode232 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:232: logger.log(TreeLogger.TRACE, "Invalid units found: " + invalidatedUnits.size()); On 2011/03/07 20:14:13, scottb wrote:
I would dig on this more. It's perfectly fine and expected that some
units will
have null dependencies. That should not force those units to
recompile...
*provided that the dependencies remain null*.
I think we need to go deeper... When you say the dependencies remain null, remain from when until when? Are you saying the Dependencies.validate() method needs revamping? or that we add a special case for checking null dependencies? http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode450 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:450: On 2011/03/07 20:14:13, scottb wrote:
It does seem kind of strange, though, like the two need to be unified.
Maybe
PersistentUnitCache should be one of two UnitCache subclasses, or
maybe
internally it should either use the disk or not.
UnitCache is now an interface MemoryUnitCache implements an in-memory cache PersistentUnitCache extends MemoryUnitCache to back up and reload the cache from disk. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/Compiler.java File dev/core/src/com/google/gwt/dev/Compiler.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/Compiler.java#newcode204 dev/core/src/com/google/gwt/dev/Compiler.java:204: // that persists between compilation runs and call PersistentCache.setCacheDir() On 2011/03/08 16:29:31, jbrosenberg wrote:
s/call/calls to/
Done. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/DevMode.java File dev/core/src/com/google/gwt/dev/DevMode.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/DevMode.java#newcode417 dev/core/src/com/google/gwt/dev/DevMode.java:417: File persistentCacheDir = new File(options.getWarDir(), "/WEB-INF/gwt-unitCache"); On 2011/03/08 02:31:25, tobyr wrote:
Make "gwt-unitCache" a constant somewhere (and replace all uses)?
Done. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/DevModeBase.java File dev/core/src/com/google/gwt/dev/DevModeBase.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/DevModeBase.java#newcode889 dev/core/src/com/google/gwt/dev/DevModeBase.java:889: PersistentUnitCache.init(getTopLogger().branch(TreeLogger.INFO, On 2011/03/08 02:31:25, tobyr wrote:
Why not just branch the TreeLogger inside of init?
Done. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/DevModeBase.java#newcode1061 dev/core/src/com/google/gwt/dev/DevModeBase.java:1061: } On 2011/03/08 16:29:31, jbrosenberg wrote:
white space
Done. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/GWTCompiler.java File dev/core/src/com/google/gwt/dev/GWTCompiler.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/GWTCompiler.java#newcode180 dev/core/src/com/google/gwt/dev/GWTCompiler.java:180: } else { On 2011/03/08 02:31:25, tobyr wrote:
This logic was a bit confusing to follow. Mind setting
persistentCacheDir to
null in the if block instead of initializing it to null? A comment
might help
too.
Done. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java (left): http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#oldcode422 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:422: CompilationUnit existingUnit = unitCache.get(contentId); On 2011/03/08 02:31:25, tobyr wrote:
Put the check of the unitCache and PUC together (for example, in a
method) so
you don't duplicate the the cachedUnits.put/compileMoreLater logic.
Or...
replace unitCache with PUC altogether.
Done. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java (right): http://gwt-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); On 2011/03/08 02:31:25, tobyr wrote:
Is there some reason this can't just happen on a thread on a periodic
timer in
PUC?
There is an opportune time to cleanup the cache, and that is right after a compile finishes. There is no good timer value we could set, seeing as we have compiles that finish in just a few seconds, to more than a minute. 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. On 2011/03/08 02:31:25, tobyr wrote:
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?
done. 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() { On 2011/03/07 20:14:13, scottb wrote:
Not used anymore.
Done. 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 On 2011/03/08 02:31:25, tobyr wrote:
s/cleanup/#cleanup.
Done. 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. On 2011/03/08 02:31:25, tobyr wrote:
This reads really strange. I think more explanation is needed.
I revamped, let me know if you think it still needs work. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode61 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:61: * uses lots of heap and takes 5-10 seconds ({@link #init(TreeLogger)} starts On 2011/03/08 02:31:25, tobyr wrote:
Missing a period?
Done. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode62 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:62: * eagerly loading the cache a background thread). On 2011/03/08 02:31:25, tobyr wrote:
s/cache a background/cache in a background
Done. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode64 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:64: * - Although duplicate units with the same type name are eventually cleaned up, On 2011/03/08 02:31:25, tobyr wrote:
It'd be nice for this doc to show up correctly in quick view for
javadoc. I.E.
use list, paragraph, etc... tags.
Done. I'm bad about that. I'll look around for more. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode77 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:77: * On 2011/03/08 02:31:25, tobyr wrote:
What happens if you run 2 devmodes or a devmode + web compile
concurrently with
the same cachdir set? Boom?
No, it won't go boom. It might cause a corrupt cache file if you start them at exactly the same second, but then the cache file will just be corrupt on the next startup, and therefore ignored.
I think that's a case where we need to make sure we don't explode,
even if it's
something along disabling cache writes or the cache entirely.
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 { On 2011/03/07 21:03:15, scottb wrote:
Is this class an artifact of a previous design? I don't think it does
anything
anymore.
I had a weak hash map at one point that this was helping with. Now I am stuffing a another bit of information in there. 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"); On 2011/03/07 21:03:15, scottb wrote:
Also, setPriority()?
Set to normal priority - this needs to happen before the compile can start. 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(); On 2011/03/07 21:03:15, scottb wrote:
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.
Sounds arbitrary, but I am not going to get into a subclass verses compositing war. Done. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode117 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:117: logger.log(logger.ERROR, "Interrupted waiting for PersistentUnitCache to load.", ex); On 2011/03/08 02:31:25, tobyr wrote:
s/logger.ERROR/TreeLogger.ERROR
Done. 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(); On 2011/03/07 21:03:15, scottb wrote:
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. Much nicer. Done. 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 On 2011/03/07 21:03:15, scottb wrote:
You'd want to bail here if you allow the Shutdown thread to interrupt
this one. Done. 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(); On 2011/03/07 21:03:15, scottb wrote:
Logger?
Done. 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; On 2011/03/07 21:03:15, scottb wrote:
It seems like the state of this variable has such radical implications
as to
warrant splitting a dummy vs. real cache class.
gone. 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", On 2011/03/08 16:29:31, jbrosenberg wrote:
cacheDirectory should be declared volatile, if it is assigned under a synchronized block, and referenced outside of one.
now encapsulated in MemoryUnitCache and not static. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode253 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:253: private static final String CACHE_PREFIX = "gwt-unitCache-"; On 2011/03/08 02:31:25, tobyr wrote:
I think this constant wants to be public and referenced from the other
places
it's already being used.
Moved to UnitCache interface 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) { On 2011/03/07 21:03:15, scottb wrote:
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?
This call is to facilitate the auto cleanup. You don't want to run cleanup until after you run the build, otherwise your cleanup will include all of the stale units plus anything new. 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) { On 2011/03/08 02:31:25, tobyr wrote:
Nobody seems to call this, except for init() on cases of enabled ==
false which
should happen... never?
I moved this into PersistentUnitCache but still call from MemoryUnitCache.cleanup(). The behavior I wanted was that if you are ru nning from just memory, then clear the persistent cache because its probably no longer valid. Its not all that pretty and you could convice me that's a harebrained idea. 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 On 2011/03/07 21:03:15, scottb wrote:
I don't think this code path is currently possible.
I removed it. 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/07 21:03:15, scottb wrote:
Why would running without a cache cause a clear?
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. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode346 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:346: logger.log(TreeLogger.TRACE, "Persistent unit cache dir set to: " On 2011/03/08 02:31:25, tobyr wrote:
Shouldn't this be cacheDirectory instead of cacheDir?
Done. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode348 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:348: On 2011/03/08 02:31:25, tobyr wrote:
Should there be some test around here that ensures the cacheDirectory
is
actually available and accessible?
The thread UnitWriterThread handles that. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode352 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:352: PersistentUnitCache.clearPersistentCache(logger); On 2011/03/08 02:31:25, tobyr wrote:
Under what conditions would you call init() and enabled not be true?
For the auto cache cleanup, both memory and persistent unit cache need the directory. 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: */ On 2011/03/08 16:29:31, jbrosenberg wrote:
This technically needs to be synchronized
Removed. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode374 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:374: * {@link #add(CompilationUnit)} and {@link #findUnit(String)} methods will be On 2011/03/08 02:31:25, tobyr wrote:
Bad link for add, but agree with scottb that it seems this method
should just
disappear.
Gone. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode397 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:397: if (current != null) { On 2011/03/08 02:31:25, tobyr wrote:
I don't understand the treatment of a null current. In the case that
current is
null, won't all of the equal tests in the for loop below fail? In
other words,
isn't this entire method a no-op with a null current?
This function gets referenced in 2 places. 1 is where you want to delete all but the currently accessed file. The other places is where you want to purge out all files in the cache (the null parameter case) http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode423 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:423: files = cacheFiles.toArray(new String[0]); On 2011/03/08 02:31:25, tobyr wrote:
s/new String[0]/new String[cacheFiles.size()]
Done. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode442 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:442: * forever in {@link ComplationStateBuilder} anyway. On 2011/03/08 02:31:25, tobyr wrote:
Bad link to CompilationStateBuilder. I think scottb's comment
resonates with
mine about dropping CSB.unitCache in favor of PUC.
I did this. I dropped a lot of weak hash map references from CSB. I think they are taken care of by the remove of the entry of the ContentId map when a unit is re-compiled. Please let me know if I'm being naive about something. http://gwt-code-reviews.appspot.com/1375802/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode471 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:471: private void cleanupCacheFiles(TreeLogger logger) { On 2011/03/07 21:03:15, scottb wrote:
I'm not sure I understand what this is doing, really. Shouldn't
everything have
already been written out to the current cache file? Why do we have to
do it
again?
This is to migrate the valid contents 10 log files into a single file. Normally, the cache only writes out newly compiled units to the current log. But when it is time to clean up, some of the currently valid units may be only stored in old files, not the currently open file. The currently open file becomes the new single cache file. I updated the comment a bit, and each cache entry now contains a source tag, so that we only re-write things we know are from old cache log files. 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 " On 2011/03/08 02:31:25, tobyr wrote:
This seems like strange behavior to me.
I don't like the idea of a cache that never gets purged. At least this gives someone a foolproof method to purge it. "Are you having problems? Try running without the unit cache flag once and see if the problem goes away." http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
