Some thoughts.
http://gwt-code-reviews.appspot.com/1045801/diff/11001/12001 File dev/core/src/com/google/gwt/core/ext/linker/CompilationMetricsArtifact.java (right): http://gwt-code-reviews.appspot.com/1045801/diff/11001/12001#newcode89 dev/core/src/com/google/gwt/core/ext/linker/CompilationMetricsArtifact.java:89: public CompilationMetricsArtifact setCompileElapsedMilliseconds(long compileElapsedMilliseconds) { High-level; unless I'm missing something, it looks like all of these artifacts are initialized when you create them and never changed afterwards, and this classes are essentially containers. Why not make them immutable? http://gwt-code-reviews.appspot.com/1045801/diff/11001/12002 File dev/core/src/com/google/gwt/core/ext/linker/ModuleMetricsArtifact.java (right): http://gwt-code-reviews.appspot.com/1045801/diff/11001/12002#newcode29 dev/core/src/com/google/gwt/core/ext/linker/ModuleMetricsArtifact.java:29: public class ModuleMetricsArtifact extends Artifact<ModuleMetricsArtifact> { High level, why is this broken out from PrecompilationMetrics? Seems like it could be merged into it. They both appear to have the same life cycle and data flow. And you could get rid of the bogus counter and use the perm id alone. http://gwt-code-reviews.appspot.com/1045801/diff/11001/12002#newcode35 dev/core/src/com/google/gwt/core/ext/linker/ModuleMetricsArtifact.java:35: private static int nextInstance = 0; Use AtomicInteger instead of rolling your own. http://gwt-code-reviews.appspot.com/1045801/diff/11001/12002#newcode37 dev/core/src/com/google/gwt/core/ext/linker/ModuleMetricsArtifact.java:37: public static ModuleMetricsArtifact create() { Any particular reason for this pattern? A zero-arg constructor could do the same thing. http://gwt-code-reviews.appspot.com/1045801/diff/11001/12003 File dev/core/src/com/google/gwt/core/ext/linker/PrecompilationMetricsArtifact.java (right): http://gwt-code-reviews.appspot.com/1045801/diff/11001/12003#newcode21 dev/core/src/com/google/gwt/core/ext/linker/PrecompilationMetricsArtifact.java:21: import java.util.List; Checkstyle wants a blank line above List. http://gwt-code-reviews.appspot.com/1045801/diff/11001/12003#newcode40 dev/core/src/com/google/gwt/core/ext/linker/PrecompilationMetricsArtifact.java:40: this.permutationId = permutationId; This is actually "permutationBase" right? http://gwt-code-reviews.appspot.com/1045801/diff/11001/12004 File dev/core/src/com/google/gwt/core/linker/CompilerMetricsLinker.java (right): http://gwt-code-reviews.appspot.com/1045801/diff/11001/12004#newcode43 dev/core/src/com/google/gwt/core/linker/CompilerMetricsLinker.java:43: @LinkerOrder(Order.POST) Actually putting sample output here in a <pre> tag would be an epic win. http://gwt-code-reviews.appspot.com/1045801/diff/11001/12004#newcode50 dev/core/src/com/google/gwt/core/linker/CompilerMetricsLinker.java:50: artifacts = new ArtifactSet(artifacts); Refactor bug; this line has to go in link(), because if the incoming artifactSet is already frozen, trying to add the output will blow up. http://gwt-code-reviews.appspot.com/1045801/diff/11001/12004#newcode188 dev/core/src/com/google/gwt/core/linker/CompilerMetricsLinker.java:188: return "Export PrecompilationMetrics and PermutationMetrics to XML"; Stale class names. http://gwt-code-reviews.appspot.com/1045801/diff/11001/12005 File dev/core/src/com/google/gwt/core/linker/compilerMetrics.xsd (right): http://gwt-code-reviews.appspot.com/1045801/diff/11001/12005#newcode1 dev/core/src/com/google/gwt/core/linker/compilerMetrics.xsd:1: <xsd:schema xmlns:xsd="http://www.w3.org/2001/XMLSchema";> FYI: greek to me http://gwt-code-reviews.appspot.com/1045801/diff/11001/12006 File dev/core/src/com/google/gwt/dev/Precompile.java (right): http://gwt-code-reviews.appspot.com/1045801/diff/11001/12006#newcode533 dev/core/src/com/google/gwt/dev/Precompile.java:533: return precompile(logger, jjsOptions, module, permutationBase, Why break this out? The other overload has no callers. http://gwt-code-reviews.appspot.com/1045801/diff/11001/12006#newcode555 dev/core/src/com/google/gwt/dev/Precompile.java:555: List<String> initialTypeOracleTypes = Lists.create(); Kind of overkill to use this here, it's only one instance. The main purpose of those APIs is when you have a lot of instances of lists, and many of them might be small, like as a field in an AST node. http://gwt-code-reviews.appspot.com/1045801/diff/11001/12006#newcode598 dev/core/src/com/google/gwt/dev/Precompile.java:598: List<Permutation> permutations = Lists.create(rpo.getPermutations()); Same as above, but this actually introduces a bug, because mergedCollapsedPermutations modifies the returned list. This would blow up if you had exactly 1 perm. http://gwt-code-reviews.appspot.com/1045801/diff/11001/12012 File dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java (left): http://gwt-code-reviews.appspot.com/1045801/diff/11001/12012#oldcode480 dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java:480: Memory.maybeDumpMemory("CompStateBuilt"); Why are these going away? They're useful for analyzing the compiler's max memory usage at various points. http://gwt-code-reviews.appspot.com/1045801/diff/11001/12012 File dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java (right): http://gwt-code-reviews.appspot.com/1045801/diff/11001/12012#newcode477 dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java:477: PrecompilationMetricsArtifact precompilationMetrics) A little yucky.... but I guess you have to have this for final type oracle types. I do think you can get rid of UnifiedAst.getReferencedJavaClasses() though and move the code into here. http://gwt-code-reviews.appspot.com/1045801/diff/11001/12013 File dev/core/src/com/google/gwt/dev/jjs/UnifiedAst.java (right): http://gwt-code-reviews.appspot.com/1045801/diff/11001/12013#newcode211 dev/core/src/com/google/gwt/dev/jjs/UnifiedAst.java:211: }; Eclipse doesn't like this excess semi. http://gwt-code-reviews.appspot.com/1045801/diff/11001/12017 File dev/core/src/com/google/gwt/dev/util/arg/OptionCompilerMetricsEnabled.java (right): http://gwt-code-reviews.appspot.com/1045801/diff/11001/12017#newcode19 dev/core/src/com/google/gwt/dev/util/arg/OptionCompilerMetricsEnabled.java:19: * Option for enabling Compiler Metrics Checkstyle wants a trailing period. http://gwt-code-reviews.appspot.com/1045801/diff/11001/12019 File user/src/com/google/gwt/core/Core.gwt.xml (right): http://gwt-code-reviews.appspot.com/1045801/diff/11001/12019#newcode38 user/src/com/google/gwt/core/Core.gwt.xml:38: <define-linker name="compilerMetrics" class="com.google.gwt.core.linker.CompilerMetricsLinker" /> It occurs to me that instead of adding a new linker, you could just merge the functionality into SoycReportLinker; you've already got the relationship established through option inheritance. http://gwt-code-reviews.appspot.com/1045801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
