Introduce MetricMap as a way of exporting compiler metrics Any method that has a TreeLogger can export a metric by calling setAmount(). These metrics are exported by Super Dev Mode inside each JobEvent. Started with one metric for DeclaredTypesInModule as proof of concept. Also, fixed error-handling bugs in Super Dev Mode: - JobEvent VALID_TAG regular expression was incorrect. - If a Job failed with a RuntimeException that happened outside the compiler, the stack trace was not being logged and the job would never complete. Clean up: - JobEvent was exposing the rebased ImmutableList class - JobRunner had an unused dependency on OutboxTable Change-Id: Icb6f7e457c6d9b3b7de341a21b18b2ea0d505177 Review-Link: https://gwt-review.googlesource.com/#/c/9862/
diff --git a/dev/codeserver/java/com/google/gwt/dev/codeserver/CodeServer.java b/dev/codeserver/java/com/google/gwt/dev/codeserver/CodeServer.java index aeb7ebb..945296f 100644 --- a/dev/codeserver/java/com/google/gwt/dev/codeserver/CodeServer.java +++ b/dev/codeserver/java/com/google/gwt/dev/codeserver/CodeServer.java
@@ -118,7 +118,7 @@ OutboxTable outboxes = makeOutboxes(options, startupLogger); JobEventTable eventTable = new JobEventTable(); - JobRunner runner = new JobRunner(eventTable, outboxes); + JobRunner runner = new JobRunner(eventTable); JsonExporter exporter = new JsonExporter(options, outboxes);
diff --git a/dev/codeserver/java/com/google/gwt/dev/codeserver/Job.java b/dev/codeserver/java/com/google/gwt/dev/codeserver/Job.java index 2b883d3..8272d35 100644 --- a/dev/codeserver/java/com/google/gwt/dev/codeserver/Job.java +++ b/dev/codeserver/java/com/google/gwt/dev/codeserver/Job.java
@@ -20,8 +20,10 @@ import com.google.gwt.dev.cfg.ModuleDef; import com.google.gwt.dev.codeserver.JobEvent.CompileStrategy; import com.google.gwt.dev.codeserver.JobEvent.Status; +import com.google.gwt.dev.util.log.AbstractTreeLogger; import com.google.gwt.thirdparty.guava.common.base.Preconditions; import com.google.gwt.thirdparty.guava.common.collect.ImmutableList; +import com.google.gwt.thirdparty.guava.common.collect.ImmutableMap; import com.google.gwt.thirdparty.guava.common.collect.ImmutableSortedMap; import com.google.gwt.thirdparty.guava.common.util.concurrent.Futures; import com.google.gwt.thirdparty.guava.common.util.concurrent.ListenableFuture; @@ -291,9 +293,18 @@ out.setCompileStrategy(compileStrategy); out.setArguments(args); out.setTags(tags); + out.setMetricMap(getMetricMapSnapshot()); return out.build(); } + private Map<String, Long> getMetricMapSnapshot() { + TreeLogger logger = getLogger(); + if (logger instanceof AbstractTreeLogger) { + return ((AbstractTreeLogger)logger).getMetricMap().getSnapshot(); + } + return ImmutableMap.of(); // not found + } + /** * Makes an event visible externally. */ @@ -331,6 +342,9 @@ synchronized TreeLogger get() { if (child == null) { child = parent.branch(Type.INFO, "Job " + jobId); + if (child instanceof AbstractTreeLogger) { + ((AbstractTreeLogger)child).resetMetricMap(); + } } return child; }
diff --git a/dev/codeserver/java/com/google/gwt/dev/codeserver/JobEvent.java b/dev/codeserver/java/com/google/gwt/dev/codeserver/JobEvent.java index bae068a..56f6e01 100644 --- a/dev/codeserver/java/com/google/gwt/dev/codeserver/JobEvent.java +++ b/dev/codeserver/java/com/google/gwt/dev/codeserver/JobEvent.java
@@ -17,6 +17,7 @@ import com.google.gwt.dev.cfg.ModuleDef; import com.google.gwt.dev.cfg.ModuleDefSchema; +import com.google.gwt.dev.util.log.MetricName; import com.google.gwt.thirdparty.guava.common.base.Preconditions; import com.google.gwt.thirdparty.guava.common.collect.ImmutableList; import com.google.gwt.thirdparty.guava.common.collect.ImmutableMap; @@ -33,7 +34,7 @@ * <p>JobEvent objects are deeply immutable, though they describe a Job that changes. */ public final class JobEvent { - private static final Pattern VALID_TAG = Pattern.compile("\\S{1,100}"); + private static final Pattern VALID_TAG = Pattern.compile("^\\S{1,100}$"); private final String jobId; @@ -47,6 +48,7 @@ private final CompileStrategy compileStrategy; private final ImmutableList<String> arguments; private final ImmutableList<String> tags; + private final ImmutableSortedMap<String, Long> metricMap; private JobEvent(Builder builder) { this.jobId = Preconditions.checkNotNull(builder.jobId); @@ -55,12 +57,14 @@ this.status = Preconditions.checkNotNull(builder.status); this.message = builder.message == null ? status.defaultMessage : builder.message; + this.arguments = ImmutableList.copyOf(builder.args); + this.tags = ImmutableList.copyOf(builder.tags); + this.metricMap = ImmutableSortedMap.copyOf(builder.metricMap); + // The following fields may be null. this.outputModuleName = builder.outputModuleName; this.compileDir = builder.compileDir; this.compileStrategy = builder.compileStrategy; - this.arguments = ImmutableList.copyOf(builder.args); - this.tags = ImmutableList.copyOf(builder.tags); // Any new fields added should allow nulls for backward compatibility. } @@ -131,14 +135,23 @@ /** * The arguments passed to Super Dev Mode at startup, or null if not available. */ - public ImmutableList<String> getArguments() { + public List<String> getArguments() { return arguments; } /** * User-defined tags associated with this job. (Not null but may be empty.) */ - public ImmutableList<String> getTags() { return tags; } + public List<String> getTags() { return tags; } + + /** + * Returns the amounts of performance-related metrics. (Not null but may be empty.) + * The keys are subject to change. + */ + public SortedMap<String, Long> getMetricMap() { + // can't return ImmutableSortedMap because it's repackaged + return metricMap; + } /** * If all the given tags are valid, returns a list containing the tags. @@ -216,11 +229,14 @@ private Map<String, String> bindings = ImmutableMap.of(); private Status status; private String message; - private CompileDir compileDir; - private CompileStrategy compileStrategy; + private List<String> args = ImmutableList.of(); private List<String> tags = ImmutableList.of(); + private Map<String, Long> metricMap = ImmutableMap.of(); + private String outputModuleName; + private CompileDir compileDir; + private CompileStrategy compileStrategy; /** * A unique id for this job. Required. @@ -311,6 +327,21 @@ this.tags = checkTags(tags); } + /** + * Sets a map containing metrics used for understanding compiler performance. + * Optional but may not be null. If not set, defaults to the empty map. + * Each key must be a valid identifier beginning with a capital letter. + * (This constraint may be relaxed later.) + */ + public void setMetricMap(Map<String, Long> nameToMetric) { + for (String key : nameToMetric.keySet()) { + // TODO: allow '.' in the key after making an API for that. + Preconditions.checkArgument(MetricName.isValidKey(key), + "invalid counter key: %s", key); + } + this.metricMap = nameToMetric; + } + public JobEvent build() { return new JobEvent(this); }
diff --git a/dev/codeserver/java/com/google/gwt/dev/codeserver/JobRunner.java b/dev/codeserver/java/com/google/gwt/dev/codeserver/JobRunner.java index 404d16c..d07f360 100644 --- a/dev/codeserver/java/com/google/gwt/dev/codeserver/JobRunner.java +++ b/dev/codeserver/java/com/google/gwt/dev/codeserver/JobRunner.java
@@ -33,12 +33,10 @@ */ public class JobRunner { private final JobEventTable table; - private final OutboxTable outboxes; private final ExecutorService executor = Executors.newSingleThreadExecutor(); - JobRunner(JobEventTable table, OutboxTable outboxes) { + JobRunner(JobEventTable table) { this.table = table; - this.outboxes = outboxes; } /** @@ -70,13 +68,29 @@ executor.submit(new Runnable() { @Override public void run() { - recompile(job, outboxes); + try { + recompile(job); + } catch (Throwable t) { + // Try to release the job so the HTTP request will return an error. + // (But this might not work if the same exception is thrown while + // sending the finished event.) + if (!job.isDone()) { + try { + job.onFinished(new Job.Result(null, null, t)); + return; + } catch (Throwable t2) { + // fall through and log original exception + } + } + // Assume everything is broken. Last-ditch attempt to report the error. + t.printStackTrace(); + } } }); job.getLogger().log(Type.TRACE, "added job to queue"); } - private static void recompile(Job job, OutboxTable outboxes) { + private static void recompile(Job job) { job.getLogger().log(Type.INFO, "starting job: " + job.getId()); job.getOutbox().recompile(job); }
diff --git a/dev/codeserver/javatests/com/google/gwt/dev/codeserver/RecompilerTest.java b/dev/codeserver/javatests/com/google/gwt/dev/codeserver/RecompilerTest.java index 9cc7656..023551b 100644 --- a/dev/codeserver/javatests/com/google/gwt/dev/codeserver/RecompilerTest.java +++ b/dev/codeserver/javatests/com/google/gwt/dev/codeserver/RecompilerTest.java
@@ -119,7 +119,7 @@ Outbox outbox = new Outbox("Transactional Cache", recompiler, options, logger); OutboxTable outboxes = new OutboxTable(); outboxes.addOutbox(outbox); - JobRunner runner = new JobRunner(new JobEventTable(), outboxes); + JobRunner runner = new JobRunner(new JobEventTable()); // Perform a first compile. This should pass since all resources are valid. Result result =
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java b/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java index f68ca3e..6295000 100644 --- a/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java +++ b/dev/core/src/com/google/gwt/dev/jjs/impl/UnifyAst.java
@@ -83,6 +83,7 @@ import com.google.gwt.dev.util.StringInterner; import com.google.gwt.dev.util.collect.IdentityHashSet; import com.google.gwt.dev.util.collect.Lists; +import com.google.gwt.dev.util.log.MetricName; import com.google.gwt.dev.util.log.speedtracer.CompilerEventType; import com.google.gwt.dev.util.log.speedtracer.SpeedTracerLogger; import com.google.gwt.dev.util.log.speedtracer.SpeedTracerLogger.Event; @@ -862,9 +863,13 @@ mainLoop(); if (incrementalCompile) { + int declaredTypesInModule = program.getModuleDeclaredTypes().size(); + + MetricName.DECLARED_TYPES_IN_MODULE.setAmount(logger, declaredTypesInModule); + logger.log(TreeLogger.INFO, "Unification traversed " + liveFieldsAndMethods.size() + " fields and methods and " + program.getDeclaredTypes().size() + " types. " - + program.getModuleDeclaredTypes().size() + + declaredTypesInModule + " are considered part of the current module and " + fullFlowTypes.size() + " had all of their fields and methods traversed.");
diff --git a/dev/core/src/com/google/gwt/dev/util/log/AbstractTreeLogger.java b/dev/core/src/com/google/gwt/dev/util/log/AbstractTreeLogger.java index 5ea39c0..edc86b8c 100644 --- a/dev/core/src/com/google/gwt/dev/util/log/AbstractTreeLogger.java +++ b/dev/core/src/com/google/gwt/dev/util/log/AbstractTreeLogger.java
@@ -23,7 +23,7 @@ /** * Abstract base class for TreeLoggers. */ -public abstract class AbstractTreeLogger extends TreeLogger { +public abstract class AbstractTreeLogger extends TreeLogger implements CanUpdateMetrics { private static class UncommittedBranchData { @@ -101,6 +101,12 @@ private UncommittedBranchData uncommitted; /** + * Descendant tree loggers share the same MetricMap by default, + * but this can be overridden with {@link #resetMetricMap()}. + */ + private volatile MetricMap metricMap = new MetricMap(); + + /** * The constructor used when creating a top-level logger. */ protected AbstractTreeLogger() { @@ -146,6 +152,9 @@ childLogger.uncommitted = new UncommittedBranchData(type, msg, caught, helpInfo); + // Inherit the parent's compiler counters by default. + childLogger.metricMap = metricMap; + // This logic is intertwined with log(). If a log message is associated // with a special error condition, then we turn it into a branch, // so this method can be called directly from log(). It is of course @@ -170,10 +179,26 @@ return childLogger; } + /** + * Clears this logger's MetricMap. If the map was shared with the + * parent logger, the parent's map will be unchanged and the new map will + * be independent of its parent. + */ + public void resetMetricMap() { + metricMap = new MetricMap(); + } + public final int getBranchedIndex() { return indexWithinMyParent; } + /** + * Returns the MetricMap currently associated with this TreeLogger. + */ + public MetricMap getMetricMap() { + return metricMap; + } + public final synchronized TreeLogger.Type getMaxDetail() { return logLevel; } @@ -314,6 +339,10 @@ protected abstract void doLog(int indexOfLogEntryWithinParentLogger, TreeLogger.Type type, String msg, Throwable caught, HelpInfo helpInfo); + public void setAmount(MetricName name, long amount) { + metricMap.setAmount(name, amount); + } + /** * Scans <code>t</code> and its causes for {@link OutOfMemoryError} or * {@link StackOverflowError}.
diff --git a/dev/core/src/com/google/gwt/dev/util/log/CanUpdateMetrics.java b/dev/core/src/com/google/gwt/dev/util/log/CanUpdateMetrics.java new file mode 100644 index 0000000..e1205b2 --- /dev/null +++ b/dev/core/src/com/google/gwt/dev/util/log/CanUpdateMetrics.java
@@ -0,0 +1,27 @@ +/* + * Copyright 2014 Google Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package com.google.gwt.dev.util.log; + +/** + * Something that accepts updates to metrics. + */ +interface CanUpdateMetrics { + + /** + * A metric's value should normally be set using {@link MetricName#setAmount}. + */ + void setAmount(MetricName name, long amount); +}
diff --git a/dev/core/src/com/google/gwt/dev/util/log/CompositeTreeLogger.java b/dev/core/src/com/google/gwt/dev/util/log/CompositeTreeLogger.java index 8f06b23..e12b343 100644 --- a/dev/core/src/com/google/gwt/dev/util/log/CompositeTreeLogger.java +++ b/dev/core/src/com/google/gwt/dev/util/log/CompositeTreeLogger.java
@@ -21,7 +21,7 @@ * Forks logging over two child loggers. This provides the graphics + file * logging of DevModeBase's -logfile option. */ -public class CompositeTreeLogger extends TreeLogger { +public class CompositeTreeLogger extends TreeLogger implements CanUpdateMetrics { private TreeLogger[] loggers; @@ -55,4 +55,11 @@ logger.log(type, msg, caught, helpInfo); } } + + @Override + public void setAmount(MetricName name, long amount) { + for (TreeLogger logger : loggers) { + name.setAmount(logger, amount); + } + } }
diff --git a/dev/core/src/com/google/gwt/dev/util/log/MetricMap.java b/dev/core/src/com/google/gwt/dev/util/log/MetricMap.java new file mode 100644 index 0000000..a5c4118 --- /dev/null +++ b/dev/core/src/com/google/gwt/dev/util/log/MetricMap.java
@@ -0,0 +1,43 @@ +/* + * Copyright 2014 Google Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package com.google.gwt.dev.util.log; + +import com.google.gwt.dev.util.collect.HashMap; +import com.google.gwt.thirdparty.guava.common.collect.ImmutableSortedMap; + +import java.util.Map; + +/** + * Holds mappings from metrics to their values. Thread-safe. + */ +public class MetricMap implements CanUpdateMetrics { + private final Map<String, Long> map = new HashMap<String, Long>(); + + /** + * Each MetricMap lives in one or more TreeLoggers. + * See {@link AbstractTreeLogger#resetMetricMap()}. + */ + MetricMap() { + } + + public synchronized void setAmount(MetricName name, long amount) { + map.put(name.key, amount); + } + + public synchronized ImmutableSortedMap<String, Long> getSnapshot() { + return ImmutableSortedMap.copyOf(map); + } +}
diff --git a/dev/core/src/com/google/gwt/dev/util/log/MetricName.java b/dev/core/src/com/google/gwt/dev/util/log/MetricName.java new file mode 100644 index 0000000..0808afe --- /dev/null +++ b/dev/core/src/com/google/gwt/dev/util/log/MetricName.java
@@ -0,0 +1,76 @@ +/* + * Copyright 2014 Google Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package com.google.gwt.dev.util.log; + +import com.google.gwt.core.ext.TreeLogger; +import com.google.gwt.core.ext.TreeLogger.Type; +import com.google.gwt.thirdparty.guava.common.base.Preconditions; + +import java.util.regex.Pattern; + +/** + * Defines a name for each performance-related metric. + */ +public enum MetricName { + + /** + * The number of types that UnifyAst considers to be part of the module being compiled. + */ + DECLARED_TYPES_IN_MODULE("DeclaredTypesInModule"); + + // Note: this constraint is used in JobEvent which is a public API (for Super Dev Mode). + // Allowing more characters in keys may break data collection. + private static final Pattern VALID_KEY = Pattern.compile("^[A-Z][A-Za-z0-9]*$"); + + static { + // Cannot check this in the enum constructor. + for (MetricName counter : values()) { + Preconditions.checkState(isValidKey(counter.key), "invalid key: %s", counter.key); + } + } + + final String key; + + /** + * @param key the string key to appear in output. Must be an identifier beginning + * with a capital letter and not containing underscores. + */ + MetricName(String key) { + this.key = key; + } + + /** + * Adds the given amount to the counter. + * @param logger the destination where the count will be logged. + */ + public void setAmount(TreeLogger logger, long amount) { + Preconditions.checkNotNull(logger); + Preconditions.checkArgument(amount >= 0, "attempted to set a negative amount"); + if (logger instanceof CanUpdateMetrics) { + ((CanUpdateMetrics) logger).setAmount(this, amount); + } else { + // Just log it. + logger.log(Type.DEBUG, "Metric: " + key + " = " + amount); + } + } + + /** + * Returns true if the given name can be used as a key for a compiler counter. + */ + public static boolean isValidKey(String name) { + return VALID_KEY.matcher(name).matches(); + } +}