submitted as r9893
Thanks for the review!
http://gwt-code-reviews.appspot.com/1375802/diff/10040/dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java
File dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java
(right):
Thanks for the review!
This looks pretty exciting. Is it ready enough that, if I was
comfortable running trunk (which I am for certain projects), I could
enable now?
Any rough stats on the speed up (particularly for DevMode) for
small/large codebases?
Thanks,
Stephen
--
I had some trouble with PersistentUnitCacheTest being flaky, which is
why there are some changes in there. The count of files in the
directory was not as expected at different points in the test. The
solution seems to be to close all 3 output streams.
While trying to make sense of the flaky failures, we noticed the tests
would consistently fail on windows. Scott tracked it down to the
current file being opened for read and then left open on an EOF
exception by the unitLoader thread.
http://gwt-code-reviews.appspot.com/1375802/
--
LGTM, all nits. No need to re-review.
http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/src/com/google/gwt/dev/javac/SourceFileCompilationUnit.java
File
dev/core/src/com/google/gwt/dev/javac/SourceFileCompilationUnit.java
(right):
http://gwt-code-reviews.appspot.com/1375802/diff/19040/dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java
File dev/core/test/com/google/gwt/dev/javac/PersistentUnitCacheTest.java
(right):
On Wed, Mar 23, 2011 at 9:57 AM, zun...@google.com wrote:
If we only write the cache to the same war dir subdirectory, then I
think my concerns with purging the cache are solved (the cache will be
in a well known place). I'm still going to make an exception so that
setting a system property
On Wed, Mar 23, 2011 at 12:23 PM, Scott Blum sco...@google.com wrote:
On Wed, Mar 23, 2011 at 9:57 AM, zun...@google.com wrote:
If we only write the cache to the same war dir subdirectory, then I
think my concerns with purging the cache are solved (the cache will be
in a well known place).
On Wed, Mar 23, 2011 at 1:02 PM, Eric Ayers zun...@google.com wrote:
FYI: I am adding a call to return the resource location to the
CompilationUnit to handle changing the key.
That's what CompilationUnitBuilder.getLocation() was used for and in
practice, CompilationUnit.getDisplayLocation()
http://gwt-code-reviews.appspot.com/1375802/diff/10027/dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java
File dev/core/test/com/google/gwt/dev/javac/MemoryUnitCacheTest.java
(right):
High-level, my major issues at this point are:
1) MemoryUnitCache having anything to do with the on-disk cache.
2) Using PUC from entry points other than DevMode and Compiler (although
we should seriously consider the JUnitShell use case).
3) Caching by type name instead of resource location.
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/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
http://gwt-code-reviews.appspot.com/1375802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
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
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):
I didn't respond to Scott's initial comments:
- My gut reaction is that it's odd to make it a command-line option. It's not
really a user-facing 'option' in the same way that draftCompile or
style=PRETTY
is, it's just an internal implementation detail. It would seem more like a
jvm
On 2011/03/15 13:59:15, zundel wrote:
First, I want to let you know I put a lot of thought into where to put
the cache files and after discussions with Toby, we came up with the
WEB-INF solution.
There is precedent for putting the cache type files in WEB-INF (app
engine integration with GWT
Couple more comments after reading Jason's feedback.
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):
Minor nits, overall looks like a good refactoring.
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):
I think I now understand the high-level design. Definitely seems
workable, and probably pretty fast, too. Just a lot of nits to work
out, I think.
http://gwt-code-reviews.appspot.com/1375802/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java
File
http://gwt-code-reviews.appspot.com/1375802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
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/PersistentUnitCache.java
File dev/core/src/com/google/gwt/dev/javac/PersistentUnitCache.java
(right):
One other thing we should talk about is ditching all of the static-ness.
The static-ness doesn't gel well with CompilationStateBuilder. CSB is
usually a singleton, but you can instantiate an isolated CSB for testing
which has a distinct cache that cannot be interfered with. It seems
like CSB
I agree with making PUC non-static. This reduces the need to synchronize
on getting instance, etc.
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/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
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
I'm looking at PUC itself now, but I wanted to go ahead and get out some
quick comments on everything else.
High-level:
- My gut reaction is that it's odd to make it a command-line option.
It's not really a user-facing 'option' in the same way that draftCompile
or style=PRETTY is, it's just an
Reading the code, I think what I'm missing is some kind of theory of
operation. I don't have a good mental model of the life cycle of
individual files as they relate to GWT processes, refreshes, builds,
etc. I can't easily answer questions like:
- Does PUC use a different cache file each
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
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
Some comments of my own to kick off the discussion.
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):
33 matches
Mail list logo
