Refactor TargetPatternPhaseFunction

- Make TargetPatternPhaseKey implement SkyKey
- Move the TargetParsingCompleteEvent posting into the function
- Split the time reporting out into TargetParsingPhaseTimeEvent

PiperOrigin-RevId: 162475743

7 files changed, 103 insertions, 71 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/TargetParsingCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/pkgcache/TargetParsingCompleteEvent.java
index 2a0763b8fc..64d4eb1f1e 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/TargetParsingCompleteEvent.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/TargetParsingCompleteEvent.java
@@ -36,7 +36,6 @@ public class TargetParsingCompleteEvent implements BuildEvent {
   private final ImmutableSet<Target> filteredTargets;
   private final ImmutableSet<Target> testFilteredTargets;
   private final ImmutableSet<Target> expandedTargets;
-  private final long timeInMs;
 
   /**
    * Construct the event.
@@ -47,10 +46,8 @@ public class TargetParsingCompleteEvent implements BuildEvent {
       Collection<Target> targets,
       Collection<Target> filteredTargets,
       Collection<Target> testFilteredTargets,
-      long timeInMs,
       List<String> originalTargetPattern,
       Collection<Target> expandedTargets) {
-    this.timeInMs = timeInMs;
     this.targets = ImmutableSet.copyOf(targets);
     this.filteredTargets = ImmutableSet.copyOf(filteredTargets);
     this.testFilteredTargets = ImmutableSet.copyOf(testFilteredTargets);
@@ -64,7 +61,6 @@ public class TargetParsingCompleteEvent implements BuildEvent {
         targets,
         ImmutableSet.<Target>of(),
         ImmutableSet.<Target>of(),
-        0,
         ImmutableList.<String>of(),
         targets);
   }
@@ -94,10 +90,6 @@ public class TargetParsingCompleteEvent implements BuildEvent {
     return testFilteredTargets;
   }
 
-  public long getTimeInMs() {
-    return timeInMs;
-  }
-
   @Override
   public BuildEventId getEventId() {
     return BuildEventId.targetPatternExpanded(originalTargetPattern);
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/TargetParsingPhaseTimeEvent.java b/src/main/java/com/google/devtools/build/lib/pkgcache/TargetParsingPhaseTimeEvent.java
new file mode 100644
index 0000000000..7c70b28d19
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/TargetParsingPhaseTimeEvent.java
@@ -0,0 +1,29 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// 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.devtools.build.lib.pkgcache;
+
+import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
+
+/** This event is fired just after target pattern evaluation is completed. */
+public class TargetParsingPhaseTimeEvent implements Postable {
+  private final long timeInMs;
+
+  public TargetParsingPhaseTimeEvent(long timeInMs) {
+    this.timeInMs = timeInMs;
+  }
+
+  public long getTimeInMs() {
+    return timeInMs;
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/LegacyLoadingPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/skyframe/LegacyLoadingPhaseRunner.java
index 90b24bd068..e5b57adecf 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/LegacyLoadingPhaseRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/LegacyLoadingPhaseRunner.java
@@ -35,6 +35,7 @@ import com.google.devtools.build.lib.pkgcache.LoadingPhaseRunner;
 import com.google.devtools.build.lib.pkgcache.LoadingResult;
 import com.google.devtools.build.lib.pkgcache.PackageManager;
 import com.google.devtools.build.lib.pkgcache.TargetParsingCompleteEvent;
+import com.google.devtools.build.lib.pkgcache.TargetParsingPhaseTimeEvent;
 import com.google.devtools.build.lib.pkgcache.TargetPatternEvaluator;
 import com.google.devtools.build.lib.pkgcache.TestFilter;
 import com.google.devtools.build.lib.util.Preconditions;
@@ -197,19 +198,18 @@ public final class LegacyLoadingPhaseRunner extends LoadingPhaseRunner {
             expandedResult.hasError(),
             filteredTargets,
             testFilteredTargets,
-            /*originalTargets=*/targets.getTargets(),
             testSuiteTargets,
             getWorkspaceName(eventHandler));
 
     // This is the same code as SkyframeLoadingPhaseRunner.
     eventHandler.post(
         new TargetParsingCompleteEvent(
-            patternParsingValue.getOriginalTargets(),
+            targets.getTargets(),
             patternParsingValue.getFilteredTargets(),
             patternParsingValue.getTestFilteredTargets(),
-            targetPatternEvalTime,
             targetPatterns,
             patternParsingValue.getTargets()));
+    eventHandler.post(new TargetParsingPhaseTimeEvent(targetPatternEvalTime));
     if (callback != null) {
       callback.notifyTargets(patternParsingValue.getTargets());
     }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 379d3de9d7..b37a1ed163 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -105,7 +105,7 @@ import com.google.devtools.build.lib.pkgcache.PackageCacheOptions;
 import com.google.devtools.build.lib.pkgcache.PackageManager;
 import com.google.devtools.build.lib.pkgcache.PackageManager.PackageManagerStatistics;
 import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
-import com.google.devtools.build.lib.pkgcache.TargetParsingCompleteEvent;
+import com.google.devtools.build.lib.pkgcache.TargetParsingPhaseTimeEvent;
 import com.google.devtools.build.lib.pkgcache.TestFilter;
 import com.google.devtools.build.lib.pkgcache.TransitivePackageLoader;
 import com.google.devtools.build.lib.profiler.AutoProfiler;
@@ -2082,17 +2082,10 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
               + Iterables.toString(errorInfo.getRootCauses()), e);
         }
       }
-      long time = timer.stop().elapsed(TimeUnit.MILLISECONDS);
+      long timeMillis = timer.stop().elapsed(TimeUnit.MILLISECONDS);
 
       TargetPatternPhaseValue patternParsingValue = evalResult.get(key);
-      eventHandler.post(
-          new TargetParsingCompleteEvent(
-              patternParsingValue.getOriginalTargets(),
-              patternParsingValue.getFilteredTargets(),
-              patternParsingValue.getTestFilteredTargets(),
-              time,
-              targetPatterns,
-              patternParsingValue.getTargets()));
+      eventHandler.post(new TargetParsingPhaseTimeEvent(timeMillis));
       if (callback != null) {
         callback.notifyTargets(patternParsingValue.getTargets());
       }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
index 13b1556f22..d90db5018e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java
@@ -27,10 +27,11 @@ import com.google.devtools.build.lib.pkgcache.CompileOneDependencyTransformer;
 import com.google.devtools.build.lib.pkgcache.FilteringPolicies;
 import com.google.devtools.build.lib.pkgcache.LoadingPhaseRunner;
 import com.google.devtools.build.lib.pkgcache.ParsingFailedEvent;
+import com.google.devtools.build.lib.pkgcache.TargetParsingCompleteEvent;
 import com.google.devtools.build.lib.pkgcache.TargetProvider;
 import com.google.devtools.build.lib.pkgcache.TestFilter;
 import com.google.devtools.build.lib.skyframe.EnvironmentBackedRecursivePackageProvider.MissingDepException;
-import com.google.devtools.build.lib.skyframe.TargetPatternPhaseValue.TargetPatternList;
+import com.google.devtools.build.lib.skyframe.TargetPatternPhaseValue.TargetPatternPhaseKey;
 import com.google.devtools.build.lib.skyframe.TargetPatternValue.TargetPatternKey;
 import com.google.devtools.build.lib.skyframe.TargetPatternValue.TargetPatternSkyKeyOrException;
 import com.google.devtools.build.lib.util.Preconditions;
@@ -54,7 +55,7 @@ final class TargetPatternPhaseFunction implements SkyFunction {
 
   @Override
   public TargetPatternPhaseValue compute(SkyKey key, Environment env) throws InterruptedException {
-    TargetPatternList options = (TargetPatternList) key.argument();
+    TargetPatternPhaseKey options = (TargetPatternPhaseKey) key.argument();
     PackageValue packageValue = null;
     boolean workspaceError = false;
     try {
@@ -176,9 +177,18 @@ final class TargetPatternPhaseFunction implements SkyFunction {
     ResolvedTargets<Target> expandedTargets = expandedTargetsBuilder.build();
     Set<Target> testSuiteTargets =
         Sets.difference(targets.getTargets(), expandedTargets.getTargets());
-    return new TargetPatternPhaseValue(expandedTargets.getTargets(), testsToRun, preExpansionError,
+    TargetPatternPhaseValue result = new TargetPatternPhaseValue(
+        expandedTargets.getTargets(), testsToRun, preExpansionError,
         expandedTargets.hasError() || workspaceError, filteredTargets, testFilteredTargets,
-        targets.getTargets(), ImmutableSet.copyOf(testSuiteTargets), workspaceName);
+        ImmutableSet.copyOf(testSuiteTargets), workspaceName);
+    env.getListener().post(
+        new TargetParsingCompleteEvent(
+            targets.getTargets(),
+            result.getFilteredTargets(),
+            result.getTestFilteredTargets(),
+            options.getTargetPatterns(),
+            result.getTargets()));
+    return result;
   }
 
   /**
@@ -187,7 +197,7 @@ final class TargetPatternPhaseFunction implements SkyFunction {
    * @param options the command-line arguments in structured form
    */
   private static ResolvedTargets<Target> getTargetsToBuild(
-      Environment env, TargetPatternList options) throws InterruptedException {
+      Environment env, TargetPatternPhaseKey options) throws InterruptedException {
     List<TargetPatternKey> patternSkyKeys = new ArrayList<>();
     for (TargetPatternSkyKeyOrException keyOrException :
         TargetPatternValue.keys(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java
index 187a94280f..8b61f73a4a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseValue.java
@@ -22,7 +22,7 @@ import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.pkgcache.LoadingResult;
 import com.google.devtools.build.lib.pkgcache.TestFilter;
 import com.google.devtools.build.lib.util.Preconditions;
-import com.google.devtools.build.skyframe.LegacySkyKey;
+import com.google.devtools.build.skyframe.SkyFunctionName;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import java.io.ObjectInputStream;
@@ -48,23 +48,21 @@ public final class TargetPatternPhaseValue implements SkyValue {
   private final ImmutableSet<Target> filteredTargets;
   private final ImmutableSet<Target> testFilteredTargets;
 
-  // These two fields are only for the purposes of generating the TargetParsingCompleteEvent.
+  // This field is only for the purposes of generating the LoadingPhaseCompleteEvent.
   // TODO(ulfjack): Support EventBus event posting in Skyframe, and remove this code again.
-  private final ImmutableSet<Target> originalTargets;
   private final ImmutableSet<Target> testSuiteTargets;
   private final String workspaceName;
 
   TargetPatternPhaseValue(ImmutableSet<Target> targets, @Nullable ImmutableSet<Target> testsToRun,
       boolean hasError, boolean hasPostExpansionError, ImmutableSet<Target> filteredTargets,
-      ImmutableSet<Target> testFilteredTargets, ImmutableSet<Target> originalTargets,
-      ImmutableSet<Target> testSuiteTargets, String workspaceName) {
+      ImmutableSet<Target> testFilteredTargets, ImmutableSet<Target> testSuiteTargets,
+      String workspaceName) {
     this.targets = Preconditions.checkNotNull(targets);
     this.testsToRun = testsToRun;
     this.hasError = hasError;
     this.hasPostExpansionError = hasPostExpansionError;
     this.filteredTargets = Preconditions.checkNotNull(filteredTargets);
     this.testFilteredTargets = Preconditions.checkNotNull(testFilteredTargets);
-    this.originalTargets = Preconditions.checkNotNull(originalTargets);
     this.testSuiteTargets = Preconditions.checkNotNull(testSuiteTargets);
     this.workspaceName = workspaceName;
   }
@@ -94,10 +92,6 @@ public final class TargetPatternPhaseValue implements SkyValue {
     return testFilteredTargets;
   }
 
-  public ImmutableSet<Target> getOriginalTargets() {
-    return originalTargets;
-  }
-
   public ImmutableSet<Target> getTestSuiteTargets() {
     return testSuiteTargets;
   }
@@ -128,29 +122,29 @@ public final class TargetPatternPhaseValue implements SkyValue {
 
   /** Create a target pattern phase value key. */
   @ThreadSafe
-  public static SkyKey key(ImmutableList<String> targetPatterns, String offset,
-      boolean compileOneDependency, boolean buildTestsOnly, boolean determineTests,
+  public static SkyKey key(
+      ImmutableList<String> targetPatterns,
+      String offset,
+      boolean compileOneDependency,
+      boolean buildTestsOnly,
+      boolean determineTests,
       ImmutableList<String> buildTargetFilter,
-      boolean buildManualTests, @Nullable TestFilter testFilter) {
-    return LegacySkyKey.create(
-        SkyFunctions.TARGET_PATTERN_PHASE,
-        new TargetPatternList(
-            targetPatterns,
-            offset,
-            compileOneDependency,
-            buildTestsOnly,
-            determineTests,
-            buildTargetFilter,
-            buildManualTests,
-            testFilter));
+      boolean buildManualTests,
+      @Nullable TestFilter testFilter) {
+    return new TargetPatternPhaseKey(
+        targetPatterns,
+        offset,
+        compileOneDependency,
+        buildTestsOnly,
+        determineTests,
+        buildTargetFilter,
+        buildManualTests,
+        testFilter);
   }
 
-  /**
-   * A TargetPattern is a tuple of pattern (eg, "foo/..."), filtering policy, a relative pattern
-   * offset, and whether it is a positive or negative match.
-   */
+  /** The configuration needed to run the target pattern evaluation phase. */
   @ThreadSafe
-  static final class TargetPatternList implements Serializable {
+  static final class TargetPatternPhaseKey implements SkyKey, Serializable {
     private final ImmutableList<String> targetPatterns;
     private final String offset;
     private final boolean compileOneDependency;
@@ -160,9 +154,14 @@ public final class TargetPatternPhaseValue implements SkyValue {
     private final boolean buildManualTests;
     @Nullable private final TestFilter testFilter;
 
-    public TargetPatternList(ImmutableList<String> targetPatterns, String offset,
-        boolean compileOneDependency, boolean buildTestsOnly, boolean determineTests,
-        ImmutableList<String> buildTargetFilter, boolean buildManualTests,
+    public TargetPatternPhaseKey(
+        ImmutableList<String> targetPatterns,
+        String offset,
+        boolean compileOneDependency,
+        boolean buildTestsOnly,
+        boolean determineTests,
+        ImmutableList<String> buildTargetFilter,
+        boolean buildManualTests,
         @Nullable TestFilter testFilter) {
       this.targetPatterns = Preconditions.checkNotNull(targetPatterns);
       this.offset = Preconditions.checkNotNull(offset);
@@ -177,6 +176,16 @@ public final class TargetPatternPhaseValue implements SkyValue {
       }
     }
 
+    @Override
+    public SkyFunctionName functionName() {
+      return SkyFunctions.TARGET_PATTERN_PHASE;
+    }
+
+    @Override
+    public Object argument() {
+      return this;
+    }
+
     public ImmutableList<String> getTargetPatterns() {
       return targetPatterns;
     }
@@ -225,8 +234,9 @@ public final class TargetPatternPhaseValue implements SkyValue {
 
     @Override
     public int hashCode() {
-      return Objects.hash(targetPatterns, offset, compileOneDependency, buildTestsOnly,
-          determineTests, buildManualTests, testFilter);
+      return Objects.hash(
+          targetPatterns, offset, compileOneDependency, buildTestsOnly, determineTests,
+          buildManualTests, testFilter);
     }
 
     @Override
@@ -234,10 +244,10 @@ public final class TargetPatternPhaseValue implements SkyValue {
       if (this == obj) {
         return true;
       }
-      if (!(obj instanceof TargetPatternList)) {
+      if (!(obj instanceof TargetPatternPhaseKey)) {
         return false;
       }
-      TargetPatternList other = (TargetPatternList) obj;
+      TargetPatternPhaseKey other = (TargetPatternPhaseKey) obj;
       return other.targetPatterns.equals(this.targetPatterns)
           && other.offset.equals(this.offset)
           && other.compileOneDependency == compileOneDependency
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseKeyTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseKeyTest.java
index a56466c01d..70ba224165 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseKeyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseKeyTest.java
@@ -24,16 +24,14 @@ import com.google.common.testing.NullPointerTester;
 import com.google.devtools.build.lib.events.NullEventHandler;
 import com.google.devtools.build.lib.pkgcache.LoadingOptions;
 import com.google.devtools.build.lib.pkgcache.TestFilter;
-import com.google.devtools.build.lib.skyframe.TargetPatternPhaseValue.TargetPatternList;
+import com.google.devtools.build.lib.skyframe.TargetPatternPhaseValue.TargetPatternPhaseKey;
 import com.google.devtools.common.options.Options;
-
+import javax.annotation.Nullable;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
-import javax.annotation.Nullable;
-
-/** Tests for {@link TargetPatternList}. */
+/** Tests for {@link TargetPatternPhaseKey}. */
 @RunWith(JUnit4.class)
 public class TargetPatternPhaseKeyTest {
   static enum Flag {
@@ -79,18 +77,18 @@ public class TargetPatternPhaseKeyTest {
        .testEquals();
   }
 
-  private TargetPatternList of(ImmutableList<String> targetPatterns, String offset,
+  private TargetPatternPhaseKey of(ImmutableList<String> targetPatterns, String offset,
       ImmutableList<String> buildTagFilter,
       boolean includeManualTests, @Nullable TestFilter testFilter, Flag... flags) {
     ImmutableSet<Flag> set = ImmutableSet.copyOf(flags);
     boolean compileOneDependency = set.contains(Flag.COMPILE_ONE_DEPENDENCY);
     boolean buildTestsOnly = set.contains(Flag.BUILD_TESTS_ONLY);
     boolean determineTests = set.contains(Flag.DETERMINE_TESTS);
-    return new TargetPatternList(targetPatterns, offset, compileOneDependency, buildTestsOnly,
+    return new TargetPatternPhaseKey(targetPatterns, offset, compileOneDependency, buildTestsOnly,
         determineTests, buildTagFilter, includeManualTests, testFilter);
   }
 
-  private TargetPatternList of(ImmutableList<String> targetPatterns, String offset) {
+  private TargetPatternPhaseKey of(ImmutableList<String> targetPatterns, String offset) {
     return of(targetPatterns, offset, ImmutableList.<String>of(), false, null);
   }
 
@@ -102,6 +100,6 @@ public class TargetPatternPhaseKeyTest {
   @Test
   public void testNull() throws Exception {
     new NullPointerTester()
-        .testAllPublicConstructors(TargetPatternList.class);
+        .testAllPublicConstructors(TargetPatternPhaseKey.class);
   }
 }