Skip to content

Commit eb34996

Browse files
tetrominocopybara-github
authored andcommitted
Allow native.existing_rule and native.existing_rules to return a lightweight 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
1 parent e49f557 commit eb34996

File tree

10 files changed

+1752
-218
lines changed

10 files changed

+1752
-218
lines changed

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

Lines changed: 52 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
import com.google.common.annotations.VisibleForTesting;
2020
import com.google.common.base.Preconditions;
2121
import com.google.common.collect.BiMap;
22-
import com.google.common.collect.HashBiMap;
2322
import com.google.common.collect.ImmutableList;
2423
import com.google.common.collect.ImmutableMap;
2524
import com.google.common.collect.ImmutableSet;
@@ -978,7 +977,10 @@ public boolean recordLoadedModules() {
978977
private License defaultLicense = License.NO_LICENSE;
979978
private Set<License.DistributionType> defaultDistributionSet = License.DEFAULT_DISTRIB;
980979

981-
private BiMap<String, Target> targets = HashBiMap.create();
980+
// All targets added to the package. We use SnapshottableBiMap to help track insertion order of
981+
// Rule targets, for use by native.existing_rules().
982+
private BiMap<String, Target> targets =
983+
new SnapshottableBiMap<>(target -> target instanceof Rule);
982984
private final Map<Label, EnvironmentGroup> environmentGroups = new HashMap<>();
983985

984986
/**
@@ -1479,20 +1481,46 @@ Target getTarget(String name) {
14791481
}
14801482

14811483
/**
1482-
* Removes a target from the {@link Package} under construction. Intended to be used only by
1483-
* {@link com.google.devtools.build.lib.skyframe.PackageFunction} to remove targets whose labels
1484-
* cross subpackage boundaries.
1484+
* Replaces a target in the {@link Package} under construction with a new target with the same
1485+
* name and belonging to the same package.
1486+
*
1487+
* <p>A hack needed for {@link WorkspaceFactoryHelper}.
14851488
*/
1486-
void removeTarget(Target target) {
1487-
if (target.getPackage() == pkg) {
1488-
this.targets.remove(target.getName());
1489-
}
1489+
void replaceTarget(Target newTarget) {
1490+
Preconditions.checkArgument(
1491+
targets.containsKey(newTarget.getName()),
1492+
"No existing target with name '%s' in the targets map",
1493+
newTarget.getName());
1494+
Preconditions.checkArgument(
1495+
newTarget.getPackage() == pkg, // pointer comparison since we're constructing `pkg`
1496+
"Replacement target belongs to package '%s', expected '%s'",
1497+
newTarget.getPackage(),
1498+
pkg);
1499+
targets.put(newTarget.getName(), newTarget);
14901500
}
14911501

14921502
public Set<Target> getTargets() {
14931503
return Package.getTargets(targets);
14941504
}
14951505

1506+
/**
1507+
* Returns a lightweight snapshot view of the names of all rule targets belonging to this
1508+
* package at the time of this call.
1509+
*
1510+
* @throws IllegalStateException if this method is called after {@link beforeBuild} has been
1511+
* called.
1512+
*/
1513+
Map<String, Rule> getRulesSnapshotView() {
1514+
if (targets instanceof SnapshottableBiMap<?, ?>) {
1515+
return Maps.transformValues(
1516+
((SnapshottableBiMap<String, Target>) targets).getTrackedSnapshot(),
1517+
target -> (Rule) target);
1518+
} else {
1519+
throw new IllegalStateException(
1520+
"getRulesSnapshotView() cannot be used after beforeBuild() has been called");
1521+
}
1522+
}
1523+
14961524
/**
14971525
* Returns an (immutable, unordered) view of all the targets belonging to this package which are
14981526
* instances of the specified class.
@@ -1556,8 +1584,10 @@ void setVisibilityAndLicense(InputFile inputFile, RuleVisibility visibility, Lic
15561584
if (!((InputFile) cacheInstance).isVisibilitySpecified()
15571585
|| cacheInstance.getVisibility() != visibility
15581586
|| !Objects.equals(cacheInstance.getLicense(), license)) {
1559-
targets.put(filename, new InputFile(
1560-
pkg, cacheInstance.getLabel(), cacheInstance.getLocation(), visibility, license));
1587+
targets.put(
1588+
filename,
1589+
new InputFile(
1590+
pkg, cacheInstance.getLabel(), cacheInstance.getLocation(), visibility, license));
15611591
}
15621592
}
15631593

@@ -1718,6 +1748,17 @@ private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPack
17181748
getPackageIdentifier(), ioExceptionMessage, ioException, ioExceptionDetailedExitCode);
17191749
}
17201750

1751+
// SnapshottableBiMap does not allow removing targets (in order to efficiently track rule
1752+
// insertion order). However, we *do* need to support removal of targets in
1753+
// PackageFunction.handleLabelsCrossingSubpackagesAndPropagateInconsistentFilesystemExceptions
1754+
// which is called *between* calls to beforeBuild and finishBuild. We thus repoint the targets
1755+
// map to the SnapshottableBiMap's underlying bimap and thus stop tracking insertion order.
1756+
// After this point, snapshots of targets should no longer be used, and any further
1757+
// getRulesSnapshotView calls will throw.
1758+
if (targets instanceof SnapshottableBiMap<?, ?>) {
1759+
targets = ((SnapshottableBiMap<String, Target>) targets).getUnderlyingBiMap();
1760+
}
1761+
17211762
// We create the original BUILD InputFile when the package filename is set; however, the
17221763
// visibility may be overridden with an exports_files directive, so we need to obtain the
17231764
// current instance here.

0 commit comments

Comments
 (0)