Add value sharing BuildOptions codec.

This codec doesn't use the `MapBackedChecksumCache`, so a fresh Bazel instance can still deserialize `BuildOptions` from a remote sharing cache. Also enroll `java.util.KeyValueHolder` to `DynamicCodec`. A test was serializing analysis objects, which indirectly had a dependency on `KeyValueHolder` through `CoreOptions`. PiperOrigin-RevId: 670913722 Change-Id: I8c5e1c65d46c3b34e136473132bf6a5fa043d804
bazelbuild · Sep 4, 2024 · 5001fdb · 5001fdb
1 parent 6324cd2
commit 5001fdb
Show file tree

Hide file tree

Showing 6 changed files with 105 additions and 8 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
@@ -16,6 +16,7 @@
 
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.collect.ImmutableMap.toImmutableMap;
+import static com.google.devtools.build.lib.skyframe.serialization.ImmutableMapCodecs.IMMUTABLE_MAP_CODEC;
 import static com.google.devtools.build.lib.skyframe.serialization.strings.UnsafeStringCodec.stringCodec;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -28,9 +29,12 @@
 import com.google.common.collect.MapDifference;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.skyframe.serialization.AsyncDeserializationContext;
+import com.google.devtools.build.lib.skyframe.serialization.DeferredObjectCodec;
 import com.google.devtools.build.lib.skyframe.serialization.LeafDeserializationContext;
 import com.google.devtools.build.lib.skyframe.serialization.LeafObjectCodec;
 import com.google.devtools.build.lib.skyframe.serialization.LeafSerializationContext;
+import com.google.devtools.build.lib.skyframe.serialization.SerializationContext;
 import com.google.devtools.build.lib.skyframe.serialization.SerializationException;
 import com.google.devtools.build.lib.skyframe.serialization.VisibleForSerialization;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
@@ -420,6 +424,81 @@ public BuildOptions build() {
     private Builder() {}
   }
 
+  public static ValueSharingCodec valueSharingCodec() {
+    return ValueSharingCodec.INSTANCE;
+  }
+
+  /**
+   * A value sharing codec for BuildOptions that does not rely on an OptionsChecksumCache.
+   *
+   * <p>This allows the BuildOptions object to be serialized remotely, and fetched with a new
+   * instance without relying on an existing local primed cache.
+   */
+  private static final class ValueSharingCodec extends DeferredObjectCodec<BuildOptions> {
+    private static final ValueSharingCodec INSTANCE = new ValueSharingCodec();
+
+    @Override
+    public boolean autoRegister() {
+      return false;
+    }
+
+    @Override
+    public Class<BuildOptions> getEncodedClass() {
+      return BuildOptions.class;
+    }
+
+    @Override
+    public void serialize(
+        SerializationContext context, BuildOptions options, CodedOutputStream codedOut)
+        throws SerializationException, IOException {
+      context.putSharedValue(options.fragmentOptionsMap, null, IMMUTABLE_MAP_CODEC, codedOut);
+      context.putSharedValue(options.starlarkOptionsMap, null, IMMUTABLE_MAP_CODEC, codedOut);
+    }
+
+    @Override
+    public DeferredValue<? extends BuildOptions> deserializeDeferred(
+        AsyncDeserializationContext context, CodedInputStream codedIn)
+        throws SerializationException, IOException {
+      var builder = new DeserializationBuilder();
+      context.getSharedValue(
+          codedIn,
+          null,
+          IMMUTABLE_MAP_CODEC,
+          builder,
+          DeserializationBuilder::setFragmentOptionsMap);
+      context.getSharedValue(
+          codedIn,
+          null,
+          IMMUTABLE_MAP_CODEC,
+          builder,
+          DeserializationBuilder::setStarlarkOptionsMap);
+      return builder;
+    }
+
+    private static final class DeserializationBuilder
+        implements DeferredObjectCodec.DeferredValue<BuildOptions> {
+
+      ImmutableMap<Class<? extends FragmentOptions>, FragmentOptions> fragmentOptionsMap;
+      ImmutableMap<Label, Object> starlarkOptionsMap;
+
+      @Override
+      public BuildOptions call() {
+        return new BuildOptions(fragmentOptionsMap, starlarkOptionsMap);
+      }
+
+      @SuppressWarnings("unchecked")
+      private static void setFragmentOptionsMap(DeserializationBuilder builder, Object value) {
+        builder.fragmentOptionsMap =
+            (ImmutableMap<Class<? extends FragmentOptions>, FragmentOptions>) value;
+      }
+
+      @SuppressWarnings("unchecked")
+      private static void setStarlarkOptionsMap(DeserializationBuilder builder, Object value) {
+        builder.starlarkOptionsMap = (ImmutableMap<Label, Object>) value;
+      }
+    }
+  }
+
   /**
    * Codec for {@link BuildOptions}.
    *

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/AutoRegistry.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/AutoRegistry.java
@@ -77,6 +77,7 @@ private static boolean packageFilter(String name) {
           "java.util.Collections$SingletonList",
           "java.util.Collections$UnmodifiableList",
           "java.util.Collections$UnmodifiableRandomAccessList",
+          "java.util.KeyValueHolder",
           "java.util.Optional");
 
   private static final ImmutableList<Object> REFERENCE_CONSTANTS_TO_REGISTER =

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/BUILD
@@ -29,6 +29,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_provider_map_impl",
         "//src/main/java/com/google/devtools/build/lib/cmdline",

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/ImmutableMapCodecs.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/ImmutableMapCodecs.java
@@ -44,13 +44,15 @@
  * ImmutableSortedMap}, arbitrary otherwise, we avoid specifying the key type as a parameter.
  */
 @SuppressWarnings({"unchecked", "rawtypes"})
-final class ImmutableMapCodecs {
+public final class ImmutableMapCodecs {
 
   @SerializationConstant static final Comparator<?> ORDERING_NATURAL = Ordering.natural();
 
   // In practice, the natural comparator seems to always be Ordering.natural(), but be flexible.
   @SerializationConstant static final Comparator<?> COMPARATOR_NATURAL_ORDER = naturalOrder();
 
+  public static final ImmutableMapCodec IMMUTABLE_MAP_CODEC = new ImmutableMapCodec();
+
   @Keep // used reflectively
   private static class ImmutableMapCodec extends DeferredObjectCodec<ImmutableMap> {
     @Override

diff --git a/...m/google/devtools/build/lib/skyframe/serialization/SerializationRegistrySetupHelpers.java b/...m/google/devtools/build/lib/skyframe/serialization/SerializationRegistrySetupHelpers.java
@@ -21,6 +21,7 @@
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.TransitiveInfoProviderMapImpl;
+import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.OutputDirectories.OutputDirectory;
 import com.google.devtools.build.lib.analysis.test.TestConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -130,6 +131,7 @@ public static ObjectCodecRegistry.Builder addStarlarkFunctionality(
           .add(ConfiguredTargetKey.valueSharingCodec())
           .add(TransitiveInfoProviderMapImpl.valueSharingCodec())
           .add(RemoteConfiguredTargetValue.codec())
+          .add(BuildOptions.valueSharingCodec())
           .addAll(ArtifactCodecs.VALUE_SHARING_CODECS)
           .build();
 

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsTest.java
@@ -31,10 +31,11 @@
 import com.google.devtools.common.options.OptionEffectTag;
 import com.google.devtools.common.options.OptionsParser;
 import com.google.protobuf.ByteString;
+import com.google.testing.junit.testparameterinjector.TestParameter;
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
 import java.util.List;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
 
 /**
  * A test for {@link BuildOptions}.
@@ -43,7 +44,7 @@
  * types of options do not interact. In the future when we begin to migrate native options to
  * Starlark options, the format of this test class will need to accommodate that overlap.
  */
-@RunWith(JUnit4.class)
+@RunWith(TestParameterInjector.class)
 public final class BuildOptionsTest {
 
   /** Extra options for this test. */
@@ -163,18 +164,29 @@ public void optionsEquality() throws Exception {
   }
 
   @Test
-  public void serialization() throws Exception {
-    new SerializationTester(
+  public void serialization(@TestParameter boolean useSharedValues) throws Exception {
+    var tester =
+        new SerializationTester(
             BuildOptions.of(makeOptionsClassBuilder().build(), "--str_option=foo"),
             BuildOptions.of(makeOptionsClassBuilder().build(), "--str_option=bar"),
             BuildOptions.of(makeOptionsClassBuilder().add(SecondDummyTestOptions.class).build()),
             BuildOptions.of(
                 makeOptionsClassBuilder().add(SecondDummyTestOptions.class).build(),
                 "--str_option=foo",
                 "--second_str_option=baz",
-                "--another_str_option=bar"))
-        .addDependency(OptionsChecksumCache.class, new MapBackedChecksumCache())
-        .runTests();
+                "--another_str_option=bar"),
+            BuildOptions.builder()
+                .addStarlarkOption(Label.parseCanonicalUnchecked("//custom:flag"), "hello")
+                .build());
+
+    if (useSharedValues) {
+      tester.makeMemoizingAndAllowFutureBlocking(true);
+      tester.addCodec(BuildOptions.valueSharingCodec());
+    } else {
+      tester.addDependency(OptionsChecksumCache.class, new MapBackedChecksumCache());
+    }
+
+    tester.runTests();
   }
 
   private static ImmutableList.Builder<Class<? extends FragmentOptions>> makeOptionsClassBuilder() {