Allow native.existing_rule and native.existing_rules to return a ligh…

…tweight view. Experimental implementation of https://github.com/bazelbuild/proposals/blob/main/designs/2021-06-15-improving-native.existing_rules.md Gated behind the --experimental_existing_rules_immutable_view flag. See #13907 To check the performance impact, I queried a toy package that in a loop, creates a genrule and then on each loop iteration performs one of the following tasks: * checks if rule with a given name existed in `existing_rules()` ("in") * for each rule in `existing_rules()`, checks if an attribute with a given name exists ("shallow iteration") * for 50% of rules in `existing_rules()`, checks if a particular attribute value exists under any name ("50% deep iteration"); * iterates every rule name, attribute name, and attribute value ("deep iteration"); Query time in seconds without --experimental_existing_rules_immutable_view: ``` n in shallow 50%deep deep 1000 2.73 2.84 3.36 3.61 2000 8.42 9.07 10.97 11.62 4000 30.95 33.81 40.88 44.17 ``` With --experimental_existing_rules_immutable_view: ``` n in shallow 50%deep deep 1000 0.40 0.64 2.58 4.19 2000 0.43 1.08 7.96 13.64 4000 0.51 2.59 28.83 52.99 ``` In other words, in the unusual case where we examine every attribute value of every existing rule, the overhead of managing immutable views would cause a 15-20% slowdown. In any other situation, where we don't need to materialize every rule's attribute values, the immutable view approach yields a substantial performance improvement. We'd want to see if we can reduce the 15-20% worst-case penalty in followup work. RELNOTES: Adds --experimental_existing_rules_immutable_view flag to make the native.existing_rule and native.existing_rules functions more efficient by returning immutable, lightweight dict-like view objects instead of mutable dicts. PiperOrigin-RevId: 397805096
bazelbuild · Sep 20, 2021 · eb34996 · eb34996
1 parent e49f557
commit eb34996
Show file tree

Hide file tree

Showing 10 changed files with 1,752 additions and 218 deletions.
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
@@ -19,7 +19,6 @@
 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;
@@ -978,7 +977,10 @@ public boolean recordLoadedModules() {
     private License defaultLicense = License.NO_LICENSE;
     private Set<License.DistributionType> defaultDistributionSet = License.DEFAULT_DISTRIB;
 
-    private BiMap<String, Target> targets = HashBiMap.create();
+    // All targets added to the package. We use SnapshottableBiMap to help track insertion order of
+    // Rule targets, for use by native.existing_rules().
+    private BiMap<String, Target> targets =
+        new SnapshottableBiMap<>(target -> target instanceof Rule);
     private final Map<Label, EnvironmentGroup> environmentGroups = new HashMap<>();
 
     /**
@@ -1479,20 +1481,46 @@ Target getTarget(String name) {
     }
 
     /**
-     * Removes a target from the {@link Package} under construction. Intended to be used only by
-     * {@link com.google.devtools.build.lib.skyframe.PackageFunction} to remove targets whose labels
-     * cross subpackage boundaries.
+     * Replaces a target in the {@link Package} under construction with a new target with the same
+     * name and belonging to the same package.
+     *
+     * <p>A hack needed for {@link WorkspaceFactoryHelper}.
      */
-    void removeTarget(Target target) {
-      if (target.getPackage() == pkg) {
-        this.targets.remove(target.getName());
-      }
+    void replaceTarget(Target newTarget) {
+      Preconditions.checkArgument(
+          targets.containsKey(newTarget.getName()),
+          "No existing target with name '%s' in the targets map",
+          newTarget.getName());
+      Preconditions.checkArgument(
+          newTarget.getPackage() == pkg, // pointer comparison since we're constructing `pkg`
+          "Replacement target belongs to package '%s', expected '%s'",
+          newTarget.getPackage(),
+          pkg);
+      targets.put(newTarget.getName(), newTarget);
     }
 
     public Set<Target> getTargets() {
       return Package.getTargets(targets);
     }
 
+    /**
+     * Returns a lightweight snapshot view of the names of all rule targets belonging to this
+     * package at the time of this call.
+     *
+     * @throws IllegalStateException if this method is called after {@link beforeBuild} has been
+     *     called.
+     */
+    Map<String, Rule> getRulesSnapshotView() {
+      if (targets instanceof SnapshottableBiMap<?, ?>) {
+        return Maps.transformValues(
+            ((SnapshottableBiMap<String, Target>) targets).getTrackedSnapshot(),
+            target -> (Rule) target);
+      } else {
+        throw new IllegalStateException(
+            "getRulesSnapshotView() cannot be used after beforeBuild() has been called");
+      }
+    }
+
     /**
      * Returns an (immutable, unordered) view of all the targets belonging to this package which are
      * instances of the specified class.
@@ -1556,8 +1584,10 @@ void setVisibilityAndLicense(InputFile inputFile, RuleVisibility visibility, Lic
       if (!((InputFile) cacheInstance).isVisibilitySpecified()
           || cacheInstance.getVisibility() != visibility
           || !Objects.equals(cacheInstance.getLicense(), license)) {
-        targets.put(filename, new InputFile(
-            pkg, cacheInstance.getLabel(), cacheInstance.getLocation(), visibility, license));
+        targets.put(
+            filename,
+            new InputFile(
+                pkg, cacheInstance.getLabel(), cacheInstance.getLocation(), visibility, license));
       }
     }
 
@@ -1718,6 +1748,17 @@ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPack
             getPackageIdentifier(), ioExceptionMessage, ioException, ioExceptionDetailedExitCode);
       }
 
+      // SnapshottableBiMap does not allow removing targets (in order to efficiently track rule
+      // insertion order). However, we *do* need to support removal of targets in
+      // PackageFunction.handleLabelsCrossingSubpackagesAndPropagateInconsistentFilesystemExceptions
+      // which is called *between* calls to beforeBuild and finishBuild. We thus repoint the targets
+      // map to the SnapshottableBiMap's underlying bimap and thus stop tracking insertion order.
+      // After this point, snapshots of targets should no longer be used, and any further
+      // getRulesSnapshotView calls will throw.
+      if (targets instanceof SnapshottableBiMap<?, ?>) {
+        targets = ((SnapshottableBiMap<String, Target>) targets).getUnderlyingBiMap();
+      }
+
       // We create the original BUILD InputFile when the package filename is set; however, the
       // visibility may be overridden with an exports_files directive, so we need to obtain the
       // current instance here.