[gwt-contrib] Re: scheglov pointed out a leak in compilation units in dev mode after a refresh (issue1490801)
On 2012/04/05 19:39:54, rdayal wrote: Ping. Is this patch dead, or do we still want to get this in? I remember that we were not able to push it into release (2.4 ?). But this problem is still causing problems in GWT Designer. http://gwt-code-reviews.appspot.com/1490801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: scheglov pointed out a leak in compilation units in dev mode after a refresh (issue1490801)
On 2012/04/05 19:44:18, rdayal wrote: We're gearing up for a GWT 2.5, so I'd either like to get this in, or close the issue if it will never make it into a GWT release. On Thu Apr 05 15:42:05 GMT-400 2012, mailto:scheg...@google.com wrote: On 2012/04/05 19:39:54, rdayal wrote: Ping. Is this patch dead, or do we still want to get this in? I remember that we were not able to push it into release (2.4 ?). But this problem is still causing problems in GWT Designer. http://gwt-code-reviews.appspot.com/1490801/%3Chttps://www.google.com/url?sa=Dq=http://gwt-code-reviews.appspot.com/1490801/ Approved by cromwellian http://code.google.com/p/google-web-toolkit/source/detail?r=10915 http://gwt-code-reviews.appspot.com/1490801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: scheglov pointed out a leak in compilation units in dev mode after a refresh (issue1490801)
The design seems a bit opaque - I'm having trouble following what the
invariants are and what's going on with the concurrency. Can you explain
it a bit more?
http://gwt-code-reviews.appspot.com/1490801/diff/11002/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/11002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode49
dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:49: * A
class that manages a persistent cache of {@link CompilationUnit}
instances.
Since the design changed, is there anything you need to update in the
JavaDoc?
http://gwt-code-reviews.appspot.com/1490801/diff/11002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode113
dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:113:
private static File createCacheFile(TreeLogger logger, File
cacheDirectory)
I haven't seen private methods at the top of the file before. Can we
move these below the public methods?
http://gwt-code-reviews.appspot.com/1490801/diff/11002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode162
dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:162:
private FutureBoolean purgeTaskStatus;
Perhaps a CountDownLatch would be better here?
CountDownLatch purgeDone
http://gwt-code-reviews.appspot.com/1490801/diff/11002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode269
dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:269:
FutureBoolean status = backgroundService.submit(shutdownThreadTask,
Boolean.TRUE);
It's unclear what you're doing with backgroundService. In particular,
couldn't it have shut down earlier? (line 304). I'm not sure why you're
going through the trouble of shutting it down early and putting checks
everywhere to see if it's shut down.
http://gwt-code-reviews.appspot.com/1490801/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: scheglov pointed out a leak in compilation units in dev mode after a refresh (issue1490801)
http://gwt-code-reviews.appspot.com/1490801/diff/11002/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/11002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode348 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:348: boolean inProgress = purgeInProgress.getAndSet(true); HmmmI don't think this is quite right stillyou atomically set inProgress to true here, then do some work, then initialize purgeTaskStatus on line 272. Meanwhile, another thread can come in and see that you've set inProgress to true already, and try to reference purgeTaskStatus (but it might not have been initialized yet). I guess, I'm not sure why the purgeTaskStatus.get() call is needed at allIf it's inProgress, another thread should simply return immediately, no? What's the reason for the purgeTaskStatus future, if we don't do anything with it when we call get() on it? If you need to check the purgeTaskStatus, then you need a synchronized block that limits getting/setting it. http://gwt-code-reviews.appspot.com/1490801/diff/11002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode353 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:353: Thread.currentThread().interrupt(); I really don't think this is necessary or correct here. It implies that if you are interrupted here, you must propagate that interruption further up. But we don't have any interrupt handling mechanism in place here (or above). http://gwt-code-reviews.appspot.com/1490801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: scheglov pointed out a leak in compilation units in dev mode after a refresh (issue1490801)
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: ListFile cacheFiles = new ArrayListFile(); 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
[gwt-contrib] Re: scheglov pointed out a leak in compilation units in dev mode after a refresh (issue1490801)
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#newcode275 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:275: // ignore 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#newcode356 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:356: // ignore Consider resetting interrupted here too. http://gwt-code-reviews.appspot.com/1490801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: scheglov pointed out a leak in compilation units in dev mode after a refresh (issue1490801)
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
(left):
http://gwt-code-reviews.appspot.com/1490801/diff/5002/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#oldcode303
dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:303: /**
Is this comment no longer relevant?
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: */
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?
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: /**
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?
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
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.
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:
ListFile cacheFiles = new ArrayListFile();
need a new line below before the if()
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:
I generally assume ALL_CAP_NAMES to refer to static constants. Seems
weird for instance classes with anonymous implementations to use that
naming?
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:
Can we give this a more specific name, like currentCacheFileStream?
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:
extra blank line here
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:
If you only want 1 thread here, consider using
Executors.newSingleThreadPool()
I'm guessing it might be a bit more lightweight, etc...
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) {
duplicate commas , ,
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);
don't need final here
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) {
Was this done to simplify things? Any ramifications?
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.
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
[gwt-contrib] Re: scheglov pointed out a leak in compilation units in dev mode after a refresh (issue1490801)
http://gwt-code-reviews.appspot.com/1490801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: scheglov pointed out a leak in compilation units in dev mode after a refresh (issue1490801)
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#newcode367 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:367: */ Note to self: Before there was a synchronize(unitMap) block around this loop and I need to put it back. http://gwt-code-reviews.appspot.com/1490801/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
