Skip to content

Commit

Permalink
Cache a soft reference to the reconstituted BuildOptions in OptionsDi…
Browse files Browse the repository at this point in the history 
…ffForReconstruction.

This makes it so that applyDiff usually does no work and also reduces garbage creation.

Although I do not think the results of applyDiff are currently being compared with equals/hashCode anywhere, this also prevents expensive re-initialization of fingerprint/hashCode if they ever are.

PiperOrigin-RevId: 245148847
  • Loading branch information
@copybara-github
Googler authored and copybara-github committed Apr 25, 2019
1 parent 916f703 commit 6ddc622
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 25 deletions.
44 changes: 36 additions & 8 deletions src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import com.google.protobuf.CodedOutputStream;
import java.io.IOException;
import java.io.Serializable;
import java.lang.ref.SoftReference;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
Expand Down Expand Up @@ -307,7 +308,11 @@ public BuildOptions applyDiff(OptionsDiffForReconstruction optionsDiff) {
}
maybeInitializeFingerprintAndHashCode();
if (!Arrays.equals(fingerprint, optionsDiff.baseFingerprint)) {
throw new IllegalArgumentException("Can not reconstruct BuildOptions with a different base.");
throw new IllegalArgumentException("Cannot reconstruct BuildOptions with a different base.");
}
BuildOptions reconstructedOptions = optionsDiff.cachedReconstructed.get();
if (reconstructedOptions != null) {
return reconstructedOptions;
}
Builder builder = builder();
for (FragmentOptions options : fragmentOptionsMap.values()) {
Expand All @@ -332,8 +337,9 @@ public BuildOptions applyDiff(OptionsDiffForReconstruction optionsDiff) {
}
}
skylarkOptions.putAll(optionsDiff.extraSecondStarlarkOptions);
builder.addStarlarkOptions(skylarkOptions);
return builder.build();
reconstructedOptions = builder.addStarlarkOptions(skylarkOptions).build();
optionsDiff.cachedReconstructed = new SoftReference<>(reconstructedOptions);
return reconstructedOptions;
}

/**
Expand Down Expand Up @@ -673,7 +679,8 @@ public static OptionsDiffForReconstruction diffForReconstruction(
second.computeChecksum(),
diff.skylarkSecond,
diff.extraStarlarkOptionsFirst,
diff.extraStarlarkOptionsSecond);
diff.extraStarlarkOptionsSecond,
second);
}

/**
Expand Down Expand Up @@ -827,7 +834,15 @@ public static final class OptionsDiffForReconstruction {
private final List<Label> extraFirstStarlarkOptions;
private final Map<Label, Object> extraSecondStarlarkOptions;

@VisibleForTesting
/**
* A soft reference to the reconstructed build options to save work and garbage creation in
* {@link #applyDiff}.
*
* <p>Promotes reuse of a single {@code BuildOptions} instance to preserve reference equality
* and limit fingerprint/hashCode initialization.
*/
private SoftReference<BuildOptions> cachedReconstructed;

public OptionsDiffForReconstruction(
Map<Class<? extends FragmentOptions>, Map<String, Object>> differingOptions,
ImmutableSet<Class<? extends FragmentOptions>> extraFirstFragmentClasses,
Expand All @@ -836,7 +851,8 @@ public OptionsDiffForReconstruction(
String checksum,
Map<Label, Object> differingStarlarkOptions,
List<Label> extraFirstStarlarkOptions,
Map<Label, Object> extraSecondStarlarkOptions) {
Map<Label, Object> extraSecondStarlarkOptions,
@Nullable BuildOptions original) {
this.differingOptions = differingOptions;
this.extraFirstFragmentClasses = extraFirstFragmentClasses;
this.extraSecondFragments = extraSecondFragments;
Expand All @@ -845,6 +861,7 @@ public OptionsDiffForReconstruction(
this.differingStarlarkOptions = differingStarlarkOptions;
this.extraFirstStarlarkOptions = extraFirstStarlarkOptions;
this.extraSecondStarlarkOptions = extraSecondStarlarkOptions;
this.cachedReconstructed = new SoftReference<>(original);
}

private static OptionsDiffForReconstruction getEmpty(byte[] baseFingerprint, String checksum) {
Expand All @@ -856,7 +873,8 @@ private static OptionsDiffForReconstruction getEmpty(byte[] baseFingerprint, Str
checksum,
ImmutableMap.of(),
ImmutableList.of(),
ImmutableMap.of());
ImmutableMap.of(),
/*original=*/ null);
}

@Nullable
Expand Down Expand Up @@ -970,6 +988,15 @@ public static ConfigurationComparer.Result compareFragments(
}
}

/**
* Clears {@link #cachedReconstructed} so that tests can cover the core logic of {@link
* #applyDiff}.
*/
@VisibleForTesting
void clearCachedReconstructedForTesting() {
cachedReconstructed = new SoftReference<>(null);
}

private boolean hasChangeToStarlarkOptionUnchangedIn(OptionsDiffForReconstruction that) {
Set<Label> starlarkOptionsChangedOrRemovedInThat =
Sets.union(
Expand Down Expand Up @@ -1133,7 +1160,8 @@ public OptionsDiffForReconstruction deserialize(
checksum,
differingStarlarkOptions,
extraFirstStarlarkOptions,
extraSecondStarlarkOptions);
extraSecondStarlarkOptions,
/*original=*/ null);
cache.putBytesFromOptionsDiff(diff, bytes);
}
return diff;
Expand Down
47 changes: 30 additions & 17 deletions src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
* skylark options, the format of this test class will need to accommodate that overlap.
*/
@RunWith(JUnit4.class)
public class BuildOptionsTest {
public final class BuildOptionsTest {
private static final ImmutableList<Class<? extends FragmentOptions>> BUILD_CONFIG_OPTIONS =
ImmutableList.of(BuildConfiguration.Options.class);

Expand Down Expand Up @@ -87,8 +87,7 @@ public void optionsEquality() throws Exception {
BuildOptions.of(BUILD_CONFIG_OPTIONS, options1)
.equals(
BuildOptions.of(
ImmutableList.<Class<? extends FragmentOptions>>of(
BuildConfiguration.Options.class, CppOptions.class),
ImmutableList.of(BuildConfiguration.Options.class, CppOptions.class),
options1)))
.isFalse();
}
Expand All @@ -112,8 +111,7 @@ public void optionsDiff() throws Exception {

@Test
public void optionsDiff_differentFragments() throws Exception {
BuildOptions one =
BuildOptions.of(ImmutableList.<Class<? extends FragmentOptions>>of(CppOptions.class));
BuildOptions one = BuildOptions.of(ImmutableList.of(CppOptions.class));
BuildOptions two = BuildOptions.of(BUILD_CONFIG_OPTIONS);

OptionsDiff diff = BuildOptions.diff(one, two);
Expand Down Expand Up @@ -160,7 +158,7 @@ public void optionsDiff_differentBaseThrowException() throws Exception {
assertThrows(IllegalArgumentException.class, () -> three.applyDiff(diffForReconstruction));
assertThat(e)
.hasMessageThat()
.contains("Can not reconstruct BuildOptions with a different base");
.contains("Cannot reconstruct BuildOptions with a different base");
}

@Test
Expand All @@ -177,7 +175,7 @@ public void optionsDiff_getEmptyAndApplyEmpty() throws Exception {
public void applyDiff_nativeOptions() throws Exception {
BuildOptions one = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=opt", "cpu=k8");
BuildOptions two = BuildOptions.of(BUILD_CONFIG_OPTIONS, "--compilation_mode=dbg", "cpu=k8");
BuildOptions reconstructedTwo = one.applyDiff(BuildOptions.diffForReconstruction(one, two));
BuildOptions reconstructedTwo = one.applyDiff(uncachedDiffForReconstruction(one, two));
assertThat(reconstructedTwo).isEqualTo(two);
assertThat(reconstructedTwo).isNotSameAs(two);
BuildOptions reconstructedOne = one.applyDiff(BuildOptions.diffForReconstruction(one, one));
Expand All @@ -190,7 +188,7 @@ public void applyDiff_nativeOptions() throws Exception {
}

@Test
public void optionsDiff_sameStarlarkOptions() throws Exception {
public void optionsDiff_sameStarlarkOptions() {
Label flagName = Label.parseAbsoluteUnchecked("//foo/flag");
String flagValue = "value";
BuildOptions one = BuildOptions.of(ImmutableMap.of(flagName, flagValue));
Expand All @@ -200,7 +198,7 @@ public void optionsDiff_sameStarlarkOptions() throws Exception {
}

@Test
public void optionsDiff_differentStarlarkOptions() throws Exception {
public void optionsDiff_differentStarlarkOptions() {
Label flagName = Label.parseAbsoluteUnchecked("//bar/flag");
String flagValueOne = "valueOne";
String flagValueTwo = "valueTwo";
Expand All @@ -218,7 +216,7 @@ public void optionsDiff_differentStarlarkOptions() throws Exception {
}

@Test
public void optionsDiff_extraStarlarkOptions() throws Exception {
public void optionsDiff_extraStarlarkOptions() {
Label flagNameOne = Label.parseAbsoluteUnchecked("//extra/flag/one");
Label flagNameTwo = Label.parseAbsoluteUnchecked("//extra/flag/two");
String flagValue = "foo";
Expand All @@ -234,13 +232,13 @@ public void optionsDiff_extraStarlarkOptions() throws Exception {
}

@Test
public void applyDiff_sameStarlarkOptions() throws Exception {
public void applyDiff_sameStarlarkOptions() {
Label flagName = Label.parseAbsoluteUnchecked("//foo/flag");
String flagValue = "value";
BuildOptions one = BuildOptions.of(ImmutableMap.of(flagName, flagValue));
BuildOptions two = BuildOptions.of(ImmutableMap.of(flagName, flagValue));

BuildOptions reconstructedTwo = one.applyDiff(BuildOptions.diffForReconstruction(one, two));
BuildOptions reconstructedTwo = one.applyDiff(uncachedDiffForReconstruction(one, two));

assertThat(reconstructedTwo).isEqualTo(two);
assertThat(reconstructedTwo).isNotSameAs(two);
Expand All @@ -251,33 +249,41 @@ public void applyDiff_sameStarlarkOptions() throws Exception {
}

@Test
public void applyDiff_differentStarlarkOptions() throws Exception {
public void applyDiff_differentStarlarkOptions() {
Label flagName = Label.parseAbsoluteUnchecked("//bar/flag");
String flagValueOne = "valueOne";
String flagValueTwo = "valueTwo";
BuildOptions one = BuildOptions.of(ImmutableMap.of(flagName, flagValueOne));
BuildOptions two = BuildOptions.of(ImmutableMap.of(flagName, flagValueTwo));

BuildOptions reconstructedTwo = one.applyDiff(BuildOptions.diffForReconstruction(one, two));
BuildOptions reconstructedTwo = one.applyDiff(uncachedDiffForReconstruction(one, two));

assertThat(reconstructedTwo).isEqualTo(two);
assertThat(reconstructedTwo).isNotSameAs(two);
}

@Test
public void applyDiff_extraStarlarkOptions() throws Exception {
public void applyDiff_extraStarlarkOptions() {
Label flagNameOne = Label.parseAbsoluteUnchecked("//extra/flag/one");
Label flagNameTwo = Label.parseAbsoluteUnchecked("//extra/flag/two");
String flagValue = "foo";
BuildOptions one = BuildOptions.of(ImmutableMap.of(flagNameOne, flagValue));
BuildOptions two = BuildOptions.of(ImmutableMap.of(flagNameTwo, flagValue));

BuildOptions reconstructedTwo = one.applyDiff(BuildOptions.diffForReconstruction(one, two));
BuildOptions reconstructedTwo = one.applyDiff(uncachedDiffForReconstruction(one, two));

assertThat(reconstructedTwo).isEqualTo(two);
assertThat(reconstructedTwo).isNotSameAs(two);
}

@Test
public void applyDiff_returnsOriginalOptionsInstanceIfStillExists() throws Exception {
BuildOptions base = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=a");
BuildOptions original = BuildOptions.of(ImmutableList.of(CppOptions.class), "--compiler=b");
BuildOptions reconstructed = base.applyDiff(BuildOptions.diffForReconstruction(base, original));
assertThat(reconstructed).isSameAs(original);
}

private static ImmutableList.Builder<Class<? extends FragmentOptions>> makeOptionsClassBuilder() {
return ImmutableList.<Class<? extends FragmentOptions>>builder()
.addAll(BUILD_CONFIG_OPTIONS)
Expand Down Expand Up @@ -334,7 +340,7 @@ public void repeatedCodec() throws Exception {
}

@Test
public void testMultiValueOptionImmutability() throws Exception {
public void testMultiValueOptionImmutability() {
BuildOptions options =
BuildOptions.of(BUILD_CONFIG_OPTIONS, OptionsParser.newOptionsParser(BUILD_CONFIG_OPTIONS));
BuildConfiguration.Options coreOptions = options.get(Options.class);
Expand Down Expand Up @@ -532,4 +538,11 @@ public void trim_nothingTrimmed_returnsSameInstance() throws Exception {
BuildOptions trimmed = original.trim(ImmutableSet.of(CppOptions.class, JavaOptions.class));
assertThat(trimmed).isSameAs(original);
}

private static OptionsDiffForReconstruction uncachedDiffForReconstruction(
BuildOptions first, BuildOptions second) {
OptionsDiffForReconstruction diff = BuildOptions.diffForReconstruction(first, second);
diff.clearCachedReconstructedForTesting();
return diff;
}
}

0 comments on commit 6ddc622

Please sign in to comment.