Split the cycle between vfs and profiler.

- Move ProfilerInfo into a subpackage (it's not necessary for profiling, just for analyzing a profile). - Make some fields in Profiler public for ProfileInfo. - Mark Profiler as ThreadSafe; there's no cyclic dependency here. This is based on ulfjack's microbazel patch series: ulfjack@44553fc PiperOrigin-RevId: 167121952
bazelbuild · Aug 31, 2017 · e9e35aa · e9e35aa
1 parent 930f703
commit e9e35aa
Show file tree

Hide file tree

Showing 26 changed files with 161 additions and 116 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD
@@ -21,6 +21,7 @@ filegroup(
         "//src/main/java/com/google/devtools/build/lib/exec/apple:srcs",
         "//src/main/java/com/google/devtools/build/lib/exec/local:srcs",
         "//src/main/java/com/google/devtools/build/lib/graph:srcs",
+        "//src/main/java/com/google/devtools/build/lib/profiler:srcs",
         "//src/main/java/com/google/devtools/build/lib/query2:srcs",
         "//src/main/java/com/google/devtools/build/lib/remote:srcs",
         "//src/main/java/com/google/devtools/build/lib/rules/apple/cpp:srcs",
@@ -68,6 +69,7 @@ java_library(
         ":os_util",
         ":preconditions",
         ":vfs",
+        "//src/main/java/com/google/devtools/build/lib/profiler",
         "//src/main/java/com/google/devtools/build/lib/shell",
         "//third_party:guava",
     ],
@@ -124,7 +126,6 @@ java_library(
 java_library(
     name = "vfs",
     srcs = glob([
-        "profiler/*.java",
         "vfs/*.java",
     ]),
     visibility = ["//visibility:public"],
@@ -134,30 +135,14 @@ java_library(
         ":concurrent",
         ":os_util",
         ":preconditions",
+        "//src/main/java/com/google/devtools/build/lib/profiler",
         "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/common/options",
         "//third_party:guava",
         "//third_party:jsr305",
     ],
 )
 
-# Profiler chart library.
-java_library(
-    name = "profiler-output",
-    srcs = glob([
-        "profiler/chart/*.java",
-        "profiler/output/*.java",
-        "profiler/statistics/*.java",
-    ]),
-    deps = [
-        ":util",
-        ":vfs",
-        "//src/main/java/com/google/devtools/build/lib/actions",
-        "//third_party:guava",
-        "//third_party:jsr305",
-    ],
-)
-
 # In-memory virtual file system.
 java_library(
     name = "inmemoryfs",
@@ -220,6 +205,7 @@ java_library(
         ":clock",
         ":concurrent",
         ":vfs",
+        "//src/main/java/com/google/devtools/build/lib/profiler",
         "//third_party:guava",
     ],
 )
@@ -451,6 +437,7 @@ java_library(
         ":util",
         ":vfs",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
+        "//src/main/java/com/google/devtools/build/lib/profiler",
         "//src/main/java/com/google/devtools/common/options",
         "//third_party:asm",
         "//third_party:asm-commons",
@@ -621,6 +608,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/causes",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/graph",
+        "//src/main/java/com/google/devtools/build/lib/profiler",
         "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization",
         "//src/main/java/com/google/devtools/build/skyframe",
@@ -1153,7 +1141,6 @@ java_library(
         ":io",
         ":packages-internal",
         ":process_util",
-        ":profiler-output",
         ":shared-base-rules",
         ":unix",
         ":util",
@@ -1165,6 +1152,8 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/exec/local",
+        "//src/main/java/com/google/devtools/build/lib/profiler",
+        "//src/main/java/com/google/devtools/build/lib/profiler:profiler-output",
         "//src/main/java/com/google/devtools/build/lib/query2",
         "//src/main/java/com/google/devtools/build/lib/query2:query-engine",
         "//src/main/java/com/google/devtools/build/lib/query2:query-output",

diff --git a/src/main/java/com/google/devtools/build/lib/actions/BUILD b/src/main/java/com/google/devtools/build/lib/actions/BUILD
@@ -22,6 +22,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib:vfs",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
         "//src/main/java/com/google/devtools/build/lib/causes",
+        "//src/main/java/com/google/devtools/build/lib/profiler",
         "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/build/skyframe",
         "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",

diff --git a/src/main/java/com/google/devtools/build/lib/profiler/BUILD b/src/main/java/com/google/devtools/build/lib/profiler/BUILD
@@ -0,0 +1,46 @@
+package(
+    default_visibility = ["//src:__subpackages__"],
+)
+
+java_library(
+    name = "profiler",
+    srcs = glob([
+        "*.java",
+    ]),
+    visibility = ["//visibility:public"],
+    deps = [
+        "//src/main/java/com/google/devtools/build/lib:base-util",
+        "//src/main/java/com/google/devtools/build/lib:clock",
+        "//src/main/java/com/google/devtools/build/lib:concurrent",
+        "//src/main/java/com/google/devtools/build/lib:os_util",
+        "//src/main/java/com/google/devtools/build/lib:preconditions",
+        "//src/main/java/com/google/devtools/build/lib/shell",
+        "//src/main/java/com/google/devtools/common/options",
+        "//third_party:guava",
+        "//third_party:jsr305",
+    ],
+)
+
+# Profiler chart library.
+java_library(
+    name = "profiler-output",
+    srcs = glob([
+        "analysis/*.java",
+        "chart/*.java",
+        "output/*.java",
+        "statistics/*.java",
+    ]),
+    deps = [
+        ":profiler",
+        "//src/main/java/com/google/devtools/build/lib:util",
+        "//src/main/java/com/google/devtools/build/lib:vfs",
+        "//src/main/java/com/google/devtools/build/lib/actions",
+        "//third_party:guava",
+        "//third_party:jsr305",
+    ],
+)
+
+filegroup(
+    name = "srcs",
+    srcs = glob(["**"]),
+)
diff --git a/src/main/java/com/google/devtools/build/lib/profiler/Profiler.java b/src/main/java/com/google/devtools/build/lib/profiler/Profiler.java
@@ -18,6 +18,8 @@
 import com.google.common.base.Predicate;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
+import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.profiler.PredicateBasedStatRecorder.RecorderAndPredicate;
 import com.google.devtools.build.lib.profiler.StatRecorder.VfsHeuristics;
 import com.google.devtools.build.lib.util.Clock;
@@ -45,28 +47,27 @@
 import java.util.zip.DeflaterOutputStream;
 
 /**
- * Blaze internal profiler. Provides facility to report various Blaze tasks and
- * store them (asynchronously) in the file for future analysis.
- * <p>
- * Implemented as singleton so any caller should use Profiler.instance() to
- * obtain reference.
- * <p>
- * Internally, profiler uses two data structures - ThreadLocal task stack to track
- * nested tasks and single ConcurrentLinkedQueue to gather all completed tasks.
- * <p>
- * Also, due to the nature of the provided functionality (instrumentation of all
- * Blaze components), build.lib.profiler package will be used by almost every
- * other Blaze package, so special attention should be paid to avoid any
- * dependencies on the rest of the Blaze code, including build.lib.util and
- * build.lib.vfs. This is important because build.lib.util and build.lib.vfs
- * contain Profiler invocations and any dependency on those two packages would
- * create circular relationship.
- * <p>
- * All gathered instrumentation data will be stored in the file. Please, note,
- * that while file format is described here it is considered internal and can
- * change at any time. For scripting, using blaze analyze-profile --dump=raw
- * would be more robust and stable solution.
+ * Blaze internal profiler. Provides facility to report various Blaze tasks and store them
+ * (asynchronously) in the file for future analysis.
+ *
+ * <p>Implemented as singleton so any caller should use Profiler.instance() to obtain reference.
+ *
+ * <p>Internally, profiler uses two data structures - ThreadLocal task stack to track nested tasks
+ * and single ConcurrentLinkedQueue to gather all completed tasks.
+ *
+ * <p>Also, due to the nature of the provided functionality (instrumentation of all Blaze
+ * components), build.lib.profiler package will be used by almost every other Blaze package, so
+ * special attention should be paid to avoid any dependencies on the rest of the Blaze code,
+ * including build.lib.util and build.lib.vfs. This is important because build.lib.util and
+ * build.lib.vfs contain Profiler invocations and any dependency on those two packages would create
+ * circular relationship.
+ *
+ * <p>All gathered instrumentation data will be stored in the file. Please, note, that while file
+ * format is described here it is considered internal and can change at any time. For scripting,
+ * using blaze analyze-profile --dump=raw would be more robust and stable solution.
+ *
  * <p>
+ *
  * <pre>
  * Profiler file consists of the deflated stream with following overall structure:
  *   HEADER
@@ -115,19 +116,19 @@
  *
  * @see ProfilerTask enum for recognized task types.
  */
-//@ThreadSafe - commented out to avoid cyclic dependency with lib.util package
+@ThreadSafe
 public final class Profiler {
   private static final Logger LOG = Logger.getLogger(Profiler.class.getName());
 
-  static final int MAGIC = 0x11223344;
+  public static final int MAGIC = 0x11223344;
 
   // File version number. Note that merely adding new record types in
   // the ProfilerTask does not require bumping version number as long as original
   // enum values are not renamed or deleted.
-  static final int VERSION = 0x03;
+  public static final int VERSION = 0x03;
 
   // EOF marker. Must be < 0.
-  static final int EOF_MARKER = -1;
+  public static final int EOF_MARKER = -1;
 
   // Profiler will check for gathered data and persist all of it in the
   // separate thread every SAVE_DELAY ms.
@@ -140,11 +141,8 @@ public final class Profiler {
 
   private static final int HISTOGRAM_BUCKETS = 20;
 
-  /**
-   *
-   * A task that was very slow.
-   */
-  public final class SlowTask implements Comparable<SlowTask> {
+  /** A task that was very slow. */
+  public static final class SlowTask implements Comparable<SlowTask> {
     final long durationNanos;
     final Object object;
     ProfilerTask type;
@@ -187,7 +185,7 @@ public ProfilerTask getType() {
    * Class itself is not thread safe, but all access to it from Profiler
    * methods is.
    */
-  //@ThreadCompatible - commented out to avoid cyclic dependency with lib.util.
+  @ThreadCompatible
   private final class TaskData {
     final long threadId;
     final long startTime;
@@ -240,7 +238,7 @@ public String toString() {
    * However, ArrayDeque is 1.6 only. For 1.5 best approach would be to utilize
    * ArrayList and emulate stack using it.
    */
-  //@ThreadSafe - commented out to avoid cyclic dependency with lib.util.
+  @ThreadSafe
   private final class TaskStack extends ThreadLocal<List<TaskData>> {
 
     @Override
@@ -293,11 +291,11 @@ private static String toDescription(Object object) {
   }
 
   /**
-   * Implements datastore for object description indices. Intended to be used
-   * only by the Profiler.save() method.
+   * Implements datastore for object description indices. Intended to be used only by the
+   * Profiler.save() method.
    */
-  //@ThreadCompatible - commented out to avoid cyclic dependency with lib.util.
-  private final class ObjectDescriber {
+  @ThreadCompatible
+  private static final class ObjectDescriber {
     private Map<Object, Integer> descMap = new IdentityHashMap<>(2000);
     private int indexCounter = 0;
 
@@ -337,7 +335,7 @@ boolean isUnassigned(int index) {
    * lock if they do the same operation at the same time. Access to the individual queues is
    * synchronized on the queue objects themselves.
    */
-  private final class SlowestTaskAggregator {
+  private static final class SlowestTaskAggregator {
     private static final int SHARDS = 16;
     private final int size;
 

diff --git a/...tools/build/lib/profiler/ProfileInfo.java → ...ld/lib/profiler/analysis/ProfileInfo.java b/...tools/build/lib/profiler/ProfileInfo.java → ...ld/lib/profiler/analysis/ProfileInfo.java
@@ -11,7 +11,7 @@
 // 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.devtools.build.lib.profiler;
+package com.google.devtools.build.lib.profiler.analysis;
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.devtools.build.lib.profiler.ProfilerTask.CRITICAL_PATH;
@@ -26,6 +26,9 @@
 import com.google.common.collect.MultimapBuilder.ListMultimapBuilder;
 import com.google.common.collect.Ordering;
 import com.google.common.collect.Sets;
+import com.google.devtools.build.lib.profiler.ProfilePhase;
+import com.google.devtools.build.lib.profiler.Profiler;
+import com.google.devtools.build.lib.profiler.ProfilerTask;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.util.VarInt;
 import com.google.devtools.build.lib.vfs.Path;
@@ -74,10 +77,8 @@ public static final class AggregateAttr {
     }
   }
 
-  /**
-   * Immutable compact representation of the Map<ProfilerTask, AggregateAttr>.
-   */
-  static final class CompactStatistics {
+  /** Immutable compact representation of the Map<ProfilerTask, AggregateAttr>. */
+  public static final class CompactStatistics {
     final byte[] content;
 
     CompactStatistics(byte[] content) {
@@ -103,13 +104,15 @@ static final class CompactStatistics {
       content = sink.position() > 0 ? Arrays.copyOf(sink.array(), sink.position()) : null;
     }
 
-    boolean isEmpty() { return content == null; }
+    public boolean isEmpty() {
+      return content == null;
+    }
 
     /**
-     * Converts instance back into AggregateAttr[TASK_COUNT]. See
-     * constructor documentation for more information.
+     * Converts instance back into AggregateAttr[TASK_COUNT]. See constructor documentation for more
+     * information.
      */
-    AggregateAttr[] toArray() {
+    public AggregateAttr[] toArray() {
       AggregateAttr[] stats = new AggregateAttr[TASK_COUNT];
       if (!isEmpty()) {
         ByteBuffer source = ByteBuffer.wrap(content);
@@ -123,10 +126,8 @@ AggregateAttr[] toArray() {
       return stats;
     }
 
-    /**
-     * Returns AggregateAttr instance for the given ProfilerTask value.
-     */
-    AggregateAttr getAttr(ProfilerTask task) {
+    /** Returns AggregateAttr instance for the given ProfilerTask value. */
+    public AggregateAttr getAttr(ProfilerTask task) {
       if (isEmpty()) { return ZERO; }
       ByteBuffer source = ByteBuffer.wrap(content);
       byte id = (byte) task.ordinal();
@@ -181,9 +182,9 @@ public final class Task implements Comparable<Task> {
     public final long startTime;
     public final long durationNanos;
     public final ProfilerTask type;
-    final CompactStatistics stats;
+    public final CompactStatistics stats;
     // Contains statistic for a task and all subtasks. Populated only for root tasks.
-    CompactStatistics aggregatedStats = null;
+    public CompactStatistics aggregatedStats = null;
     // Subtasks are stored as an array for performance and memory utilization
     // reasons (we can easily deal with millions of those objects).
     public Task[] subtasks = NO_TASKS;