From b9c9cd6f7d20d1fadec23e8a0541204f8f096ab8 Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 11 Oct 2024 06:35:34 -0700 Subject: [PATCH] Add FileDependencyDeserializer.getDirectoryListingDependencies. The implementation is closely resembles that of getFileDependencies. * Adds the DirectoryListingDependencies type to contain the result. * Creates common base class FileSystemDependencies, for DirectoryListingDependencies and FileDependencies. These classes will be put into the same containers. * Pulls FileDependencyDeserializer.FutureFileDependencies out into SettableFutureWithOwnership and makes it generic for reuse. PiperOrigin-RevId: 684818562 Change-Id: I963276a534b04e19b17f390c85d0d5a73cf0f481 --- .../lib/skyframe/serialization/analysis/BUILD | 9 + .../DirectoryListingDependencies.java | 26 ++ .../analysis/FileDependencies.java | 3 +- .../analysis/FileDependencyDeserializer.java | 233 ++++++++++++------ .../analysis/FileDependencySerializer.java | 2 +- .../analysis/FileSystemDependencies.java | 25 ++ .../analysis/SettableFutureWithOwnership.java | 82 ++++++ 7 files changed, 307 insertions(+), 73 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/DirectoryListingDependencies.java create mode 100644 src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileSystemDependencies.java create mode 100644 src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/SettableFutureWithOwnership.java diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/BUILD index de25f48dca331d..dc5af33efc3de2 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/BUILD @@ -81,11 +81,14 @@ java_library( java_library( name = "file_dependency_deserializer", srcs = [ + "DirectoryListingDependencies.java", "FileDependencies.java", "FileDependencyDeserializer.java", + "FileSystemDependencies.java", ], deps = [ ":file_dependency_key_support", + ":settable_future_with_ownership", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization", "//src/main/java/com/google/devtools/build/lib/vfs:ospathpolicy", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", @@ -98,6 +101,12 @@ java_library( ], ) +java_library( + name = "settable_future_with_ownership", + srcs = ["SettableFutureWithOwnership.java"], + deps = ["//third_party:guava"], +) + java_library( name = "file_dependency_key_support", srcs = ["FileDependencyKeySupport.java"], diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/DirectoryListingDependencies.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/DirectoryListingDependencies.java new file mode 100644 index 00000000000000..a6df57f05cf027 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/DirectoryListingDependencies.java @@ -0,0 +1,26 @@ +// Copyright 2024 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.skyframe.serialization.analysis; + +import com.google.common.collect.ImmutableSet; + +/** Type representing a directory listing operation. */ +record DirectoryListingDependencies(FileDependencies realDirectory) + implements FileDependencyDeserializer.GetDirectoryListingDependenciesResult, + FileSystemDependencies { + /** True if this entry matches any directory name in {@code directoryPaths}. */ + boolean matchesAnyDirectory(ImmutableSet directoryPaths) { + return directoryPaths.contains(realDirectory.resolvedPath()); + } +} diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileDependencies.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileDependencies.java index ccd9c3eee666c9..01ad942b810bc7 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileDependencies.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileDependencies.java @@ -28,7 +28,8 @@ * #getDependencyCount} and {@link #getDependency}. If any matches are encountered, the associated * value is invalidated. */ -sealed interface FileDependencies extends FileDependencyDeserializer.GetDependenciesResult +sealed interface FileDependencies + extends FileSystemDependencies, FileDependencyDeserializer.GetDependenciesResult permits FileDependencies.SingleResolvedPath, FileDependencies.SingleResolvedPathAndDependency, FileDependencies.MultiplePaths { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileDependencyDeserializer.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileDependencyDeserializer.java index d4c954a526e96f..da50172bb49814 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileDependencyDeserializer.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileDependencyDeserializer.java @@ -17,6 +17,7 @@ import static com.google.common.util.concurrent.Futures.immediateFailedFuture; import static com.google.common.util.concurrent.Futures.immediateFuture; import static com.google.common.util.concurrent.MoreExecutors.directExecutor; +import static com.google.devtools.build.lib.skyframe.serialization.analysis.FileDependencyKeySupport.DIRECTORY_KEY_DELIMITER; import static com.google.devtools.build.lib.skyframe.serialization.analysis.FileDependencyKeySupport.FILE_KEY_DELIMITER; import static com.google.devtools.build.lib.skyframe.serialization.analysis.FileDependencyKeySupport.MAX_KEY_LENGTH; import static com.google.devtools.build.lib.skyframe.serialization.analysis.FileDependencyKeySupport.MTSV_SENTINEL; @@ -27,7 +28,7 @@ import com.github.benmanes.caffeine.cache.Cache; import com.github.benmanes.caffeine.cache.Caffeine; -import com.google.common.util.concurrent.AbstractFuture; +import com.google.common.base.Function; import com.google.common.util.concurrent.AsyncFunction; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; @@ -35,34 +36,47 @@ import com.google.devtools.build.lib.skyframe.serialization.KeyBytesProvider; import com.google.devtools.build.lib.skyframe.serialization.SerializationException; import com.google.devtools.build.lib.skyframe.serialization.StringKey; +import com.google.devtools.build.lib.skyframe.serialization.proto.DirectoryListingInvalidationData; import com.google.devtools.build.lib.skyframe.serialization.proto.FileInvalidationData; import com.google.devtools.build.lib.skyframe.serialization.proto.Symlink; import com.google.devtools.build.lib.vfs.OsPathPolicy; import com.google.protobuf.InvalidProtocolBufferException; import java.io.IOException; -import java.lang.invoke.MethodHandles; -import java.lang.invoke.VarHandle; import javax.annotation.Nullable; /** * Deserializes dependency information persisted by {@link FileDependencySerializer}. * - *

Fetching a dependency is a mostly linear asynchronous state machine that performs actions then - * waits in an alternating manner. + *

Fetching a file dependency is a mostly linear asynchronous state machine that performs actions + * then waits in an alternating manner. * *

    *
  1. Request the data for a given key. - *
  2. {@link WaitForData}. + *
  3. {@link WaitForFileInvalidationData}. *
  4. Request the data for the parent directory (a recursive call). *
  5. {@link WaitForParent}. *
  6. Process any symlinks, resolving symlink parents as needed. *
  7. {@link WaitForSymlinkParent}. *
  8. Processing symlinks repeats for all the symlinks associated with an entry. *
+ * + *

A similar, but simpler state machine is used for directory listings. + * + *

    + *
  1. Request the data for a given key. + *
  2. {@link WaitForListingInvalidationData}. + *
  3. Request the file data corresponding to the directory (delegating to {@link + * #getFileDependencies}). + *
  4. {@link WaitForListingFileDependencies}. + *
  5. Create and cache the {@link DirectoryListingDependencies} instance. + *
*/ final class FileDependencyDeserializer { private static final OsPathPolicy OS = OsPathPolicy.getFilePathOs(); + /** Singleton representing the root file. */ + static final FileDependencies ROOT_FILE = FileDependencies.builder("").build(); + private final FingerprintValueService fingerprintValueService; /** @@ -79,8 +93,16 @@ final class FileDependencyDeserializer { * retained by the {@code SkyValue}s that depend on them. When all such associated {@code * SkyValue}s are invalidated, the dependency information becomes eligible for GC. */ - private final Cache dependenciesCache = - Caffeine.newBuilder().weakValues().build(); + private final Cache fileCache = + Caffeine.newBuilder().weakValues().build(); + + /** + * A cache for {@link DirectoryListingDependencies}, primarily for deduplication. + * + *

This follows the design of {@link #fileCache} but is for directory listings. + */ + private final Cache listingCache = + Caffeine.newBuilder().weakValues().build(); FileDependencyDeserializer(FingerprintValueService fingerprintValueService) { this.fingerprintValueService = fingerprintValueService; @@ -88,6 +110,15 @@ final class FileDependencyDeserializer { sealed interface GetDependenciesResult permits FileDependencies, FutureFileDependencies {} + /** + * The main purpose of this class is to act as a {@link ListenableFuture}. + * + *

Its specific type is explicitly visible to clients to allow them to cleanly distinguish it + * as a permitted subtype of {@link GetDependenciesResult}. + */ + static final class FutureFileDependencies extends SettableFutureWithOwnership + implements GetDependenciesResult {} + /** * Reconstitutes the set of file dependencies associated with {@code key}. * @@ -100,7 +131,7 @@ sealed interface GetDependenciesResult permits FileDependencies, FutureFileDepen */ GetDependenciesResult getFileDependencies(String key) { FutureFileDependencies ownedFuture; - switch (dependenciesCache.get(key, unused -> new FutureFileDependencies())) { + switch (fileCache.get(key, unused -> new FutureFileDependencies())) { case FileDependencies dependencies: return dependencies; case FutureFileDependencies future: @@ -111,79 +142,54 @@ GetDependenciesResult getFileDependencies(String key) { break; } // `ownedFuture` is owned by this thread, which must complete its value. - try { - ListenableFuture futureBytes; - try { - futureBytes = fingerprintValueService.get(getKeyBytes(key)); - } catch (IOException e) { - ownedFuture.setIoException(e); - return ownedFuture; - } - - ownedFuture.setFutureFiles( - Futures.transformAsync( - futureBytes, new WaitForData(key), fingerprintValueService.getExecutor())); - return ownedFuture; - } finally { - ownedFuture.verifySet(); - } + fetchInvalidationData(key, WaitForFileInvalidationData::new, ownedFuture); + return ownedFuture; } + sealed interface GetDirectoryListingDependenciesResult + permits DirectoryListingDependencies, FutureDirectoryListingDependencies {} + /** - * The main purpose of this class is to act as a {@link ListenableFuture}. + * The main purpose of this class is to act as a {@link + * ListenableFuture}. * *

Its specific type is explicitly visible to clients to allow them to cleanly distinguish it - * as a permitted subtype of {@link GetDependenciesResult}. + * as a permitted subtype of {@link GetDirectoryListingDependenciesResult}. */ - static final class FutureFileDependencies extends AbstractFuture - implements GetDependenciesResult { - /** Used to establish exactly-once ownership of this future with {@link #tryTakeOwnership}. */ - @SuppressWarnings({"UnusedVariable", "FieldCanBeFinal"}) // set with OWNED_HANDLE - private boolean owned = false; - - private boolean isSet = false; - - private boolean tryTakeOwnership() { - return OWNED_HANDLE.compareAndSet(this, false, true); - } - - private void setFutureFiles(ListenableFuture files) { - checkState(setFuture(files), "already set %s", this); - isSet = true; - } - - private void setIoException(IOException e) { - checkState(setException(e)); - isSet = true; - } - - private void verifySet() { - if (!isSet) { - checkState( - setException( - new IllegalStateException( - "future was unexpectedly unset, look for unchecked exceptions in" - + " FileDependencyDeserializer"))); - } - } + static final class FutureDirectoryListingDependencies + extends SettableFutureWithOwnership + implements GetDirectoryListingDependenciesResult {} - private static final VarHandle OWNED_HANDLE; - - static { - try { - OWNED_HANDLE = - MethodHandles.lookup() - .findVarHandle(FutureFileDependencies.class, "owned", boolean.class); - } catch (ReflectiveOperationException e) { - throw new ExceptionInInitializerError(e); - } + /** + * Deserializes the resolved directory listing information associated with {@code key}. + * + * @param key should be as described at {@link DirectoryListingInvalidationData}. + * @return either an immediate {@link DirectoryListingDependencies} instance or effectively a + * {@link ListenableFuture} instance. + */ + GetDirectoryListingDependenciesResult getDirectoryListingDependencies(String key) { + FutureDirectoryListingDependencies ownedFuture; + switch (listingCache.get(key, unused -> new FutureDirectoryListingDependencies())) { + case DirectoryListingDependencies dependencies: + return dependencies; + case FutureDirectoryListingDependencies future: + if (!future.tryTakeOwnership()) { + return future; // Owned by another thread. + } + ownedFuture = future; + break; } + // `ownedFuture` is owned by this thread, which must complete its value. + fetchInvalidationData(key, WaitForListingInvalidationData::new, ownedFuture); + return ownedFuture; } - private class WaitForData implements AsyncFunction { + // ---------- Begin FileDependencies deserialization implementation ---------- + + private class WaitForFileInvalidationData implements AsyncFunction { private final String key; - private WaitForData(String key) { + private WaitForFileInvalidationData(String key) { this.key = key; } @@ -280,7 +286,7 @@ private ListenableFuture processSymlinks( // Replaces the cache value with the completed value. The future is likely to become eligible // for GC shortly after the return below. Clients are expected to retain the meaningful // top-level values. - dependenciesCache.put(key, dependencies); + fileCache.put(key, dependencies); return immediateFuture(dependencies); } @@ -406,6 +412,91 @@ private static boolean doesSymlinkParentNeedResolution( return !previousParent.startsWith(newParent); } + // ---------- Begin DirectoryListingDependencies deserialization implementation ---------- + + private class WaitForListingInvalidationData + implements AsyncFunction { + private final String key; + + private WaitForListingInvalidationData(String key) { + this.key = key; + } + + @Override + public ListenableFuture apply(byte[] bytes) + throws InvalidProtocolBufferException { + var data = DirectoryListingInvalidationData.parseFrom(bytes, getEmptyRegistry()); + if (data.hasOverflowKey() && !data.getOverflowKey().equals(key)) { + return immediateFailedFuture( + new SerializationException( + String.format( + "Non-matching overflow key. This is possible if there is a key fingerprint" + + " collision. Expected %s got %s", + key, data))); + } + + int pathBegin = key.indexOf(DIRECTORY_KEY_DELIMITER) + 1; + + String path = key.substring(pathBegin); + if (path.isEmpty()) { + return immediateFuture(createAndCacheListingDependencies(key, ROOT_FILE)); + } + + String fileKey = + computeCacheKey( + path, data.hasFileMtsv() ? data.getFileMtsv() : MTSV_SENTINEL, FILE_KEY_DELIMITER); + switch (getFileDependencies(fileKey)) { + case FileDependencies dependencies: + return immediateFuture(createAndCacheListingDependencies(key, dependencies)); + case FutureFileDependencies future: + return Futures.transform( + future, new WaitForListingFileDependencies(key), directExecutor()); + } + } + } + + private class WaitForListingFileDependencies + implements Function { + private final String key; + + private WaitForListingFileDependencies(String key) { + this.key = key; + } + + @Override + public DirectoryListingDependencies apply(FileDependencies dependencies) { + return createAndCacheListingDependencies(key, dependencies); + } + } + + private DirectoryListingDependencies createAndCacheListingDependencies( + String key, FileDependencies dependencies) { + var result = new DirectoryListingDependencies(dependencies); + listingCache.put(key, result); + return result; + } + + // ---------- Begin shared helpers ---------- + + private > void fetchInvalidationData( + String key, Function> waitFactory, FutureT ownedFuture) { + try { + ListenableFuture futureBytes; + try { + futureBytes = fingerprintValueService.get(getKeyBytes(key)); + } catch (IOException e) { + ownedFuture.failWith(e); + return; + } + + ownedFuture.completeWith( + Futures.transformAsync( + futureBytes, waitFactory.apply(key), fingerprintValueService.getExecutor())); + } finally { + ownedFuture.verifyComplete(); + } + } + private KeyBytesProvider getKeyBytes(String cacheKey) { if (cacheKey.length() > MAX_KEY_LENGTH) { return fingerprintValueService.fingerprint(cacheKey.getBytes(UTF_8)); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileDependencySerializer.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileDependencySerializer.java index ad0b618d6a58aa..cf990082b99f24 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileDependencySerializer.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileDependencySerializer.java @@ -194,7 +194,7 @@ DirectoryInvalidationDataReference registerDependency(DirectoryListingValue.Key reference.populate( computeCacheKey(rootedPath.getRootRelativePath(), mtsv, DIRECTORY_KEY_DELIMITER)); } - // If this is reached, this thread owns `reference` and must complete it's future. + // If this is reached, this thread owns `reference` and must complete its future. boolean writeStatusSet = false; try { DirectoryListingInvalidationData.Builder data = DirectoryListingInvalidationData.newBuilder(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileSystemDependencies.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileSystemDependencies.java new file mode 100644 index 00000000000000..072e135fbd1c8f --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/FileSystemDependencies.java @@ -0,0 +1,25 @@ +// Copyright 2024 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.skyframe.serialization.analysis; + +/** + * Union type for {@link FileDependencies} and {@link DirectoryListingDependencies}. + * + *

At a structural level these two classes are very similar and {@link + * DirectoryListingDependencies} could be modeled plainly as {@link FileDependencies}. The crucial + * difference is that {@link FileDependencies#containsMatch(ImmutableSet)} takes a set of + * files and {@link DirectoryListingDependencies#matchesAnyDirectories(ImmutableSet)} takes + * a set of directory names so the two types are deliberately separated. + */ +sealed interface FileSystemDependencies permits FileDependencies, DirectoryListingDependencies {} diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/SettableFutureWithOwnership.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/SettableFutureWithOwnership.java new file mode 100644 index 00000000000000..5d8efd31a5b7b2 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/analysis/SettableFutureWithOwnership.java @@ -0,0 +1,82 @@ +// Copyright 2024 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.skyframe.serialization.analysis; + +import static com.google.common.base.Preconditions.checkState; + +import com.google.common.util.concurrent.AbstractFuture; +import com.google.common.util.concurrent.ListenableFuture; +import java.io.IOException; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.VarHandle; + +/** A tailored settable future with ownership tracking. */ +abstract class SettableFutureWithOwnership extends AbstractFuture { + /** Used to establish exactly-once ownership of this future with {@link #tryTakeOwnership}. */ + @SuppressWarnings({"UnusedVariable", "FieldCanBeFinal"}) // set with OWNED_HANDLE + private boolean owned = false; + + private boolean isSet = false; + + /** + * Returns true once. + * + *

When using {@link com.github.benmanes.caffeine.cache.Cache#get} with future values and a + * mapping function, there's a need to determine which thread owns the future. This method + * provides such a mechanism. + * + *

When this returns true, the caller must call either {@link #completeWith} or {@link + * #failWith}. + */ + boolean tryTakeOwnership() { + return OWNED_HANDLE.compareAndSet(this, false, true); + } + + void completeWith(ListenableFuture future) { + checkState(setFuture(future), "already set %s", this); + isSet = true; + } + + void failWith(IOException e) { + checkState(setException(e)); + isSet = true; + } + + /** + * Verifies that the future was complete. + * + *

With settable futures, there's a risk of deadlock-like behavior if the future is not + * complete. The owning thread should call this in a finally clause to fail-fast instead. + */ + void verifyComplete() { + if (!isSet) { + checkState( + setException( + new IllegalStateException( + "future was unexpectedly unset, look for unchecked exceptions"))); + } + } + + private static final VarHandle OWNED_HANDLE; + + static { + try { + OWNED_HANDLE = + MethodHandles.lookup() + .findVarHandle(SettableFutureWithOwnership.class, "owned", boolean.class); + } catch (ReflectiveOperationException e) { + throw new ExceptionInInitializerError(e); + } + } +}