Skip to content

Commit

Permalink
Create separate class for Aspect keys with base aspects
Browse files Browse the repository at this point in the history 
Extend `AspectKey` with 2 classes: `SimpleAspectKey` for aspects not depending on other aspects and `AspectKeyWithBaseAspect` for aspects depending on one or more base aspects. This is for memory optimization as in most cases the aspect will not depend on other aspects and the `baseKeys` list will be empty and there is no need to hold a reference to it.

Also replace `Objects.hashCode` with `HashCodes.hashObjects`

PiperOrigin-RevId: 558245545
Change-Id: Iea8dc3ab71cc7f72dc591136a76f92b3b3831527
  • Loading branch information
@mai93 @copybara-github
mai93 authored and copybara-github committed Aug 18, 2023
1 parent 0fa5e35 commit 5658b43
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 35 deletions.
115 changes: 80 additions & 35 deletions src/main/java/com/google/devtools/build/lib/skyframe/AspectKeyCreator.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.skyframe;

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

import com.google.common.base.Objects;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
Expand All @@ -23,6 +25,7 @@
import com.google.devtools.build.lib.packages.AspectDescriptor;
import com.google.devtools.build.lib.packages.AspectParameters;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.util.HashCodes;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
import java.util.Comparator;
Expand Down Expand Up @@ -81,21 +84,25 @@ public final int hashCode() {

// Specific subtypes of aspect keys.

/** Represents an aspect applied to a particular target. */
/**
* Represents an aspect applied to a particular target.
*
* <p>Extended by two classes: {@link SimpleAspectKey} for aspects that do not depend on other
* aspects and {@link AspectKeyWithBaseAspects} for aspects depending on one or more base aspects.
* This separation is for memory optimization as in most cases the aspect will not depend on other
* aspects and its {@code baseKeys} list will be empty.
*/
@AutoCodec
public static final class AspectKey extends AspectBaseKey {
public abstract static class AspectKey extends AspectBaseKey {
private static final SkyKeyInterner<AspectKey> interner = SkyKey.newInterner();

private final ImmutableList<AspectKey> baseKeys;
private final AspectDescriptor aspectDescriptor;

private AspectKey(
ConfiguredTargetKey baseConfiguredTargetKey,
ImmutableList<AspectKey> baseKeys,
AspectDescriptor aspectDescriptor,
int hashCode) {
super(baseConfiguredTargetKey, hashCode);
this.baseKeys = baseKeys;
this.aspectDescriptor = aspectDescriptor;
}

Expand All @@ -105,26 +112,37 @@ static AspectKey createAspectKey(
ConfiguredTargetKey baseConfiguredTargetKey,
ImmutableList<AspectKey> baseKeys,
AspectDescriptor aspectDescriptor) {
// Keep the list of {@code baseKeys} sorted to avoid running the same aspect twice because of
// different {@code baseKeys} order even if the {@link AspectKey} objects in the list are the
// same.
if (baseKeys.isEmpty()) {
return interner.intern(
new SimpleAspectKey(
baseConfiguredTargetKey,
aspectDescriptor,
HashCodes.hashObjects(baseConfiguredTargetKey, aspectDescriptor)));
}
// Keep the list of {@code baseKeys} sorted to avoid running the same aspect twice because
// of different {@code baseKeys} order even if the {@link AspectKey} objects in the list are
// the same.
ImmutableList<AspectKey> sortedBaseKeys =
ImmutableList.sortedCopyOf(
Comparator.comparing((AspectKey k) -> k.getAspectClass().getName())
// For aspects that appear more than once, comparing aspects parameters based on
// their string representation to avoid adding a lot of logic for this comparison
// which is expected to be not frequently needed.
// their string representation to avoid adding a lot of logic for this
// comparison which is expected to be not frequently needed.
.thenComparing(k -> k.getParameters().toString()),
baseKeys);

return interner.intern(
new AspectKey(
new AspectKeyWithBaseAspects(
baseConfiguredTargetKey,
sortedBaseKeys,
aspectDescriptor,
Objects.hashCode(baseConfiguredTargetKey, sortedBaseKeys, aspectDescriptor)));
HashCodes.hashObjects(baseConfiguredTargetKey, sortedBaseKeys, aspectDescriptor)));
}

public abstract ImmutableList<AspectKey> getBaseKeys();

public abstract String getDescription();

@Override
public SkyFunctionName functionName() {
return SkyFunctions.ASPECT;
Expand Down Expand Up @@ -158,20 +176,6 @@ public AspectDescriptor getAspectDescriptor() {
return aspectDescriptor;
}

@Nullable
public ImmutableList<AspectKey> getBaseKeys() {
return baseKeys;
}

public String getDescription() {
if (baseKeys.isEmpty()) {
return String.format("%s of %s", aspectDescriptor.getAspectClass().getName(), getLabel());
} else {
return String.format(
"%s on top of %s", aspectDescriptor.getAspectClass().getName(), baseKeys);
}
}

/**
* Returns the key of the configured target of the aspect; that is, the configuration in which
* the aspect will be evaluated.
Expand All @@ -192,7 +196,7 @@ public boolean equals(Object other) {
}
AspectKey that = (AspectKey) other;
return hashCode() == that.hashCode()
&& Objects.equal(baseKeys, that.baseKeys)
&& Objects.equal(getBaseKeys(), that.getBaseKeys())
&& Objects.equal(getBaseConfiguredTargetKey(), that.getBaseConfiguredTargetKey())
&& Objects.equal(aspectDescriptor, that.aspectDescriptor);
}
Expand All @@ -202,15 +206,16 @@ public String prettyPrint() {
return "null";
}

String baseKeysString = baseKeys.isEmpty() ? "" : String.format(" (over %s)", baseKeys);
String baseKeysString =
getBaseKeys().isEmpty() ? "" : String.format(" (over %s)", getBaseKeys());
return String.format(
"%s with aspect %s%s",
getLabel(), aspectDescriptor.getAspectClass().getName(), baseKeysString);
}

@Override
public String toString() {
return (baseKeys == null ? getLabel() : baseKeys.toString())
return (getBaseKeys().isEmpty() ? getLabel() : getBaseKeys().toString())
+ "#"
+ aspectDescriptor
+ " "
Expand All @@ -220,24 +225,64 @@ public String toString() {
}

AspectKey withLabel(Label label) {
ImmutableList.Builder<AspectKey> newBaseKeys = ImmutableList.builder();
for (AspectKey baseKey : baseKeys) {
newBaseKeys.add(baseKey.withLabel(label));
}
ImmutableList<AspectKey> newBaseKeys =
getBaseKeys().stream().map(k -> k.withLabel(label)).collect(toImmutableList());

return createAspectKey(
ConfiguredTargetKey.builder()
.setLabel(label)
.setConfigurationKey(getBaseConfiguredTargetKey().getConfigurationKey())
.build(),
newBaseKeys.build(),
newBaseKeys,
aspectDescriptor);
}

@Override
public SkyKeyInterner<AspectKey> getSkyKeyInterner() {
return interner;
}

static class SimpleAspectKey extends AspectKey {
SimpleAspectKey(
ConfiguredTargetKey baseConfiguredTargetKey,
AspectDescriptor aspectDescriptor,
int hashCode) {
super(baseConfiguredTargetKey, aspectDescriptor, hashCode);
}

@Override
public ImmutableList<AspectKey> getBaseKeys() {
return ImmutableList.of();
}

@Override
public String getDescription() {
return String.format("%s of %s", getAspectClass().getName(), getLabel());
}
}

static class AspectKeyWithBaseAspects extends AspectKey {
private final ImmutableList<AspectKey> baseKeys;

private AspectKeyWithBaseAspects(
ConfiguredTargetKey baseConfiguredTargetKey,
ImmutableList<AspectKey> baseKeys,
AspectDescriptor aspectDescriptor,
int hashCode) {
super(baseConfiguredTargetKey, aspectDescriptor, hashCode);
this.baseKeys = baseKeys;
}

@Override
public ImmutableList<AspectKey> getBaseKeys() {
return baseKeys;
}

@Override
public String getDescription() {
return String.format("%s on top of %s", getAspectClass().getName(), baseKeys);
}
}
}

/**
Expand Down Expand Up @@ -265,7 +310,7 @@ static TopLevelAspectsKey createInternal(
targetLabel,
baseConfiguredTargetKey,
topLevelAspectsParameters,
Objects.hashCode(
HashCodes.hashObjects(
topLevelAspectsClasses,
targetLabel,
baseConfiguredTargetKey,
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/skyframe/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -738,6 +738,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
"//src/main/java/com/google/devtools/build/lib/util:hash_codes",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//third_party:guava",
"//third_party:jsr305",
Expand Down

0 comments on commit 5658b43

Please sign in to comment.