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 (10000
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 below....Alternatively, 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 final, or use an
AtomicReference or
perhaps can get away with volatile. Complicated by the initial
static
initializer below + setCacheDirectory static method.
This variable should be set only once. I'll document it as such. We
need to be able to override this method based on how the program is
invoked. There is a good place to put files for an instance of Compiler
which is not the same good place as DevMode, which is not the same as
GWTShell.
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) {
On 2011/03/06 17:58:14, jbrosenberg wrote:
Do you need to create instance here? Can't you just return null?
The idea was to make it auto initializing. Maybe that is not needed.
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)
On 2011/03/06 17:58:14, jbrosenberg wrote:
Use System.getProperty("java.io.tmpdir"), which is generally set to a
useful
default for each OS, etc.
Awesome! just what I was looking for.
http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode425
dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:425: */
On 2011/03/06 17:58:14, jbrosenberg wrote:
Declare this final, initialize here (or in constructor)
Done.
http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode427
dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:427:
On 2011/03/06 17:58:14, jbrosenberg wrote:
make this a final AtomicInteger if you need it to be accurate in
multithreaded
environment.
There is only one thread adding to the cache at the moment, but I'll go
ahead and make this change.
http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode464
dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:464: }
On 2011/03/06 17:58:14, jbrosenberg wrote:
Update comment?
Done.
http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode484
dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:484:
SpeedTracerLogger.start(DevModeEventType.LOAD_PERSISTENT_UNIT_CACHE);
On 2011/03/06 17:58:14, jbrosenberg wrote:
Perhaps use a ConcurrentHashMap here? It looks like you create an
iterator
over unitMap's entries above, so you either need to synchronize access
while
iterating, or use a concurrent map.
Since there is no real contention right now, I opted to synchronized on
the SynchronizedMap in the iterator.
http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode520
dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:520: }
On 2011/03/06 17:58:14, jbrosenberg wrote:
put in finally clause?
Done.
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
On 2011/03/05 14:58:25, zundel wrote:
This is the only change to this file. I'll back out the formatting
change.
Done.
http://gwt-code-reviews.appspot.com/1375802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
