Updated patch to remove compilerMetrics linker and move this output to the soyc linker.
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) { On 2010/10/23 00:53:27, scottb wrote:
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?
Are you saying to init them in the constructor and then not provide setters? that would work for most values, but not for PrecompilationMetrics which has to be passed between classes and the elapsed time set at the end. I'd like to leave this as is. The current metrics logic always gathers all information, but I've been thinking that some of the data is more expensive to collect, and we might conditionally only collect the counts of things as opposed to all the details, or do more intensive collection activies, such as the since nixed heap memory analysis. 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> { Because this work is common across all precompile steps. It represents work that is being duplicated in our current sharding strategy and I'd like to see that timing. I have some preliminary design to break all the work represented in this data structure out into a step separate from JavaToJavaScript.precompile(). The list of initial types would be the same too - there's no point duplicating it across multiple <precompilation> instances in the XML output. On 2010/10/23 00:53:27, scottb wrote:
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; On 2010/10/23 00:53:27, scottb wrote:
Use AtomicInteger instead of rolling your own.
Done. 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() { On 2010/10/23 00:53:27, scottb wrote:
Any particular reason for this pattern? A zero-arg constructor could
do the
same thing.
I'm not planning to mock this class, so I don't have a good reason... Changed. 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; On 2010/10/23 00:53:27, scottb wrote:
Checkstyle wants a blank line above List.
Done. 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; On 2010/10/23 00:53:27, scottb wrote:
This is actually "permutationBase" right?
This confuses me. The value is called different things in different parts of the code, right? I see 'permutationBase' in Precompile.java and 'permutationId' in JavaToJavaScriptCompiler. I realize there is some permutation merging going on, sometimes, but I'm not sure if the permutation Id referred to in JavaToJavaScriptCompiler is different. 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) On 2010/10/23 00:53:27, scottb wrote:
Actually putting sample output here in a <pre> tag would be an epic
win. Doesn't that mean you'll never be able to run eclipse's auto formatter again? or is there a way to supress that? 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); On 2010/10/23 00:53:27, scottb wrote:
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.
Done. 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"; On 2010/10/23 00:53:27, scottb wrote:
Stale class names.
Done. 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";> On 2010/10/23 00:53:27, scottb wrote:
FYI: greek to me
Don't worry, if you get it wrong, the schema validator will have a cow: xmllint -s compilerMetrics.xsd compilerMetrics.xml The important parts for human review are the documentation annotation. 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, On 2010/10/23 00:53:27, scottb wrote:
Why break this out? The other overload has no callers
There are derived projects I found outside the GWT repository that use the overload. 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(); On 2010/10/23 00:53:27, scottb wrote:
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.
You are right. Reverted. 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()); On 2010/10/23 00:53:27, scottb wrote:
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.
Thanks, reverted. 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"); On 2010/10/23 00:53:27, scottb wrote:
Why are these going away? They're useful for analyzing the compiler's
max
memory usage at various points.
Oops, I was too eager in cleaning up the memory measurements. I added a bunch in compile() and in the act of removing them I got these. Added back. 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) On 2010/10/23 00:53:27, scottb wrote:
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.
I wasn't sure if you meant move the reference to unifiedAst.getReferencdJavaClasses() or the body of that method into here. I did the former. If you mean the latter, there is no way to export 'initialAst' from UnifiedAst at the moment. In fact, it is carefully guarded in getFreshAst() which has a side effect of clobbering initialAst. Since this detail is private to UnifiedAst, I moved the call there. 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: }; On 2010/10/23 00:53:27, scottb wrote:
Eclipse doesn't like this excess semi.
Done. 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 On 2010/10/23 00:53:27, scottb wrote:
Checkstyle wants a trailing period.
Done. 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" /> On 2010/10/23 00:53:27, scottb wrote:
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.
In a nutshell, it was kind of confusing for me to get all this working and understand how SoyC works at the same time. Now that I understand what's going on, moving it into Soyc was trivial. For historical purposes, these are some of design issues I wrestled with: That the SoyC report gathering is fundamentally different from gathering in that all of its analysis runs in the compilePermutations step and output artifacts wait until the end of the compilePermutations() step, and then get written out immediately in XML. Compiler metrics may not run in the same process as compilePermutations() when the build is sharded. Because of the nature of how data is passed between the precompile step and compilePerms step, it creates more work in the build infrastructure to create new files and pass them between. That's why I stuffed ModuleMetricsArtifact and PrecompilationMetricsArtifact into the UnifiedAST that gets written out as precompilation.ser. Now the build infrastructure is none the wiser that we have turned on the special reporting feature. Another difference is that all the artifacts from the build are merged together into a single XML file in link - that's not the case with soyc - it assumes all the input artifacts are already xml. It isn't that it wouldn't work, but all the xml creating seems a bit out of place there for the moment. So anyway, I diverged a bit from the previously blazed SoyC path in the way I designed compiler metrics, so it might look a little awkward. http://gwt-code-reviews.appspot.com/1045801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
