http://gwt-code-reviews.appspot.com/1384807/diff/1/dev/core/src/com/google/gwt/dev/javac/CachedCompilationUnit.java File dev/core/src/com/google/gwt/dev/javac/CachedCompilationUnit.java (right):
http://gwt-code-reviews.appspot.com/1384807/diff/1/dev/core/src/com/google/gwt/dev/javac/CachedCompilationUnit.java#newcode166 dev/core/src/com/google/gwt/dev/javac/CachedCompilationUnit.java:166: sourceToken = diskCache.writeByteArray(sourceBytes); Yeah, I did think about this, and I agree it's ugly. The reason I left it this way is that CU.getSource() is going to go away soon. But if you think it's worth it, we could add methods to DiskCache that write the size. Alternatively, we could wrap the raw tokens in a wrapper object that does nothing but serialize itself, and allow serializable token holders to use that class and default serialization. class CachedCompilationUnit { DiskCacheToken ast; // not transient DiskCacheToken source; // not transient } class DiskCacheToken { transient long token; readObject()/writeObject() } Thoughts? http://gwt-code-reviews.appspot.com/1384807/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/1384807/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode129 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:129: builder.setClasses(compiledClasses).setTypes(types).setDependencies(dependencies) Alas, the other one is 'wrong'. :( http://gwt-code-reviews.appspot.com/1384807/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode204 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:204: final ArrayList<CompilationUnit> newlyBuiltUnits = new ArrayList<CompilationUnit>(); On 2011/03/24 17:43:42, zundel wrote:
First of all, be aware that this block is going to have a lot of
conflicts with
the PersistentUnitCache patch that's up.
Guess I'll try to get this in quickly then :P
I'm not 100% sure what time you're trying to save in here by putting
this in a
thread; could you add a comment in the body of the thread as to what
is
expensive? I guess its the calculation of contentId.
Actually, it's converting the GWT AST to serialized bytes that I'm saving. Will add a comment.
Warning - I think you're in thread unsafeness land in ResourceCompilationUnitBuilder when the
compiler
tries to lazily compute contentId, waiting until the source is read.
That happens on the main thread when JDT reads the source to do the compile. Content id is set then, and I can definitely read it because the builder had to pass through the buildQueue barriers.
Did you measure the amount of time saved? It would be nice to see
some
speedtracer calls around it.
I found that dropping events in here was too fine-grain to be useful, but the macro effect that I compared made a definitely difference. Really, what this is doing is avoiding the wall time penalty for serializing the build GWT AST to bytes by doing it concurrently. http://gwt-code-reviews.appspot.com/1384807/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode204 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:204: final ArrayList<CompilationUnit> newlyBuiltUnits = new ArrayList<CompilationUnit>(); On 2011/03/24 19:22:38, jbrosenberg wrote:
My first thought, was that this might be chance for parallelism, can
we have
multiple threads doing compilation here at once? Could use a thread
pool
executor here, and take advantage of multiple cpus. Seems that
compilation is a
compute intensive task that would scale over threads, no? (A general
refrain
from me, let's use more CPU's, and TPE = good).
Sadly, JDT isn't setup for that, it's not thread-safe internally. I've put as much work here as I can without tickling JDT. http://gwt-code-reviews.appspot.com/1384807/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode205 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:205: final CompilationUnitBuilder sentinal = CompilationUnitBuilder.create((GeneratedUnit) null); On 2011/03/24 17:43:42, zundel wrote:
spelling: sentinel
Done. http://gwt-code-reviews.appspot.com/1384807/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode253 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:253: } On 2011/03/24 17:43:42, zundel wrote:
null out buildQueue?
Done; added a finally block. http://gwt-code-reviews.appspot.com/1384807/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationUnit.java File dev/core/src/com/google/gwt/dev/javac/CompilationUnit.java (right): http://gwt-code-reviews.appspot.com/1384807/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationUnit.java#newcode316 dev/core/src/com/google/gwt/dev/javac/CompilationUnit.java:316: throw new RuntimeException("Unexpected error deserializing AST", e); On 2011/03/24 17:43:42, zundel wrote:
this might actually happen if the serialized object is old and the
compiler
internals have changed - want to put the unit name in the exception so
we know
what to clean up?
Done; but I'll need to think about the general problem at a higher level later anyhow. http://gwt-code-reviews.appspot.com/1384807/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationUnit.java#newcode317 dev/core/src/com/google/gwt/dev/javac/CompilationUnit.java:317: } On 2011/03/24 19:22:38, jbrosenberg wrote:
close ois?
Not necessary for in-memory stream. GC will take care of OIS internals without doing any the explicit clean-up work on the main thread. http://gwt-code-reviews.appspot.com/1384807/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationUnitImpl.java File dev/core/src/com/google/gwt/dev/javac/CompilationUnitImpl.java (right): http://gwt-code-reviews.appspot.com/1384807/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationUnitImpl.java#newcode74 dev/core/src/com/google/gwt/dev/javac/CompilationUnitImpl.java:74: } On 2011/03/24 19:22:38, jbrosenberg wrote:
perhaps move out.close() to a finally block, to assure it gets closed
if an
exception.
If an exception happens, it doesn't need to be closed because it's in-memory. I do have to close it, however, before I call baos.toByteArray(). http://gwt-code-reviews.appspot.com/1384807/diff/1/dev/core/src/com/google/gwt/dev/jjs/ast/JDeclaredType.java File dev/core/src/com/google/gwt/dev/jjs/ast/JDeclaredType.java (right): http://gwt-code-reviews.appspot.com/1384807/diff/1/dev/core/src/com/google/gwt/dev/jjs/ast/JDeclaredType.java#newcode288 dev/core/src/com/google/gwt/dev/jjs/ast/JDeclaredType.java:288: On 2011/03/24 19:22:38, jbrosenberg wrote:
Can you add a comment describing what the intention is for this?
Done. http://gwt-code-reviews.appspot.com/1384807/diff/1/user/test/com/google/gwt/dev/jjs/GwtAstBuilderTest.java File user/test/com/google/gwt/dev/jjs/GwtAstBuilderTest.java (right): http://gwt-code-reviews.appspot.com/1384807/diff/1/user/test/com/google/gwt/dev/jjs/GwtAstBuilderTest.java#newcode145 user/test/com/google/gwt/dev/jjs/GwtAstBuilderTest.java:145: } On 2011/03/24 17:43:42, zundel wrote:
Shouldn't you also loop through the compiled classes in
compilationState and
make sure there is a corresponding deserialized type? This test won't
catch if
there are values in compilationState that didn't make it through the serialize/deserialize process.
Done. http://gwt-code-reviews.appspot.com/1384807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
