Skip to content

Commit

Permalink
Add NestedSet<PathFragment>-typed compile build variable
Browse files Browse the repository at this point in the history 
This change is intended to reduce memory usage slightly by possibly reusing nested set instances retained elsewhere. It also prepares for C++ path mapping.
  • Loading branch information
@fmeum
fmeum committed May 21, 2024
1 parent 8bce174 commit b1b0313
Show file tree
Hide file tree
Showing 2 changed files with 102 additions and 40 deletions.
70 changes: 67 additions & 3 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.ExpansionException;
import com.google.devtools.build.lib.starlarkbuildapi.cpp.CcToolchainVariablesApi;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.util.LinkedHashMap;
import java.util.List;
Expand Down Expand Up @@ -1026,7 +1027,8 @@ private StringSequence(Iterable<String> values) {

@Override
public ImmutableList<VariableValue> getSequenceValue(String variableName) {
ImmutableList.Builder<VariableValue> sequences = ImmutableList.builder();
ImmutableList.Builder<VariableValue> sequences =
ImmutableList.builderWithExpectedSize(values.size());
for (String value : values) {
sequences.add(new StringValue(value));
}
Expand Down Expand Up @@ -1085,8 +1087,10 @@ private StringSetSequence(NestedSet<String> values) {

@Override
public ImmutableList<VariableValue> getSequenceValue(String variableName) {
ImmutableList.Builder<VariableValue> sequences = ImmutableList.builder();
for (String value : values.toList()) {
List<String> valuesList = values.toList();
ImmutableList.Builder<VariableValue> sequences =
ImmutableList.builderWithExpectedSize(valuesList.size());
for (String value : valuesList) {
sequences.add(new StringValue(value));
}
return sequences.build();
Expand Down Expand Up @@ -1119,6 +1123,53 @@ public int hashCode() {
}
}

@Immutable
private static final class PathFragmentSetSequence extends VariableValueAdapter {
private final NestedSet<PathFragment> values;

private PathFragmentSetSequence(NestedSet<PathFragment> values) {
Preconditions.checkNotNull(values);
this.values = values;
}

@Override
public ImmutableList<VariableValue> getSequenceValue(String variableName) {
List<PathFragment> valuesList = values.toList();
ImmutableList.Builder<VariableValue> sequences =
ImmutableList.builderWithExpectedSize(valuesList.size());
for (PathFragment value : valuesList) {
sequences.add(new StringValue(value.getSafePathString()));
}
return sequences.build();
}

@Override
public String getVariableTypeName() {
return Sequence.SEQUENCE_VARIABLE_TYPE_NAME;
}

@Override
public boolean isTruthy() {
return !values.isEmpty();
}

@Override
public boolean equals(Object other) {
if (this == other) {
return true;
}
if (!(other instanceof PathFragmentSetSequence otherPathFragments)) {
return false;
}
return values.shallowEquals(otherPathFragments.values);
}

@Override
public int hashCode() {
return values.shallowHashCode();
}
}

/**
* Single structure value. Be careful not to create sequences of single structures, as the memory
* overhead is prohibitively big.
Expand Down Expand Up @@ -1410,6 +1461,19 @@ public Builder addStringSequenceVariable(String name, Iterable<String> values) {
return this;
}

/**
* Add a sequence variable that expands {@code name} to {@link PathFragment} {@code values}.
*
* <p>Accepts values as NestedSet. Nested set is stored directly, not cloned, not flattened.
*/
@CanIgnoreReturnValue
public Builder addPathFragmentSequenceVariable(String name, NestedSet<PathFragment> values) {
checkVariableNotPresentAlready(name);
Preconditions.checkNotNull(values, "Cannot set null as a value for variable '%s'", name);
variablesMap.put(name, new PathFragmentSetSequence(values));
return this;
}

/**
* Add a variable built using {@code VariableValueBuilder} api that expands {@code name} to the
* value returned by the {@code builder}.
Expand Down
72 changes: 35 additions & 37 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariables.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.rules.cpp;

import static com.google.common.collect.ImmutableList.toImmutableList;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -193,10 +195,10 @@ public static CcToolchainVariables setupVariablesOrReportRuleError(
variablesExtensions,
additionalBuildVariables,
directModuleMaps,
getSafePathStrings(includeDirs),
getSafePathStrings(quoteIncludeDirs),
getSafePathStrings(systemIncludeDirs),
getSafePathStrings(frameworkIncludeDirs),
includeDirs,
quoteIncludeDirs,
systemIncludeDirs,
frameworkIncludeDirs,
defines,
localDefines);
}
Expand Down Expand Up @@ -260,10 +262,10 @@ public static CcToolchainVariables setupVariablesOrThrowEvalException(
variablesExtensions,
additionalBuildVariables,
directModuleMaps,
includeDirs,
quoteIncludeDirs,
systemIncludeDirs,
frameworkIncludeDirs,
asPathFragments(includeDirs),
asPathFragments(quoteIncludeDirs),
asPathFragments(systemIncludeDirs),
asPathFragments(frameworkIncludeDirs),
defines,
localDefines);
}
Expand Down Expand Up @@ -292,10 +294,10 @@ private static CcToolchainVariables setupVariables(
ImmutableList<VariablesExtension> variablesExtensions,
ImmutableMap<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
NestedSet<String> includeDirs,
NestedSet<String> quoteIncludeDirs,
NestedSet<String> systemIncludeDirs,
NestedSet<String> frameworkIncludeDirs,
NestedSet<PathFragment> includeDirs,
NestedSet<PathFragment> quoteIncludeDirs,
NestedSet<PathFragment> systemIncludeDirs,
NestedSet<PathFragment> frameworkIncludeDirs,
Iterable<String> defines,
Iterable<String> localDefines) {
CcToolchainVariables.Builder buildVariables = CcToolchainVariables.builder(parent);
Expand Down Expand Up @@ -466,10 +468,10 @@ public static void setupCommonVariables(
List<VariablesExtension> variablesExtensions,
Map<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
List<PathFragment> includeDirs,
List<PathFragment> quoteIncludeDirs,
List<PathFragment> systemIncludeDirs,
List<PathFragment> frameworkIncludeDirs,
ImmutableList<PathFragment> includeDirs,
ImmutableList<PathFragment> quoteIncludeDirs,
ImmutableList<PathFragment> systemIncludeDirs,
ImmutableList<PathFragment> frameworkIncludeDirs,
Iterable<String> defines,
Iterable<String> localDefines) {
setupCommonVariablesInternal(
Expand All @@ -482,10 +484,12 @@ public static void setupCommonVariables(
variablesExtensions,
additionalBuildVariables,
directModuleMaps,
getSafePathStrings(includeDirs),
getSafePathStrings(quoteIncludeDirs),
getSafePathStrings(systemIncludeDirs),
getSafePathStrings(frameworkIncludeDirs),
// Stable order NestedSets wrapping ImmutableLists are interned, otherwise this would be
// a clear waste of memory as the single caller ensure that there are no duplicates.
NestedSetBuilder.wrap(Order.STABLE_ORDER, includeDirs),
NestedSetBuilder.wrap(Order.STABLE_ORDER, quoteIncludeDirs),
NestedSetBuilder.wrap(Order.STABLE_ORDER, systemIncludeDirs),
NestedSetBuilder.wrap(Order.STABLE_ORDER, frameworkIncludeDirs),
defines,
localDefines);
}
Expand All @@ -500,10 +504,10 @@ private static void setupCommonVariablesInternal(
List<VariablesExtension> variablesExtensions,
Map<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
NestedSet<String> includeDirs,
NestedSet<String> quoteIncludeDirs,
NestedSet<String> systemIncludeDirs,
NestedSet<String> frameworkIncludeDirs,
NestedSet<PathFragment> includeDirs,
NestedSet<PathFragment> quoteIncludeDirs,
NestedSet<PathFragment> systemIncludeDirs,
NestedSet<PathFragment> frameworkIncludeDirs,
Iterable<String> defines,
Iterable<String> localDefines) {
Preconditions.checkNotNull(directModuleMaps);
Expand All @@ -526,17 +530,17 @@ private static void setupCommonVariablesInternal(
// Module inputs will be set later when the action is executed.
buildVariables.addStringSequenceVariable(MODULE_FILES.getVariableName(), ImmutableSet.of());
}
buildVariables.addStringSequenceVariable(INCLUDE_PATHS.getVariableName(), includeDirs);
buildVariables.addStringSequenceVariable(
buildVariables.addPathFragmentSequenceVariable(INCLUDE_PATHS.getVariableName(), includeDirs);
buildVariables.addPathFragmentSequenceVariable(
QUOTE_INCLUDE_PATHS.getVariableName(), quoteIncludeDirs);
buildVariables.addStringSequenceVariable(
buildVariables.addPathFragmentSequenceVariable(
SYSTEM_INCLUDE_PATHS.getVariableName(), systemIncludeDirs);

if (!includes.isEmpty()) {
buildVariables.addStringSequenceVariable(INCLUDES.getVariableName(), includes);
}

buildVariables.addStringSequenceVariable(
buildVariables.addPathFragmentSequenceVariable(
FRAMEWORK_PATHS.getVariableName(), frameworkIncludeDirs);

Iterable<String> allDefines;
Expand All @@ -563,17 +567,11 @@ private static void setupCommonVariablesInternal(
}
}

/** Get the safe path strings for a list of paths to use in the build variables. */
private static NestedSet<String> getSafePathStrings(NestedSet<PathFragment> paths) {
return getSafePathStrings(paths.toList());
}

/** Get the safe path strings for a list of paths to use in the build variables. */
private static NestedSet<String> getSafePathStrings(List<PathFragment> paths) {
// Using ImmutableSet first to remove duplicates, then NestedSet for smaller memory footprint.
private static NestedSet<PathFragment> asPathFragments(NestedSet<String> paths) {
// Using ImmutableList as the final type to benefit from NestedSet interning.
return NestedSetBuilder.wrap(
Order.STABLE_ORDER,
Iterables.transform(ImmutableSet.copyOf(paths), PathFragment::getSafePathString));
paths.toList().stream().map(PathFragment::create).collect(toImmutableList()));
}

public String getVariableName() {
Expand Down

0 comments on commit b1b0313

Please sign in to comment.