Skip to content

Commit

Permalink
Remove dep fingerprinting, part 2 of 2.
Browse files Browse the repository at this point in the history 
Removes SkyValue#getValueFingerprint and Version#getFingerprint. This should also free a little bit of memory that was used in configured target values.

getValueFingerprint is kept in two SkyValue subclasses (FileStateValue and FileArtifactValue) where it is actually used for another purpose. However, the method is removed from the SkyValue interface and other classes where it is not being used.

PiperOrigin-RevId: 286601483
  • Loading branch information
Googler authored and Copybara-Service committed Dec 20, 2019
1 parent 9c1853a commit ce6ad29
Show file tree
Hide file tree
Showing 27 changed files with 42 additions and 259 deletions.
18 changes: 2 additions & 16 deletions src/main/java/com/google/devtools/build/lib/actions/ActionLookupValue.java
View file
Expand Up @@ -18,18 +18,11 @@
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.math.BigInteger;
import javax.annotation.Nullable;

/**
* Base class for all values which can provide the generating action of an artifact.
*/
/** Base class for all values which can provide the generating action of an artifact. */
public abstract class ActionLookupValue implements SkyValue {
@Nullable private final transient BigInteger nonceVersion;

protected ActionLookupValue(@Nullable BigInteger nonceVersion) {
this.nonceVersion = nonceVersion;
}
/** Returns a list of actions registered by this {@link SkyValue}. */
public abstract ImmutableList<ActionAnalysisMetadata> getActions();

Expand Down Expand Up @@ -57,9 +50,7 @@ public ActionTemplate<?> getActionTemplate(int index) {
return (ActionTemplate<?>) result;
}

/**
* Returns the number of {@link Action} objects present in this value.
*/
/** Returns the number of {@link Action} objects present in this value. */
public int getNumActions() {
return getActions().size();
}
Expand All @@ -70,11 +61,6 @@ public SourceArtifact getSourceArtifact() {
return null;
}

@Override
public BigInteger getValueFingerprint() {
return nonceVersion;
}

/**
* All subclasses of ActionLookupValue "own" artifacts with {@link ArtifactOwner}s that are
* subclasses of ActionLookupKey. This allows callers to easily find the value key, while
Expand Down
12 changes: 3 additions & 9 deletions src/main/java/com/google/devtools/build/lib/actions/BasicActionLookupValue.java
View file
Expand Up @@ -17,8 +17,6 @@
import com.google.common.base.MoreObjects;
import com.google.common.base.MoreObjects.ToStringHelper;
import com.google.common.collect.ImmutableList;
import java.math.BigInteger;
import javax.annotation.Nullable;

/**
* Basic implementation of {@link ActionLookupValue} where the value itself owns and maintains
Expand All @@ -27,17 +25,13 @@
public class BasicActionLookupValue extends ActionLookupValue {
protected final ImmutableList<ActionAnalysisMetadata> actions;

protected BasicActionLookupValue(
ImmutableList<ActionAnalysisMetadata> actions,
@Nullable BigInteger nonceVersion) {
super(nonceVersion);
protected BasicActionLookupValue(ImmutableList<ActionAnalysisMetadata> actions) {
this.actions = actions;
}

@VisibleForTesting
public BasicActionLookupValue(
Actions.GeneratingActions generatingActions, @Nullable BigInteger nonceVersion) {
this(generatingActions.getActions(), nonceVersion);
public BasicActionLookupValue(Actions.GeneratingActions generatingActions) {
this(generatingActions.getActions());
}

@Override
Expand Down
1 change: 0 additions & 1 deletion src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java
View file
Expand Up @@ -92,7 +92,6 @@ public abstract class FileArtifactValue implements SkyValue, HasDigest {
public abstract FileContentsProxy getContentsProxy();

@Nullable
@Override
public BigInteger getValueFingerprint() {
byte[] digest = getDigest();
if (digest != null) {
Expand Down
1 change: 0 additions & 1 deletion src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java
View file
Expand Up @@ -180,7 +180,6 @@ public byte[] getDigest() {
throw new IllegalStateException();
}

@Override
public abstract BigInteger getValueFingerprint();

@Override
Expand Down
49 changes: 2 additions & 47 deletions src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
View file
Expand Up @@ -31,20 +31,13 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.util.BigIntegerFingerprint;
import com.google.devtools.build.skyframe.SkyValue;
import java.math.BigInteger;
import java.util.Comparator;
import java.util.Map;
import java.util.Map.Entry;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import javax.annotation.Nullable;

/**
* A value representing an executed action.
*/
/** A value representing an executed action. */
@Immutable
@ThreadSafe
public class ActionExecutionValue implements SkyValue {
Expand All @@ -59,11 +52,6 @@ public class ActionExecutionValue implements SkyValue {

@Nullable private final NestedSet<Artifact> discoveredModules;

/**
* Transient because it can be reconstituted on demand, and {@link BigInteger} isn't serializable.
*/
@Nullable private transient BigInteger valueFingerprint;

/**
* @param artifactData Map from Artifacts to corresponding {@link FileArtifactValue}.
* @param treeArtifactData All tree artifact data.
Expand Down Expand Up @@ -172,38 +160,6 @@ public NestedSet<Artifact> getDiscoveredModules() {
return discoveredModules;
}

@Override
public BigInteger getValueFingerprint() {
if (valueFingerprint == null) {
BigIntegerFingerprint fp = new BigIntegerFingerprint();
sortMapByArtifactExecPathAndStream(artifactData)
.forEach(
(entry) -> {
fp.addPath(entry.getKey().getExecPath());
fp.addBigIntegerOrdered(entry.getValue().getValueFingerprint());
});
sortMapByArtifactExecPathAndStream(treeArtifactData)
.forEach(
(entry) -> {
fp.addPath(entry.getKey().getExecPath());
fp.addBigIntegerOrdered(entry.getValue().getValueFingerprint());
});
if (outputSymlinks != null) {
for (FilesetOutputSymlink symlink : outputSymlinks) {
fp.addBigIntegerOrdered(symlink.getFingerprint());
}
}
valueFingerprint = fp.getFingerprint();
}
return valueFingerprint;
}

private static <T> Stream<Entry<Artifact, T>> sortMapByArtifactExecPathAndStream(
Map<Artifact, T> inputMap) {
return inputMap.entrySet().stream()
.sorted(Comparator.comparing(Entry::getKey, Artifact.EXEC_PATH_COMPARATOR));
}

@Override
public String toString() {
return MoreObjects.toStringHelper(this)
Expand Down Expand Up @@ -310,8 +266,7 @@ private static <V> ImmutableMap<Artifact, V> transformKeys(

ActionExecutionValue transformForSharedAction(ImmutableSet<Artifact> outputs) {
Map<OwnerlessArtifactWrapper, Artifact> newArtifactMap =
outputs
.stream()
outputs.stream()
.collect(Collectors.toMap(OwnerlessArtifactWrapper::new, Function.identity()));
// This is only called for shared actions, so we'll almost certainly have to transform all keys
// in all sets.
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionValue.java
View file
Expand Up @@ -27,7 +27,7 @@
*/
public final class ActionTemplateExpansionValue extends BasicActionLookupValue {
ActionTemplateExpansionValue(GeneratingActions generatingActions) {
super(generatingActions, /*nonceVersion=*/ null);
super(generatingActions);
}

static ActionTemplateExpansionKey key(ActionLookupKey actionLookupKey, int actionIndex) {
Expand Down
27 changes: 0 additions & 27 deletions src/main/java/com/google/devtools/build/lib/skyframe/AggregatingArtifactValue.java
View file
Expand Up @@ -17,12 +17,9 @@
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.util.BigIntegerFingerprint;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.skyframe.SkyValue;
import java.math.BigInteger;
import java.util.Collection;
import javax.annotation.Nullable;

/** Value for aggregating artifacts, which must be expanded to a set of other artifacts. */
class AggregatingArtifactValue implements SkyValue {
Expand Down Expand Up @@ -57,30 +54,6 @@ FileArtifactValue getSelfData() {
return selfData;
}

protected final BigIntegerFingerprint getFingerprintBuilder() {
BigIntegerFingerprint fp = new BigIntegerFingerprint();
fp.addNullableBigIntegerOrdered(selfData.getValueFingerprint());
addFingerprint(fileInputs, fp);
addFingerprint(directoryInputs, fp);
return fp;
}

private static void addFingerprint(
ImmutableList<? extends Pair<Artifact, ? extends SkyValue>> list,
BigIntegerFingerprint fingerprint) {
list.forEach(
pair -> {
fingerprint.addPath(pair.first.getExecPath());
fingerprint.addBigIntegerOrdered(pair.second.getValueFingerprint());
});
}

@Nullable
@Override
public BigInteger getValueFingerprint() {
return getFingerprintBuilder().getFingerprint();
}

@SuppressWarnings("EqualsGetClass") // RunfilesArtifactValue not equal to Aggregating.
@Override
public boolean equals(Object o) {
Expand Down
14 changes: 3 additions & 11 deletions src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
View file
Expand Up @@ -74,11 +74,9 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.build.skyframe.ValueOrException;
import java.math.BigInteger;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Map;
import java.util.function.Supplier;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -111,22 +109,18 @@ public final class AspectFunction implements SkyFunction {
*/
private final boolean storeTransitivePackagesForPackageRootResolution;

private final Supplier<BigInteger> nonceVersion;

AspectFunction(
BuildViewProvider buildViewProvider,
RuleClassProvider ruleClassProvider,
@Nullable SkylarkImportLookupFunction skylarkImportLookupFunctionForInlining,
boolean storeTransitivePackagesForPackageRootResolution,
BuildOptions defaultBuildOptions,
Supplier<BigInteger> nonceVersion) {
BuildOptions defaultBuildOptions) {
this.buildViewProvider = buildViewProvider;
this.ruleClassProvider = ruleClassProvider;
this.skylarkImportLookupFunctionForInlining = skylarkImportLookupFunctionForInlining;
this.storeTransitivePackagesForPackageRootResolution =
storeTransitivePackagesForPackageRootResolution;
this.defaultBuildOptions = defaultBuildOptions;
this.nonceVersion = nonceVersion;
}

/**
Expand Down Expand Up @@ -616,8 +610,7 @@ private AspectValue createAliasAspect(
originalTarget.getLabel(),
originalTarget.getLocation(),
ConfiguredAspect.forAlias(real.getConfiguredAspect()),
transitivePackagesForPackageRootResolution,
nonceVersion.get());
transitivePackagesForPackageRootResolution);
}

@Nullable
Expand Down Expand Up @@ -703,8 +696,7 @@ private AspectValue createAspect(
configuredAspect,
transitivePackagesForPackageRootResolution == null
? null
: transitivePackagesForPackageRootResolution.build(),
nonceVersion.get());
: transitivePackagesForPackageRootResolution.build());
}

@Override
Expand Down
6 changes: 2 additions & 4 deletions src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
View file
Expand Up @@ -34,7 +34,6 @@
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey.KeyAndHost;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.skyframe.SkyFunctionName;
import java.math.BigInteger;
import javax.annotation.Nullable;

/** An aspect in the context of the Skyframe graph. */
Expand Down Expand Up @@ -440,9 +439,8 @@ public AspectValue(
Label label,
Location location,
ConfiguredAspect configuredAspect,
NestedSet<Package> transitivePackagesForPackageRootResolution,
BigInteger nonceVersion) {
super(configuredAspect.getActions(), nonceVersion);
NestedSet<Package> transitivePackagesForPackageRootResolution) {
super(configuredAspect.getActions());
this.label = Preconditions.checkNotNull(label, actions);
this.aspect = Preconditions.checkNotNull(aspect, label);
this.location = Preconditions.checkNotNull(location, label);
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionValue.java
View file
Expand Up @@ -34,7 +34,7 @@ public class BuildInfoCollectionValue extends BasicActionLookupValue {
private final BuildInfoCollection collection;

BuildInfoCollectionValue(BuildInfoCollection collection, GeneratingActions generatingActions) {
super(generatingActions, /*nonceVersion=*/ null);
super(generatingActions);
this.collection = collection;
}

Expand Down
16 changes: 4 additions & 12 deletions src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
View file
Expand Up @@ -79,7 +79,6 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.build.skyframe.ValueOrException;
import java.math.BigInteger;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
Expand All @@ -90,7 +89,6 @@
import java.util.Set;
import java.util.concurrent.Semaphore;
import java.util.concurrent.TimeUnit;
import java.util.function.Supplier;
import java.util.stream.Collectors;
import javax.annotation.Nullable;

Expand Down Expand Up @@ -139,7 +137,6 @@ public synchronized Exception getCause() {
private final Semaphore cpuBoundSemaphore;
private final BuildOptions defaultBuildOptions;
@Nullable private final ConfiguredTargetProgressReceiver configuredTargetProgress;
private final Supplier<BigInteger> nonceVersion;

/**
* Indicates whether the set of packages transitively loaded for a given {@link
Expand All @@ -157,8 +154,7 @@ public synchronized Exception getCause() {
boolean storeTransitivePackagesForPackageRootResolution,
boolean shouldUnblockCpuWorkWhenFetchingDeps,
BuildOptions defaultBuildOptions,
@Nullable ConfiguredTargetProgressReceiver configuredTargetProgress,
Supplier<BigInteger> nonceVersion) {
@Nullable ConfiguredTargetProgressReceiver configuredTargetProgress) {
this.buildViewProvider = buildViewProvider;
this.ruleClassProvider = ruleClassProvider;
this.cpuBoundSemaphore = cpuBoundSemaphore;
Expand All @@ -167,7 +163,6 @@ public synchronized Exception getCause() {
this.shouldUnblockCpuWorkWhenFetchingDeps = shouldUnblockCpuWorkWhenFetchingDeps;
this.defaultBuildOptions = defaultBuildOptions;
this.configuredTargetProgress = configuredTargetProgress;
this.nonceVersion = nonceVersion;
}

private void acquireWithLogging(SkyKey key) throws InterruptedException {
Expand Down Expand Up @@ -246,8 +241,7 @@ public SkyValue compute(SkyKey key, Environment env) throws ConfiguredTargetFunc
GeneratingActions.EMPTY,
transitivePackagesForPackageRootResolution == null
? null
: transitivePackagesForPackageRootResolution.build(),
nonceVersion.get());
: transitivePackagesForPackageRootResolution.build());
}

// This line is only needed for accurate error messaging. Say this target has a circular
Expand Down Expand Up @@ -950,8 +944,7 @@ private ConfiguredTargetValue createConfiguredTarget(
ruleConfiguredTarget,
transitivePackagesForPackageRootResolution == null
? null
: transitivePackagesForPackageRootResolution.build(),
nonceVersion.get());
: transitivePackagesForPackageRootResolution.build());
} else {
GeneratingActions generatingActions;
// Check for conflicting actions within this configured target (that indicates a bug in the
Expand All @@ -971,8 +964,7 @@ private ConfiguredTargetValue createConfiguredTarget(
generatingActions,
transitivePackagesForPackageRootResolution == null
? null
: transitivePackagesForPackageRootResolution.build(),
nonceVersion.get());
: transitivePackagesForPackageRootResolution.build());
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportValue.java
View file
Expand Up @@ -28,7 +28,7 @@ public class CoverageReportValue extends BasicActionLookupValue {
@AutoCodec public static final CoverageReportKey COVERAGE_REPORT_KEY = new CoverageReportKey();

CoverageReportValue(GeneratingActions generatingActions) {
super(generatingActions, /*nonceVersion=*/ null);
super(generatingActions);
}

static class CoverageReportKey extends ActionLookupKey {
Expand Down

0 comments on commit ce6ad29

Please sign in to comment.