Root: Rather than use @AutoCodec, use a handwritten ObjectCodec that allows us to optimize for the common case of a very popular Root.
Our serialization scheme is: We are magically given a likely-to-be-popular Root instance `p` and then whenever we would serialize a Root `r`, we canonicalize `r` to `p` if they are `r.equals(p)`.
The Root#fromPath and Root#absoluteRoot methods permit creation of Root instances that are equivalent, and these methods are unfortunately very widely used in the codebase. Therefore, a serialization scheme that tries to make use of Object identity is brittle in the face of future changes.
While I'm here, delete Root#getFingerprint. An earlier commit (https://github.com/bazelbuild/bazel/commit/ce6ad29c88084fb327856149a39e2771e544cfa5) made this dead code.
RELNOTES: None
PiperOrigin-RevId: 300826154
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java b/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java
index 0e6f142..013d4bc 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java
@@ -263,6 +263,8 @@
anotherArtifact.setGeneratingActionKey(ActionsTestUtil.NULL_ACTION_LOOKUP_DATA);
new SerializationTester(artifact, anotherArtifact)
.addDependency(FileSystem.class, scratch.getFileSystem())
+ .addDependency(
+ Root.RootCodecDependencies.class, new Root.RootCodecDependencies(anotherRoot.getRoot()))
.runTests();
}
@@ -282,9 +284,13 @@
.addReferenceConstant(scratch.getFileSystem())
.setAllowDefaultCodec(true)
.build(),
- ImmutableMap.of(
- FileSystem.class, scratch.getFileSystem(),
- ArtifactResolverSupplier.class, artifactResolverSupplierForTest));
+ ImmutableMap.<Class<?>, Object>builder()
+ .put(FileSystem.class, scratch.getFileSystem())
+ .put(ArtifactResolverSupplier.class, artifactResolverSupplierForTest)
+ .put(
+ Root.RootCodecDependencies.class,
+ new Root.RootCodecDependencies(artifactRoot.getRoot()))
+ .build());
PathFragment pathFragment = PathFragment.create("src/foo.cc");
ArtifactOwner owner = new LabelArtifactOwner(Label.parseAbsoluteUnchecked("//foo:bar"));
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java b/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java
index 1715e15b..704a060 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ExecutableSymlinkActionTest.java
@@ -31,6 +31,7 @@
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.Root;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -137,6 +138,9 @@
SymlinkAction action = SymlinkAction.toExecutable(NULL_ACTION_OWNER, input, output, "progress");
new SerializationTester(action)
.addDependency(FileSystem.class, scratch.getFileSystem())
+ .addDependency(
+ Root.RootCodecDependencies.class,
+ new Root.RootCodecDependencies(Root.absoluteRoot(scratch.getFileSystem())))
.setVerificationFunction(
(in, out) -> {
SymlinkAction inAction = (SymlinkAction) in;
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkActionTest.java
index c8389b1..4c5b88a 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkActionTest.java
@@ -34,6 +34,7 @@
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.Root;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -110,6 +111,7 @@
public void testCodec() throws Exception {
new SerializationTester(action)
.addDependency(FileSystem.class, scratch.getFileSystem())
+ .addDependency(Root.RootCodecDependencies.class, new Root.RootCodecDependencies(root))
.addDependencies(SerializationDepsUtils.SERIALIZATION_DEPS_FOR_TEST)
.setVerificationFunction(
(in, out) -> {
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
index b96d5c1..597f7a9 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
@@ -289,6 +289,9 @@
true);
new SerializationTester(actionExecutionValue)
.addDependency(FileSystem.class, root.getFileSystem())
+ .addDependency(
+ Root.RootCodecDependencies.class,
+ new Root.RootCodecDependencies(Root.absoluteRoot(root.getFileSystem())))
.runTests();
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/CollectPackagesUnderDirectoryCodecTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/CollectPackagesUnderDirectoryCodecTest.java
index 0472323..94a49a1 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/CollectPackagesUnderDirectoryCodecTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/CollectPackagesUnderDirectoryCodecTest.java
@@ -20,7 +20,6 @@
import com.google.devtools.build.lib.skyframe.serialization.testutils.FsUtils;
import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationTester;
import com.google.devtools.build.lib.skyframe.serialization.testutils.TestUtils;
-import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
@@ -34,7 +33,8 @@
@Test
public void testCodec() throws Exception {
- new SerializationTester(
+ SerializationTester serializationTester =
+ new SerializationTester(
NoErrorCollectPackagesUnderDirectoryValue.EMPTY,
CollectPackagesUnderDirectoryValue.ofNoError(
true,
@@ -50,9 +50,9 @@
"my error message",
ImmutableMap.of(
rootedPath("/a", "b"), false,
- rootedPath("/c", "d"), true)))
- .addDependency(FileSystem.class, FsUtils.TEST_FILESYSTEM)
- .runTests();
+ rootedPath("/c", "d"), true)));
+ FsUtils.addDependencies(serializationTester);
+ serializationTester.runTests();
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileValueTest.java
index fad42f0..56e4acc 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FileValueTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileValueTest.java
@@ -19,7 +19,6 @@
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.skyframe.serialization.testutils.FsUtils;
import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationTester;
-import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.PathFragment;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -30,28 +29,29 @@
public class FileValueTest {
@Test
public void testCodec() throws Exception {
- new SerializationTester(
+ SerializationTester serializationTester =
+ new SerializationTester(
// Assume we have adequate coverage for FileStateValue serialization.
new FileValue.RegularFileValue(
- FsUtils.TEST_ROOT, FileStateValue.NONEXISTENT_FILE_STATE_NODE),
+ FsUtils.TEST_ROOTED_PATH, FileStateValue.NONEXISTENT_FILE_STATE_NODE),
new FileValue.DifferentRealPathFileValueWithStoredChain(
- FsUtils.TEST_ROOT,
+ FsUtils.TEST_ROOTED_PATH,
FileStateValue.DIRECTORY_FILE_STATE_NODE,
- ImmutableList.of(FsUtils.TEST_ROOT)),
+ ImmutableList.of(FsUtils.TEST_ROOTED_PATH)),
new FileValue.DifferentRealPathFileValueWithoutStoredChain(
- FsUtils.TEST_ROOT, FileStateValue.DIRECTORY_FILE_STATE_NODE),
+ FsUtils.TEST_ROOTED_PATH, FileStateValue.DIRECTORY_FILE_STATE_NODE),
new FileValue.SymlinkFileValueWithStoredChain(
- FsUtils.TEST_ROOT,
+ FsUtils.TEST_ROOTED_PATH,
new FileStateValue.RegularFileStateValue(
/*size=*/ 100, /*digest=*/ new byte[] {1, 2, 3, 4, 5}, /*contentsProxy=*/ null),
- ImmutableList.of(FsUtils.TEST_ROOT),
+ ImmutableList.of(FsUtils.TEST_ROOTED_PATH),
PathFragment.create("somewhere/else")),
new FileValue.SymlinkFileValueWithoutStoredChain(
- FsUtils.TEST_ROOT,
+ FsUtils.TEST_ROOTED_PATH,
new FileStateValue.RegularFileStateValue(
/*size=*/ 100, /*digest=*/ new byte[] {1, 2, 3, 4, 5}, /*contentsProxy=*/ null),
- PathFragment.create("somewhere/else")))
- .addDependency(FileSystem.class, FsUtils.TEST_FILESYSTEM)
- .runTests();
+ PathFragment.create("somewhere/else")));
+ FsUtils.addDependencies(serializationTester);
+ serializationTester.runTests();
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/GlobDescriptorTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/GlobDescriptorTest.java
index 3d2e4bd..7eac47f 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/GlobDescriptorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/GlobDescriptorTest.java
@@ -19,7 +19,6 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.skyframe.serialization.testutils.FsUtils;
import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationTester;
-import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import org.junit.Test;
@@ -32,22 +31,23 @@
@Test
public void testSerialization() throws Exception {
- new SerializationTester(
- GlobDescriptor.create(
- PackageIdentifier.create("@foo", PathFragment.create("//bar")),
- Root.fromPath(FsUtils.TEST_FILESYSTEM.getPath("/packageRoot")),
- PathFragment.create("subdir"),
- "pattern",
- /*excludeDirs=*/ false),
- GlobDescriptor.create(
- PackageIdentifier.create("@bar", PathFragment.create("//foo")),
- Root.fromPath(FsUtils.TEST_FILESYSTEM.getPath("/anotherPackageRoot")),
- PathFragment.create("anotherSubdir"),
- "pattern",
- /*excludeDirs=*/ true))
- .addDependency(FileSystem.class, FsUtils.TEST_FILESYSTEM)
- .setVerificationFunction(GlobDescriptorTest::verifyEquivalent)
- .runTests();
+ SerializationTester serializationTester =
+ new SerializationTester(
+ GlobDescriptor.create(
+ PackageIdentifier.create("@foo", PathFragment.create("//bar")),
+ Root.fromPath(FsUtils.TEST_FILESYSTEM.getPath("/packageRoot")),
+ PathFragment.create("subdir"),
+ "pattern",
+ /*excludeDirs=*/ false),
+ GlobDescriptor.create(
+ PackageIdentifier.create("@bar", PathFragment.create("//foo")),
+ Root.fromPath(FsUtils.TEST_FILESYSTEM.getPath("/anotherPackageRoot")),
+ PathFragment.create("anotherSubdir"),
+ "pattern",
+ /*excludeDirs=*/ true))
+ .setVerificationFunction(GlobDescriptorTest::verifyEquivalent);
+ FsUtils.addDependencies(serializationTester);
+ serializationTester.runTests();
}
private static void verifyEquivalent(GlobDescriptor orig, GlobDescriptor deserialized) {
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryKeyCodecTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryKeyCodecTest.java
index 2c80f1d..523776d 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryKeyCodecTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfTargetsUnderDirectoryKeyCodecTest.java
@@ -19,7 +19,6 @@
import com.google.devtools.build.lib.skyframe.PrepareDepsOfTargetsUnderDirectoryValue.PrepareDepsOfTargetsUnderDirectoryKey;
import com.google.devtools.build.lib.skyframe.serialization.testutils.FsUtils;
import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationTester;
-import com.google.devtools.build.lib.vfs.FileSystem;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -30,14 +29,16 @@
@Test
public void testCodec() throws Exception {
- new SerializationTester(
+ SerializationTester serializationTester =
+ new SerializationTester(
PrepareDepsOfTargetsUnderDirectoryKey.create(
new RecursivePkgKey(
RepositoryName.MAIN,
- FsUtils.TEST_ROOT,
+ FsUtils.TEST_ROOTED_PATH,
ImmutableSet.of(FsUtils.rootPathRelative("here"))),
- FilteringPolicies.and(FilteringPolicies.NO_FILTER, FilteringPolicies.FILTER_TESTS)))
- .addDependency(FileSystem.class, FsUtils.TEST_FILESYSTEM)
- .runTests();
+ FilteringPolicies.and(
+ FilteringPolicies.NO_FILTER, FilteringPolicies.FILTER_TESTS)));
+ FsUtils.addDependencies(serializationTester);
+ serializationTester.runTests();
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupKeyCodecTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupKeyCodecTest.java
index 0e84bb9..1be78b9 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupKeyCodecTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupKeyCodecTest.java
@@ -18,7 +18,6 @@
import com.google.devtools.build.lib.skyframe.SkylarkImportLookupValue.SkylarkImportLookupKey;
import com.google.devtools.build.lib.skyframe.serialization.testutils.FsUtils;
import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationTester;
-import com.google.devtools.build.lib.vfs.FileSystem;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -29,7 +28,8 @@
@Test
public void testCodec() throws Exception {
- new SerializationTester(
+ SerializationTester serializationTester =
+ new SerializationTester(
SkylarkImportLookupKey.create(
Label.parseAbsolute("//foo/bar:baz", ImmutableMap.of()), false, -1, null),
SkylarkImportLookupKey.create(
@@ -40,8 +40,8 @@
Label.parseAbsolute("//foo/bar:baz", ImmutableMap.of()),
true,
4,
- FsUtils.TEST_ROOT))
- .addDependency(FileSystem.class, FsUtils.TEST_FILESYSTEM)
- .runTests();
+ FsUtils.TEST_ROOTED_PATH));
+ FsUtils.addDependencies(serializationTester);
+ serializationTester.runTests();
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
index afe1d43..1c73180 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
@@ -65,6 +65,7 @@
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.Symlinks;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyKey;
@@ -131,6 +132,9 @@
parent.setGeneratingActionKey(ActionLookupData.create(ACTION_LOOKUP_KEY, 0));
new SerializationTester(parent, ActionInputHelper.treeFileArtifact(parent, "child"))
.addDependency(FileSystem.class, scratch.getFileSystem())
+ .addDependency(
+ Root.RootCodecDependencies.class,
+ new Root.RootCodecDependencies(Root.absoluteRoot(scratch.getFileSystem())))
.runTests();
}
diff --git a/src/test/java/com/google/devtools/build/lib/vfs/RootTest.java b/src/test/java/com/google/devtools/build/lib/vfs/RootTest.java
index 888ecfb..ace28cc 100644
--- a/src/test/java/com/google/devtools/build/lib/vfs/RootTest.java
+++ b/src/test/java/com/google/devtools/build/lib/vfs/RootTest.java
@@ -16,9 +16,13 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import com.google.common.testing.EqualsTester;
import com.google.devtools.build.lib.clock.BlazeClock;
+import com.google.devtools.build.lib.skyframe.serialization.AutoRegistry;
+import com.google.devtools.build.lib.skyframe.serialization.ObjectCodecRegistry;
+import com.google.devtools.build.lib.skyframe.serialization.ObjectCodecs;
import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationTester;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
import java.util.Comparator;
@@ -104,9 +108,56 @@
}
@Test
- public void testSerialization() throws Exception {
- new SerializationTester(Root.absoluteRoot(fs), Root.fromPath(fs.getPath("/foo")))
+ public void testSerialization_Simple() throws Exception {
+ Root fooPathRoot = Root.fromPath(fs.getPath("/foo"));
+ Root barPathRoot = Root.fromPath(fs.getPath("/bar"));
+ new SerializationTester(Root.absoluteRoot(fs), fooPathRoot, barPathRoot)
.addDependency(FileSystem.class, fs)
+ .addDependency(
+ Root.RootCodecDependencies.class,
+ new Root.RootCodecDependencies(/*likelyPopularRoot=*/ fooPathRoot))
.runTests();
}
+
+ @Test
+ public void testSerialization_LikelyPopularRootIsCanonicalized() throws Exception {
+ Root fooPathRoot = Root.fromPath(fs.getPath("/foo"));
+ Root otherFooPathRoot = Root.fromPath(fs.getPath("/foo"));
+ Root barPathRoot = Root.fromPath(fs.getPath("/bar"));
+ Root fsAabsoluteRoot = Root.absoluteRoot(fs);
+
+ assertThat(fooPathRoot).isNotSameInstanceAs(otherFooPathRoot);
+ assertThat(fooPathRoot).isEqualTo(otherFooPathRoot);
+
+ ObjectCodecRegistry registry = AutoRegistry.get();
+ ImmutableMap<Class<?>, Object> dependencies =
+ ImmutableMap.<Class<?>, Object>builder()
+ .put(FileSystem.class, fs)
+ .put(
+ Root.RootCodecDependencies.class,
+ new Root.RootCodecDependencies(/*likelyPopularRoot=*/ fooPathRoot))
+ .build();
+ ObjectCodecRegistry.Builder registryBuilder = registry.getBuilder();
+ for (Object val : dependencies.values()) {
+ registryBuilder.addReferenceConstant(val);
+ }
+ ObjectCodecs objectCodecs = new ObjectCodecs(registryBuilder.build(), dependencies);
+
+ Root fooPathRootDeserialized =
+ (Root) objectCodecs.deserialize(objectCodecs.serialize(fooPathRoot));
+ Root otherFooPathRootDeserialized =
+ (Root) objectCodecs.deserialize(objectCodecs.serialize(otherFooPathRoot));
+ assertThat(fooPathRootDeserialized).isSameInstanceAs(fooPathRoot);
+ assertThat(otherFooPathRootDeserialized).isSameInstanceAs(fooPathRoot);
+
+ Root barPathRootDeserialized =
+ (Root) objectCodecs.deserialize(objectCodecs.serialize(barPathRoot));
+ assertThat(barPathRootDeserialized).isNotSameInstanceAs(barPathRoot);
+ assertThat(barPathRootDeserialized).isEqualTo(barPathRoot);
+
+ Root fsAabsoluteRootDeserialized =
+ (Root) objectCodecs.deserialize(objectCodecs.serialize(fsAabsoluteRoot));
+ assertThat(fsAabsoluteRootDeserialized).isNotSameInstanceAs(fsAabsoluteRoot);
+ assertThat(fsAabsoluteRootDeserialized).isEqualTo(fsAabsoluteRoot);
+ }
}
