commit a03479fa0819ea6a42f3153988b07e8e923396cb
author felly <felly@google.com> Thu May 16 07:59:05 2019 -0700
committer Copybara-Service <copybara-worker@google.com> Thu May 16 08:01:28 2019 -0700
tree f7c16a586c1640bde3ac801310c86e77f23b42d0
parent 8b23eecaf8527f67ad38805eab4ddb5fa4ab3a80

More package performance: Avoid copies when doing Target enumeration in package. By using a BiMap we efficiently retrieve the Set of all Targets.

RELNOTES: None
PiperOrigin-RevId: 248526789

src/main/java/com/google/devtools/build/lib/packages/Package.java

diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index 4103194..6048ca6d 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -16,6 +16,8 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
+import com.google.common.collect.BiMap;
+import com.google.common.collect.HashBiMap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -23,6 +25,7 @@
 import com.google.common.collect.Interner;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelConstants;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
@@ -55,7 +58,6 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -508,11 +510,9 @@
     return targets;
   }
 
-  /**
-   * Common getTargets implementation, accessible by {@link Package.Builder}.
-   */
-  private static Collection<Target> getTargets(Map<String, Target> targetMap) {
-    return Collections.unmodifiableCollection(targetMap.values());
+  /** Common getTargets implementation, accessible by {@link Package.Builder}. */
+  private static Set<Target> getTargets(BiMap<String, Target> targetMap) {
+    return targetMap.values();
   }
 
   /**
@@ -830,7 +830,7 @@
     private License defaultLicense = License.NO_LICENSE;
     private Set<License.DistributionType> defaultDistributionSet = License.DEFAULT_DISTRIB;
 
-    private Map<String, Target> targets = new LinkedHashMap<>();
+    private BiMap<String, Target> targets = HashBiMap.create();
     private final Map<Label, EnvironmentGroup> environmentGroups = new HashMap<>();
 
     private ImmutableList<Label> skylarkFileDependencies = ImmutableList.of();
@@ -1212,7 +1212,7 @@
       }
     }
 
-    public Collection<Target> getTargets() {
+    public Set<Target> getTargets() {
       return Package.getTargets(targets);
     }
 
@@ -1472,7 +1472,7 @@
       }
 
       // Freeze targets and distributions.
-      targets = Collections.unmodifiableMap(targets);
+      targets = Maps.unmodifiableBiMap(targets);
       defaultDistributionSet =
           Collections.unmodifiableSet(defaultDistributionSet);
 

src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index 3fee29d..de658d7 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -81,6 +81,7 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.Iterator;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
@@ -716,10 +717,11 @@
                 containingPkgLookupKeys,
                 BuildFileNotFoundException.class,
                 InconsistentFilesystemException.class);
-    if (env.valuesMissing()) {
+    if (env.valuesMissing() || containingPkgLookupKeys.isEmpty()) {
       return;
     }
-    for (Target target : ImmutableSet.copyOf(pkgBuilder.getTargets())) {
+    for (Iterator<Target> iterator = pkgBuilder.getTargets().iterator(); iterator.hasNext(); ) {
+      Target target = iterator.next();
       SkyKey key = targetToKey.get(target);
       if (!containingPkgLookupValues.containsKey(key)) {
         continue;
@@ -733,7 +735,7 @@
           target.getLabel(),
           target.getLocation(),
           containingPackageLookupValue)) {
-        pkgBuilder.removeTarget(target);
+        iterator.remove();
         pkgBuilder.setContainsErrors();
       }
     }