Skip to content

Commit

Permalink
Automated rollback of commit d491bf1.
Browse files Browse the repository at this point in the history 
*** Reason for rollback ***

Still bugs lurking. See linked bug.

*** Original change description ***

Automated rollback of commit eb58707.

*** Reason for rollback ***

Depot has been fixed.

RELNOTES[INC]: If the same artifact is generated by two distinct but identical actions, and a downstream action has both those actions' outputs in its inputs, the artifact will now appear twice in the downstream action's inputs. If this causes problems in Skylark actions, you can use the uniquify=True argument in Args.add_args.

PiperOrigin-RevId: 204997569
  • Loading branch information
@janakdr
janakdr authored and Copybara-Service committed Jul 17, 2018
1 parent ceafeae commit f309ad3
Show file tree
Hide file tree
Showing 16 changed files with 214 additions and 259 deletions.
30 changes: 3 additions & 27 deletions src/main/java/com/google/devtools/build/lib/actions/Actions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@
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.common.escape.Escaper;
import com.google.common.escape.Escapers;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.vfs.OsPathPolicy;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Collection;
import java.util.Comparator;
import java.util.HashMap;
import java.util.Iterator;
Expand Down Expand Up @@ -74,39 +74,15 @@ public static boolean canBeShared(
}
// Don't bother to check input and output counts first; the expected result for these tests is
// to always be true (i.e., that this method returns true).
if (!artifactsEqualWithoutOwner(actionA.getMandatoryInputs(), actionB.getMandatoryInputs())) {
if (!Iterables.elementsEqual(actionA.getMandatoryInputs(), actionB.getMandatoryInputs())) {
return false;
}
if (!artifactsEqualWithoutOwner(actionA.getOutputs(), actionB.getOutputs())) {
if (!Iterables.elementsEqual(actionA.getOutputs(), actionB.getOutputs())) {
return false;
}
return true;
}

private static boolean artifactsEqualWithoutOwner(
Iterable<Artifact> iterable1, Iterable<Artifact> iterable2) {
if (iterable1 instanceof Collection && iterable2 instanceof Collection) {
Collection<?> collection1 = (Collection<?>) iterable1;
Collection<?> collection2 = (Collection<?>) iterable2;
if (collection1.size() != collection2.size()) {
return false;
}
}
Iterator<Artifact> iterator1 = iterable1.iterator();
Iterator<Artifact> iterator2 = iterable2.iterator();
while (iterator1.hasNext()) {
if (!iterator2.hasNext()) {
return false;
}
Artifact artifact1 = iterator1.next();
Artifact artifact2 = iterator2.next();
if (!artifact1.equalsWithoutOwner(artifact2)) {
return false;
}
}
return !iterator2.hasNext();
}

/**
* Finds action conflicts. An action conflict happens if two actions generate the same output
* artifact. Shared actions are tolerated. See {@link #canBeShared} for details.
Expand Down
115 changes: 60 additions & 55 deletions src/main/java/com/google/devtools/build/lib/actions/Artifact.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,14 @@
import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.base.Predicate;
import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Interner;
import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata.MiddlemanType;
import com.google.devtools.build.lib.actions.ArtifactResolver.ArtifactResolverSupplier;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.concurrent.BlazeInterners;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.shell.ShellUtils;
import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext;
Expand All @@ -47,7 +47,6 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.protobuf.CodedInputStream;
import com.google.protobuf.CodedOutputStream;
import java.io.IOException;
Expand All @@ -56,6 +55,7 @@
import java.util.Comparator;
import java.util.List;
import java.util.Objects;
import java.util.concurrent.ExecutionException;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -110,8 +110,7 @@ public class Artifact
ActionInput,
FileApi,
Comparable<Object>,
CommandLineItem,
SkyKey {
CommandLineItem {

/** Compares artifact according to their exec paths. Sorts null values first. */
@SuppressWarnings("ReferenceEquality") // "a == b" is an optimization
Expand Down Expand Up @@ -168,7 +167,8 @@ public interface ArtifactExpander {
/** A Predicate that evaluates to true if the Artifact is not a middleman artifact. */
public static final Predicate<Artifact> MIDDLEMAN_FILTER = input -> !input.isMiddlemanArtifact();

private static final Interner<Artifact> ARTIFACT_INTERNER = BlazeInterners.newWeakInterner();
private static final Cache<InternedArtifact, Artifact> ARTIFACT_INTERNER =
CacheBuilder.newBuilder().weakValues().build();

private final int hashCode;
private final ArtifactRoot root;
Expand Down Expand Up @@ -203,7 +203,15 @@ static Artifact createForSerialization(
if (artifact.isSourceArtifact()) {
return artifact;
} else {
return ARTIFACT_INTERNER.intern(artifact);
return intern(artifact);
}
}

private static Artifact intern(Artifact artifact) {
try {
return ARTIFACT_INTERNER.get(new InternedArtifact(artifact), () -> artifact);
} catch (ExecutionException e) {
throw new IllegalStateException(e);
}
}

Expand Down Expand Up @@ -249,9 +257,6 @@ private Artifact(
throw new IllegalArgumentException(
"it is illegal to create an artifact with an empty execPath");
}
// The ArtifactOwner is not part of this computation because it is very rare that two Artifacts
// have the same execPath and different owners, so a collision is fine there. If this is
// changed, OwnerlessArtifactWrapper must also be changed.
this.hashCode = execPath.hashCode() + this.getClass().hashCode() * 13;
this.root = root;
this.execPath = execPath;
Expand Down Expand Up @@ -479,15 +484,6 @@ public SourceArtifact(ArtifactRoot root, PathFragment execPath, ArtifactOwner ow
public SourceArtifact asSourceArtifact() {
return this;
}

/**
* SourceArtifacts are compared without their owners, since owners do not affect behavior,
* unlike with derived artifacts, whose owners determine their generating actions.
*/
@Override
public boolean equals(Object other) {
return other instanceof SourceArtifact && equalsWithoutOwner((SourceArtifact) other);
}
}

/**
Expand Down Expand Up @@ -617,6 +613,34 @@ public PathFragment getParentRelativePath() {
}
}

/**
* Wraps an artifact for interning because we need to check the artifact owner when doing equals.
*/
private static final class InternedArtifact {
private final Artifact wrappedArtifact;

InternedArtifact(Artifact artifact) {
this.wrappedArtifact = artifact;
}

@Override
public boolean equals(Object other) {
if (!(other instanceof InternedArtifact)) {
return false;
}
if (!getClass().equals(other.getClass())) {
return false;
}
InternedArtifact that = (InternedArtifact) other;
return Artifact.equalWithOwner(wrappedArtifact, that.wrappedArtifact);
}

@Override
public final int hashCode() {
return wrappedArtifact.hashCode();
}
}

/**
* Returns the relative path to this artifact relative to its root. (Useful
* when deriving output filenames from input files, etc.)
Expand Down Expand Up @@ -670,26 +694,36 @@ public final String prettyPrint() {
}

@Override
public boolean equals(Object other) {
public final boolean equals(Object other) {
if (!(other instanceof Artifact)) {
return false;
}
if (!getClass().equals(other.getClass())) {
return false;
}
// We don't bother to check root in the equivalence relation, because we
// assume that no root is an ancestor of another one.
Artifact that = (Artifact) other;
return equalsWithoutOwner(that) && owner.equals(that.getArtifactOwner());
return Objects.equals(this.execPath, that.execPath) && Objects.equals(this.root, that.root);
}

public boolean equalsWithoutOwner(Artifact other) {
return Objects.equals(this.execPath, other.execPath) && Objects.equals(this.root, other.root);
/**
* Compare equality including Artifact owner equality, a notable difference compared to the
* {@link #equals(Object)} method of {@link Artifact}.
*/
public static boolean equalWithOwner(@Nullable Artifact first, @Nullable Artifact second) {
if (first != null) {
return first.equals(second) && first.getArtifactOwner().equals(second.getArtifactOwner());
} else {
return second == null;
}
}

@Override
public final int hashCode() {
// This is just execPath.hashCode() (along with the class). We cache a copy in the Artifact
// object to reduce LLC misses during operations which build a HashSet out of many Artifacts.
// This is a slight loss for memory but saves ~1% overall CPU in some real builds.
// This is just path.hashCode(). We cache a copy in the Artifact object to reduce LLC misses
// during operations which build a HashSet out of many Artifacts. This is a slight loss for
// memory but saves ~1% overall CPU in some real builds.
return hashCode;
}

Expand Down Expand Up @@ -997,33 +1031,4 @@ public void repr(SkylarkPrinter printer) {
printer.append("<generated file " + rootRelativePath + ">");
}
}

/**
* A utility class that compares {@link Artifact}s without taking their owners into account.
* Should only be used for detecting action conflicts and merging shared action data.
*/
public static class OwnerlessArtifactWrapper {
private final Artifact artifact;

public OwnerlessArtifactWrapper(Artifact artifact) {
this.artifact = artifact;
}

@Override
public int hashCode() {
// Depends on the fact that Artifact#hashCode does not use ArtifactOwner.
return artifact.hashCode();
}

@Override
public boolean equals(Object obj) {
return obj instanceof OwnerlessArtifactWrapper
&& this.artifact.equalsWithoutOwner(((OwnerlessArtifactWrapper) obj).artifact);
}
}

@Override
public SkyFunctionName functionName() {
return ARTIFACT;
}
}
Loading

0 comments on commit f309ad3

Please sign in to comment.