Skip to content

Commit

Permalink
Fix memory regression in RuleVisibility
Browse files Browse the repository at this point in the history
Benchmarks show that bazelbuild@cd82f68 introduced a memory regression. Fix it by:

* memoizing the very frequently used ["//visibility:public"] and
  ["//visibility:private"] declared labels lists
* for filtering out private labels, using a list builder with a known
  expected size instead of relying on the streams api

I also experimented with feeding the simplified visibility label lists
back into the package builder's list interner - but decided against it
because benchmarks show it results in a slightly higher memory usage.

PiperOrigin-RevId: 681511741
Change-Id: I498e4adca976834d6b137b106f9d202c793bb54d
  • Loading branch information
tetromino authored and copybara-github committed Oct 2, 2024
1 parent 25bb5f0 commit 2642004
Showing 1 changed file with 20 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,20 @@ public interface RuleVisibility {
@SerializationConstant
Label PRIVATE_LABEL = Label.parseCanonicalUnchecked("//visibility:private");

// Constant for memory efficiency; see b/370873477.
@SerializationConstant
ImmutableList<Label> PUBLIC_DECLARED_LABELS = ImmutableList.of(PUBLIC_LABEL);

// Constant for memory efficiency; see b/370873477.
@SerializationConstant
ImmutableList<Label> PRIVATE_DECLARED_LABELS = ImmutableList.of(PRIVATE_LABEL);

@SerializationConstant
RuleVisibility PUBLIC =
new RuleVisibility() {
@Override
public ImmutableList<Label> getDeclaredLabels() {
return ImmutableList.of(PUBLIC_LABEL);
return PUBLIC_DECLARED_LABELS;
}

@Override
Expand All @@ -79,7 +87,7 @@ public String toString() {
new RuleVisibility() {
@Override
public ImmutableList<Label> getDeclaredLabels() {
return ImmutableList.of(PRIVATE_LABEL);
return PRIVATE_DECLARED_LABELS;
}

@Override
Expand Down Expand Up @@ -175,17 +183,22 @@ static List<Label> validateAndSimplify(List<Label> labels) throws EvalException
}
}
if (hasPublicLabel) {
return PUBLIC.getDeclaredLabels();
return PUBLIC_DECLARED_LABELS;
}
if (numPrivateLabels == labels.size()) {
return PRIVATE.getDeclaredLabels();
return PRIVATE_DECLARED_LABELS;
}
if (numPrivateLabels == 0) {
return labels;
}
return labels.stream()
.filter(label -> !label.equals(PRIVATE_LABEL))
.collect(ImmutableList.toImmutableList());
ImmutableList.Builder<Label> withoutPrivateLabels =
ImmutableList.builderWithExpectedSize(labels.size() - numPrivateLabels);
for (Label label : labels) {
if (!label.equals(PRIVATE_LABEL)) {
withoutPrivateLabels.add(label);
}
}
return withoutPrivateLabels.build();
}

/**
Expand Down

0 comments on commit 2642004

Please sign in to comment.