http://gwt-code-reviews.appspot.com/1490801/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/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode100 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:100: */ On 2011/08/13 22:47:00, jbrosenberg wrote:
what's the rationale for this change? Couldn't it lead to a larger
backlog of
old files that need to be read in at startup?
Now that the files rotate every time a CompilationStateBuilder pass finishes, there are more files created, but they are smaller. The consolidation pass is not needed that often, and I was finding that a 10 file limit mean that one dev mode session was running purge multiple times. http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode109 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:109: /** On 2011/08/13 22:47:00, jbrosenberg wrote:
Just thinking aloud here (since I am wondering about this too with my
caching
stuff too)?
What happens if multiple processes are using the cache, if an open
cache file
which is being written to is subsequently read by another process, and
then the
first process adds more to the file, etc., but meanwhile the second
process has
decided to compact things, etc.? Is this worth worrying about? Does
it make
sense to instead write new cache info to a temp file, and then only
move it to
it's final file name once it's complete and being closed? Or should
we have the
concept of cache locking, whereby only one process can ever use a
cache
directory at a time?
The worst thing that could happen: some of the files are corrupt. In that case that cache just stops loading the file, deletes it, and moves on, probably having to re-compile units instead of using them from the cache. I don't think the code is going to be able to cache effectively if multiple processes are sharing the same cache dir, but then again, it shouldn't break either. http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode120 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:120: // This isn't 100% reliable if multiple processes are in contention On 2011/08/13 22:47:00, jbrosenberg wrote:
You could change this so the do-while loop above has while (!newFile.createNewFile()) as it's loop condition, so that you
atomically know
that you've found the next free file name, and that you've also
created it, etc. Done. http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode147 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:147: List<File> cacheFiles = new ArrayList<File>(); On 2011/08/13 22:47:00, jbrosenberg wrote:
need a new line below before the if()
Done. http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode164 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:164: On 2011/08/13 22:47:00, jbrosenberg wrote:
I generally assume ALL_CAP_NAMES to refer to static constants. Seems
weird for
instance classes with anonymous implementations to use that naming
renamed using camel case http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode231 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:231: On 2011/08/13 22:47:00, jbrosenberg wrote:
Can we give this a more specific name, like "currentCacheFileStream"?
Done. http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode247 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:247: On 2011/08/13 22:47:00, jbrosenberg wrote:
extra blank line here
Done. http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode265 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:265: On 2011/08/13 22:47:00, jbrosenberg wrote:
If you only want 1 thread here, consider using Executors.newSingleThreadPool().... I'm guessing it might be a bit more lightweight, etc...
Done. http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode275 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:275: // ignore I read the article, but I don't think its relevant here - the finally() clause shuts down the thread hard with shutdownNow(). On 2011/08/14 20:04:16, stephenh wrote:
Consider resetting the thread's interrupted state:
Thread.currentThread().interrupt();
http://www.javaspecialists.eu/archive/Issue056.html
http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode276 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:276: } catch (RejectedExecutionException e) { On 2011/08/13 22:47:00, jbrosenberg wrote:
duplicate commas , ,
Done. http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode318 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:318: super.add(newUnit); On 2011/08/13 22:47:00, jbrosenberg wrote:
don't need final here
Done. http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode345 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:345: if (shouldRotate) { On 2011/08/13 22:47:00, jbrosenberg wrote:
Was this done to simplify things? Any ramifications?
This gets rid of the memory leak caused by keeping a single output stream open for the entire duration of the dev mode session. http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode351 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:351: // Check to see if the previous purge task finished. On 2011/08/13 22:47:00, jbrosenberg wrote:
Unless there's some synchronization, it might not be thread safe to
inspect
purgeTaskStatus here (and to set it below). Is there any reason not
to make
this whole cleanup() method synchronized? Or at least the block from
here to
where it's reset below?
Re-introduced an atomic boolean here. http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode356 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:356: // ignore On 2011/08/14 20:04:16, stephenh wrote:
Consider resetting interrupted here too.
Done. http://gwt-code-reviews.appspot.com/1490801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
