Skip to content

Commit c82168e

Browse files
justinhorvitzcopybara-github
justinhorvitz
authored and
copybara-github
committed
Avoid storing LateBoundDefault attribute values in Rule.
The change that produces the desired effect is to compare the attribute value with `Attribute#getDefaultValueUnchecked`. The unchecked method returns `LateBoundDefault`, not its default value. This allows the optimization added in 1d03d53 to also work for `LateBoundDefault`. Rename `getRawAttrValue` to `getAttrIfStored` to better reflect that it does not fall back to the default value. PiperOrigin-RevId: 518720504 Change-Id: I3141a1d731aaede8ff81b46ebc819cdb6af0fa0a
1 parent 745ca28 commit c82168e

File tree

3 files changed

+86
-40
lines changed

3 files changed

+86
-40
lines changed

‎src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ private <T> void visitLabels(
115115
if (type.getLabelClass() == LabelClass.NONE) {
116116
// The only way for LabelClass.NONE to contain labels is in select keys.
117117
if (includeSelectKeys && attr.isConfigurable()) {
118-
rawVal = rule.getRawAttrValue(i);
118+
rawVal = rule.getAttrIfStored(i);
119119
if (rawVal instanceof SelectorList) {
120120
visitLabelsInSelect(
121121
(SelectorList<T>) rawVal,
@@ -128,7 +128,7 @@ private <T> void visitLabels(
128128
}
129129
return;
130130
}
131-
rawVal = rule.getRawAttrValue(i);
131+
rawVal = rule.getAttrIfStored(i);
132132
if (rawVal == null) {
133133
// Frozen rules don't store computed defaults.
134134
if (!attr.hasComputedDefault() || rule.isFrozen()) {

‎src/main/java/com/google/devtools/build/lib/packages/Rule.java

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -505,7 +505,7 @@ public <T> Object getAttr(String attrName, Type<T> type) {
505505
*/
506506
@Nullable
507507
private Object getAttrWithIndex(int attrIndex) {
508-
Object value = getRawAttrValue(attrIndex);
508+
Object value = getAttrIfStored(attrIndex);
509509
if (value != null) {
510510
return value;
511511
}
@@ -519,6 +519,11 @@ private Object getAttrWithIndex(int attrIndex) {
519519
// compute the value.
520520
return isFrozen() ? attr.getDefaultValue() : null;
521521
}
522+
if (attr.isLateBound()) {
523+
// Frozen rules don't store late bound defaults.
524+
checkState(isFrozen(), "Mutable rule missing LateBoundDefault");
525+
return attr.getLateBoundDefault();
526+
}
522527
switch (attr.getName()) {
523528
case GENERATOR_FUNCTION:
524529
return callstack.size() > 1 ? callstack.getFrame(1).name : "";
@@ -536,7 +541,7 @@ private Object getAttrWithIndex(int attrIndex) {
536541
* <p>Unlike {@link #getAttr}, does not fall back to the default value.
537542
*/
538543
@Nullable
539-
Object getRawAttrValue(int attrIndex) {
544+
Object getAttrIfStored(int attrIndex) {
540545
checkPositionIndex(attrIndex, attrCount() - 1);
541546
switch (getAttrState()) {
542547
case MUTABLE:
@@ -642,8 +647,9 @@ private void checkAttrType(String attrName, Type<?> requestedType, Attribute att
642647
* Returns {@code true} if this rule's attributes are immutable.
643648
*
644649
* <p>Frozen rules optimize for space by omitting storage for non-explicit attribute values that
645-
* match the {@link Attribute} default. If {@link #getRawAttrValue} returns {@code null}, the
646-
* value should be taken from {@link Attribute#getDefaultValue}, even for computed defaults.
650+
* match the {@link Attribute} default. If {@link #getAttrIfStored} returns {@code null}, the
651+
* value should be taken from either {@link Attribute#getLateBoundDefault} for late-bound defaults
652+
* or {@link Attribute#getDefaultValue} for all other attributes (including computed defaults).
647653
*
648654
* <p>Mutable rules have no such optimization. During rule creation, this allows for
649655
* distinguishing whether a computed default (which may depend on other unset attributes) is
@@ -667,7 +673,7 @@ void freeze() {
667673
}
668674
if (!getExplicitBit(i)) {
669675
Attribute attr = ruleClass.getAttribute(i);
670-
if (value.equals(attr.getDefaultValue())) {
676+
if (value.equals(attr.getDefaultValueUnchecked())) {
671677
// Non-explicit value matches the attribute's default. Save space by omitting storage.
672678
continue;
673679
}
@@ -774,7 +780,7 @@ public <T> BuildType.SelectorList<T> getSelectorList(String attributeName, Type<
774780
if (index == null) {
775781
return null;
776782
}
777-
Object attrValue = getRawAttrValue(index);
783+
Object attrValue = getAttrIfStored(index);
778784
if (!(attrValue instanceof BuildType.SelectorList)) {
779785
return null;
780786
}
@@ -1108,7 +1114,7 @@ private List<Label> getRawVisibilityLabels() {
11081114
if (visibilityIndex == null) {
11091115
return null;
11101116
}
1111-
return (List<Label>) getRawAttrValue(visibilityIndex);
1117+
return (List<Label>) getAttrIfStored(visibilityIndex);
11121118
}
11131119

11141120
private RuleVisibility getDefaultVisibility() {

‎src/test/java/com/google/devtools/build/lib/packages/RuleAttributeStorageTest.java

Lines changed: 71 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import com.google.devtools.build.lib.analysis.util.MockRule;
2424
import com.google.devtools.build.lib.analysis.util.MockRuleDefaults;
2525
import com.google.devtools.build.lib.packages.Attribute.ComputedDefault;
26+
import com.google.devtools.build.lib.packages.Attribute.LateBoundDefault;
2627
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
2728
import com.google.testing.junit.testparameterinjector.TestParameter;
2829
import com.google.testing.junit.testparameterinjector.TestParameterInjector;
@@ -39,6 +40,7 @@ public final class RuleAttributeStorageTest extends BuildViewTestCase {
3940

4041
private static final String STRING_DEFAULT = Type.STRING.getDefaultValue();
4142
private static final int COMPUTED_DEFAULT_OFFSET = 1;
43+
private static final int LATE_BOUND_DEFAULT_OFFSET = 2;
4244

4345
private enum ContainerSize {
4446
SMALL(16),
@@ -60,6 +62,8 @@ private enum ContainerSize {
6062
private Attribute lastCustomAttr;
6163
private int computedDefaultIndex;
6264
private Attribute computedDefaultAttr;
65+
private int lateBoundDefaultIndex;
66+
private Attribute lateBoundDefaultAttr;
6367

6468
@Override
6569
protected ConfiguredRuleClassProvider createRuleClassProvider() {
@@ -72,18 +76,29 @@ protected ConfiguredRuleClassProvider createRuleClassProvider() {
7276
IntStream.range(0, numCustomAttrs)
7377
.mapToObj(
7478
i -> {
75-
var attr = attr("attr" + i, Type.STRING);
76-
// Make one of the attributes a computed default.
79+
// Make one attribute a computed default and one a late bound default.
7780
if (i == COMPUTED_DEFAULT_OFFSET) {
78-
attr.value(
79-
new ComputedDefault() {
80-
@Override
81-
public Object getDefault(AttributeMap rule) {
82-
return "computed";
83-
}
84-
});
81+
return attr("attr" + i + "_computed_default", Type.STRING)
82+
.value(
83+
new ComputedDefault() {
84+
@Override
85+
public Object getDefault(AttributeMap rule) {
86+
return "computed";
87+
}
88+
});
8589
}
86-
return attr;
90+
if (i == LATE_BOUND_DEFAULT_OFFSET) {
91+
return attr(":attr" + i + "_late_bound_default", Type.STRING)
92+
.value(
93+
new LateBoundDefault<>(Void.class, "late_bound") {
94+
@Override
95+
public String resolve(
96+
Rule rule, AttributeMap attributes, Void input) {
97+
return "late_bound";
98+
}
99+
});
100+
}
101+
return attr("attr" + i, Type.STRING);
87102
})
88103
.toArray(Attribute.Builder[]::new));
89104
var builder = new ConfiguredRuleClassProvider.Builder().addRuleDefinition(exampleRule);
@@ -111,6 +126,8 @@ public void setUpForRule() throws Exception {
111126
lastCustomAttr = attrAt(lastCustomAttrIndex);
112127
computedDefaultIndex = firstCustomAttrIndex + COMPUTED_DEFAULT_OFFSET;
113128
computedDefaultAttr = attrAt(computedDefaultIndex);
129+
lateBoundDefaultIndex = firstCustomAttrIndex + LATE_BOUND_DEFAULT_OFFSET;
130+
lateBoundDefaultAttr = attrAt(lateBoundDefaultIndex);
114131
}
115132

116133
@Test
@@ -122,9 +139,9 @@ public void attributeSettingAndRetrieval(@TestParameter boolean frozen) {
122139
rule.freeze();
123140
}
124141

125-
assertThat(rule.getRawAttrValue(firstCustomAttrIndex)).isEqualTo("val1");
142+
assertThat(rule.getAttrIfStored(firstCustomAttrIndex)).isEqualTo("val1");
126143
assertThat(rule.isAttributeValueExplicitlySpecified(firstCustomAttr)).isTrue();
127-
assertThat(rule.getRawAttrValue(lastCustomAttrIndex)).isEqualTo("val2");
144+
assertThat(rule.getAttrIfStored(lastCustomAttrIndex)).isEqualTo("val2");
128145
assertThat(rule.isAttributeValueExplicitlySpecified(lastCustomAttr)).isTrue();
129146
}
130147

@@ -134,7 +151,7 @@ public void indexOutOfBounds_throws(@TestParameter boolean frozen) {
134151
rule.freeze();
135152
}
136153
assertThrows(
137-
IndexOutOfBoundsException.class, () -> rule.getRawAttrValue(lastCustomAttrIndex + 1));
154+
IndexOutOfBoundsException.class, () -> rule.getAttrIfStored(lastCustomAttrIndex + 1));
138155
}
139156

140157
@Test
@@ -146,10 +163,10 @@ public void testForOffByOneError(@TestParameter boolean frozen) {
146163
rule.freeze();
147164
}
148165

149-
assertThat(rule.getRawAttrValue(firstCustomAttrIndex - 1)).isNull();
166+
assertThat(rule.getAttrIfStored(firstCustomAttrIndex - 1)).isNull();
150167
assertThat(rule.isAttributeValueExplicitlySpecified(attrAt(firstCustomAttrIndex - 1)))
151168
.isFalse();
152-
assertThat(rule.getRawAttrValue(firstCustomAttrIndex + 1)).isNull();
169+
assertThat(rule.getAttrIfStored(firstCustomAttrIndex + 1)).isNull();
153170
assertThat(rule.isAttributeValueExplicitlySpecified(attrAt(firstCustomAttrIndex + 1)))
154171
.isFalse();
155172
}
@@ -166,9 +183,9 @@ public void testFreezeWorks() {
166183
// Double freezing is a no-op
167184
rule.freeze();
168185
// reads/explicit bits work as expected
169-
assertThat(rule.getRawAttrValue(firstCustomAttrIndex)).isEqualTo("val1");
186+
assertThat(rule.getAttrIfStored(firstCustomAttrIndex)).isEqualTo("val1");
170187
assertThat(rule.isAttributeValueExplicitlySpecified(firstCustomAttr)).isTrue();
171-
assertThat(rule.getRawAttrValue(lastCustomAttrIndex)).isEqualTo("val2");
188+
assertThat(rule.getAttrIfStored(lastCustomAttrIndex)).isEqualTo("val2");
172189
assertThat(rule.isAttributeValueExplicitlySpecified(lastCustomAttr)).isFalse();
173190
// writes no longer work.
174191
assertThrows(
@@ -189,7 +206,7 @@ public void allAttributesSet(@TestParameter boolean frozen) {
189206
}
190207

191208
for (int i = 1; i < size; i++) { // Skip attribute 0 (name) which is never stored.
192-
assertThat(rule.getRawAttrValue(i)).isEqualTo("value " + i);
209+
assertThat(rule.getAttrIfStored(i)).isEqualTo("value " + i);
193210
assertWithMessage("attribute " + i)
194211
.that(rule.isAttributeValueExplicitlySpecified(attrAt(i)))
195212
.isEqualTo(i % 2 == 0);
@@ -235,7 +252,7 @@ public void boundaryOfFrozenContainer() {
235252

236253
assertThat(rule.getAttr("name")).isEqualTo(ruleName);
237254
assertThat(rule.isAttributeValueExplicitlySpecified("name")).isTrue();
238-
assertThat(rule.getRawAttrValue(lastCustomAttrIndex)).isEqualTo("last");
255+
assertThat(rule.getAttrIfStored(lastCustomAttrIndex)).isEqualTo("last");
239256
assertThat(rule.isAttributeValueExplicitlySpecified(lastCustomAttr)).isTrue();
240257
}
241258

@@ -248,7 +265,7 @@ public void nameNotStoredAsRawAttr(@TestParameter boolean frozen) {
248265
rule.freeze();
249266
}
250267

251-
assertThat(rule.getRawAttrValue(0)).isNull();
268+
assertThat(rule.getAttrIfStored(0)).isNull();
252269
assertThat(rule.getRawAttrValues()).doesNotContain(ruleName);
253270
assertThat(rule.getAttr("name")).isEqualTo(ruleName);
254271
assertThat(rule.isAttributeValueExplicitlySpecified("name")).isTrue();
@@ -262,15 +279,15 @@ public void explicitDefaultValue_stored(@TestParameter boolean frozen) {
262279
rule.freeze();
263280
}
264281

265-
assertThat(rule.getRawAttrValue(firstCustomAttrIndex)).isNotNull();
282+
assertThat(rule.getAttrIfStored(firstCustomAttrIndex)).isNotNull();
266283
assertThat(rule.isAttributeValueExplicitlySpecified(firstCustomAttr)).isTrue();
267284
}
268285

269286
@Test
270287
public void nonExplicitDefaultValue_mutable_stored() {
271288
rule.setAttributeValue(firstCustomAttr, STRING_DEFAULT, /* explicit= */ false);
272289

273-
assertThat(rule.getRawAttrValue(firstCustomAttrIndex)).isNotNull();
290+
assertThat(rule.getAttrIfStored(firstCustomAttrIndex)).isNotNull();
274291
assertThat(rule.isAttributeValueExplicitlySpecified(firstCustomAttr)).isFalse();
275292
}
276293

@@ -280,37 +297,60 @@ public void nonExplicitDefaultValue_frozen_notStored() {
280297

281298
rule.freeze();
282299

283-
assertThat(rule.getRawAttrValue(firstCustomAttrIndex)).isNull();
300+
assertThat(rule.getAttrIfStored(firstCustomAttrIndex)).isNull();
284301
assertThat(rule.isAttributeValueExplicitlySpecified(firstCustomAttr)).isFalse();
285302
}
286303

287304
@Test
288305
public void computedDefault_mutable_stored() {
289-
Attribute attr = rule.getRuleClassObject().getAttribute(computedDefaultIndex);
290-
var computedDefault = attr.getDefaultValue();
291-
assertThat(attr.hasComputedDefault()).isTrue();
306+
var computedDefault = computedDefaultAttr.getDefaultValue();
307+
assertThat(computedDefaultAttr.hasComputedDefault()).isTrue();
292308
assertThat(computedDefault).isInstanceOf(ComputedDefault.class);
293309

294310
rule.setAttributeValue(computedDefaultAttr, computedDefault, /* explicit= */ false);
295311

296-
assertThat(rule.getRawAttrValue(computedDefaultIndex)).isEqualTo(computedDefault);
312+
assertThat(rule.getAttrIfStored(computedDefaultIndex)).isEqualTo(computedDefault);
313+
assertThat(rule.getAttr(computedDefaultAttr.getName())).isEqualTo(computedDefault);
297314
assertThat(rule.isAttributeValueExplicitlySpecified(computedDefaultAttr)).isFalse();
298315
}
299316

300317
@Test
301318
public void computedDefault_frozen_notStored() {
302-
Attribute attr = rule.getRuleClassObject().getAttribute(computedDefaultIndex);
303-
var computedDefault = attr.getDefaultValue();
304-
assertThat(attr.hasComputedDefault()).isTrue();
319+
var computedDefault = computedDefaultAttr.getDefaultValue();
320+
assertThat(computedDefaultAttr.hasComputedDefault()).isTrue();
305321
assertThat(computedDefault).isInstanceOf(ComputedDefault.class);
306322

307323
rule.setAttributeValue(computedDefaultAttr, computedDefault, /* explicit= */ false);
308324
rule.freeze();
309325

310-
assertThat(rule.getRawAttrValue(computedDefaultIndex)).isNull();
326+
assertThat(rule.getAttrIfStored(computedDefaultIndex)).isNull();
327+
assertThat(rule.getAttr(computedDefaultAttr.getName())).isEqualTo(computedDefault);
311328
assertThat(rule.isAttributeValueExplicitlySpecified(computedDefaultAttr)).isFalse();
312329
}
313330

331+
@Test
332+
public void lateBoundDefault_mutable_stored() {
333+
var lateBoundDefault = lateBoundDefaultAttr.getLateBoundDefault();
334+
335+
rule.setAttributeValue(lateBoundDefaultAttr, lateBoundDefault, /* explicit= */ false);
336+
337+
assertThat(rule.getAttrIfStored(lateBoundDefaultIndex)).isEqualTo(lateBoundDefault);
338+
assertThat(rule.getAttr(lateBoundDefaultAttr.getName())).isEqualTo(lateBoundDefault);
339+
assertThat(rule.isAttributeValueExplicitlySpecified(lateBoundDefaultAttr)).isFalse();
340+
}
341+
342+
@Test
343+
public void lateBoundDefault_frozen_notStored() {
344+
var lateBoundDefault = lateBoundDefaultAttr.getLateBoundDefault();
345+
346+
rule.setAttributeValue(lateBoundDefaultAttr, lateBoundDefault, /* explicit= */ false);
347+
rule.freeze();
348+
349+
assertThat(rule.getAttrIfStored(lateBoundDefaultIndex)).isNull();
350+
assertThat(rule.getAttr(lateBoundDefaultAttr.getName())).isEqualTo(lateBoundDefault);
351+
assertThat(rule.isAttributeValueExplicitlySpecified(lateBoundDefaultAttr)).isFalse();
352+
}
353+
314354
private Attribute attrAt(int attrIndex) {
315355
return rule.getRuleClassObject().getAttribute(attrIndex);
316356
}

0 commit comments

Comments
 (0)