One existing TypeOracle unit test still fails, and I haven't figured out a unit test for PersistentUnitCache yet, but I wanted to get feedback on the other changes I've made.
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#newcode231 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:231: if (invalidatedUnits.size() > 0) { On 2011/03/08 02:31:25, tobyr wrote:
Have we figured out why this would ever be true on a no-op refresh?
Yes - fix is in another CL for the lightweight HashMap. I also worked around it in Dependencies by not using 'put' inside an iteration, but instead updating the entry in place. 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; On 2011/03/15 20:56:17, jbrosenberg wrote:
What is the default here? Will we have a good persistentCacheDir in
the default
case, for free? I'd think that would be desirable.
there is a mediocre default of the java.io.tmpdir system property. I'm not that fond of it - maybe a 'null' directory parameter should disable the persistent cache. This is an "old" entry point anyway, hopefully it isn't used much. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/GWTShell.java File dev/core/src/com/google/gwt/dev/GWTShell.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/GWTShell.java#newcode200 dev/core/src/com/google/gwt/dev/GWTShell.java:200: protected boolean doStartup() { On 2011/03/15 20:56:17, jbrosenberg wrote:
If getWorkDir() is null, what will be the behavior here (shouldn't be
the same
as in GWTCompiler.java?
Yeah, its the java.io.tmpdir system property again. 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 (left): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/Precompile.java#oldcode133 dev/core/src/com/google/gwt/dev/Precompile.java:133: Serializable { On 2011/03/15 21:08:13, scottb wrote:
Does this change have anything to do with this CL?
No, I was just cleaning up a warning. The Serializable is redundant, because PrecompilationResult already implements Serializable. 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()); On 2011/03/15 21:08:13, scottb wrote:
I would have expected this to be war dir.
That would be nice, but war dir is a linker option. We could change it, but we'd have to modify all the tools that invoke Precompile in this way too. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/PrecompileOnePerm.java File dev/core/src/com/google/gwt/dev/PrecompileOnePerm.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/PrecompileOnePerm.java#newcode186 dev/core/src/com/google/gwt/dev/PrecompileOnePerm.java:186: CompilationStateBuilder.init(logger, options.getWorkDir()); On 2011/03/15 21:08:13, scottb wrote:
Seems weird that this is workDir sometimes, and war dir other times.
In the build with multiple entry points, war dir is a link option. Here, I'm not sure war dir would be such a great place, because there are parallel invocations of PrecompileOnePerm that might conflict if we put the cache into a shared dir between all of them. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java File dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java#newcode51 dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java:51: ~(Opcodes.ACC_DEPRECATED | Opcodes.ACC_NATIVE | Opcodes.ACC_STRICT On 2011/03/15 20:56:17, jbrosenberg wrote:
line wrap? Bad formatter artifact?
This is a consequence of the updated 100 char wrap setting for the Eclipse formatter. Sorry for the churn. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java#newcode104 dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java:104: if ((access & (Opcodes.ACC_SYNTHETIC)) == 0) { This line has a non-formatting change. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java#newcode132 dev/core/src/com/google/gwt/dev/javac/BytecodeSignatureMaker.java:132: if ((access & (Opcodes.ACC_SYNTHETIC)) == 0) { This line has a non-formatting change. 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 (left): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#oldcode394 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:394: ResourceTag tag = resourceContentCache.get(location); On 2011/03/15 21:08:13, scottb wrote:
I think this cache is actually important. Essentially, this cache
says "assume
a source file is the same if its lastModified is the same". Without
this cache,
you have no choice but to read in the contents of ALL source files on
every
refresh just to compute the content ID, and you can't take advantage
of
lastModified.
I think you either need to roll into UnitCache the concept of a (Location,lastModified) -> ContentId, or else resurrect some of this
code and
only compute ContentID on the initial load sequence rather than on
every
refresh.
Location, last modified is already there if you lookup the cache by 'typeName', so I'm using that. (FYI, I measured a 2.5 second difference on refresh on an app with >9000 units between reading the source and computing content id and just using last modified date.) 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#newcode258 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:258: } On 2011/03/15 20:56:17, jbrosenberg wrote:
whitespace
Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode267 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:267: static class ResourceTag { On 2011/03/15 21:08:13, scottb wrote:
This class is now unused (although, see below for why it was useful).
I've removed it. 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/15 21:08:13, scottb wrote:
Calling this twice with a different value should be some kind of
error, right? I had an assert in there at one point, but removed it because of unit testing, which invokes Compiler.run() multiple times in one process. 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/15 21:08:13, scottb wrote:
Suggest auto-initializing this, or else having a specific constructor.
If
you're concerned about startup performance, CSB.get() could lazy
initialize
instance instead of static-initializing it.
I'm not sure what you're getting at. the unit cache is not needed until you compile. I don't know whether the caller will invoke get() or not before then (unit testing). There is no performance penalty for initializing the in-memory cache only - it just creates a couple of hash maps, does the clean up (now in a background thread) and goes on. I wanted to delay instantiating it in hopes that someone would explicitly initialize CSB and give a good place to store the persistent cache. If the top level entry point has a good directory to give us, we want to make the 'best' cache which is a persistent cache. This fallback is just for unit tests, which have a wide variety of entry points and no good default place to put the cache anyway. Adding a persistent cache to unit tests might improve performance, but I'l prefer to be on the safe side and only impact DevMode and WebMode for now. http://gwt-code-reviews.appspot.com/1375802/diff/9001/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/9001/dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java#newcode102 dev/core/src/com/google/gwt/dev/javac/CompilationUnitBuilder.java:102: doGetSource(); On 2011/03/15 21:08:13, scottb wrote:
This needs to be getSource(); otherwise you'll have to read in the
source twice.
Reading the source twice, in addition to being inefficient, could
cause the
content ID to appear to change. Although I worry about loading ALL
the source
into memory at the same time... see comment in CSB.
Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/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/9001/dev/core/src/com/google/gwt/dev/javac/Dependencies.java#newcode100 dev/core/src/com/google/gwt/dev/javac/Dependencies.java:100: for (String ref : qualified.keySet()) { On 2011/03/15 21:08:13, scottb wrote:
In other to work around a bug in HashMap (patch outstanding), please
iterate
over the entrySet() instead and use entry.setValue() to avoid the bug.
(Also
saves you extra hash computations / index lookups internally). I can
verify
that this fixes the problem you were seeing where certain units are
always
turning up invalid.
Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/Dependencies.java#newcode182 dev/core/src/com/google/gwt/dev/javac/Dependencies.java:182: } else { On 2011/03/15 20:56:17, jbrosenberg wrote:
all on one line?
reformatted file. http://gwt-code-reviews.appspot.com/1375802/diff/9001/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/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode32 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:32: /** On 2011/03/15 20:56:17, jbrosenberg wrote:
What "weak" hash map are you referring to here?
sorry, stale comment. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode59 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:59: protected static enum UnitSource { On 2011/03/15 21:08:13, scottb wrote:
Confusing name, since "Source" generally means "java source code"
around these
parts. Perhaps "UnitOrigin"?
Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode74 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:74: */ On 2011/03/15 20:56:17, jbrosenberg wrote:
Seems this doesn't need to be initialized to a value here, it can be
declared
final, and set to this value as a default else case in the constructor
below. To do this, I de-staticified cacheDirectory and had to do some refactoring which includes some persistent cache file manipulation from PersistentUnitCache http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode75 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:75: protected static File cacheDirectory = new File(System.getProperty("java.io.tmpdir")); On 2011/03/15 21:08:13, scottb wrote:
This should have gone away, right? And definitely shouldn't be
static? Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode93 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:93: On 2011/03/15 20:56:17, jbrosenberg wrote:
This should be AtomicBoolean (or at least volatile).
only touched by one function (cleanup) which I made synchronized. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode96 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:96: MemoryUnitCache(TreeLogger logger, File cacheDir) { On 2011/03/15 21:08:13, scottb wrote:
Seems like MemoryUnitCache shouldn't take any parameters, only PersistentUnitCache. Any way, this whole ctor seems different from
the
conversational descri
The directory parameter is to support the cleanup() logic. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode130 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:130: PersistentUnitCache.deleteOldCacheFiles(logger, null); On 2011/03/15 21:08:13, scottb wrote:
This seems like it wants to run on a background thread to not block
the main
thread of execution.
Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java#newcode131 dev/core/src/com/google/gwt/dev/javac/MemoryUnitCache.java:131: clearedUnitCache = true; On 2011/03/15 21:08:13, scottb wrote:
I think I agree with Toby, this should be a no-op. Totally strange
that this
would do this. And what happens if you're running dev mode with PUC
enabled and
have open files, and then you run a compile without PUC and it blows
away the
files out from under you?
Nothing bad would happen. At worst, you might only get some files cached. An error reading a file just causes the cache to stop loading. http://gwt-code-reviews.appspot.com/1375802/diff/9001/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/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode109 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:109: setName("UnitCacheLoader"); On 2011/03/15 20:56:17, jbrosenberg wrote:
Isn't this the default priority anyway?
Scott wanted me to set priority for the other threads. I'll defend this by saying that it explicitly shows what the intention is and that I didn't forget to do it. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode126 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:126: loadCompleteLatch.countDown(); On 2011/03/15 21:08:13, scottb wrote:
Reorder these two statements, in case the log() call throws an
exception. Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode174 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:174: shutDownLatch.countDown(); On 2011/03/15 21:08:13, scottb wrote:
There's no reason to do this, this is the only thread that's ever
waiting on the
latch.
Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode188 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:188: ObjectOutputStream stream = null; Probably not an issue as the stream is frequently flushed. Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode228 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:228: if (unitWriteQueue.isEmpty()) { On 2011/03/15 20:56:17, jbrosenberg wrote:
flush if msg == UnitWriteMessage.SHUTDOWN_THREAD also?
added a stream.close() in the finally block - should take care of it. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode231 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:231: } catch (IOException ex) { On 2011/03/15 20:56:17, jbrosenberg wrote:
Why use a level variable here?
sorry, stale code http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode233 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:233: if (!errorLogged) { On 2011/03/15 21:08:13, scottb wrote:
doesn't seem to do anything
Sorry, stale code. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode237 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:237: .log(level, "Error saving unit to file:" + currentCacheFile.getAbsolutePath(), ex); On 2011/03/15 21:08:13, scottb wrote:
Checkstyle says, put a space after the colon.
Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode242 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:242: shutDownLatch.countDown(); On 2011/03/15 21:08:13, scottb wrote:
Reorder these.
Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode250 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:250: * If there are more than this many files in the cache, clean up the old files On 2011/03/15 21:08:13, scottb wrote:
checkstyle wants a period
Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode272 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:272: if (!file.equals(currentCacheFilename)) { On 2011/03/15 21:08:13, scottb wrote:
Using File.equals() would be simpler and more reliable here.
Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode303 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:303: */ On 2011/03/15 20:56:17, jbrosenberg wrote:
make final
Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode307 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:307: // TODO(zundel): do we need to be thread safe? On 2011/03/15 20:56:17, jbrosenberg wrote:
It looks like addCount is only ever used for logging (in TRACE mode),
so it's
probably not required to use AtomicInteger for program correctness.
But if you
really want an accurate value, then AtomicInteger is a good choice (no
locking,
little contention).
Removed TODO http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode313 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:313: super(logger, cacheDirectory); On 2011/03/15 21:08:13, scottb wrote:
"cacheDirectory" is definitely wrong; but I think MemoryUnitCache
doesn't want
to deal with files at all.
Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode324 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:324: super.add(newUnit); On 2011/03/15 21:14:43, scottb wrote:
Agreed; although I also wonder if super.add() needs to wait for load
to finish,
otherwise newly-loaded-from-disk units will tend to clobber built
units, unless
you handle this in loadUnitMap().
The idea is to start loading the cache early and start the UnitCacheMapLoader to start up, and then postpone the 'await()' until the last possible moment. We want find() to returned cache elements, otherwise the load is in vain. @scottb: I think you are right, we might accidentally clobber the first unit we try to add to the cache if add() is called before find(). It would have resulted in a single cached entry being out of date the next time its looked up. Fixed. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode350 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:350: * Re send all units read in from the cache to the writer thread. They will On 2011/03/15 20:56:17, jbrosenberg wrote:
typo?
hopefully it makes more sense now. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode386 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:386: Arrays.sort(sortedDirnames); On 2011/03/15 20:56:17, jbrosenberg wrote:
loop on sortedDirNames?
Ack! Fixed. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode401 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:401: UnitCacheEntry oldEntry = unitMap.get(unit.getTypeName()); On 2011/03/15 21:08:13, scottb wrote:
Seems like you want to double-check timestamp to validate which one is
actually
the "old" entry. You can't really rely on timestamp of the enclosing
file due
to two-process concurrency.
Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java#newcode417 dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java:417: } On 2011/03/15 21:08:13, scottb wrote:
finally close the input stream
Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/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/9001/dev/core/src/com/google/gwt/dev/javac/UnitCache.java#newcode20 dev/core/src/com/google/gwt/dev/javac/UnitCache.java:20: /** On 2011/03/15 20:56:17, jbrosenberg wrote:
Is this correct? Seems the persistent version is a sub-class of the
memory
version, and uses both memory and persistent storage.
reworded a bit to clarify. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/UnitCache.java#newcode28 dev/core/src/com/google/gwt/dev/javac/UnitCache.java:28: public static final String UNIT_CACHE_PREFIX = "gwt-unitCache"; On 2011/03/15 21:08:13, scottb wrote:
"public static final" is redundant; checkstyle warning
Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/UnitCache.java#newcode51 dev/core/src/com/google/gwt/dev/javac/UnitCache.java:51: CompilationUnit find(TreeLogger logger, String typeName); On 2011/03/15 21:08:13, scottb wrote:
This overload isn't used anywhere (nor are the implementations in the
concrete
classes).
You are right, but In the next update to this patch re-used it to replace the ResourceKey cache. Even if that is removed, I'd like to leave it in, as it is used in a followon change that helps cut down on spurious warnings. http://gwt-code-reviews.appspot.com/1375802/diff/9001/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/9001/dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java#newcode27 dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java:27: /** On 2011/03/15 20:56:17, jbrosenberg wrote:
Need to update comments here and below, it seems what's enabled is
whether to
use persistent caching or not (and it falls back to a MemoryUnitCache otherwise). e.g. it instantiates a cache regardlessly.
clarified to say 'persistent' caching. We've always had in-memory caching, so I think that's where the confusion comes from. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java#newcode38 dev/core/src/com/google/gwt/dev/javac/UnitCacheFactory.java:38: public static synchronized UnitCache create(TreeLogger logger) { On 2011/03/15 21:08:13, scottb wrote:
Does not appear to be used.
stale - removed http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/jjs/JJSOptionsImpl.java File dev/core/src/com/google/gwt/dev/jjs/JJSOptionsImpl.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/jjs/JJSOptionsImpl.java#newcode1 dev/core/src/com/google/gwt/dev/jjs/JJSOptionsImpl.java:1: /* On 2011/03/15 21:08:13, scottb wrote:
Think you can revert this file from your change.
Done. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/src/com/google/gwt/dev/jjs/JJSOptionsImpl.java#newcode27 dev/core/src/com/google/gwt/dev/jjs/JJSOptionsImpl.java:27: private boolean aggressivelyOptimize = true; On 2011/03/15 20:56:17, jbrosenberg wrote:
Seems these changes are formatting only, so should be removed from the
patch. missed this when reverting the command line option. removed. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/test/com/google/gwt/dev/javac/BytecodeSignatureMakerTest.java File dev/core/test/com/google/gwt/dev/javac/BytecodeSignatureMakerTest.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/test/com/google/gwt/dev/javac/BytecodeSignatureMakerTest.java#newcode565 dev/core/test/com/google/gwt/dev/javac/BytecodeSignatureMakerTest.java:565: fail("Signatures match. raw data expected=<" + originalRaw On 2011/03/15 21:08:13, scottb wrote:
Seemed right the first time?
Reverted. http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/test/com/google/gwt/dev/javac/CompilationStateTestBase.java File dev/core/test/com/google/gwt/dev/javac/CompilationStateTestBase.java (right): http://gwt-code-reviews.appspot.com/1375802/diff/9001/dev/core/test/com/google/gwt/dev/javac/CompilationStateTestBase.java#newcode99 dev/core/test/com/google/gwt/dev/javac/CompilationStateTestBase.java:99: protected final CompilationStateBuilder isolatedBuilder = new CompilationStateBuilder(); Non formatting change here. http://gwt-code-reviews.appspot.com/1375802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
