Skip to content

Commit

Permalink
Custom-serialize derived artifacts. This allows us to intern them mor…
Browse files Browse the repository at this point in the history 
…e efficiently.

Also simplify ArtifactFactory handling in SkyframeExecutor: the ArtifactFactory is set at construction time, so there's no need to mess around with suppliers.

PiperOrigin-RevId: 315383662
  • Loading branch information
@janakdr @copybara-github
janakdr authored and copybara-github committed Jun 9, 2020
1 parent 917e119 commit 40f2f0e
Show file tree
Hide file tree
Showing 18 changed files with 131 additions and 112 deletions.
116 changes: 79 additions & 37 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 @@ -24,7 +24,6 @@
import com.google.common.base.Preconditions;
import com.google.common.base.Predicate;
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;
Expand All @@ -34,7 +33,6 @@
import com.google.devtools.build.lib.cmdline.LabelConstants;
import com.google.devtools.build.lib.collect.nestedset.Depset;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.concurrent.BlazeInterners;
import com.google.devtools.build.lib.concurrent.ThreadSafety;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.skyframe.SkyFunctions;
Expand Down Expand Up @@ -326,10 +324,7 @@ public boolean hasKnownGeneratingAction() {
}

/** An artifact corresponding to a file in the output tree, generated by an {@link Action}. */
@AutoCodec
public static class DerivedArtifact extends Artifact {
/** Only used for deserializing artifacts. */
private static final Interner<DerivedArtifact> INTERNER = BlazeInterners.newWeakInterner();

/**
* An {@link ActionLookupKey} until {@link #setGeneratingActionKey} is set, at which point it is
Expand Down Expand Up @@ -417,16 +412,48 @@ boolean ownersEqual(Artifact other) {
}
return this.owner.equals(that.owner);
}
}

/**
* The {@code rootRelativePath is a few characters shorter than the {@code execPath}, so we save
* a few bytes by serializing it rather than the {@code execPath}, especially when the {@code
* root} is common to many artifacts and therefore memoized.
*/
@AutoCodec.VisibleForSerialization
@AutoCodec.Instantiator
static DerivedArtifact createForSerialization(
ArtifactRoot root, PathFragment rootRelativePath, ActionLookupData generatingActionKey) {
@SuppressWarnings("unused") // Codec used by reflection.
private static class DerivedArtifactCodec implements ObjectCodec<DerivedArtifact> {
@Override
public Class<DerivedArtifact> getEncodedClass() {
return DerivedArtifact.class;
}

@Override
public void serialize(
SerializationContext context, DerivedArtifact obj, CodedOutputStream codedOut)
throws SerializationException, IOException {
context.serialize(obj.getRoot(), codedOut);
context.serialize(obj.getGeneratingActionKey(), codedOut);
context.serialize(obj.getRootRelativePath(), codedOut);
}

@Override
public DerivedArtifact deserialize(DeserializationContext context, CodedInputStream codedIn)
throws SerializationException, IOException {
ArtifactRoot root = context.deserialize(codedIn);
ActionLookupData generatingActionKey = context.deserialize(codedIn);
DerivedArtifact artifact =
new DerivedArtifact(
root,
validateAndGetRootExecPath(root, generatingActionKey, context, codedIn),
generatingActionKey.getActionLookupKey(),
/*contentBasedPath=*/ false);
artifact.setGeneratingActionKey(generatingActionKey);
return context
.getDependency(ArtifactResolver.ArtifactResolverSupplier.class)
.intern(artifact);
}

static PathFragment validateAndGetRootExecPath(
ArtifactRoot root,
ActionLookupData generatingActionKey,
DeserializationContext context,
CodedInputStream codedIn)
throws IOException, SerializationException {
PathFragment rootRelativePath = context.deserialize(codedIn);
if (rootRelativePath == null
|| rootRelativePath.isAbsolute() != root.getRoot().isAbsolute()) {
throw new IllegalArgumentException(
Expand All @@ -439,15 +466,7 @@ static DerivedArtifact createForSerialization(
}
Preconditions.checkState(
!root.isSourceRoot(), "Root not derived: %s %s", root, rootRelativePath);
PathFragment rootExecPath = root.getExecPath();
DerivedArtifact artifact =
new DerivedArtifact(
root,
rootExecPath.getRelative(rootRelativePath),
generatingActionKey.getActionLookupKey(),
/*contentBasedPath=*/ false);
artifact.setGeneratingActionKey(generatingActionKey);
return INTERNER.intern(artifact);
return root.getExecPath().getRelative(rootRelativePath);
}
}

Expand Down Expand Up @@ -729,7 +748,6 @@ public enum SpecialArtifactType {
* having to keep around the attribute for the rest we save some memory.
*/
@Immutable
@AutoCodec
public static final class SpecialArtifact extends DerivedArtifact {
private final SpecialArtifactType type;

Expand All @@ -740,19 +758,6 @@ public SpecialArtifact(
this.type = type;
}

@AutoCodec.VisibleForSerialization
@AutoCodec.Instantiator
static SpecialArtifact create(
ArtifactRoot root,
PathFragment execPath,
SpecialArtifactType type,
ActionLookupData generatingActionKey) {
SpecialArtifact result =
new SpecialArtifact(root, execPath, generatingActionKey.getActionLookupKey(), type);
result.setGeneratingActionKey(generatingActionKey);
return result;
}

@Override
public final boolean isFileset() {
return type == SpecialArtifactType.FILESET;
Expand Down Expand Up @@ -796,6 +801,43 @@ public ShareabilityOfValue getShareabilityOfValue() {
}
}

// Keep in sync with DerivedArtifactCodec.
@SuppressWarnings("unused") // Used by reflection
private static class SpecialArtifactCodec implements ObjectCodec<SpecialArtifact> {
@Override
public Class<SpecialArtifact> getEncodedClass() {
return SpecialArtifact.class;
}

@Override
public void serialize(
SerializationContext context, SpecialArtifact obj, CodedOutputStream codedOut)
throws SerializationException, IOException {
context.serialize(obj.getRoot(), codedOut);
context.serialize(obj.getGeneratingActionKey(), codedOut);
context.serialize(obj.type, codedOut);
context.serialize(obj.getRootRelativePath(), codedOut);
}

@Override
public SpecialArtifact deserialize(DeserializationContext context, CodedInputStream codedIn)
throws SerializationException, IOException {
ArtifactRoot root = context.deserialize(codedIn);
ActionLookupData generatingActionKey = context.deserialize(codedIn);
SpecialArtifactType type = context.deserialize(codedIn);
SpecialArtifact artifact =
new SpecialArtifact(
root,
DerivedArtifactCodec.validateAndGetRootExecPath(
root, generatingActionKey, context, codedIn),
generatingActionKey.getActionLookupKey(),
type);
artifact.setGeneratingActionKey(generatingActionKey);
return (SpecialArtifact)
context.getDependency(ArtifactResolverSupplier.class).intern(artifact);
}
}

/**
* A special kind of artifact that represents a concrete file created at execution time under its
* associated parent TreeArtifact.
Expand Down
9 changes: 4 additions & 5 deletions src/main/java/com/google/devtools/build/lib/actions/ArtifactResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,8 @@ Map<PathFragment, Artifact> resolveSourceArtifacts(

Path getPathFromSourceExecPath(Path execRoot, PathFragment execPath);

/**
* Supplies an {@link ArtifactFactory}. We define a custom interface because parameterized types
* are not allowed as dependencies to serialization.
*/
interface ArtifactResolverSupplier extends Supplier<ArtifactResolver> {}
/** Supplies an {@link ArtifactFactory} and allows for interning of derived artifacts. */
interface ArtifactResolverSupplier extends Supplier<ArtifactResolver> {
Artifact.DerivedArtifact intern(Artifact.DerivedArtifact original);
}
}
6 changes: 4 additions & 2 deletions src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,8 @@ TreeArtifactValue getTreeArtifactValue(Artifact artifact) {
* <p>Primarily needed by {@link FilesystemValueChecker}. Also called by {@link ArtifactFunction}
* when aggregating a {@link TreeArtifactValue} out of action template expansion outputs.
*/
ImmutableMap<Artifact, FileArtifactValue> getAllFileValues() {
// Visible only for testing: should be package-private.
public ImmutableMap<Artifact, FileArtifactValue> getAllFileValues() {
return artifactData;
}

Expand All @@ -174,7 +175,8 @@ ImmutableMap<Artifact, FileArtifactValue> getAllFileValues() {
*
* <p>Should only be needed by {@link FilesystemValueChecker}.
*/
ImmutableMap<Artifact, TreeArtifactValue> getAllTreeArtifactValues() {
// Visible only for testing: should be package-private.
public ImmutableMap<Artifact, TreeArtifactValue> getAllTreeArtifactValues() {
return treeArtifactData;
}

Expand Down
24 changes: 6 additions & 18 deletions src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,11 @@
package com.google.devtools.build.lib.skyframe;

import com.google.common.base.Preconditions;
import com.google.common.base.Supplier;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.actions.ActionKeyContext;
import com.google.devtools.build.lib.actions.Actions;
import com.google.devtools.build.lib.actions.Actions.GeneratingActions;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactFactory;
import com.google.devtools.build.lib.actions.ArtifactRoot;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoCollection;
import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory;
Expand All @@ -32,7 +29,6 @@
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.skyframe.BuildInfoCollectionValue.BuildInfoKeyAndConfig;
import com.google.devtools.build.lib.skyframe.PrecomputedValue.Precomputed;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
Expand All @@ -48,11 +44,9 @@ public class BuildInfoCollectionFunction implements SkyFunction {
new Precomputed<>("build_info_factories");

private final ActionKeyContext actionKeyContext;
// Supplier only because the artifact factory has not yet been created at constructor time.
private final Supplier<ArtifactFactory> artifactFactory;
private final ArtifactFactory artifactFactory;

BuildInfoCollectionFunction(
ActionKeyContext actionKeyContext, Supplier<ArtifactFactory> artifactFactory) {
BuildInfoCollectionFunction(ActionKeyContext actionKeyContext, ArtifactFactory artifactFactory) {
this.actionKeyContext = actionKeyContext;
this.artifactFactory = artifactFactory;
}
Expand Down Expand Up @@ -81,17 +75,11 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept
BuildInfoFactory buildInfoFactory = buildInfoFactories.get(keyAndConfig.getInfoKey());
Preconditions.checkState(buildInfoFactory.isEnabled(config));

final ArtifactFactory factory = artifactFactory.get();
BuildInfoContext context =
new BuildInfoContext() {
@Override
public Artifact getBuildInfoArtifact(
PathFragment rootRelativePath, ArtifactRoot root, BuildInfoType type) {
return type == BuildInfoType.NO_REBUILD
? factory.getConstantMetadataArtifact(rootRelativePath, root, keyAndConfig)
: factory.getDerivedArtifact(rootRelativePath, root, keyAndConfig);
}
};
(rootRelativePath, root, type) ->
type == BuildInfoType.NO_REBUILD
? artifactFactory.getConstantMetadataArtifact(rootRelativePath, root, keyAndConfig)
: artifactFactory.getDerivedArtifact(rootRelativePath, root, keyAndConfig);
BuildInfoCollection collection =
buildInfoFactory.create(
context,
Expand Down
10 changes: 0 additions & 10 deletions src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,6 @@ private SequencedSkyframeExecutor(
ExternalPackageHelper externalPackageHelper,
ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile,
BuildOptions defaultBuildOptions,
MutableArtifactFactorySupplier mutableArtifactFactorySupplier,
@Nullable ManagedDirectoriesKnowledge managedDirectoriesKnowledge) {
super(
skyframeExecutorConsumerOnInit,
Expand All @@ -195,7 +194,6 @@ private SequencedSkyframeExecutor(
GraphInconsistencyReceiver.THROWING,
defaultBuildOptions,
new PackageProgressReceiver(),
mutableArtifactFactorySupplier,
new ConfiguredTargetProgressReceiver(),
/*nonexistentFileReceiver=*/ null,
managedDirectoriesKnowledge);
Expand Down Expand Up @@ -1072,8 +1070,6 @@ public static final class Builder {
private Factory workspaceStatusActionFactory;
private Iterable<? extends DiffAwareness.Factory> diffAwarenessFactories = ImmutableList.of();
private Iterable<SkyValueDirtinessChecker> customDirtinessCheckers = ImmutableList.of();
private MutableArtifactFactorySupplier mutableArtifactFactorySupplier =
new MutableArtifactFactorySupplier();
private Consumer<SkyframeExecutor> skyframeExecutorConsumerOnInit = skyframeExecutor -> {};
private SkyFunction blacklistedPackagePrefixesFunction;

Expand Down Expand Up @@ -1110,7 +1106,6 @@ public SequencedSkyframeExecutor build() {
externalPackageHelper,
actionOnIOExceptionReadingBuildFile,
defaultBuildOptions,
mutableArtifactFactorySupplier,
managedDirectoriesKnowledge);
skyframeExecutor.init();
return skyframeExecutor;
Expand Down Expand Up @@ -1192,11 +1187,6 @@ public Builder setActionOnIOExceptionReadingBuildFile(
return this;
}

public Builder setMutableArtifactFactorySupplier(
MutableArtifactFactorySupplier mutableArtifactFactorySupplier) {
this.mutableArtifactFactorySupplier = mutableArtifactFactorySupplier;
return this;
}
public Builder setSkyframeExecutorConsumerOnInit(
Consumer<SkyframeExecutor> skyframeExecutorConsumerOnInit) {
this.skyframeExecutorConsumerOnInit = skyframeExecutorConsumerOnInit;
Expand Down
Loading

0 comments on commit 40f2f0e

Please sign in to comment.