From 63c887b339499c5178f9535f821a3256134e1c08 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 18 Sep 2024 09:03:46 -0700 Subject: [PATCH] Use PackedFingerprint throughout serialization code. This should have less overhead than ByteString and being a custom type, it is easier to make guarantees about its properties. PiperOrigin-RevId: 676012196 Change-Id: I65afb00fea5960cf228815d7f1c1c9a31c197171 --- .../nestedset/NestedSetCodecWithStore.java | 6 +- .../NestedSetSerializationCache.java | 24 ++----- .../lib/collect/nestedset/NestedSetStore.java | 20 +++--- .../serialization/FingerprintValueCache.java | 18 ++--- .../FingerprintValueService.java | 41 +++++++---- .../serialization/FingerprintValueStore.java | 16 ++--- .../serialization/PackedFingerprint.java | 22 +++++- .../skyframe/serialization/PutOperation.java | 22 ++---- .../serialization/SerializationModule.java | 3 +- .../SharedValueDeserializationContext.java | 5 +- .../SharedValueSerializationContext.java | 15 ++-- .../serialization/SkyValueRetriever.java | 4 +- .../analysis/FrontierSerializer.java | 3 +- .../testutils/GetRecordingStore.java | 10 +-- .../collect/nestedset/NestedSetCodecTest.java | 26 +++---- .../NestedSetSerializationCacheTest.java | 69 ++++++++++--------- .../lib/collect/nestedset/NestedSetTest.java | 8 +-- .../FingerprintValueCacheTest.java | 24 +++---- .../FingerprintValueServiceTest.java | 47 ++++--------- .../SharedValueSerializationContextTest.java | 8 +-- .../serialization/SkyValueRetrieverTest.java | 10 ++- 21 files changed, 201 insertions(+), 200 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecWithStore.java b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecWithStore.java index 26acc4fa9eda00..122340f843c528 100644 --- a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecWithStore.java +++ b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecWithStore.java @@ -19,10 +19,10 @@ import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext; import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec; +import com.google.devtools.build.lib.skyframe.serialization.PackedFingerprint; import com.google.devtools.build.lib.skyframe.serialization.PutOperation; import com.google.devtools.build.lib.skyframe.serialization.SerializationContext; import com.google.devtools.build.lib.skyframe.serialization.SerializationException; -import com.google.protobuf.ByteString; import com.google.protobuf.CodedInputStream; import com.google.protobuf.CodedOutputStream; import java.io.IOException; @@ -86,7 +86,7 @@ public void serialize(SerializationContext context, NestedSet obj, CodedOutpu PutOperation fingerprintComputationResult = nestedSetStore.computeFingerprintAndStore((Object[]) obj.getChildren(), context); context.addFutureToBlockWritingOn(fingerprintComputationResult.writeStatus()); - codedOut.writeByteArrayNoTag(fingerprintComputationResult.fingerprint().toByteArray()); + fingerprintComputationResult.fingerprint().writeTo(codedOut); } interner.put(new EqualsWrapper(obj), obj); } @@ -106,7 +106,7 @@ public NestedSet deserialize(DeserializationContext context, CodedInputStream } case NONLEAF -> { int depth = codedIn.readInt32(); - ByteString fingerprint = ByteString.copyFrom(codedIn.readByteArray()); + var fingerprint = PackedFingerprint.readFrom(codedIn); return intern(order, depth, nestedSetStore.getContentsAndDeserialize(fingerprint, context)); } } diff --git a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetSerializationCache.java b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetSerializationCache.java index 9c435e36451a05..1b2adb0c2ae0d7 100644 --- a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetSerializationCache.java +++ b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetSerializationCache.java @@ -20,15 +20,14 @@ import com.github.benmanes.caffeine.cache.Cache; import com.github.benmanes.caffeine.cache.Caffeine; -import com.google.auto.value.AutoValue; import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.SettableFuture; import com.google.devtools.build.lib.bugreport.BugReporter; +import com.google.devtools.build.lib.skyframe.serialization.PackedFingerprint; import com.google.devtools.build.lib.skyframe.serialization.PutOperation; import com.google.devtools.build.lib.skyframe.serialization.SerializationConstants; -import com.google.protobuf.ByteString; import javax.annotation.Nullable; /** @@ -93,10 +92,10 @@ class NestedSetSerializationCache { */ @Nullable Object putFutureIfAbsent( - ByteString fingerprint, SettableFuture future, Object context) { + PackedFingerprint fingerprint, SettableFuture future, Object context) { checkArgument(!future.isDone(), "Must pass a fresh future: %s", future); Object existing = - fingerprintToContents.asMap().putIfAbsent(FingerprintKey.of(fingerprint, context), future); + fingerprintToContents.asMap().putIfAbsent(new FingerprintKey(fingerprint, context), future); if (existing != null) { return existing; } @@ -110,7 +109,7 @@ Object putFutureIfAbsent( * the future, when it completes. */ private void unwrapWhenDone( - ByteString fingerprint, ListenableFuture futureContents, Object context) { + PackedFingerprint fingerprint, ListenableFuture futureContents, Object context) { Futures.addCallback( futureContents, new FutureCallback() { @@ -126,7 +125,7 @@ public void onSuccess(Object[] contents) { // has no effect). var unused = putIfAbsent( - contents, PutOperation.create(fingerprint, immediateVoidFuture()), context); + contents, new PutOperation(fingerprint, immediateVoidFuture()), context); } @Override @@ -165,18 +164,9 @@ PutOperation putIfAbsent(Object[] contents, PutOperation result, Object context) if (existingResult != null) { return existingResult; } - fingerprintToContents.put(FingerprintKey.of(result.fingerprint(), context), contents); + fingerprintToContents.put(new FingerprintKey(result.fingerprint(), context), contents); return null; } - @AutoValue - abstract static class FingerprintKey { - abstract ByteString fingerprint(); - - abstract Object context(); - - static FingerprintKey of(ByteString fingerprint, Object context) { - return new AutoValue_NestedSetSerializationCache_FingerprintKey(fingerprint, context); - } - } + record FingerprintKey(PackedFingerprint fingerprint, Object context) {} } diff --git a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetStore.java b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetStore.java index 9261740d1a39a2..9ceda8ce6c111f 100644 --- a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetStore.java +++ b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetStore.java @@ -29,11 +29,11 @@ import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext; import com.google.devtools.build.lib.skyframe.serialization.FingerprintValueStore; import com.google.devtools.build.lib.skyframe.serialization.FingerprintValueStore.MissingFingerprintValueException; +import com.google.devtools.build.lib.skyframe.serialization.PackedFingerprint; import com.google.devtools.build.lib.skyframe.serialization.PutOperation; import com.google.devtools.build.lib.skyframe.serialization.SerializationContext; import com.google.devtools.build.lib.skyframe.serialization.SerializationDependencyProvider; import com.google.devtools.build.lib.skyframe.serialization.SerializationException; -import com.google.protobuf.ByteString; import com.google.protobuf.CodedInputStream; import com.google.protobuf.CodedOutputStream; import java.io.ByteArrayOutputStream; @@ -182,8 +182,9 @@ private PutOperation computeFingerprintAndStore( } byte[] serializedBytes = byteArrayOutputStream.toByteArray(); - ByteString fingerprint = - ByteString.copyFrom(Hashing.md5().hashBytes(serializedBytes).asBytes()); + // TODO: b/368012715 - reconsider use of md5. + PackedFingerprint fingerprint = + PackedFingerprint.fromBytes(Hashing.md5().hashBytes(serializedBytes).asBytes()); SettableFuture localWriteFuture = SettableFuture.create(); futureBuilder.add(localWriteFuture); @@ -196,7 +197,7 @@ private PutOperation computeFingerprintAndStore( ListenableFuture writeFuture = Futures.whenAllSucceed(futureBuilder.build()).call(() -> null, directExecutor()); - PutOperation result = PutOperation.create(fingerprint, writeFuture); + var result = new PutOperation(fingerprint, writeFuture); PutOperation existingResult = nestedSetCache.putIfAbsent(contents, result, cacheContext); if (existingResult != null) { @@ -228,7 +229,8 @@ private static ListenableFuture maybeWrapInFuture(Object contents) { * which may be completed with a {@link MissingFingerprintValueException}. */ Object getContentsAndDeserialize( - ByteString fingerprint, DeserializationContext deserializationContext) throws IOException { + PackedFingerprint fingerprint, DeserializationContext deserializationContext) + throws IOException { return getContentsAndDeserialize( fingerprint, deserializationContext, cacheContextFn.apply(deserializationContext)); } @@ -236,7 +238,9 @@ Object getContentsAndDeserialize( // All callers will test on type and check return value if it's a future. @SuppressWarnings("FutureReturnValueIgnored") private Object getContentsAndDeserialize( - ByteString fingerprint, DeserializationContext deserializationContext, Object cacheContext) + PackedFingerprint fingerprint, + DeserializationContext deserializationContext, + Object cacheContext) throws IOException { SettableFuture future = SettableFuture.create(); Object contents = nestedSetCache.putFutureIfAbsent(fingerprint, future, cacheContext); @@ -273,10 +277,10 @@ private Object getContentsAndDeserialize( ImmutableList.builderWithExpectedSize(numberOfElements); for (int i = 0; i < numberOfElements; i++) { Object deserializedElement = newDeserializationContext.deserialize(codedIn); - if (deserializedElement instanceof ByteString) { + if (deserializedElement instanceof PackedFingerprint transitiveFingerprint) { Object innerContents = getContentsAndDeserialize( - (ByteString) deserializedElement, deserializationContext, cacheContext); + transitiveFingerprint, deserializationContext, cacheContext); deserializationFutures.add(maybeWrapInFuture(innerContents)); } else { deserializationFutures.add(Futures.immediateFuture(deserializedElement)); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueCache.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueCache.java index f962beb732b0ce..27fa3532a7930e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueCache.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueCache.java @@ -22,7 +22,6 @@ import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.skyframe.serialization.FingerprintValueStore.MissingFingerprintValueException; -import com.google.protobuf.ByteString; import java.io.IOException; import java.util.concurrent.ExecutionException; import javax.annotation.Nullable; @@ -44,7 +43,8 @@ public final class FingerprintValueCache { *

Used to deduplicate fetches, or in some cases, where the object to be fetched was already * serialized, retrieves the already existing object. * - *

The keys can either be a {@link ByteString} or a {@link FingerprintWithDistinguisher}. + *

The keys can either be a {@link PackedFingerprint} or a {@link + * FingerprintWithDistinguisher}. * *

The values in this cache are always {@code Object} or {@code ListenableFuture}. We * avoid a common wrapper object both for memory efficiency and because our cache eviction policy @@ -69,7 +69,7 @@ public final class FingerprintValueCache { *
    *
  • key: the content value object, using reference equality *
  • value: either a {@code ListenableFuture} when the operation is in flight or - * a {@link ByteString} fingerprint when it is complete + * a {@link PackedFingerprint} fingerprint when it is complete *
* *

{@code ListenableFuture} contains two distinct asynchronous operations. @@ -132,7 +132,7 @@ public FingerprintValueCache(SyncMode mode) { * *

    *
  • a {@code ListenableFuture} if it is still in flight; or - *
  • a {@link ByteString} fingerprint if writing to remote storage is successful. + *
  • a {@link PackedFingerprint} fingerprint if writing to remote storage is successful. *
* *

If a {@code ListenableFuture} is returned, its expected {@link @@ -185,7 +185,7 @@ Object getOrClaimPutOperation( */ @Nullable Object getOrClaimGetOperation( - ByteString fingerprint, + PackedFingerprint fingerprint, @Nullable Object distinguisher, ListenableFuture getOperation) { Object key = createKey(fingerprint, distinguisher); @@ -244,7 +244,7 @@ public void onFailure(Throwable t) { /** Unwraps the future and populates the reverse mapping when done. */ private void unwrapValueWhenDone( - ByteString fingerprint, Object key, ListenableFuture getOperation) { + PackedFingerprint fingerprint, Object key, ListenableFuture getOperation) { Futures.addCallback( getOperation, new FutureCallback() { @@ -265,7 +265,7 @@ public void onFailure(Throwable t) { directExecutor()); } - private static Object createKey(ByteString fingerprint, @Nullable Object distinguisher) { + private static Object createKey(PackedFingerprint fingerprint, @Nullable Object distinguisher) { if (distinguisher == null) { return fingerprint; } @@ -289,12 +289,12 @@ private static Object createKey(ByteString fingerprint, @Nullable Object disting @AutoValue abstract static class FingerprintWithDistinguisher { /** The primary key for a {@link #deserializationCache} entry. */ - abstract ByteString fingerprint(); + abstract PackedFingerprint fingerprint(); /** A secondary key, sometimes needed to resolve ambiguity. */ abstract Object distinguisher(); - static FingerprintWithDistinguisher of(ByteString fingerprint, Object distinguisher) { + static FingerprintWithDistinguisher of(PackedFingerprint fingerprint, Object distinguisher) { return new AutoValue_FingerprintValueCache_FingerprintWithDistinguisher( fingerprint, distinguisher); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueService.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueService.java index eaddde287684c9..4cfbf4e6931266 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueService.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueService.java @@ -13,11 +13,10 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe.serialization; +import static com.google.common.hash.Hashing.murmur3_128; import static java.util.concurrent.Executors.newSingleThreadExecutor; import com.google.common.annotations.VisibleForTesting; -import com.google.common.hash.HashFunction; -import com.google.common.hash.Hashing; import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.common.options.OptionsParsingResult; import com.google.protobuf.ByteString; @@ -38,6 +37,15 @@ public interface Factory { FingerprintValueService create(OptionsParsingResult options); } + /** Injectable implementation of the fingerprint function. */ + public interface Fingerprinter { + PackedFingerprint fingerprint(byte[] input); + } + + /** A {@link Fingerprinter} implementation for non-production use. */ + public static final Fingerprinter NONPROD_FINGERPRINTER = + input -> PackedFingerprint.fromBytes(murmur3_128().hashBytes(input).asBytes()); + private final Executor executor; private final FingerprintValueStore store; private final FingerprintValueCache cache; @@ -47,9 +55,9 @@ public interface Factory { * *

Used to derive {@link #fingerprintPlaceholder} and {@link #fingerprintLength}. */ - private final HashFunction hashFunction; + private final Fingerprinter fingerprinter; - private final ByteString fingerprintPlaceholder; + private final PackedFingerprint fingerprintPlaceholder; private final int fingerprintLength; @VisibleForTesting @@ -71,30 +79,30 @@ public static FingerprintValueService createForTesting(FingerprintValueCache.Syn private static FingerprintValueService createForTesting( FingerprintValueStore store, FingerprintValueCache.SyncMode mode) { return new FingerprintValueService( - newSingleThreadExecutor(), store, new FingerprintValueCache(mode), Hashing.murmur3_128()); + newSingleThreadExecutor(), store, new FingerprintValueCache(mode), NONPROD_FINGERPRINTER); } public FingerprintValueService( Executor executor, FingerprintValueStore store, FingerprintValueCache cache, - HashFunction hashFunction) { + Fingerprinter fingerprinter) { this.executor = executor; this.store = store; this.cache = cache; - this.hashFunction = hashFunction; + this.fingerprinter = fingerprinter; this.fingerprintPlaceholder = fingerprint(new byte[] {}); - this.fingerprintLength = fingerprintPlaceholder.size(); + this.fingerprintLength = fingerprintPlaceholder.toBytes().length; } /** Delegates to {@link FingerprintValueStore#put}. */ - public ListenableFuture put(ByteString fingerprint, byte[] serializedBytes) { + public ListenableFuture put(PackedFingerprint fingerprint, byte[] serializedBytes) { return store.put(fingerprint, serializedBytes); } /** Delegates to {@link FingerprintValueStore#get}. */ - ListenableFuture get(ByteString fingerprint) throws IOException { + ListenableFuture get(PackedFingerprint fingerprint) throws IOException { return store.get(fingerprint); } @@ -108,15 +116,20 @@ Object getOrClaimPutOperation( /** Delegates to {@link FingerprintValueCache#getOrClaimGetOperation}. */ @Nullable Object getOrClaimGetOperation( - ByteString fingerprint, + PackedFingerprint fingerprint, @Nullable Object distinguisher, ListenableFuture getOperation) { return cache.getOrClaimGetOperation(fingerprint, distinguisher, getOperation); } /** Computes the fingerprint of {@code bytes}. */ - ByteString fingerprint(byte[] bytes) { - return ByteString.copyFrom(hashFunction.hashBytes(bytes).asBytes()); + public PackedFingerprint fingerprint(byte[] bytes) { + return fingerprinter.fingerprint(bytes); + } + + /** Convenience overload of {@link #fingerprint(byte[])}. */ + public PackedFingerprint fingerprint(ByteString bytes) { + return fingerprint(bytes.toByteArray()); } /** @@ -125,7 +138,7 @@ ByteString fingerprint(byte[] bytes) { *

The placeholder has the same length as the real fingerprint so the real fingerprint can * overwrite the placeholder when it becomes available. */ - ByteString fingerprintPlaceholder() { + PackedFingerprint fingerprintPlaceholder() { return fingerprintPlaceholder; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueStore.java index 82c44952017634..ed7993d29e4e46 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueStore.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueStore.java @@ -18,7 +18,6 @@ import static com.google.common.util.concurrent.Futures.immediateVoidFuture; import com.google.common.util.concurrent.ListenableFuture; -import com.google.protobuf.ByteString; import java.io.IOException; import java.util.concurrent.ConcurrentHashMap; import javax.annotation.Nullable; @@ -33,7 +32,7 @@ public interface FingerprintValueStore { * * @return a future that completes when the write completes */ - ListenableFuture put(ByteString fingerprint, byte[] serializedBytes); + ListenableFuture put(PackedFingerprint fingerprint, byte[] serializedBytes); /** * Retrieves the serialized bytes associated with {@code fingerprint}. @@ -44,7 +43,7 @@ public interface FingerprintValueStore { *

The caller should deduplicate {@code get} calls to avoid multiple fetches of the same * fingerprint. */ - ListenableFuture get(ByteString fingerprint) throws IOException; + ListenableFuture get(PackedFingerprint fingerprint) throws IOException; /** * {@link FingerprintValueStore#get} was called with a fingerprint that does not exist in the @@ -52,11 +51,12 @@ public interface FingerprintValueStore { */ final class MissingFingerprintValueException extends Exception { - public MissingFingerprintValueException(ByteString fingerprint) { + public MissingFingerprintValueException(PackedFingerprint fingerprint) { this(fingerprint, /* cause= */ null); } - public MissingFingerprintValueException(ByteString fingerprint, @Nullable Throwable cause) { + public MissingFingerprintValueException( + PackedFingerprint fingerprint, @Nullable Throwable cause) { super("No remote value for " + fingerprint, cause); } } @@ -67,17 +67,17 @@ static InMemoryFingerprintValueStore inMemoryStore() { /** An in-memory {@link FingerprintValueStore} for testing. */ static class InMemoryFingerprintValueStore implements FingerprintValueStore { - private final ConcurrentHashMap fingerprintToContents = + private final ConcurrentHashMap fingerprintToContents = new ConcurrentHashMap<>(); @Override - public ListenableFuture put(ByteString fingerprint, byte[] serializedBytes) { + public ListenableFuture put(PackedFingerprint fingerprint, byte[] serializedBytes) { fingerprintToContents.put(fingerprint, serializedBytes); return immediateVoidFuture(); } @Override - public ListenableFuture get(ByteString fingerprint) { + public ListenableFuture get(PackedFingerprint fingerprint) { byte[] serializedBytes = fingerprintToContents.get(fingerprint); if (serializedBytes == null) { return immediateFailedFuture(new MissingFingerprintValueException(fingerprint)); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/PackedFingerprint.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/PackedFingerprint.java index cda4296d58b0e0..4740702124e2a7 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/PackedFingerprint.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/PackedFingerprint.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.skyframe.serialization; import static com.google.common.base.Preconditions.checkArgument; +import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.annotations.VisibleForTesting; import com.google.errorprone.annotations.Keep; @@ -63,6 +64,16 @@ public static PackedFingerprint fromBytesOffsetZeros(byte[] bytes) { offsetZeros((long) LONG_ARRAY_HANDLE.get(bytes, 8))); } + /** Reads a fingerprint from {@code codedIn} that was written by {@link #writeTo}. */ + public static PackedFingerprint readFrom(CodedInputStream codedIn) throws IOException { + return new PackedFingerprint(codedIn.readFixed64(), codedIn.readFixed64()); + } + + @VisibleForTesting + public static PackedFingerprint getFingerprintForTesting(String key) { + return FingerprintValueService.NONPROD_FINGERPRINTER.fingerprint(key.getBytes(UTF_8)); + } + /** Produces the {@code byte[]} representation of this fingerprint. */ public byte[] toBytes() { byte[] result = new byte[BYTES]; @@ -84,6 +95,12 @@ public void copyTo(byte[] bytes, int offset) { LONG_ARRAY_HANDLE.set(bytes, offset + 8, hi); } + /** Writes fingerprint data to {@code codedOut} such that it can be read by {@link #readFrom}. */ + public void writeTo(CodedOutputStream codedOut) throws IOException { + codedOut.writeFixed64NoTag(lo); + codedOut.writeFixed64NoTag(hi); + } + @Override public int hashCode() { return (int) lo; @@ -130,14 +147,13 @@ public Class getEncodedClass() { public void serialize( LeafSerializationContext context, PackedFingerprint obj, CodedOutputStream codedOut) throws IOException { - codedOut.writeInt64NoTag(obj.lo()); - codedOut.writeInt64NoTag(obj.hi()); + obj.writeTo(codedOut); } @Override public PackedFingerprint deserialize( LeafDeserializationContext context, CodedInputStream codedIn) throws IOException { - return new PackedFingerprint(codedIn.readInt64(), codedIn.readInt64()); + return PackedFingerprint.readFrom(codedIn); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/PutOperation.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/PutOperation.java index 399a1751734229..7997430f570e1b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/PutOperation.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/PutOperation.java @@ -13,20 +13,12 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe.serialization; -import com.google.auto.value.AutoValue; import com.google.common.util.concurrent.ListenableFuture; -import com.google.protobuf.ByteString; -/** Tuple representing a {@link FingerprintValueStore#put} operation. */ -@AutoValue -public abstract class PutOperation { - public static PutOperation create(ByteString fingerprint, ListenableFuture writeStatus) { - return new AutoValue_PutOperation(fingerprint, writeStatus); - } - - /** Key used to store the value. */ - public abstract ByteString fingerprint(); - - /** The result of storing the value in the {@link FingerprintValueStore}. */ - public abstract ListenableFuture writeStatus(); -} +/** + * Tuple representing a {@link FingerprintValueStore#put} operation. + * + * @param fingerprint key used to store the value. + * @param writeStatus result of storing the value in the {@link FingerprintValueStore}. + */ +public record PutOperation(PackedFingerprint fingerprint, ListenableFuture writeStatus) {} diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SerializationModule.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SerializationModule.java index ae74feb06b0c9a..995dcc0e826911 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SerializationModule.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SerializationModule.java @@ -15,7 +15,6 @@ import static java.util.concurrent.ForkJoinPool.commonPool; -import com.google.common.hash.Hashing; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.runtime.BlazeModule; import com.google.devtools.build.lib.runtime.BlazeRuntime; @@ -75,7 +74,7 @@ private static final class InMemoryFingerprintValueServiceFactory // TODO: b/358347099 - use a persistent store FingerprintValueStore.inMemoryStore(), new FingerprintValueCache(FingerprintValueCache.SyncMode.NOT_LINKED), - Hashing.murmur3_128()); + FingerprintValueService.NONPROD_FINGERPRINTER); private InMemoryFingerprintValueServiceFactory() {} diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SharedValueDeserializationContext.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SharedValueDeserializationContext.java index 5ea4179d945c62..bfd3a93ac275b4 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SharedValueDeserializationContext.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SharedValueDeserializationContext.java @@ -274,8 +274,7 @@ public void getSharedValue( T parent, FieldSetter setter) throws IOException, SerializationException { - ByteString fingerprint = - ByteString.copyFrom(codedIn.readRawBytes(fingerprintValueService.fingerprintLength())); + PackedFingerprint fingerprint = PackedFingerprint.readFrom(codedIn); SettableFuture getOperation = SettableFuture.create(); Object previous = fingerprintValueService.getOrClaimGetOperation(fingerprint, distinguisher, getOperation); @@ -313,7 +312,7 @@ public void getSkyValue(SkyKey key, T parent, FieldSetter setter) } private void readValueForFingerprint( - ByteString fingerprint, + PackedFingerprint fingerprint, DeferredObjectCodec codec, T parent, FieldSetter setter, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SharedValueSerializationContext.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SharedValueSerializationContext.java index d849c41b78786d..4777f512a2c44c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SharedValueSerializationContext.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SharedValueSerializationContext.java @@ -132,7 +132,7 @@ public final void putSharedValue( recordFuturePut(inflight, codedOut); return; } - codedOut.writeRawBytes((ByteString) previous); + ((PackedFingerprint) previous).writeTo(codedOut); return; } @@ -217,12 +217,12 @@ private final void putOwnedSharedValue( if (childDeferredBytes == null) { // There are no deferred bytes so `childBytes` is complete. Starts the upload. - ByteString fingerprint = fingerprintValueService.fingerprint(childBytes); - codedOut.writeRawBytes(fingerprint); // Writes only the fingerprint to the stream. + PackedFingerprint fingerprint = fingerprintValueService.fingerprint(childBytes); + fingerprint.writeTo(codedOut); // Writes only the fingerprint to the stream. childWriteStatuses.add(fingerprintValueService.put(fingerprint, childBytes)); ListenableFuture writeStatus = aggregateStatusFutures(childWriteStatuses); - putOperation.set(PutOperation.create(fingerprint, writeStatus)); + putOperation.set(new PutOperation(fingerprint, writeStatus)); addFutureToBlockWritingOn(writeStatus); return; } @@ -238,10 +238,9 @@ private final void putOwnedSharedValue( fillPlaceholdersAndCollectWriteStatuses( childDeferredBytes, childBytes, childWriteStatuses); // All placeholders are filled-in. Starts the upload. - ByteString fingerprint = fingerprintValueService.fingerprint(childBytes); + PackedFingerprint fingerprint = fingerprintValueService.fingerprint(childBytes); childWriteStatuses.add(fingerprintValueService.put(fingerprint, childBytes)); - return PutOperation.create( - fingerprint, aggregateStatusFutures(childWriteStatuses)); + return new PutOperation(fingerprint, aggregateStatusFutures(childWriteStatuses)); }, directExecutor()); @@ -292,7 +291,7 @@ private final void recordFuturePut( } futurePuts.add(new FuturePut(codedOut.getTotalBytesWritten(), futurePut)); // Adds a placeholder for the real fingerprint to be filled in after the future completes. - codedOut.writeRawBytes(fingerprintValueService.fingerprintPlaceholder()); + fingerprintValueService.fingerprintPlaceholder().writeTo(codedOut); } private final SerializationResult createResult(byte[] bytes) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetriever.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetriever.java index fccd02b53a44f2..7e296ab8a45e64 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetriever.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetriever.java @@ -195,7 +195,9 @@ public static RetrievalResult tryRetrieve( } ListenableFuture valueBytes; try { - valueBytes = fingerprintValueService.get(keyBytes.getObject()); + valueBytes = + fingerprintValueService.get( + fingerprintValueService.fingerprint(keyBytes.getObject())); } catch (IOException e) { throw new SerializationException("key lookup failed for " + key, e); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FrontierSerializer.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FrontierSerializer.java index 626fd5b0e04acb..e054a32db5e1d1 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FrontierSerializer.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FrontierSerializer.java @@ -138,7 +138,8 @@ public static Optional serializeAndUploadFrontier( // of this key. writeStatuses.add( fingerprintValueService.put( - keyBytes.getObject(), valueBytes.getObject().toByteArray())); + fingerprintValueService.fingerprint(keyBytes.getObject()), + valueBytes.getObject().toByteArray())); frontierValueCount.getAndIncrement(); eventBus.post(new SerializedNodeEvent(key)); } catch (SerializationException e) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/GetRecordingStore.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/GetRecordingStore.java index 964ed61152378b..c4c495f62e5135 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/GetRecordingStore.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/GetRecordingStore.java @@ -19,7 +19,7 @@ import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.SettableFuture; import com.google.devtools.build.lib.skyframe.serialization.FingerprintValueStore; -import com.google.protobuf.ByteString; +import com.google.devtools.build.lib.skyframe.serialization.PackedFingerprint; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.LinkedBlockingQueue; import javax.annotation.Nullable; @@ -29,19 +29,19 @@ * operations and makes their completion controllable by the caller. */ public final class GetRecordingStore implements FingerprintValueStore { - private final ConcurrentHashMap fingerprintToContents = + private final ConcurrentHashMap fingerprintToContents = new ConcurrentHashMap<>(); private final LinkedBlockingQueue requestQueue = new LinkedBlockingQueue<>(); @Override - public ListenableFuture put(ByteString fingerprint, byte[] serializedBytes) { + public ListenableFuture put(PackedFingerprint fingerprint, byte[] serializedBytes) { fingerprintToContents.put(fingerprint, serializedBytes); return immediateVoidFuture(); } @Override - public ListenableFuture get(ByteString fingerprint) { + public ListenableFuture get(PackedFingerprint fingerprint) { SettableFuture response = SettableFuture.create(); requestQueue.offer(new GetRequest(this, fingerprint, response)); return response; @@ -58,7 +58,7 @@ public GetRequest pollRequest() { /** Encapsulates a {@link #get} operation. */ public record GetRequest( - GetRecordingStore parent, ByteString fingerprint, SettableFuture response) { + GetRecordingStore parent, PackedFingerprint fingerprint, SettableFuture response) { /** * Completes the {@link #response} by looking up the {@link #fingerprint} in the {@link * #parent}'s in-memory map. diff --git a/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecTest.java b/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecTest.java index 4ef83e1218f38e..9182bb5f8ed22e 100644 --- a/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecTest.java +++ b/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecTest.java @@ -17,6 +17,7 @@ import static com.google.common.util.concurrent.Futures.immediateFailedFuture; import static com.google.common.util.concurrent.Futures.immediateVoidFuture; import static com.google.common.util.concurrent.MoreExecutors.directExecutor; +import static com.google.devtools.build.lib.skyframe.serialization.PackedFingerprint.getFingerprintForTesting; import static org.junit.Assert.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; @@ -44,6 +45,7 @@ import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec; 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.PackedFingerprint; import com.google.devtools.build.lib.skyframe.serialization.PutOperation; import com.google.devtools.build.lib.skyframe.serialization.SerializationContext; import com.google.devtools.build.lib.skyframe.serialization.SerializationDependencyProvider; @@ -98,12 +100,12 @@ public void onlyOneReadPerArray() throws Exception { final FingerprintValueStore delegate = FingerprintValueStore.inMemoryStore(); @Override - public ListenableFuture put(ByteString fingerprint, byte[] serializedBytes) { + public ListenableFuture put(PackedFingerprint fingerprint, byte[] serializedBytes) { return delegate.put(fingerprint, serializedBytes); } @Override - public ListenableFuture get(ByteString fingerprint) throws IOException { + public ListenableFuture get(PackedFingerprint fingerprint) throws IOException { reads.incrementAndGet(); return delegate.get(fingerprint); } @@ -141,16 +143,16 @@ public ListenableFuture get(ByteString fingerprint) throws IOException { @Test public void missingNestedSetException_hiddenUntilNestedSetIsConsumed() throws Exception { Throwable missingNestedSetException = - new MissingFingerprintValueException(ByteString.copyFromUtf8("fingerprint")); + new MissingFingerprintValueException(getFingerprintForTesting("fingerprint")); FingerprintValueStore fingerprintValueStore = new FingerprintValueStore() { @Override - public ListenableFuture put(ByteString fingerprint, byte[] serializedBytes) { + public ListenableFuture put(PackedFingerprint fingerprint, byte[] serializedBytes) { return immediateVoidFuture(); } @Override - public ListenableFuture get(ByteString fingerprint) { + public ListenableFuture get(PackedFingerprint fingerprint) { return immediateFailedFuture(missingNestedSetException); } }; @@ -181,12 +183,12 @@ public void exceptionOnPut_propagatedToFutureToBlockWritesOn() throws Exception FingerprintValueStore fingerprintValueStore = new FingerprintValueStore() { @Override - public ListenableFuture put(ByteString fingerprint, byte[] serializedBytes) { + public ListenableFuture put(PackedFingerprint fingerprint, byte[] serializedBytes) { return immediateFailedFuture(e); } @Override - public ListenableFuture get(ByteString fingerprint) { + public ListenableFuture get(PackedFingerprint fingerprint) { throw new UnsupportedOperationException(); } }; @@ -209,12 +211,12 @@ public void exceptionOnGet_hiddenUntilNestedSetIsConsumed() throws Exception { FingerprintValueStore fingerprintValueStore = new FingerprintValueStore() { @Override - public ListenableFuture put(ByteString fingerprint, byte[] serializedBytes) { + public ListenableFuture put(PackedFingerprint fingerprint, byte[] serializedBytes) { return immediateVoidFuture(); } @Override - public ListenableFuture get(ByteString fingerprint) { + public ListenableFuture get(PackedFingerprint fingerprint) { return immediateFailedFuture(e); } }; @@ -371,8 +373,8 @@ public void testDeserializationInParallel() throws Exception { // We capture the arguments to #put() during serialization, so as to correctly mock results for // #get() - ArgumentCaptor fingerprintCaptor = ArgumentCaptor.forClass(ByteString.class); - ByteString fingerprint = + var fingerprintCaptor = ArgumentCaptor.forClass(PackedFingerprint.class); + PackedFingerprint fingerprint = nestedSetStore .computeFingerprintAndStore( (Object[]) set.getChildren(), @@ -413,7 +415,7 @@ public void racingDeserialization() throws Exception { NestedSetStore nestedSetStore = createStoreWithCache(nestedSetFingerprintValueStore, nestedSetCache); DeserializationContext deserializationContext = mock(DeserializationContext.class); - ByteString fingerprint = ByteString.copyFromUtf8("fingerprint"); + PackedFingerprint fingerprint = getFingerprintForTesting("fingerprint"); // Future never completes, so we don't have to exercise that code in NestedSetStore. SettableFuture storageFuture = SettableFuture.create(); when(nestedSetFingerprintValueStore.get(fingerprint)).thenReturn(storageFuture); diff --git a/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetSerializationCacheTest.java b/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetSerializationCacheTest.java index fe0ce28a35a59c..9fe8e44a558348 100644 --- a/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetSerializationCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetSerializationCacheTest.java @@ -15,6 +15,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.util.concurrent.Futures.immediateVoidFuture; +import static com.google.devtools.build.lib.skyframe.serialization.PackedFingerprint.getFingerprintForTesting; import static org.junit.Assert.assertThrows; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -23,8 +24,8 @@ import com.google.common.util.concurrent.SettableFuture; import com.google.devtools.build.lib.bugreport.BugReporter; import com.google.devtools.build.lib.skyframe.serialization.FingerprintValueStore.MissingFingerprintValueException; +import com.google.devtools.build.lib.skyframe.serialization.PackedFingerprint; import com.google.devtools.build.lib.skyframe.serialization.PutOperation; -import com.google.protobuf.ByteString; import java.lang.ref.WeakReference; import org.junit.Test; import org.junit.runner.RunWith; @@ -41,8 +42,8 @@ public final class NestedSetSerializationCacheTest { @Test public void putFutureIfAbsent_newFingerprint_returnsNull() { - ByteString fingerprint1 = ByteString.copyFromUtf8("abc"); - ByteString fingerprint2 = ByteString.copyFromUtf8("xyz"); + PackedFingerprint fingerprint1 = getFingerprintForTesting("abc"); + PackedFingerprint fingerprint2 = getFingerprintForTesting("xyz"); SettableFuture future1 = SettableFuture.create(); SettableFuture future2 = SettableFuture.create(); @@ -52,7 +53,7 @@ public void putFutureIfAbsent_newFingerprint_returnsNull() { @Test public void putFutureIfAbsent_existingFingerprint_returnsExistingFuture() { - ByteString fingerprint = ByteString.copyFromUtf8("abc"); + PackedFingerprint fingerprint = getFingerprintForTesting("abc"); SettableFuture future = SettableFuture.create(); assertThat(cache.putFutureIfAbsent(fingerprint, future, DEFAULT_CONTEXT)).isNull(); @@ -64,7 +65,7 @@ public void putFutureIfAbsent_existingFingerprint_returnsExistingFuture() { @Test public void putFutureIfAbsent_rejectsAlreadyDoneFuture() { - ByteString fingerprint = ByteString.copyFromUtf8("abc"); + PackedFingerprint fingerprint = getFingerprintForTesting("abc"); SettableFuture future = SettableFuture.create(); future.set(new Object[0]); @@ -75,12 +76,12 @@ public void putFutureIfAbsent_rejectsAlreadyDoneFuture() { @Test public void putFutureIfAbsent_futureCompletes_unwrapsContents() { - ByteString fingerprint = ByteString.copyFromUtf8("abc"); + PackedFingerprint fingerprint = getFingerprintForTesting("abc"); SettableFuture future1 = SettableFuture.create(); SettableFuture future2 = SettableFuture.create(); Object[] contents = new Object[0]; - cache.putFutureIfAbsent(fingerprint, future1, DEFAULT_CONTEXT); + var unused = cache.putFutureIfAbsent(fingerprint, future1, DEFAULT_CONTEXT); future1.set(contents); assertThat(cache.putFutureIfAbsent(fingerprint, future2, DEFAULT_CONTEXT)) @@ -89,11 +90,11 @@ public void putFutureIfAbsent_futureCompletes_unwrapsContents() { @Test public void putFutureIfAbsent_futureCompletes_cachesFingerprint() { - ByteString fingerprint = ByteString.copyFromUtf8("abc"); + PackedFingerprint fingerprint = getFingerprintForTesting("abc"); SettableFuture future = SettableFuture.create(); Object[] contents = new Object[0]; - cache.putFutureIfAbsent(fingerprint, future, DEFAULT_CONTEXT); + var unused = cache.putFutureIfAbsent(fingerprint, future, DEFAULT_CONTEXT); future.set(contents); PutOperation result = cache.fingerprintForContents(contents); @@ -106,11 +107,11 @@ public void putFutureIfAbsent_futureFails_notifiesBugReporter() { BugReporter mockBugReporter = mock(BugReporter.class); NestedSetSerializationCache cacheWithCustomBugReporter = new NestedSetSerializationCache(mockBugReporter); - ByteString fingerprint = ByteString.copyFromUtf8("abc"); + PackedFingerprint fingerprint = getFingerprintForTesting("abc"); SettableFuture future = SettableFuture.create(); Throwable e = new MissingFingerprintValueException(fingerprint); - cacheWithCustomBugReporter.putFutureIfAbsent(fingerprint, future, DEFAULT_CONTEXT); + var unused = cacheWithCustomBugReporter.putFutureIfAbsent(fingerprint, future, DEFAULT_CONTEXT); future.setException(e); verify(mockBugReporter).sendNonFatalBugReport(e); @@ -118,12 +119,12 @@ public void putFutureIfAbsent_futureFails_notifiesBugReporter() { @Test public void putIfAbsent_newFingerprintAndContents_returnsNullAndCachesBothDirections() { - ByteString fingerprint1 = ByteString.copyFromUtf8("abc"); - ByteString fingerprint2 = ByteString.copyFromUtf8("xyz"); + PackedFingerprint fingerprint1 = getFingerprintForTesting("abc"); + PackedFingerprint fingerprint2 = getFingerprintForTesting("xyz"); Object[] contents1 = new Object[] {"abc"}; Object[] contents2 = new Object[] {"xyz"}; - PutOperation result1 = PutOperation.create(fingerprint1, SettableFuture.create()); - PutOperation result2 = PutOperation.create(fingerprint2, SettableFuture.create()); + PutOperation result1 = new PutOperation(fingerprint1, SettableFuture.create()); + PutOperation result2 = new PutOperation(fingerprint2, SettableFuture.create()); assertThat(cache.putIfAbsent(contents1, result1, DEFAULT_CONTEXT)).isNull(); assertThat(cache.putIfAbsent(contents2, result2, DEFAULT_CONTEXT)).isNull(); @@ -137,11 +138,11 @@ public void putIfAbsent_newFingerprintAndContents_returnsNullAndCachesBothDirect @Test public void putIfAbsent_existingFingerprintAndContents_returnsExistingResult() { - ByteString fingerprint = ByteString.copyFromUtf8("abc"); + PackedFingerprint fingerprint = getFingerprintForTesting("abc"); Object[] contents = new Object[0]; - PutOperation result1 = PutOperation.create(fingerprint, SettableFuture.create()); - PutOperation result2 = PutOperation.create(fingerprint, SettableFuture.create()); - PutOperation result3 = PutOperation.create(fingerprint, SettableFuture.create()); + PutOperation result1 = new PutOperation(fingerprint, SettableFuture.create()); + PutOperation result2 = new PutOperation(fingerprint, SettableFuture.create()); + PutOperation result3 = new PutOperation(fingerprint, SettableFuture.create()); assertThat(cache.putIfAbsent(contents, result1, DEFAULT_CONTEXT)).isNull(); assertThat(cache.putIfAbsent(contents, result2, DEFAULT_CONTEXT)).isSameInstanceAs(result1); @@ -150,10 +151,10 @@ public void putIfAbsent_existingFingerprintAndContents_returnsExistingResult() { @Test public void putIfAbsent_calledDuringPendingDeserialization_overwritesFuture() { - ByteString fingerprint = ByteString.copyFromUtf8("abc"); + PackedFingerprint fingerprint = getFingerprintForTesting("abc"); SettableFuture future = SettableFuture.create(); Object[] contents = new Object[0]; - PutOperation result = PutOperation.create(fingerprint, SettableFuture.create()); + PutOperation result = new PutOperation(fingerprint, SettableFuture.create()); assertThat(cache.putFutureIfAbsent(fingerprint, future, DEFAULT_CONTEXT)).isNull(); assertThat(cache.putIfAbsent(contents, result, DEFAULT_CONTEXT)).isNull(); @@ -176,10 +177,10 @@ public void putIfAbsent_calledDuringPendingDeserialization_overwritesFuture() { @Test public void putFutureIfAbsent_cacheEntriesHaveLifetimeOfContents() { - ByteString fingerprint = ByteString.copyFromUtf8("abc"); + PackedFingerprint fingerprint = getFingerprintForTesting("abc"); SettableFuture future = SettableFuture.create(); - cache.putFutureIfAbsent(fingerprint, future, DEFAULT_CONTEXT); + var unused = cache.putFutureIfAbsent(fingerprint, future, DEFAULT_CONTEXT); // Before completing, still cached while future in memory. GcFinalization.awaitFullGc(); @@ -208,9 +209,9 @@ public void putFutureIfAbsent_cacheEntriesHaveLifetimeOfContents() { @Test public void putIfAbsent_cacheEntriesHaveLifetimeOfContents() { - ByteString fingerprint = ByteString.copyFromUtf8("abc"); + PackedFingerprint fingerprint = getFingerprintForTesting("abc"); Object[] contents = new Object[0]; - PutOperation result = PutOperation.create(fingerprint, immediateVoidFuture()); + PutOperation result = new PutOperation(fingerprint, immediateVoidFuture()); var unused = cache.putIfAbsent(contents, result, DEFAULT_CONTEXT); @@ -230,7 +231,7 @@ public void putIfAbsent_cacheEntriesHaveLifetimeOfContents() { @Test public void putFutureIfAbsent_usesContextToDistinguish() { - ByteString fingerprint = ByteString.copyFromUtf8("abc"); + PackedFingerprint fingerprint = getFingerprintForTesting("abc"); String contextLower = "lower"; String contextUpper = "UPPER"; SettableFuture futureLower = SettableFuture.create(); @@ -262,15 +263,15 @@ public void putFutureIfAbsent_usesContextToDistinguish() { @Test public void putIfAbsent_usesContextToDistinguish() { - ByteString fingerprint = ByteString.copyFromUtf8("abc"); + PackedFingerprint fingerprint = getFingerprintForTesting("abc"); String contextLower = "lower"; String contextUpper = "UPPER"; Object[] contentsLower = new Object[] {"abc"}; Object[] contentsUpper = new Object[] {"ABC"}; - PutOperation resultLower1 = PutOperation.create(fingerprint, SettableFuture.create()); - PutOperation resultUpper1 = PutOperation.create(fingerprint, SettableFuture.create()); - PutOperation resultLower2 = PutOperation.create(fingerprint, SettableFuture.create()); - PutOperation resultUpper2 = PutOperation.create(fingerprint, SettableFuture.create()); + PutOperation resultLower1 = new PutOperation(fingerprint, SettableFuture.create()); + PutOperation resultUpper1 = new PutOperation(fingerprint, SettableFuture.create()); + PutOperation resultLower2 = new PutOperation(fingerprint, SettableFuture.create()); + PutOperation resultUpper2 = new PutOperation(fingerprint, SettableFuture.create()); assertThat(cache.putIfAbsent(contentsLower, resultLower1, contextLower)).isNull(); assertThat(cache.putIfAbsent(contentsUpper, resultUpper1, contextUpper)).isNull(); @@ -300,11 +301,11 @@ public boolean equals(Object o) { return o instanceof Context; } } - ByteString fingerprint = ByteString.copyFromUtf8("abc"); + PackedFingerprint fingerprint = getFingerprintForTesting("abc"); SettableFuture future = SettableFuture.create(); Object[] contents = new Object[0]; - PutOperation result1 = PutOperation.create(fingerprint, SettableFuture.create()); - PutOperation result2 = PutOperation.create(fingerprint, SettableFuture.create()); + PutOperation result1 = new PutOperation(fingerprint, SettableFuture.create()); + PutOperation result2 = new PutOperation(fingerprint, SettableFuture.create()); assertThat(cache.putFutureIfAbsent(fingerprint, future, new Context())).isNull(); assertThat(cache.putFutureIfAbsent(fingerprint, SettableFuture.create(), new Context())) diff --git a/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetTest.java b/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetTest.java index 50baea8799e4b6..1b52884301c15c 100644 --- a/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetTest.java +++ b/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetTest.java @@ -17,6 +17,7 @@ import static com.google.common.util.concurrent.Futures.immediateCancelledFuture; import static com.google.common.util.concurrent.Futures.immediateFailedFuture; import static com.google.common.util.concurrent.Futures.immediateFuture; +import static com.google.devtools.build.lib.skyframe.serialization.PackedFingerprint.getFingerprintForTesting; import static org.junit.Assert.assertThrows; import com.google.common.collect.ImmutableList; @@ -27,7 +28,6 @@ import com.google.devtools.build.lib.skyframe.serialization.FingerprintValueStore.MissingFingerprintValueException; import com.google.devtools.build.lib.testutil.TestThread; import com.google.devtools.build.lib.testutil.TestUtils; -import com.google.protobuf.ByteString; import java.time.Duration; import java.util.ArrayList; import java.util.List; @@ -427,7 +427,7 @@ public void toListInterruptibly_propagatesMissingFingerprintValueException() { Order.STABLE_ORDER, UNKNOWN_DEPTH, immediateFailedFuture( - new MissingFingerprintValueException(ByteString.copyFromUtf8("fingerprint")))); + new MissingFingerprintValueException(getFingerprintForTesting("fingerprint")))); assertThrows( MissingFingerprintValueException.class, deserializingNestedSet::toListInterruptibly); } @@ -449,7 +449,7 @@ public void toListWithTimeout_propagatesMissingFingerprintValueException() { Order.STABLE_ORDER, UNKNOWN_DEPTH, immediateFailedFuture( - new MissingFingerprintValueException(ByteString.copyFromUtf8("fingerprint")))); + new MissingFingerprintValueException(getFingerprintForTesting("fingerprint")))); assertThrows( MissingFingerprintValueException.class, () -> deserializingNestedSet.toListWithTimeout(Duration.ofNanos(1))); @@ -521,7 +521,7 @@ public void isReady_fromStorage_failed() { Order.STABLE_ORDER, UNKNOWN_DEPTH, immediateFailedFuture( - new MissingFingerprintValueException(ByteString.copyFromUtf8("fingerprint")))); + new MissingFingerprintValueException(getFingerprintForTesting("fingerprint")))); assertThat(deserializingNestedSet.isReady()).isFalse(); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueCacheTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueCacheTest.java index 44d01bd92bc6e2..008505732c71e2 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueCacheTest.java @@ -16,10 +16,10 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.util.concurrent.Futures.immediateFuture; import static com.google.common.util.concurrent.Futures.immediateVoidFuture; +import static com.google.devtools.build.lib.skyframe.serialization.PackedFingerprint.getFingerprintForTesting; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.SettableFuture; -import com.google.protobuf.ByteString; import com.google.testing.junit.testparameterinjector.TestParameter; import com.google.testing.junit.testparameterinjector.TestParameterInjector; import javax.annotation.Nullable; @@ -71,7 +71,7 @@ public void getOperation_isCached(@TestParameter Distinguisher distinguisher) { FingerprintValueService service = FingerprintValueService.createForTesting(FingerprintValueCache.SyncMode.LINKED); - ByteString fingerprint = ByteString.copyFromUtf8("foo"); + PackedFingerprint fingerprint = getFingerprintForTesting("foo"); SettableFuture op1 = SettableFuture.create(); Object result = service.getOrClaimGetOperation(fingerprint, distinguisher.value(), op1); @@ -95,9 +95,9 @@ public void putOperation_isUnwrapped(@TestParameter Distinguisher distinguisher) // Sets the `PutOperation` in `putOp1`, which triggers the first stage of unwrapping and // populates the reverse service. - ByteString fingerprint = ByteString.copyFromUtf8("foo"); + PackedFingerprint fingerprint = getFingerprintForTesting("foo"); SettableFuture writeStatus = SettableFuture.create(); - putOp1.set(PutOperation.create(fingerprint, writeStatus)); + putOp1.set(new PutOperation(fingerprint, writeStatus)); // A get of `fingerprint` now returns `value` immediately. SettableFuture getOp = SettableFuture.create(); @@ -120,7 +120,7 @@ public void getOperation_isUnwrapped(@TestParameter Distinguisher distinguisher) FingerprintValueService service = FingerprintValueService.createForTesting(FingerprintValueCache.SyncMode.LINKED); - ByteString fingerprint = ByteString.copyFromUtf8("foo"); + PackedFingerprint fingerprint = getFingerprintForTesting("foo"); SettableFuture getOp = SettableFuture.create(); Object result = service.getOrClaimGetOperation(fingerprint, distinguisher.value(), getOp); @@ -142,8 +142,8 @@ public void getOperation_isUnwrapped(@TestParameter Distinguisher distinguisher) // Completing `putOp` overwrites values, but this is benign because `value`s fingerprint should // be deterministic. - ByteString fingerprint2 = ByteString.copyFromUtf8("foo"); - putOp.set(PutOperation.create(fingerprint2, immediateVoidFuture())); + PackedFingerprint fingerprint2 = getFingerprintForTesting("foo"); + putOp.set(new PutOperation(fingerprint2, immediateVoidFuture())); SettableFuture putOp3 = SettableFuture.create(); putResult = service.getOrClaimPutOperation(value, distinguisher.value(), putOp3); @@ -157,10 +157,10 @@ public void distinguisher_distinguishesSameFingerprint() { FingerprintValueService service = FingerprintValueService.createForTesting(FingerprintValueCache.SyncMode.LINKED); - ByteString fingerprint = ByteString.copyFromUtf8("foo"); + PackedFingerprint fingerprint = getFingerprintForTesting("foo"); ListenableFuture put = - immediateFuture(PutOperation.create(fingerprint, immediateVoidFuture())); + immediateFuture(new PutOperation(fingerprint, immediateVoidFuture())); Object value1 = new Object(); Object distinguisher1 = new Object(); @@ -188,13 +188,13 @@ public void notLinkedPut_doesNotAddGetEntry( FingerprintValueService service = FingerprintValueService.createForTesting(mode); // Puts the `fingerprint` to `value` association into the service. - ByteString fingerprint = ByteString.copyFromUtf8("foo"); + PackedFingerprint fingerprint = getFingerprintForTesting("foo"); Object value = new Object(); Object result = service.getOrClaimPutOperation( value, distinguisher.value(), - immediateFuture(PutOperation.create(fingerprint, immediateVoidFuture()))); + immediateFuture(new PutOperation(fingerprint, immediateVoidFuture()))); assertThat(result).isNull(); SettableFuture getOperation = SettableFuture.create(); @@ -217,7 +217,7 @@ public void notLinkedGet_doesNotAddPutEntry( FingerprintValueService service = FingerprintValueService.createForTesting(mode); // Puts the `value` to `fingerprint` association into the service. - ByteString fingerprint = ByteString.copyFromUtf8("foo"); + PackedFingerprint fingerprint = getFingerprintForTesting("foo"); Object value = new Object(); Object result = service.getOrClaimGetOperation(fingerprint, distinguisher.value(), immediateFuture(value)); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueServiceTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueServiceTest.java index 4a205528688d94..f1779c3be2c85a 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/FingerprintValueServiceTest.java @@ -14,18 +14,15 @@ package com.google.devtools.build.lib.skyframe.serialization; import static com.google.common.truth.Truth.assertThat; +import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.concurrent.Executors.newSingleThreadExecutor; -import com.google.common.hash.HashFunction; -import com.google.common.hash.Hashing; -import com.google.protobuf.ByteString; -import com.google.testing.junit.testparameterinjector.TestParameter; -import com.google.testing.junit.testparameterinjector.TestParameterInjector; import java.util.concurrent.Executor; import org.junit.Test; import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; -@RunWith(TestParameterInjector.class) +@RunWith(JUnit4.class) public final class FingerprintValueServiceTest { // FingerprintValueService is a thin wrapper over a few loosely related objects. The contained // FingerprintValueCache is covered in FingerprintValueCacheTest. @@ -36,7 +33,7 @@ public void get_returnsPreviouslyPut() throws Exception { // wiring. FingerprintValueService service = FingerprintValueService.createForTesting(); - ByteString key = ByteString.copyFromUtf8("key"); + PackedFingerprint key = service.fingerprint("key".getBytes(UTF_8)); byte[] value = new byte[] {0, 1, 2}; Void unused = service.put(key, value).get(); @@ -44,54 +41,34 @@ public void get_returnsPreviouslyPut() throws Exception { assertThat(service.get(key).get()).isSameInstanceAs(value); } - enum FingerprintFunction { - MURMUR3_64(Hashing.murmur3_32_fixed()), - MURMUR3_128(Hashing.murmur3_128()), - SHA_256(Hashing.sha256()); - - private final HashFunction hashFunction; - - FingerprintFunction(HashFunction hashFunction) { - this.hashFunction = hashFunction; - } - - HashFunction hashFunction() { - return hashFunction; - } - - int expectedLength() { - return hashFunction.bits() / 8; - } - } - @Test - public void fingerprintConsistent(@TestParameter FingerprintFunction fingerprinter) { + public void fingerprint_isConsistent() { FingerprintValueService service = new FingerprintValueService( newSingleThreadExecutor(), FingerprintValueStore.inMemoryStore(), new FingerprintValueCache(), - fingerprinter.hashFunction()); + FingerprintValueService.NONPROD_FINGERPRINTER); - assertThat(service.fingerprintPlaceholder().size()).isEqualTo(fingerprinter.expectedLength()); - assertThat(service.fingerprintLength()).isEqualTo(fingerprinter.expectedLength()); + assertThat(service.fingerprintPlaceholder().toBytes().length).isEqualTo(16); + assertThat(service.fingerprintLength()).isEqualTo(16); byte[] testValue = new byte[] {0, 1, 2}; - ByteString testFingerprint = service.fingerprint(testValue); + PackedFingerprint testFingerprint = service.fingerprint(testValue); assertThat(testFingerprint).isNotEqualTo(service.fingerprintPlaceholder()); - assertThat(testFingerprint.size()).isEqualTo(fingerprinter.expectedLength()); + assertThat(testFingerprint.toBytes().length).isEqualTo(16); } @Test - public void exectutor_passesThrough() { + public void executor_passesThrough() { Executor executor = newSingleThreadExecutor(); FingerprintValueService service = new FingerprintValueService( executor, FingerprintValueStore.inMemoryStore(), new FingerprintValueCache(), - Hashing.murmur3_128()); + FingerprintValueService.NONPROD_FINGERPRINTER); assertThat(service.getExecutor()).isSameInstanceAs(executor); } } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SharedValueSerializationContextTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SharedValueSerializationContextTest.java index d6650268640e30..9a8c50ab616e02 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SharedValueSerializationContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SharedValueSerializationContextTest.java @@ -47,7 +47,7 @@ private static final class PutRecordingStore implements FingerprintValueStore { private final ArrayList> putResponses = new ArrayList<>(); @Override - public ListenableFuture put(ByteString fingerprint, byte[] serializedBytes) { + public ListenableFuture put(PackedFingerprint fingerprint, byte[] serializedBytes) { var response = SettableFuture.create(); synchronized (putResponses) { putResponses.add(response); @@ -56,7 +56,7 @@ public ListenableFuture put(ByteString fingerprint, byte[] serializedBytes } @Override - public ListenableFuture get(ByteString fingerprint) { + public ListenableFuture get(PackedFingerprint fingerprint) { throw new UnsupportedOperationException(); } @@ -208,7 +208,7 @@ public void concurrentSharing_waitsForCompleteBytes() throws Exception { var blockingStore = new FingerprintValueStore() { @Override - public ListenableFuture put(ByteString fingerprint, byte[] serializedBytes) { + public ListenableFuture put(PackedFingerprint fingerprint, byte[] serializedBytes) { var response = SettableFuture.create(); synchronized (putResponses) { putResponses.add(response); @@ -225,7 +225,7 @@ public ListenableFuture put(ByteString fingerprint, byte[] serializedBytes } @Override - public ListenableFuture get(ByteString fingerprint) { + public ListenableFuture get(PackedFingerprint fingerprint) { throw new UnsupportedOperationException(); } }; diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetrieverTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetrieverTest.java index f0a1d6830a0910..06e02166be2d72 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetrieverTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/SkyValueRetrieverTest.java @@ -89,7 +89,11 @@ public void initialQueryState_progressesToWaiting(@TestParameter InitialQueryCas assertThat(keyBytes.getFutureToBlockWritesOn()).isNull(); if (testCase.equals(InitialQueryCases.IMMEDIATE_EMPTY_VALUE)) { - assertThat(fingerprintValueService.put(keyBytes.getObject(), new byte[0]).get()).isNull(); + assertThat( + fingerprintValueService + .put(fingerprintValueService.fingerprint(keyBytes.getObject()), new byte[0]) + .get()) + .isNull(); } RetrievalResult result = @@ -497,7 +501,9 @@ private void uploadKeyValuePair( var unused = fingerprintValueService - .put(keyBytes.getObject(), valueBytes.getObject().toByteArray()) + .put( + fingerprintValueService.fingerprint(keyBytes.getObject()), + valueBytes.getObject().toByteArray()) .get(); }