Skip to content

Commit

Permalink
Fix memory regression introduced by default providers
Browse files Browse the repository at this point in the history 
The memory regression was introduced in bazelbuild@360fb4d , now default providers
are optimized and are built only on demand for all types of targets.

PiperOrigin-RevId: 152957220
  • Loading branch information
@vladmos @buchgr
vladmos authored and buchgr committed Apr 13, 2017
1 parent 57daac0 commit aa5e060
Show file tree
Hide file tree
Showing 6 changed files with 84 additions and 70 deletions.
47 changes: 36 additions & 11 deletions src/main/java/com/google/devtools/build/lib/analysis/AbstractConfiguredTarget.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.packages.ClassObjectConstructor;
import com.google.devtools.build.lib.packages.PackageSpecification;
import com.google.devtools.build.lib.packages.SkylarkClassObject;
import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.syntax.ClassObject;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.EvalUtils;
import java.util.concurrent.atomic.AtomicReference;
import javax.annotation.Nullable;

/**
Expand All @@ -42,6 +44,9 @@ public abstract class AbstractConfiguredTarget

private final NestedSet<PackageSpecification> visibility;

// Cached on-demand default provider
private final AtomicReference<DefaultProvider> defaultProvider = new AtomicReference<>();

// Accessors for Skylark
private static final String DATA_RUNFILES_FIELD = "data_runfiles";
private static final String DEFAULT_RUNFILES_FIELD = "default_runfiles";
Expand Down Expand Up @@ -106,15 +111,13 @@ public <P extends TransitiveInfoProvider> P getProvider(Class<P> provider) {

@Override
public Object getValue(String name) {
// Standard fields should be proxied to their default provider object
DefaultProvider defaultProvider =
(DefaultProvider) get(DefaultProvider.SKYLARK_CONSTRUCTOR.getKey());
switch (name) {
case FILES_FIELD:
case DEFAULT_RUNFILES_FIELD:
case DATA_RUNFILES_FIELD:
case FilesToRunProvider.SKYLARK_NAME:
return defaultProvider.getValue(name);
// Standard fields should be proxied to their default provider object
return getDefaultProvider().getValue(name);
case LABEL_FIELD:
return getLabel();
default:
Expand All @@ -130,14 +133,10 @@ public Object getIndex(Object key, Location loc) throws EvalException {
EvalUtils.getDataTypeName(key)));
}
ClassObjectConstructor constructor = (ClassObjectConstructor) key;
SkylarkProviders provider = getProvider(SkylarkProviders.class);
if (provider != null) {
Object declaredProvider = provider.getDeclaredProvider(constructor.getKey());
if (declaredProvider != null) {
return declaredProvider;
}
Object declaredProvider = get(constructor.getKey());
if (declaredProvider != null) {
return declaredProvider;
}
// Either provider or declaredProvider is null
throw new EvalException(loc, String.format(
"Object of type Target doesn't contain declared provider %s",
constructor.getPrintableName()));
Expand Down Expand Up @@ -175,4 +174,30 @@ public ImmutableCollection<String> getKeys() {
FILES_FIELD,
FilesToRunProvider.SKYLARK_NAME);
}

private DefaultProvider getDefaultProvider() {
if (defaultProvider.get() == null) {
defaultProvider.compareAndSet(
null,
DefaultProvider.build(
getProvider(RunfilesProvider.class),
getProvider(FileProvider.class),
getProvider(FilesToRunProvider.class)));
}
return defaultProvider.get();
}

/** Returns a declared provider provided by this target. Only meant to use from Skylark. */
@Nullable
@Override
public SkylarkClassObject get(ClassObjectConstructor.Key providerKey) {
if (providerKey.equals(DefaultProvider.SKYLARK_CONSTRUCTOR.getKey())) {
return getDefaultProvider();
}
SkylarkProviders skylarkProviders = getProvider(SkylarkProviders.class);
if (skylarkProviders != null) {
return skylarkProviders.getDeclaredProvider(providerKey);
}
return null;
}
}
62 changes: 48 additions & 14 deletions src/main/java/com/google/devtools/build/lib/analysis/DefaultProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
// limitations under the License.
package com.google.devtools.build.lib.analysis;

import com.google.common.collect.ImmutableCollection;
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.concurrent.ThreadSafety.Immutable;
Expand All @@ -22,6 +24,7 @@
import com.google.devtools.build.lib.packages.SkylarkClassObject;
import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
import java.util.Map;
import java.util.concurrent.atomic.AtomicReference;

/** DefaultProvider is provided by all targets implicitly and contains all standard fields. */
@Immutable
Expand All @@ -31,6 +34,17 @@ public final class DefaultProvider extends SkylarkClassObject {
private static final String DATA_RUNFILES_FIELD = "data_runfiles";
private static final String DEFAULT_RUNFILES_FIELD = "default_runfiles";
private static final String FILES_FIELD = "files";
private static final ImmutableList<String> KEYS =
ImmutableList.of(
DATA_RUNFILES_FIELD,
DEFAULT_RUNFILES_FIELD,
FILES_FIELD,
FilesToRunProvider.SKYLARK_NAME);

private final RunfilesProvider runfilesProvider;
private final FileProvider fileProvider;
private final FilesToRunProvider filesToRunProvider;
private final AtomicReference<SkylarkNestedSet> files = new AtomicReference<>();

public static final String SKYLARK_NAME = "DefaultInfo";
public static final ClassObjectConstructor SKYLARK_CONSTRUCTOR =
Expand All @@ -43,27 +57,47 @@ protected SkylarkClassObject createInstanceFromSkylark(Object[] args, Location l
}
};

private DefaultProvider(ClassObjectConstructor constructor, Map<String, Object> values) {
super(constructor, values);
private DefaultProvider(
ClassObjectConstructor constructor,
RunfilesProvider runfilesProvider,
FileProvider fileProvider,
FilesToRunProvider filesToRunProvider) {
// Fields map is not used here to prevent memory regression
super(constructor, ImmutableMap.<String, Object>of());
this.runfilesProvider = runfilesProvider;
this.fileProvider = fileProvider;
this.filesToRunProvider = filesToRunProvider;
}

public static DefaultProvider build(
RunfilesProvider runfilesProvider,
FileProvider fileProvider,
FilesToRunProvider filesToRunProvider) {
ImmutableMap.Builder<String, Object> attrBuilder = new ImmutableMap.Builder<>();
if (runfilesProvider != null) {
attrBuilder.put(DATA_RUNFILES_FIELD, runfilesProvider.getDataRunfiles());
attrBuilder.put(DEFAULT_RUNFILES_FIELD, runfilesProvider.getDefaultRunfiles());
} else {
attrBuilder.put(DATA_RUNFILES_FIELD, Runfiles.EMPTY);
attrBuilder.put(DEFAULT_RUNFILES_FIELD, Runfiles.EMPTY);
}
return new DefaultProvider(
SKYLARK_CONSTRUCTOR, runfilesProvider, fileProvider, filesToRunProvider);
}

attrBuilder.put(
FILES_FIELD, SkylarkNestedSet.of(Artifact.class, fileProvider.getFilesToBuild()));
attrBuilder.put(FilesToRunProvider.SKYLARK_NAME, filesToRunProvider);
@Override
public Object getValue(String name) {
switch (name) {
case DATA_RUNFILES_FIELD:
return (runfilesProvider == null) ? Runfiles.EMPTY : runfilesProvider.getDataRunfiles();
case DEFAULT_RUNFILES_FIELD:
return (runfilesProvider == null) ? Runfiles.EMPTY : runfilesProvider.getDefaultRunfiles();
case FILES_FIELD:
if (files.get() == null) {
files.compareAndSet(
null, SkylarkNestedSet.of(Artifact.class, fileProvider.getFilesToBuild()));
}
return files.get();
case FilesToRunProvider.SKYLARK_NAME:
return filesToRunProvider;
}
return null;
}

return new DefaultProvider(SKYLARK_CONSTRUCTOR, attrBuilder.build());
@Override
public ImmutableCollection<String> getKeys() {
return KEYS;
}
}
17 changes: 0 additions & 17 deletions src/main/java/com/google/devtools/build/lib/analysis/FileConfiguredTarget.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,14 @@

package com.google.devtools.build.lib.analysis;

import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.packages.ClassObjectConstructor;
import com.google.devtools.build.lib.packages.FileTarget;
import com.google.devtools.build.lib.packages.SkylarkClassObject;
import com.google.devtools.build.lib.rules.fileset.FilesetProvider;
import com.google.devtools.build.lib.rules.test.InstrumentedFilesProvider;
import com.google.devtools.build.lib.util.FileType;
import javax.annotation.Nullable;

/**
* A ConfiguredTarget for a source FileTarget. (Generated files use a
Expand All @@ -44,17 +40,10 @@ public abstract class FileConfiguredTarget extends AbstractConfiguredTarget
FileProvider fileProvider = new FileProvider(filesToBuild);
FilesToRunProvider filesToRunProvider =
FilesToRunProvider.fromSingleExecutableArtifact(artifact);
SkylarkClassObject defaultProvider =
DefaultProvider.build(null, fileProvider, filesToRunProvider);
SkylarkProviders skylarkProviders =
new SkylarkProviders(
ImmutableMap.<String, Object>of(),
ImmutableMap.of(DefaultProvider.SKYLARK_CONSTRUCTOR.getKey(), defaultProvider));
TransitiveInfoProviderMap.Builder builder =
TransitiveInfoProviderMap.builder()
.put(VisibilityProvider.class, this)
.put(LicensesProvider.class, this)
.put(SkylarkProviders.class, skylarkProviders)
.add(fileProvider)
.add(filesToRunProvider);
if (this instanceof FilesetProvider) {
Expand Down Expand Up @@ -93,10 +82,4 @@ public <P extends TransitiveInfoProvider> P getProvider(Class<P> provider) {
public Object get(String providerKey) {
return null;
}

@Nullable
@Override
public SkylarkClassObject get(ClassObjectConstructor.Key providerKey) {
return getProvider(SkylarkProviders.class).getDeclaredProvider(providerKey);
}
}
9 changes: 0 additions & 9 deletions src/main/java/com/google/devtools/build/lib/analysis/MergedConfiguredTarget.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,9 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.packages.ClassObjectConstructor;
import com.google.devtools.build.lib.packages.SkylarkClassObject;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;

/**
* A single dependency with its configured target and aspects merged together.
Expand Down Expand Up @@ -57,12 +54,6 @@ public Object get(String providerKey) {
return getProvider(SkylarkProviders.class).getValue(providerKey);
}

@Nullable
@Override
public SkylarkClassObject get(ClassObjectConstructor.Key providerKey) {
return getProvider(SkylarkProviders.class).getDeclaredProvider(providerKey);
}

@Override
public <P extends TransitiveInfoProvider> P getProvider(Class<P> providerClass) {
AnalysisUtils.checkProvider(providerClass);
Expand Down
11 changes: 0 additions & 11 deletions src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTarget.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,8 @@
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
import com.google.devtools.build.lib.analysis.config.RunUnder;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.packages.ClassObjectConstructor;
import com.google.devtools.build.lib.packages.OutputFile;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.SkylarkClassObject;
import com.google.devtools.build.lib.util.Preconditions;
import javax.annotation.Nullable;

Expand Down Expand Up @@ -110,15 +108,6 @@ public Object get(String providerKey) {
return getProvider(SkylarkProviders.class).getValue(providerKey);
}

/**
* Returns a declared provider provided by this target. Only meant to use from Skylark.
*/
@Override
public SkylarkClassObject get(ClassObjectConstructor.Key providerKey) {
return getProvider(SkylarkProviders.class).getDeclaredProvider(providerKey);
}


@Override
public final Rule getTarget() {
return (Rule) super.getTarget();
Expand Down
8 changes: 0 additions & 8 deletions src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,14 +135,6 @@ public ConfiguredTarget build() {
addSkylarkTransitiveInfo(OutputGroupProvider.SKYLARK_NAME, outputGroupProvider);
}

// Populate default provider fields and build it
DefaultProvider defaultProvider =
DefaultProvider.build(
providersBuilder.getProvider(RunfilesProvider.class),
providersBuilder.getProvider(FileProvider.class),
filesToRunProvider);
skylarkDeclaredProviders.put(defaultProvider.getConstructor().getKey(), defaultProvider);

TransitiveInfoProviderMap providers = providersBuilder.build();
addRegisteredProvidersToSkylarkProviders(providers);

Expand Down

0 comments on commit aa5e060

Please sign in to comment.