Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Switch to bespoke attribute name iterator to improve --experimental_e…
…xisting_rules_immutable_view performance

The initial implementation of
https://github.com/bazelbuild/proposals/blob/main/designs/2021-06-15-improving-native.existing_rules.md
was a performance win in most scenarios, but with one notable exception: it was
slower on packages with "deep iteration" (a package that repeatedly calls
`native.existing_rules` to examine every rule name, every attribute name, and
every attribute value).

To my surprise, profiler results showed that most of the slowdown came from
iterating rule attribute names in ExistingRuleView - specifically, from the
stream().map(...).filter(...).iterator() construction. Switching to a bespoke
iterator improves performance considerably.

Mean loading phase wall times for a 4000-target package with "deep iteration":

* --noexperimental_existing_rules_immutable_view: 45.85 s
* --experimental_existing_rules_immutable_view before this change: 52.77 s
* --experimental_existing_rules_immutable_view after this change: 44.86 s

With this change, `--experimental_existing_rules_immutable_view` should be a
performance win in all situations.

PiperOrigin-RevId: 401629667
  • Loading branch information
tetromino authored and Copybara-Service committed Oct 7, 2021
1 parent 541669b commit dec8b5a
Showing 1 changed file with 51 additions and 16 deletions.
Expand Up @@ -22,6 +22,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterators;
import com.google.common.collect.UnmodifiableIterator;
import com.google.common.flogger.GoogleLogger;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
Expand All @@ -42,6 +43,7 @@
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.NoSuchElementException;
import javax.annotation.Nullable;
import net.starlark.java.annot.Param;
import net.starlark.java.annot.StarlarkMethod;
Expand Down Expand Up @@ -276,22 +278,55 @@ public Object get(Object key, @Nullable Object defaultValue) throws EvalExceptio
public Iterator<String> iterator() {
return Iterators.concat(
ImmutableList.of("name", "kind").iterator(),
rule.getAttributes().stream()
.map(Attribute::getName)
.filter(
attributeName -> {
switch (attributeName) {
case "name":
case "kind":
// handled specially
return false;
default:
return isPotentiallyExportableAttribute(
rule.getRuleClassObject(), attributeName)
&& isPotentiallyStarlarkifiableValue(rule.getAttr(attributeName));
}
})
.iterator());
// Compared to using stream().map(...).filter(...).iterator(), this bespoke iterator
// reduces loading time by 15% for a 4000-target package making heavy use of
// `native.existing_rules`.
new UnmodifiableIterator<String>() {
private final Iterator<Attribute> attributes = rule.getAttributes().iterator();
@Nullable private String nextRelevantAttributeName;

private boolean isRelevant(String attributeName) {
switch (attributeName) {
case "name":
case "kind":
// pseudo-names handled specially
return false;
default:
return isPotentiallyExportableAttribute(rule.getRuleClassObject(), attributeName)
&& isPotentiallyStarlarkifiableValue(rule.getAttr(attributeName));
}
}

private void findNextRelevantName() {
if (nextRelevantAttributeName == null) {
while (attributes.hasNext()) {
String attributeName = attributes.next().getName();
if (isRelevant(attributeName)) {
nextRelevantAttributeName = attributeName;
break;
}
}
}
}

@Override
public boolean hasNext() {
findNextRelevantName();
return nextRelevantAttributeName != null;
}

@Override
public String next() {
findNextRelevantName();
if (nextRelevantAttributeName != null) {
String attributeName = nextRelevantAttributeName;
nextRelevantAttributeName = null;
return attributeName;
} else {
throw new NoSuchElementException();
}
}
});
}

@Override
Expand Down

0 comments on commit dec8b5a

Please sign in to comment.