From b0b33a76cd70930ed55c31e3d0ce4125676bd8c0 Mon Sep 17 00:00:00 2001 From: Janak Ramakrishnan Date: Mon, 1 Aug 2016 19:20:31 +0000 Subject: Avoid unnecessarily nesting FilesetTraversalParams if the nesting adds no information. -- MOS_MIGRATED_REVID=129012839 --- .../build/lib/actions/FilesetTraversalParams.java | 3 +- .../lib/actions/FilesetTraversalParamsFactory.java | 50 ++++++++++++++++------ .../build/lib/skyframe/FilesetEntryFunction.java | 24 +++++++---- 3 files changed, 53 insertions(+), 24 deletions(-) (limited to 'src/main') diff --git a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java index dc6c247fe7..6c59132601 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java @@ -19,7 +19,6 @@ import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.RootedPath; - import java.util.Set; /** @@ -155,7 +154,7 @@ public interface FilesetTraversalParams { } /** Label of the Fileset rule that owns this traversal. */ - Label getOwnerLabel(); + Label getOwnerLabelForErrorMessages(); /** Returns the directory under the output path where the files will be mapped. May be empty. */ PathFragment getDestPath(); diff --git a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParamsFactory.java b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParamsFactory.java index 6b64fc9fe1..992cfc1a64 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParamsFactory.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParamsFactory.java @@ -105,7 +105,10 @@ public final class FilesetTraversalParamsFactory { } /** - * Creates traversal request parameters for a FilesetEntry wrapping another Fileset. + * Creates traversal request parameters for a FilesetEntry wrapping another Fileset. If possible, + * the original {@code nested} is returned to avoid unnecessary object creation. In that case, the + * {@code ownerLabelForErrorMessages} may be ignored. Since the wrapping traversal could not have + * an error on its own, any error messages printed will still be correct. * * @param ownerLabel the rule that created this object * @param nested the traversal params that were used for the nested (inner) Fileset @@ -114,22 +117,31 @@ public final class FilesetTraversalParamsFactory { * @param excludes optional; set of files directly below (not in a subdirectory of) the nested * Fileset that should be excluded from the outer Fileset */ - public static FilesetTraversalParams nestedTraversal(Label ownerLabel, - FilesetTraversalParams nested, PathFragment destDir, @Nullable Set excludes) { + public static FilesetTraversalParams nestedTraversal( + Label ownerLabel, + FilesetTraversalParams nested, + PathFragment destDir, + @Nullable Set excludes) { + if (destDir.segmentCount() == 0 && (excludes == null || excludes.isEmpty())) { + // Wrapping the traversal here would not lead to a different result: the output location is + // the same and there are no additional excludes. + return nested; + } // When srcdir is another Fileset, then files must be null so strip_prefix must also be null. return new NestedTraversalParams(ownerLabel, nested, destDir, excludes); } private abstract static class ParamsCommon implements FilesetTraversalParams { - private final Label ownerLabel; + private final Label ownerLabelForErrorMessages; private final PathFragment destDir; private final ImmutableSet excludes; - ParamsCommon(Label ownerLabel, PathFragment destDir, @Nullable Set excludes) { - this.ownerLabel = ownerLabel; + ParamsCommon( + Label ownerLabelForErrorMessages, PathFragment destDir, @Nullable Set excludes) { + this.ownerLabelForErrorMessages = ownerLabelForErrorMessages; this.destDir = destDir; if (excludes == null) { - this.excludes = ImmutableSet.of(); + this.excludes = ImmutableSet.of(); } else { // Order the set for the sake of deterministic fingerprinting. this.excludes = ImmutableSet.copyOf(Ordering.natural().immutableSortedCopy(excludes)); @@ -137,8 +149,8 @@ public final class FilesetTraversalParamsFactory { } @Override - public Label getOwnerLabel() { - return ownerLabel; + public Label getOwnerLabelForErrorMessages() { + return ownerLabelForErrorMessages; } @Override @@ -160,17 +172,24 @@ public final class FilesetTraversalParamsFactory { @Override public String toString() { - return super.toString() + "[" + destDir + ", " + ownerLabel + ", " + excludes + "]"; + return super.toString() + + "[" + + destDir + + ", " + + ownerLabelForErrorMessages + + ", " + + excludes + + "]"; } protected boolean internalEquals(ParamsCommon that) { - return Objects.equals(this.ownerLabel, that.ownerLabel) + return Objects.equals(this.ownerLabelForErrorMessages, that.ownerLabelForErrorMessages) && Objects.equals(this.destDir, that.destDir) && Objects.equals(this.excludes, that.excludes); } protected int internalHashCode() { - return Objects.hash(ownerLabel, destDir, excludes); + return Objects.hash(ownerLabelForErrorMessages, destDir, excludes); } } @@ -287,8 +306,11 @@ public final class FilesetTraversalParamsFactory { private static final class NestedTraversalParams extends ParamsCommon { private final FilesetTraversalParams nested; - public NestedTraversalParams(Label ownerLabel, FilesetTraversalParams nested, - PathFragment destDir, @Nullable Set excludes) { + NestedTraversalParams( + Label ownerLabel, + FilesetTraversalParams nested, + PathFragment destDir, + @Nullable Set excludes) { super(ownerLabel, destDir, excludes); this.nested = nested; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java index 80f1d419a7..777794654f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java @@ -31,7 +31,6 @@ import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; - import java.util.LinkedHashMap; import java.util.Map; import java.util.Map.Entry; @@ -190,15 +189,21 @@ public final class FilesetEntryFunction implements SkyFunction { return FilesetEntryValue.of(ImmutableSet.copyOf(outputSymlinks.values())); } - /** Stores an output symlink unless it's excluded or would overwrite an existing one. */ - private static void maybeStoreSymlink(FilesetOutputSymlink nestedLink, PathFragment destPath, + /** Stores an output symlink unless it would overwrite an existing one. */ + private static void maybeStoreSymlink( + FilesetOutputSymlink nestedLink, + PathFragment destPath, Map result) { maybeStoreSymlink(nestedLink.name, nestedLink.target, nestedLink.metadata, destPath, result); } - /** Stores an output symlink unless it's excluded or would overwrite an existing one. */ - private static void maybeStoreSymlink(PathFragment linkName, PathFragment linkTarget, - String metadata, PathFragment destPath, Map result) { + /** Stores an output symlink unless it would overwrite an existing one. */ + private static void maybeStoreSymlink( + PathFragment linkName, + PathFragment linkTarget, + String metadata, + PathFragment destPath, + Map result) { linkName = destPath.getRelative(linkName); if (!result.containsKey(linkName)) { result.put(linkName, new FilesetOutputSymlink(linkName, linkTarget, metadata)); @@ -237,11 +242,14 @@ public final class FilesetEntryFunction implements SkyFunction { private static String createErrorInfo(FilesetTraversalParams t) { if (t.getDirectTraversal().isPresent()) { DirectTraversal direct = t.getDirectTraversal().get(); - return String.format("Fileset '%s' traversing %s '%s'", t.getOwnerLabel(), + return String.format( + "Fileset '%s' traversing %s '%s'", + t.getOwnerLabelForErrorMessages(), direct.isPackage() ? "package" : "file (or directory)", direct.getRoot().getRelativePart().getPathString()); } else { - return String.format("Fileset '%s' traversing another Fileset", t.getOwnerLabel()); + return String.format( + "Fileset '%s' traversing another Fileset", t.getOwnerLabelForErrorMessages()); } } -- cgit v1.2.3