From ed7ec3bf45ce529aa1ccd53654b8daf8aef9ac29 Mon Sep 17 00:00:00 2001 From: jhorvitz Date: Fri, 24 Jul 2020 15:08:03 -0700 Subject: [PATCH] Always create TreeArtifactValue through a builder. Ensures that no unnecessary (unsorted) temporary maps are created prior to the ImmutableSortedMap being created for the final TreeArtifactValue. MetadataInjector becomes aware of TreeArtifactValue now that the dependency cycle is broken after work in b/161911915, so it can take the TreeArtifactValue instead of a map of its children. The method is renamed to injectTree. RELNOTES: None. PiperOrigin-RevId: 323080321 --- .../google/devtools/build/lib/actions/BUILD | 1 + .../lib/actions/cache/MetadataHandler.java | 6 +- .../lib/actions/cache/MetadataInjector.java | 18 +- .../google/devtools/build/lib/remote/BUILD | 1 + .../build/lib/remote/RemoteCache.java | 21 +-- .../lib/skyframe/ActionExecutionValue.java | 7 +- .../lib/skyframe/ActionMetadataHandler.java | 15 +- .../build/lib/skyframe/ArtifactFunction.java | 71 ++++---- .../google/devtools/build/lib/skyframe/BUILD | 7 +- .../lib/skyframe/TreeArtifactInjector.java | 31 ++++ .../build/lib/skyframe/TreeArtifactValue.java | 156 +++++++++++------ .../lib/actions/util/ActionsTestUtil.java | 4 +- .../devtools/build/lib/actions/util/BUILD | 1 + .../google/devtools/build/lib/remote/BUILD | 1 + .../build/lib/remote/RemoteCacheTests.java | 20 ++- .../skyframe/ActionMetadataHandlerTest.java | 32 ++-- .../ActionTemplateExpansionFunctionTest.java | 9 +- .../lib/skyframe/ArtifactFunctionTest.java | 28 +-- .../skyframe/FilesystemValueCheckerTest.java | 75 +++----- ...ursiveFilesystemTraversalFunctionTest.java | 28 ++- .../lib/skyframe/TreeArtifactBuildTest.java | 8 +- .../skyframe/TreeArtifactMetadataTest.java | 10 +- .../lib/skyframe/TreeArtifactValueTest.java | 164 +++++++++++++----- 23 files changed, 416 insertions(+), 298 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactInjector.java diff --git a/src/main/java/com/google/devtools/build/lib/actions/BUILD b/src/main/java/com/google/devtools/build/lib/actions/BUILD index 932a8bb8b6d8d6..db5e4d7caa158f 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BUILD +++ b/src/main/java/com/google/devtools/build/lib/actions/BUILD @@ -85,6 +85,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:sane_analysis_exception", "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions", "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_aware_action", + "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi", diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java index 8bfcd776f30b11..19c214dcf2b52c 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java @@ -33,8 +33,8 @@ public interface MetadataHandler extends MetadataProvider, MetadataInjector { *

Freshly created output files (i.e. from an action that just executed) that require a stat to * obtain the metadata will first be set read-only and executable during this call. This ensures * that the returned metadata has an appropriate ctime, which is affected by chmod. Note that this - * does not apply to outputs injected via {@link #injectFile} or {@link #injectDirectory} since a - * stat is not required for them. + * does not apply to outputs injected via {@link #injectFile} or {@link #injectTree} since a stat + * is not required for them. */ @Override @Nullable @@ -47,7 +47,7 @@ public interface MetadataHandler extends MetadataProvider, MetadataInjector { * Constructs a {@link FileArtifactValue} for the given output whose digest is known. * *

This call does not inject the returned metadata. It should be injected with a followup call - * to {@link #injectFile} or {@link #injectDirectory} as appropriate. + * to {@link #injectFile} or {@link #injectTree} as appropriate. * *

chmod will not be called on the output. */ diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java index ccea7b32fde3d8..a425ee8cefb8ec 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java @@ -14,13 +14,11 @@ package com.google.devtools.build.lib.actions.cache; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; -import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue; -import java.util.Map; +import com.google.devtools.build.lib.skyframe.TreeArtifactInjector; /** Supports metadata injection of action outputs into skyframe. */ -public interface MetadataInjector { +public interface MetadataInjector extends TreeArtifactInjector { /** * Injects the metadata of a file. @@ -29,20 +27,10 @@ public interface MetadataInjector { * *

{@linkplain Artifact#isTreeArtifact Tree artifacts} and their {@linkplain * Artifact#isChildOfDeclaredDirectory children} must not be passed here. Instead, they should be - * passed to {@link #injectDirectory}. + * passed to {@link #injectTree}. * * @param output a regular output file * @param metadata the file metadata */ void injectFile(Artifact output, FileArtifactValue metadata); - - /** - * Injects the metadata of a tree artifact. - * - *

This can be used to save filesystem operations when the metadata is already known. - * - * @param output an output directory {@linkplain Artifact#isTreeArtifact tree artifact} - * @param children the metadata of the files stored in the directory - */ - void injectDirectory(SpecialArtifact output, Map children); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD index 04e0f2e0561fd7..9cf2c6064c9469 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD @@ -78,6 +78,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/remote/options", "//src/main/java/com/google/devtools/build/lib/remote/util", "//src/main/java/com/google/devtools/build/lib/skyframe:mutable_supplier", + "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code", "//src/main/java/com/google/devtools/build/lib/util:exit_code", diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java index afc1562fdbfff5..7c5bdfd130bddc 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java @@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.remote; +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.remote.util.Utils.getFromFuture; @@ -44,7 +45,6 @@ import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.EnvironmentalExecException; import com.google.devtools.build.lib.actions.ExecException; -import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.actions.cache.MetadataInjector; @@ -63,6 +63,7 @@ import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution; import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution.Code; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.util.io.OutErr; import com.google.devtools.build.lib.vfs.Dirent; @@ -98,13 +99,8 @@ interface OutputFilesLocker { void lock() throws InterruptedException, IOException; } - private static final ListenableFuture COMPLETED_SUCCESS = SettableFuture.create(); - private static final ListenableFuture EMPTY_BYTES = SettableFuture.create(); - - static { - ((SettableFuture) COMPLETED_SUCCESS).set(null); - ((SettableFuture) EMPTY_BYTES).set(new byte[0]); - } + private static final ListenableFuture COMPLETED_SUCCESS = immediateFuture(null); + private static final ListenableFuture EMPTY_BYTES = immediateFuture(new byte[0]); protected final RemoteCacheClient cacheProtocol; protected final RemoteOptions options; @@ -440,7 +436,7 @@ private void createSymlinks(Iterable symlinks) throws IOExcepti } } - /** Download a file (that is not a directory). The content is fetched from the digest. */ + /** Downloads a file (that is not a directory). The content is fetched from the digest. */ public ListenableFuture downloadFile(Path path, Digest digest) throws IOException { Preconditions.checkNotNull(path.getParentDirectory()).createDirectoryAndParents(); if (digest.getSizeBytes() == 0) { @@ -621,8 +617,7 @@ private void injectRemoteArtifact( + "--experimental_remote_download_outputs=minimal"); } SpecialArtifact parent = (SpecialArtifact) output; - ImmutableMap.Builder childMetadata = - ImmutableMap.builderWithExpectedSize(directory.files.size()); + TreeArtifactValue.Builder tree = TreeArtifactValue.newBuilder(parent); for (FileMetadata file : directory.files()) { TreeFileArtifact child = TreeFileArtifact.createTreeOutput(parent, file.path().relativeTo(parent.getPath())); @@ -632,9 +627,9 @@ private void injectRemoteArtifact( file.digest().getSizeBytes(), /*locationIndex=*/ 1, actionId); - childMetadata.put(child, value); + tree.putChild(child, value); } - metadataInjector.injectDirectory(parent, childMetadata.build()); + metadataInjector.injectTree(parent, tree.build()); } else { FileMetadata outputMetadata = metadata.file(execRoot.getRelative(output.getExecPathString())); if (outputMetadata == null) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java index 50a9ee2ca59827..e021d3bbfcc77b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java @@ -271,16 +271,15 @@ private static TreeArtifactValue transformSharedTree( } SpecialArtifact newParent = (SpecialArtifact) newArtifact; + TreeArtifactValue.Builder newTree = TreeArtifactValue.newBuilder(newParent); - Map newChildren = - Maps.newHashMapWithExpectedSize(tree.getChildValues().size()); for (Map.Entry child : tree.getChildValues().entrySet()) { - newChildren.put( + newTree.putChild( TreeFileArtifact.createTreeOutput(newParent, child.getKey().getParentRelativePath()), child.getValue()); } - return TreeArtifactValue.create(newChildren); + return newTree.build(); } ActionExecutionValue transformForSharedAction(ImmutableSet outputs) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java index ef44f38540be1b..3112097c801e81 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java @@ -20,7 +20,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.Sets; import com.google.common.flogger.GoogleLogger; import com.google.common.io.BaseEncoding; @@ -322,8 +321,7 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa setPathReadOnlyAndExecutable(treeDir); } - ImmutableSortedMap.Builder values = - ImmutableSortedMap.naturalOrder(); + TreeArtifactValue.Builder tree = TreeArtifactValue.newBuilder(parent); TreeArtifactValue.visitTree( treeDir, @@ -347,10 +345,10 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa throw new IOException(errorMessage, e); } - values.put(child, metadata); + tree.putChild(child, metadata); }); - return TreeArtifactValue.create(values.build()); + return tree.build(); } @Override @@ -379,7 +377,7 @@ public void injectFile(Artifact output, FileArtifactValue metadata) { checkArgument(isKnownOutput(output), "%s is not a declared output of this action", output); checkArgument( !output.isTreeArtifact() && !output.isChildOfDeclaredDirectory(), - "Tree artifacts and their children must be injected via injectDirectory: %s", + "Tree artifacts and their children must be injected via injectTree: %s", output); checkState(executionMode.get(), "Tried to inject %s outside of execution", output); @@ -387,13 +385,12 @@ public void injectFile(Artifact output, FileArtifactValue metadata) { } @Override - public void injectDirectory( - SpecialArtifact output, Map children) { + public void injectTree(SpecialArtifact output, TreeArtifactValue tree) { checkArgument(isKnownOutput(output), "%s is not a declared output of this action", output); checkArgument(output.isTreeArtifact(), "Output must be a tree artifact: %s", output); checkState(executionMode.get(), "Tried to inject %s outside of execution", output); - store.putTreeArtifactData(output, TreeArtifactValue.create(children)); + store.putTreeArtifactData(output, tree); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java index d4696b98cd022f..98a749f657d647 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java @@ -16,8 +16,6 @@ import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.ActionExecutionException; @@ -27,6 +25,7 @@ import com.google.devtools.build.lib.actions.ActionTemplate; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ArtifactOwner; import com.google.devtools.build.lib.actions.FileArtifactValue; @@ -189,11 +188,14 @@ private static TreeArtifactValue createTreeArtifactValueFromActionKey( return null; } - // Aggregate the ArtifactValues for individual TreeFileArtifacts into a TreeArtifactValue for - // the parent TreeArtifact. - ImmutableMap.Builder map = ImmutableMap.builder(); + // Aggregate the metadata for individual TreeFileArtifacts into a TreeArtifactValue for the + // parent TreeArtifact. + SpecialArtifact parent = (SpecialArtifact) artifactDependencies.artifact; + TreeArtifactValue.Builder treeBuilder = TreeArtifactValue.newBuilder(parent); boolean omitted = false; + for (ActionLookupData actionKey : expandedActionExecutionKeys) { + boolean sawTreeChild = false; ActionExecutionValue actionExecutionValue = (ActionExecutionValue) Preconditions.checkNotNull( @@ -202,48 +204,47 @@ private static TreeArtifactValue createTreeArtifactValueFromActionKey( artifactDependencies, expansionValue, expandedActionValueMap); - Iterable treeFileArtifacts = - Iterables.transform( - Iterables.filter( - actionExecutionValue.getAllFileValues().keySet(), - artifact -> { - Preconditions.checkState( - artifact.hasParent(), - "No parent: %s %s %s", - artifact, - actionExecutionValue, - artifactDependencies); - return artifact.getParent().equals(artifactDependencies.artifact); - }), - artifact -> (TreeFileArtifact) artifact); - Preconditions.checkState( - !Iterables.isEmpty(treeFileArtifacts), - "Action denoted by %s does not output TreeFileArtifact from %s", - actionKey, - artifactDependencies); + for (Map.Entry entry : + actionExecutionValue.getAllFileValues().entrySet()) { + Artifact artifact = entry.getKey(); + Preconditions.checkState( + artifact.hasParent(), + "Parentless artifact %s found in ActionExecutionValue for %s: %s %s", + artifact, + actionKey, + actionExecutionValue, + artifactDependencies); - for (TreeFileArtifact treeFileArtifact : treeFileArtifacts) { - FileArtifactValue value = - actionExecutionValue.getExistingFileArtifactValue(treeFileArtifact); - if (FileArtifactValue.OMITTED_FILE_MARKER.equals(value)) { - omitted = true; - } else { - map.put(treeFileArtifact, value); + if (artifact.getParent().equals(parent)) { + sawTreeChild = true; + if (FileArtifactValue.OMITTED_FILE_MARKER.equals(entry.getValue())) { + omitted = true; + } else { + treeBuilder.putChild((TreeFileArtifact) artifact, entry.getValue()); + } } } + + Preconditions.checkState( + sawTreeChild, + "Action denoted by %s does not output any TreeFileArtifacts from %s", + actionKey, + artifactDependencies); } - ImmutableMap children = map.build(); + TreeArtifactValue tree = treeBuilder.build(); if (omitted) { Preconditions.checkState( - children.isEmpty(), + tree.getChildValues().isEmpty(), "Action template expansion has some but not all outputs omitted, present outputs: %s", - children); + artifactDependencies, + tree.getChildValues()); return TreeArtifactValue.OMITTED_TREE_MARKER; } - return TreeArtifactValue.create(children); + + return tree; } private static SkyValue createSourceValue(Artifact artifact, Environment env) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 09603fa15a685e..b3473b40e8c536 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -2674,11 +2674,14 @@ java_library( java_library( name = "tree_artifact_value", - srcs = ["TreeArtifactValue.java"], + srcs = [ + "TreeArtifactInjector.java", + "TreeArtifactValue.java", + ], deps = [ - "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", + "//src/main/java/com/google/devtools/build/lib/actions:has_digest", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant", "//src/main/java/com/google/devtools/build/lib/util", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactInjector.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactInjector.java new file mode 100644 index 00000000000000..22a7df5987134e --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactInjector.java @@ -0,0 +1,31 @@ +// Copyright 2020 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; + +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; + +/** Supports metadata injection of action outputs into skyframe. */ +public interface TreeArtifactInjector { + + /** + * Injects the metadata of a tree artifact. + * + *

This can be used to save filesystem operations when the metadata is already known. + * + * @param output an output directory {@linkplain Artifact#isTreeArtifact tree artifact} + * @param tree a {@link TreeArtifactValue} with the metadata of the files stored in the directory + */ + void injectTree(SpecialArtifact output, TreeArtifactValue tree); +} diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java index 1f570d41d11a0e..6727ecfd59db31 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java @@ -13,10 +13,11 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.collect.ImmutableSet.toImmutableSet; import com.google.common.base.MoreObjects; -import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; @@ -25,7 +26,6 @@ import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.HasDigest; import com.google.devtools.build.lib.actions.cache.DigestUtils; -import com.google.devtools.build.lib.actions.cache.MetadataInjector; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant; import com.google.devtools.build.lib.util.Fingerprint; @@ -49,19 +49,36 @@ */ public class TreeArtifactValue implements HasDigest, SkyValue { + /** Returns an empty {@link TreeArtifactValue}. */ + public static TreeArtifactValue empty() { + return EMPTY; + } + + /** + * Returns a new {@link Builder} for the given parent tree artifact. + * + *

The returned builder only supports adding children under this parent. To build multiple tree + * artifacts at once, use {@link MultiBuilder}. + */ + public static Builder newBuilder(SpecialArtifact parent) { + return new Builder(parent); + } + /** Builder for constructing multiple instances of {@link TreeArtifactValue} at once. */ public interface MultiBuilder { /** * Puts a child tree file into this builder under its {@linkplain TreeFileArtifact#getParent * parent}. + * + * @return {@code this} for convenience */ - void putChild(TreeFileArtifact child, FileArtifactValue metadata); + MultiBuilder putChild(TreeFileArtifact child, FileArtifactValue metadata); /** * For each unique parent seen by this builder, passes the aggregated metadata to {@link - * MetadataInjector#injectDirectory}. + * TreeArtifactInjector#injectTree}. */ - void injectTo(MetadataInjector metadataInjector); + void injectTo(TreeArtifactInjector treeInjector); } /** Returns a new {@link MultiBuilder}. */ @@ -85,8 +102,7 @@ public static MultiBuilder newConcurrentMultiBuilder() { private final ImmutableSortedMap childData; private final boolean entirelyRemote; - @AutoCodec.VisibleForSerialization - TreeArtifactValue( + private TreeArtifactValue( byte[] digest, ImmutableSortedMap childData, boolean entirelyRemote) { @@ -95,37 +111,6 @@ public static MultiBuilder newConcurrentMultiBuilder() { this.entirelyRemote = entirelyRemote; } - /** - * Returns a TreeArtifactValue out of the given Artifact-relative path fragments and their - * corresponding FileArtifactValues. - */ - static TreeArtifactValue create(Map childData) { - if (childData.isEmpty()) { - return EMPTY; - } - - // Sort the children to ensure a deterministic TreeArtifactValue (including digest). - ImmutableSortedMap sortedChildData = - ImmutableSortedMap.copyOf(childData); - Fingerprint fingerprint = new Fingerprint(); - boolean entirelyRemote = true; - - for (Map.Entry e : sortedChildData.entrySet()) { - TreeFileArtifact child = e.getKey(); - FileArtifactValue value = e.getValue(); - Preconditions.checkState( - !FileArtifactValue.OMITTED_FILE_MARKER.equals(value), - "Cannot construct TreeArtifactValue because child %s was omitted", - child); - // Tolerate a tree artifact having a mix of local and remote children (b/152496153#comment80). - entirelyRemote &= value.isRemote(); - fingerprint.addPath(child.getParentRelativePath()); - value.addTo(fingerprint); - } - - return new TreeArtifactValue(fingerprint.digestAndReset(), sortedChildData, entirelyRemote); - } - FileArtifactValue getMetadata() { return FileArtifactValue.createProxy(digest); } @@ -136,7 +121,6 @@ ImmutableSet getChildPaths() { .collect(toImmutableSet()); } - @Nullable @Override public byte[] getDigest() { return digest.clone(); @@ -180,7 +164,7 @@ public boolean equals(Object other) { @Override public String toString() { - return MoreObjects.toStringHelper(TreeArtifactValue.class) + return MoreObjects.toStringHelper(this) .add("digest", digest) .add("childData", childData) .toString(); @@ -277,7 +261,7 @@ public interface TreeArtifactVisitor { * artifact directory, or if {@link TreeArtifactVisitor#visit} throws {@link IOException} */ public static void visitTree(Path parentDir, TreeArtifactVisitor visitor) throws IOException { - visitTree(parentDir, PathFragment.EMPTY_FRAGMENT, Preconditions.checkNotNull(visitor)); + visitTree(parentDir, PathFragment.EMPTY_FRAGMENT, checkNotNull(visitor)); } private static void visitTree(Path parentDir, PathFragment subdir, TreeArtifactVisitor visitor) @@ -330,20 +314,82 @@ private static void checkSymlink(PathFragment subDir, Path path) throws IOExcept } } + /** Builder for a {@link TreeArtifactValue}. */ + public static final class Builder { + private final ImmutableSortedMap.Builder childData = + ImmutableSortedMap.naturalOrder(); + private final SpecialArtifact parent; + + Builder(SpecialArtifact parent) { + checkArgument(parent.isTreeArtifact(), "%s is not a tree artifact", parent); + this.parent = parent; + } + + /** + * Adds a child to this builder. + * + *

The child's {@linkplain TreeFileArtifact#getParent parent} must match the parent + * with which this builder was initialized. + * + *

Children may be added in any order. The children are sorted prior to constructing the + * final {@link TreeArtifactValue}. + * + *

It is illegal to call this method with {@link FileArtifactValue.OMITTED_FILE_MARKER}. When + * children are omitted, use {@link TreeArtifactValue#OMITTED_TREE_MARKER}. + * + * @return {@code this} for convenience + */ + public Builder putChild(TreeFileArtifact child, FileArtifactValue metadata) { + checkArgument( + child.getParent().equals(parent), + "While building TreeArtifactValue for %s, got %s with parent %s", + parent, + child, + child.getParent()); + checkArgument( + !FileArtifactValue.OMITTED_FILE_MARKER.equals(metadata), + "Cannot construct TreeArtifactValue for %s because child %s was omitted", + parent, + child); + childData.put(child, metadata); + return this; + } + + /** Builds the final {@link TreeArtifactValue}. */ + public TreeArtifactValue build() { + ImmutableSortedMap finalChildData = childData.build(); + if (finalChildData.isEmpty()) { + return EMPTY; + } + + Fingerprint fingerprint = new Fingerprint(); + boolean entirelyRemote = true; + + for (Map.Entry childData : finalChildData.entrySet()) { + // Digest will be deterministic because children are sorted. + fingerprint.addPath(childData.getKey().getParentRelativePath()); + childData.getValue().addTo(fingerprint); + + // Tolerate a mix of local and remote children(b/152496153#comment80). + entirelyRemote &= childData.getValue().isRemote(); + } + + return new TreeArtifactValue(fingerprint.digestAndReset(), finalChildData, entirelyRemote); + } + } + private static final class BasicMultiBuilder implements MultiBuilder { - private final Map< - SpecialArtifact, ImmutableSortedMap.Builder> - map = new HashMap<>(); + private final Map map = new HashMap<>(); @Override - public void putChild(TreeFileArtifact child, FileArtifactValue metadata) { - map.computeIfAbsent(child.getParent(), parent -> ImmutableSortedMap.naturalOrder()) - .put(child, metadata); + public MultiBuilder putChild(TreeFileArtifact child, FileArtifactValue metadata) { + map.computeIfAbsent(child.getParent(), Builder::new).putChild(child, metadata); + return this; } @Override - public void injectTo(MetadataInjector metadataInjector) { - map.forEach((parent, builder) -> metadataInjector.injectDirectory(parent, builder.build())); + public void injectTo(TreeArtifactInjector treeInjector) { + map.forEach((parent, builder) -> treeInjector.injectTree(parent, builder.build())); } } @@ -353,14 +399,20 @@ private static final class ConcurrentMultiBuilder implements MultiBuilder { map = new ConcurrentHashMap<>(); @Override - public void putChild(TreeFileArtifact child, FileArtifactValue metadata) { + public MultiBuilder putChild(TreeFileArtifact child, FileArtifactValue metadata) { map.computeIfAbsent(child.getParent(), parent -> new ConcurrentHashMap<>()) .put(child, metadata); + return this; } @Override - public void injectTo(MetadataInjector metadataInjector) { - map.forEach(metadataInjector::injectDirectory); + public void injectTo(TreeArtifactInjector treeInjector) { + map.forEach( + (parent, children) -> { + Builder builder = new Builder(parent); + children.forEach(builder::putChild); + treeInjector.injectTree(parent, builder.build()); + }); } } } diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java index 44c32c9b108d6a..2fb2df4cd4919e 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java @@ -74,6 +74,7 @@ import com.google.devtools.build.lib.exec.SingleBuildFileCache; import com.google.devtools.build.lib.skyframe.ActionTemplateExpansionValue; import com.google.devtools.build.lib.skyframe.ActionTemplateExpansionValue.ActionTemplateExpansionKey; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant; import com.google.devtools.build.lib.syntax.Location; import com.google.devtools.build.lib.util.FileType; @@ -921,8 +922,7 @@ public void injectFile(Artifact output, FileArtifactValue metadata) { } @Override - public void injectDirectory( - SpecialArtifact treeArtifact, Map children) { + public void injectTree(SpecialArtifact treeArtifact, TreeArtifactValue tree) { throw new UnsupportedOperationException(); } diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/BUILD b/src/test/java/com/google/devtools/build/lib/actions/util/BUILD index b21b068c12aeb7..f888b582d6ac21 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/util/BUILD +++ b/src/test/java/com/google/devtools/build/lib/actions/util/BUILD @@ -32,6 +32,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/exec:single_build_file_cache", "//src/main/java/com/google/devtools/build/lib/skyframe:action_template_expansion_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant", "//src/main/java/com/google/devtools/build/lib/syntax:frontend", diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index d6435178e09ae5..b6d93876ce2542 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -66,6 +66,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/remote/merkletree", "//src/main/java/com/google/devtools/build/lib/remote/options", "//src/main/java/com/google/devtools/build/lib/remote/util", + "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", "//src/main/java/com/google/devtools/build/lib/util:exit_code", diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java index 48081596da0291..96039baaa95f7c 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java @@ -38,7 +38,6 @@ import com.google.common.base.Charsets; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; @@ -50,7 +49,6 @@ import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.ExecException; -import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.cache.MetadataInjector; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; @@ -63,6 +61,7 @@ import com.google.devtools.build.lib.remote.util.InMemoryCacheClient; import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.remote.util.Utils.InMemoryOutput; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.testutil.TestUtils; import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.util.io.RecordingOutErr; @@ -993,13 +992,16 @@ public void testDownloadMinimalDirectory() throws Exception { // assert assertThat(inMemoryOutput).isNull(); - Map m = - ImmutableMap.of( - TreeFileArtifact.createTreeOutput(dir, "file1"), - new RemoteFileArtifactValue(toBinaryDigest(d1), d1.getSizeBytes(), 1, "action-id"), - TreeFileArtifact.createTreeOutput(dir, "a/file2"), - new RemoteFileArtifactValue(toBinaryDigest(d2), d2.getSizeBytes(), 1, "action-id")); - verify(injector).injectDirectory(eq(dir), eq(m)); + TreeArtifactValue tree = + TreeArtifactValue.newBuilder(dir) + .putChild( + TreeFileArtifact.createTreeOutput(dir, "file1"), + new RemoteFileArtifactValue(toBinaryDigest(d1), d1.getSizeBytes(), 1, "action-id")) + .putChild( + TreeFileArtifact.createTreeOutput(dir, "a/file2"), + new RemoteFileArtifactValue(toBinaryDigest(d2), d2.getSizeBytes(), 1, "action-id")) + .build(); + verify(injector).injectTree(dir, tree); Path outputBase = artifactRoot.getRoot().asPath(); assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty(); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java index 687ba3c057fb30..d59daff6232e3c 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java @@ -44,7 +44,6 @@ import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import java.io.FileNotFoundException; import java.io.IOException; -import java.util.Map; import java.util.Set; import org.junit.Before; import org.junit.Test; @@ -382,34 +381,31 @@ public void injectRemoteTreeArtifactMetadata() throws Exception { /*outputs=*/ ImmutableSet.of(treeArtifact)); handler.prepareForActionExecution(); - RemoteFileArtifactValue fooValue = - new RemoteFileArtifactValue(new byte[] {1, 2, 3}, 5, 1, "foo"); - RemoteFileArtifactValue barValue = - new RemoteFileArtifactValue(new byte[] {4, 5, 6}, 10, 1, "bar"); - Map children = - ImmutableMap.of( - TreeFileArtifact.createTreeOutput(treeArtifact, "foo"), fooValue, - TreeFileArtifact.createTreeOutput(treeArtifact, "bar"), barValue); + TreeArtifactValue tree = + TreeArtifactValue.newBuilder(treeArtifact) + .putChild( + TreeFileArtifact.createTreeOutput(treeArtifact, "foo"), + new RemoteFileArtifactValue(new byte[] {1, 2, 3}, 5, 1, "foo")) + .putChild( + TreeFileArtifact.createTreeOutput(treeArtifact, "bar"), + new RemoteFileArtifactValue(new byte[] {4, 5, 6}, 10, 1, "bar")) + .build(); - handler.injectDirectory(treeArtifact, children); + handler.injectTree(treeArtifact, tree); FileArtifactValue value = handler.getMetadata(treeArtifact); assertThat(value).isNotNull(); - TreeArtifactValue treeValue = handler.getOutputStore().getTreeArtifactData(treeArtifact); - assertThat(treeValue).isNotNull(); - assertThat(treeValue.getDigest()).isEqualTo(value.getDigest()); + assertThat(value.getDigest()).isEqualTo(tree.getDigest()); + assertThat(handler.getOutputStore().getTreeArtifactData(treeArtifact)).isEqualTo(tree); assertThat(chmodCalls).isEmpty(); - assertThat(treeValue.getChildPaths()) - .containsExactly(PathFragment.create("foo"), PathFragment.create("bar")); - assertThat(treeValue.getChildValues().values()).containsExactly(fooValue, barValue); - assertThat(handler.getTreeArtifactChildren(treeArtifact)).isEqualTo(treeValue.getChildren()); + assertThat(handler.getTreeArtifactChildren(treeArtifact)).isEqualTo(tree.getChildren()); // Make sure that all children are transferred properly into the ActionExecutionValue. If any // child is missing, getExistingFileArtifactValue will throw. ActionExecutionValue actionExecutionValue = ActionExecutionValue.createFromOutputStore(handler.getOutputStore(), null, null, false); - treeValue.getChildren().forEach(actionExecutionValue::getExistingFileArtifactValue); + tree.getChildren().forEach(actionExecutionValue::getExistingFileArtifactValue); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java index 32047a4d3913ad..9a63d6cc6a2a9f 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java @@ -384,20 +384,17 @@ private SpecialArtifact createAndPopulateTreeArtifact(String path, String... chi throws Exception { SpecialArtifact treeArtifact = createTreeArtifact(path); treeArtifact.setGeneratingActionKey(ActionLookupData.create(CTKEY, /*actionIndex=*/ 0)); - Map treeFileArtifactMap = new LinkedHashMap<>(); + TreeArtifactValue.Builder tree = TreeArtifactValue.newBuilder(treeArtifact); for (String childRelativePath : childRelativePaths) { TreeFileArtifact treeFileArtifact = TreeFileArtifact.createTreeOutput(treeArtifact, childRelativePath); scratch.file(treeFileArtifact.getPath().toString(), childRelativePath); // We do not care about the FileArtifactValues in this test. - treeFileArtifactMap.put( - treeFileArtifact, FileArtifactValue.createForTesting(treeFileArtifact)); + tree.putChild(treeFileArtifact, FileArtifactValue.createForTesting(treeFileArtifact)); } - artifactValueMap.put( - treeArtifact, TreeArtifactValue.create(ImmutableMap.copyOf(treeFileArtifactMap))); - + artifactValueMap.put(treeArtifact, tree.build()); return treeArtifact; } 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 b4753f483bb485..06c5861c749dab 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 @@ -51,7 +51,6 @@ import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter; 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.PathFragment; import com.google.devtools.build.lib.vfs.Root; @@ -324,10 +323,10 @@ public void actionExecutionValueSerialization() throws Exception { Path path = treeFileArtifact.getPath(); path.getParentDirectory().createDirectoryAndParents(); writeFile(path, "contents"); - TreeArtifactValue treeArtifactValue = - TreeArtifactValue.create( - ImmutableMap.of( - treeFileArtifact, FileArtifactValue.createForTesting(treeFileArtifact))); + TreeArtifactValue tree = + TreeArtifactValue.newBuilder(treeArtifact) + .putChild(treeFileArtifact, FileArtifactValue.createForTesting(treeFileArtifact)) + .build(); DerivedArtifact artifact3 = createDerivedArtifact("three"); FilesetOutputSymlink filesetOutputSymlink = FilesetOutputSymlink.createForTesting( @@ -335,7 +334,7 @@ public void actionExecutionValueSerialization() throws Exception { ActionExecutionValue actionExecutionValue = ActionExecutionValue.create( ImmutableMap.of(artifact1, metadata1, artifact3, FileArtifactValue.DEFAULT_MIDDLEMAN), - ImmutableMap.of(treeArtifact, treeArtifactValue), + ImmutableMap.of(treeArtifact, tree), ImmutableList.of(filesetOutputSymlink), null, true); @@ -349,7 +348,7 @@ public void actionExecutionValueSerialization() throws Exception { } private static void file(Path path, String contents) throws Exception { - FileSystemUtils.createDirectoryAndParents(path.getParentDirectory()); + path.getParentDirectory().createDirectoryAndParents(); writeFile(path, contents); } @@ -493,16 +492,19 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept Preconditions.checkState(!output.isTreeArtifact(), "Cannot omit %s", output); artifactData.put(output, FileArtifactValue.OMITTED_FILE_MARKER); } else if (output.isTreeArtifact()) { + SpecialArtifact parent = (SpecialArtifact) output; TreeFileArtifact treeFileArtifact1 = TreeFileArtifact.createTreeOutput((SpecialArtifact) output, "child1"); TreeFileArtifact treeFileArtifact2 = TreeFileArtifact.createTreeOutput((SpecialArtifact) output, "child2"); - TreeArtifactValue treeArtifactValue = - TreeArtifactValue.create( - ImmutableMap.of( - treeFileArtifact1, FileArtifactValue.createForTesting(treeFileArtifact1), - treeFileArtifact2, FileArtifactValue.createForTesting(treeFileArtifact2))); - treeArtifactData.put(output, treeArtifactValue); + TreeArtifactValue tree = + TreeArtifactValue.newBuilder(parent) + .putChild( + treeFileArtifact1, FileArtifactValue.createForTesting(treeFileArtifact1)) + .putChild( + treeFileArtifact2, FileArtifactValue.createForTesting(treeFileArtifact2)) + .build(); + treeArtifactData.put(output, tree); } else if (action.getActionType() == MiddlemanType.NORMAL) { Path path = output.getPath(); FileArtifactValue noDigest = diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java index 75e5f174a35a2f..ef0009ca2f8dfa 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java @@ -33,7 +33,6 @@ import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.FileStateValue; import com.google.devtools.build.lib.actions.FileValue; -import com.google.devtools.build.lib.actions.cache.MetadataInjector; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.actions.util.TestAction; import com.google.devtools.build.lib.analysis.BlazeDirectories; @@ -84,7 +83,6 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -531,11 +529,11 @@ private void checkDirtyTreeArtifactActions(BatchStat batchStatter) throws Except actionKey2, actionValueWithTreeArtifacts(ImmutableList.of(file21, file22)), actionKeyEmpty, - actionValueWithEmptyDirectory(outEmpty), + actionValueWithTreeArtifact(outEmpty, TreeArtifactValue.empty()), actionKeyUnchanging, - actionValueWithEmptyDirectory(outUnchanging), + actionValueWithTreeArtifact(outUnchanging, TreeArtifactValue.empty()), actionKeyLast, - actionValueWithEmptyDirectory(last))); + actionValueWithTreeArtifact(last, TreeArtifactValue.empty()))); EvaluationContext evaluationContext = EvaluationContext.newBuilder() @@ -861,15 +859,6 @@ private static ActionExecutionValue actionValue(Action action) { /*actionDependsOnBuildId=*/ false); } - private static ActionExecutionValue actionValueWithEmptyDirectory(SpecialArtifact emptyDir) { - return ActionExecutionValue.create( - /*artifactData=*/ ImmutableMap.of(), - ImmutableMap.of(emptyDir, TreeArtifactValue.create(ImmutableMap.of())), - /*outputSymlinks=*/ null, - /*discoveredModules=*/ null, - /*actionDependsOnBuildId=*/ false); - } - private static ActionExecutionValue actionValueWithTreeArtifacts( List contents) { TreeArtifactValue.MultiBuilder treeArtifacts = TreeArtifactValue.newMultiBuilder(); @@ -892,17 +881,7 @@ private static ActionExecutionValue actionValueWithTreeArtifacts( } Map treeArtifactData = new HashMap<>(); - treeArtifacts.injectTo( - new MetadataInjector() { - @Override - public void injectFile(Artifact output, FileArtifactValue metadata) {} - - @Override - public void injectDirectory( - SpecialArtifact output, Map children) { - treeArtifactData.put(output, TreeArtifactValue.create(children)); - } - }); + treeArtifacts.injectTo(treeArtifactData::put); return ActionExecutionValue.create( /*artifactData=*/ ImmutableMap.of(), @@ -912,31 +891,24 @@ public void injectDirectory( /*actionDependsOnBuildId=*/ false); } - private static ActionExecutionValue actionValueWithRemoteTreeArtifact( - SpecialArtifact output, Map children) { - ImmutableMap.Builder childFileValues = - ImmutableMap.builder(); - for (Map.Entry child : children.entrySet()) { - childFileValues.put( - TreeFileArtifact.createTreeOutput(output, child.getKey()), child.getValue()); - } - TreeArtifactValue treeArtifactValue = TreeArtifactValue.create(childFileValues.build()); + private static ActionExecutionValue actionValueWithTreeArtifact( + SpecialArtifact output, TreeArtifactValue tree) { return ActionExecutionValue.create( ImmutableMap.of(), - Collections.singletonMap(output, treeArtifactValue), - /* outputSymlinks= */ null, - /* discoveredModules= */ null, - /* actionDependsOnBuildId= */ false); + ImmutableMap.of(output, tree), + /*outputSymlinks=*/ null, + /*discoveredModules=*/ null, + /*actionDependsOnBuildId=*/ false); } private static ActionExecutionValue actionValueWithRemoteArtifact( Artifact output, RemoteFileArtifactValue value) { return ActionExecutionValue.create( - Collections.singletonMap(output, value), + ImmutableMap.of(output, value), ImmutableMap.of(), - /* outputSymlinks= */ null, - /* discoveredModules= */ null, - /* actionDependsOnBuildId= */ false); + /*outputSymlinks=*/ null, + /*discoveredModules=*/ null, + /*actionDependsOnBuildId=*/ false); } private RemoteFileArtifactValue createRemoteFileArtifactValue(String contents) { @@ -1008,16 +980,17 @@ public void testRemoteAndLocalTreeArtifacts() throws Exception { SpecialArtifact treeArtifact = createTreeArtifact("dir"); treeArtifact.getPath().createDirectoryAndParents(); - Map treeArtifactMetadata = new HashMap<>(); - treeArtifactMetadata.put( - PathFragment.create("foo"), createRemoteFileArtifactValue("foo-content")); - treeArtifactMetadata.put( - PathFragment.create("bar"), createRemoteFileArtifactValue("bar-content")); + TreeArtifactValue tree = + TreeArtifactValue.newBuilder(treeArtifact) + .putChild( + TreeFileArtifact.createTreeOutput(treeArtifact, "foo"), + createRemoteFileArtifactValue("foo-content")) + .putChild( + TreeFileArtifact.createTreeOutput(treeArtifact, "bar"), + createRemoteFileArtifactValue("bar-content")) + .build(); - Map metadataToInject = new HashMap<>(); - metadataToInject.put( - actionKey, actionValueWithRemoteTreeArtifact(treeArtifact, treeArtifactMetadata)); - differencer.inject(metadataToInject); + differencer.inject(ImmutableMap.of(actionKey, actionValueWithTreeArtifact(treeArtifact, tree))); EvaluationContext evaluationContext = EvaluationContext.newBuilder() diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java index 4b03f6bf4fadce..82cb374619c466 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java @@ -1008,18 +1008,16 @@ public void symlinkChainError() throws Exception { assertThat(error.getException()).hasMessageThat().contains("Symlink cycle"); } - private static class NonHermeticArtifactFakeFunction implements SkyFunction { + private static final class NonHermeticArtifactFakeFunction implements SkyFunction { - private final Map allTreeFiles = new HashMap<>(); + private TreeArtifactValue.Builder tree; - @Nullable @Override - public SkyValue compute(SkyKey skyKey, Environment env) - throws SkyFunctionException, InterruptedException { + public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException { try { if (skyKey.argument() instanceof Artifact && ((Artifact) skyKey.argument()).isTreeArtifact()) { - return TreeArtifactValue.create(allTreeFiles); + return tree.build(); } return FileArtifactValue.createForTesting(((Artifact) skyKey.argument()).getPath()); } catch (IOException e) { @@ -1033,16 +1031,17 @@ public String extractTag(SkyKey skyKey) { return null; } - public void addNewTreeFileArtifact(TreeFileArtifact input) throws IOException { - allTreeFiles.put(input, FileArtifactValue.createForTesting(input.getPath())); + void addNewTreeFileArtifact(TreeFileArtifact input) throws IOException { + if (tree == null) { + tree = TreeArtifactValue.newBuilder(input.getParent()); + } + tree.putChild(input, FileArtifactValue.createForTesting(input.getPath())); } } - private static class ArtifactFakeFunction implements SkyFunction { - @Nullable + private static final class ArtifactFakeFunction implements SkyFunction { @Override - public SkyValue compute(SkyKey skyKey, Environment env) - throws SkyFunctionException, InterruptedException { + public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException { return env.getValue(new NonHermeticArtifactSkyKey(skyKey)); } @@ -1053,11 +1052,10 @@ public String extractTag(SkyKey skyKey) { } } - private class ActionFakeFunction implements SkyFunction { + private final class ActionFakeFunction implements SkyFunction { @Nullable @Override - public SkyValue compute(SkyKey skyKey, Environment env) - throws SkyFunctionException, InterruptedException { + public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException { return env.getValue( new NonHermeticArtifactSkyKey( Preconditions.checkNotNull( 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 747726fa2b1b15..5bc62eef6d2bc9 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 @@ -21,7 +21,6 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.hash.Hashing; import com.google.devtools.build.lib.actions.Action; @@ -627,7 +626,12 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException { actionExecutionContext .getMetadataHandler() - .injectDirectory(out, ImmutableMap.of(child1, remoteFile1, child2, remoteFile2)); + .injectTree( + out, + TreeArtifactValue.newBuilder(out) + .putChild(child1, remoteFile1) + .putChild(child2, remoteFile2) + .build()); } }; diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java index b8d1bc272725ee..4a6ccf27d8a19f 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java @@ -57,9 +57,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.Comparator; -import java.util.HashMap; import java.util.List; -import java.util.Map; import java.util.Random; import java.util.stream.Collectors; import java.util.stream.IntStream; @@ -263,12 +261,12 @@ private class TreeArtifactExecutionFunction implements SkyFunction { @Override public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException, InterruptedException { - Map treeArtifactData = new HashMap<>(); ActionLookupData actionLookupData = (ActionLookupData) skyKey.argument(); ActionLookupValue actionLookupValue = (ActionLookupValue) env.getValue(actionLookupData.getActionLookupKey()); Action action = actionLookupValue.getAction(actionLookupData.getActionIndex()); SpecialArtifact output = (SpecialArtifact) Iterables.getOnlyElement(action.getOutputs()); + TreeArtifactValue.Builder tree = TreeArtifactValue.newBuilder(output); for (PathFragment subpath : testTreeArtifactContents) { try { TreeFileArtifact suboutput = TreeFileArtifact.createTreeOutput(output, subpath); @@ -281,17 +279,15 @@ public SkyValue compute(SkyKey skyKey, Environment env) FileArtifactValue withDigest = FileArtifactValue.createFromInjectedDigest( noDigest, path.getDigest(), !output.isConstantMetadata()); - treeArtifactData.put(suboutput, withDigest); + tree.putChild(suboutput, withDigest); } catch (IOException e) { throw new SkyFunctionException(e, Transience.TRANSIENT) {}; } } - TreeArtifactValue treeArtifactValue = TreeArtifactValue.create(treeArtifactData); - return ActionExecutionValue.create( /*artifactData=*/ ImmutableMap.of(), - ImmutableMap.of(output, treeArtifactValue), + ImmutableMap.of(output, tree.build()), /*outputSymlinks=*/ null, /*discoveredModules=*/ null, /*actionDependsOnBuildId=*/ false); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java index fb47a56050a437..5a21c1e9768a23 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java @@ -20,14 +20,12 @@ import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; -import com.google.devtools.build.lib.actions.cache.MetadataInjector; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.testutil.Scratch; import com.google.devtools.build.lib.util.Pair; @@ -55,6 +53,86 @@ public final class TreeArtifactValueTest { private final Scratch scratch = new Scratch(); private final ArtifactRoot root = ArtifactRoot.asDerivedRoot(scratch.resolve("root"), "bin"); + @Test + public void builderCreatesCorrectValue() { + SpecialArtifact parent = createTreeArtifact("bin/tree"); + TreeFileArtifact child1 = TreeFileArtifact.createTreeOutput(parent, "child1"); + TreeFileArtifact child2 = TreeFileArtifact.createTreeOutput(parent, "child2"); + FileArtifactValue metadata1 = metadataWithId(1); + FileArtifactValue metadata2 = metadataWithId(2); + + TreeArtifactValue tree = + TreeArtifactValue.newBuilder(parent) + .putChild(child1, metadata1) + .putChild(child2, metadata2) + .build(); + + assertThat(tree.getChildren()).containsExactly(child1, child2); + assertThat(tree.getChildValues()).containsExactly(child1, metadata1, child2, metadata2); + assertThat(tree.getChildPaths()) + .containsExactly(child1.getParentRelativePath(), child2.getParentRelativePath()); + assertThat(tree.getDigest()).isNotNull(); + assertThat(tree.getMetadata().getDigest()).isEqualTo(tree.getDigest()); + } + + @Test + public void empty() { + TreeArtifactValue emptyTree = TreeArtifactValue.empty(); + + assertThat(emptyTree.getChildren()).isEmpty(); + assertThat(emptyTree.getChildValues()).isEmpty(); + assertThat(emptyTree.getChildPaths()).isEmpty(); + assertThat(emptyTree.getDigest()).isNotNull(); + assertThat(emptyTree.getMetadata().getDigest()).isEqualTo(emptyTree.getDigest()); + } + + @Test + public void canonicalEmptyInstance() { + SpecialArtifact parent = createTreeArtifact("bin/tree"); + + TreeArtifactValue emptyTreeFromBuilder = TreeArtifactValue.newBuilder(parent).build(); + + assertThat(emptyTreeFromBuilder).isSameInstanceAs(TreeArtifactValue.empty()); + } + + @Test + public void cannotCreateBuilderForNonTreeArtifact() { + SpecialArtifact notTreeArtifact = + new SpecialArtifact( + root, + PathFragment.create("bin/not_tree"), + ActionsTestUtil.NULL_ARTIFACT_OWNER, + SpecialArtifactType.FILESET); + + assertThrows( + IllegalArgumentException.class, () -> TreeArtifactValue.newBuilder(notTreeArtifact)); + } + + @Test + public void cannotMixParentsWithinSingleBuilder() { + SpecialArtifact parent = createTreeArtifact("bin/tree"); + TreeFileArtifact childOfAnotherParent = + TreeFileArtifact.createTreeOutput(createTreeArtifact("bin/other_tree"), "child"); + + TreeArtifactValue.Builder builderForParent = TreeArtifactValue.newBuilder(parent); + + assertThrows( + IllegalArgumentException.class, + () -> builderForParent.putChild(childOfAnotherParent, metadataWithId(1))); + } + + @Test + public void cannotAddOmittedChildToBuilder() { + SpecialArtifact parent = createTreeArtifact("bin/tree"); + TreeFileArtifact child = TreeFileArtifact.createTreeOutput(parent, "child"); + + TreeArtifactValue.Builder builder = TreeArtifactValue.newBuilder(parent); + + assertThrows( + IllegalArgumentException.class, + () -> builder.putChild(child, FileArtifactValue.OMITTED_FILE_MARKER)); + } + @Test public void orderIndependence() { SpecialArtifact parent = createTreeArtifact("bin/tree"); @@ -64,9 +142,15 @@ public void orderIndependence() { FileArtifactValue metadata2 = metadataWithId(2); TreeArtifactValue tree1 = - TreeArtifactValue.create(ImmutableMap.of(child1, metadata1, child2, metadata2)); + TreeArtifactValue.newBuilder(parent) + .putChild(child1, metadata1) + .putChild(child2, metadata2) + .build(); TreeArtifactValue tree2 = - TreeArtifactValue.create(ImmutableMap.of(child2, metadata2, child1, metadata1)); + TreeArtifactValue.newBuilder(parent) + .putChild(child2, metadata2) + .putChild(child1, metadata1) + .build(); assertThat(tree1).isEqualTo(tree2); } @@ -77,8 +161,10 @@ public void nullDigests_equal() { TreeFileArtifact child = TreeFileArtifact.createTreeOutput(parent, "child"); FileArtifactValue metadataNoDigest = metadataWithIdNoDigest(1); - TreeArtifactValue tree1 = TreeArtifactValue.create(ImmutableMap.of(child, metadataNoDigest)); - TreeArtifactValue tree2 = TreeArtifactValue.create(ImmutableMap.of(child, metadataNoDigest)); + TreeArtifactValue tree1 = + TreeArtifactValue.newBuilder(parent).putChild(child, metadataNoDigest).build(); + TreeArtifactValue tree2 = + TreeArtifactValue.newBuilder(parent).putChild(child, metadataNoDigest).build(); assertThat(metadataNoDigest.getDigest()).isNull(); assertThat(tree1.getDigest()).isNotNull(); @@ -93,8 +179,10 @@ public void nullDigests_notEqual() { FileArtifactValue metadataNoDigest1 = metadataWithIdNoDigest(1); FileArtifactValue metadataNoDigest2 = metadataWithIdNoDigest(2); - TreeArtifactValue tree1 = TreeArtifactValue.create(ImmutableMap.of(child, metadataNoDigest1)); - TreeArtifactValue tree2 = TreeArtifactValue.create(ImmutableMap.of(child, metadataNoDigest2)); + TreeArtifactValue tree1 = + TreeArtifactValue.newBuilder(parent).putChild(child, metadataNoDigest1).build(); + TreeArtifactValue tree2 = + TreeArtifactValue.newBuilder(parent).putChild(child, metadataNoDigest2).build(); assertThat(metadataNoDigest1.getDigest()).isNull(); assertThat(metadataNoDigest2.getDigest()).isNull(); @@ -256,7 +344,7 @@ TreeArtifactValue.MultiBuilder newMultiBuilder() { } @Parameter public MultiBuilderType multiBuilderType; - private final FakeMetadataInjector metadataInjector = new FakeMetadataInjector(); + private final Map results = new HashMap<>(); @Parameters(name = "{0}") public static MultiBuilderType[] params() { @@ -267,9 +355,9 @@ public static MultiBuilderType[] params() { public void empty() { TreeArtifactValue.MultiBuilder treeArtifacts = multiBuilderType.newMultiBuilder(); - treeArtifacts.injectTo(metadataInjector); + treeArtifacts.injectTo(results::put); - assertThat(metadataInjector.injectedTreeArtifacts).isEmpty(); + assertThat(results).isEmpty(); } @Test @@ -279,15 +367,18 @@ public void singleTreeArtifact() { TreeFileArtifact child1 = TreeFileArtifact.createTreeOutput(parent, "child1"); TreeFileArtifact child2 = TreeFileArtifact.createTreeOutput(parent, "child2"); - treeArtifacts.putChild(child1, metadataWithId(1)); - treeArtifacts.putChild(child2, metadataWithId(2)); - treeArtifacts.injectTo(metadataInjector); + treeArtifacts + .putChild(child1, metadataWithId(1)) + .putChild(child2, metadataWithId(2)) + .injectTo(results::put); - assertThat(metadataInjector.injectedTreeArtifacts) + assertThat(results) .containsExactly( parent, - TreeArtifactValue.create( - ImmutableMap.of(child1, metadataWithId(1), child2, metadataWithId(2)))); + TreeArtifactValue.newBuilder(parent) + .putChild(child1, metadataWithId(1)) + .putChild(child2, metadataWithId(2)) + .build()); } @Test @@ -299,39 +390,28 @@ public void multipleTreeArtifacts() { SpecialArtifact parent2 = createTreeArtifact("bin/tree2"); TreeFileArtifact parent2Child = TreeFileArtifact.createTreeOutput(parent2, "child"); - treeArtifacts.putChild(parent1Child1, metadataWithId(1)); - treeArtifacts.putChild(parent2Child, metadataWithId(3)); - treeArtifacts.putChild(parent1Child2, metadataWithId(2)); - treeArtifacts.injectTo(metadataInjector); + treeArtifacts + .putChild(parent1Child1, metadataWithId(1)) + .putChild(parent2Child, metadataWithId(3)) + .putChild(parent1Child2, metadataWithId(2)) + .injectTo(results::put); - assertThat(metadataInjector.injectedTreeArtifacts) + assertThat(results) .containsExactly( parent1, - TreeArtifactValue.create( - ImmutableMap.of( - parent1Child1, metadataWithId(1), parent1Child2, metadataWithId(2))), + TreeArtifactValue.newBuilder(parent1) + .putChild(parent1Child1, metadataWithId(1)) + .putChild(parent1Child2, metadataWithId(2)) + .build(), parent2, - TreeArtifactValue.create(ImmutableMap.of(parent2Child, metadataWithId(3)))); + TreeArtifactValue.newBuilder(parent2) + .putChild(parent2Child, metadataWithId(3)) + .build()); } private static SpecialArtifact createTreeArtifact(String execPath) { return TreeArtifactValueTest.createTreeArtifact(execPath, ROOT); } - - private static final class FakeMetadataInjector implements MetadataInjector { - private final Map injectedTreeArtifacts = new HashMap<>(); - - @Override - public void injectFile(Artifact output, FileArtifactValue metadata) { - throw new UnsupportedOperationException(); - } - - @Override - public void injectDirectory( - SpecialArtifact output, Map children) { - injectedTreeArtifacts.put(output, TreeArtifactValue.create(children)); - } - } } private SpecialArtifact createTreeArtifact(String execPath) {