From e1726e2522a2fc7bf24b29b82fa4d9a105980819 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 19 Dec 2023 11:35:59 -0800 Subject: [PATCH] Attempt to make main repo mapping inverse more efficient During `bazel query`, `Label#getDisplayForm(mainRepoMapping)` might be called many many times. We can optimize for that case without sacrificing too much memory by computing a reverse mapping for the main repo mapping only. Fixes https://github.com/bazelbuild/bazel/issues/20613. Closes https://github.com/bazelbuild/bazel/pull/20617. PiperOrigin-RevId: 592297440 Change-Id: Iaaa709a51fe39556f03408080c1fe5e73689b761 --- .../build/lib/cmdline/RepositoryMapping.java | 84 ++++++++++++++----- .../skyframe/RepositoryMappingFunction.java | 7 +- .../lib/skyframe/RepositoryMappingValue.java | 11 +++ 3 files changed, 80 insertions(+), 22 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java index 0246c930171776..b15bdadc61cc22 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java @@ -14,9 +14,10 @@ package com.google.devtools.build.lib.cmdline; -import com.google.auto.value.AutoValue; +import com.google.common.base.Objects; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Maps; import java.util.HashMap; import java.util.Map; import java.util.Map.Entry; @@ -24,43 +25,66 @@ import javax.annotation.Nullable; /** - * A class to distinguish repository mappings for repos from WORKSPACE and Bzlmod. + * Stores the mapping from apparent repo name to canonical repo name, from the viewpoint of an + * "owner repo". * *

For repositories from the WORKSPACE file, if the requested repo doesn't exist in the mapping, * we fallback to the requested name. For repositories from Bzlmod, we return null to let the caller - * decide what to do. This class won't be needed if one day we don't define external repositories in - * the WORKSPACE file since {@code fallback} would always be false. + * decide what to do. */ -@AutoValue -public abstract class RepositoryMapping { - - // Always fallback to the requested name +public class RepositoryMapping { + /* A repo mapping that always falls back to using the apparent name as the canonical name. */ public static final RepositoryMapping ALWAYS_FALLBACK = createAllowingFallback(ImmutableMap.of()); + private final ImmutableMap entries; + @Nullable private final RepositoryName ownerRepo; + + private RepositoryMapping( + Map entries, @Nullable RepositoryName ownerRepo) { + this.entries = ImmutableMap.copyOf(entries); + this.ownerRepo = ownerRepo; + } + /** Returns all the entries in this repo mapping. */ - public abstract ImmutableMap entries(); + public final ImmutableMap entries() { + return entries; + } /** * The owner repo of this repository mapping. It is for providing useful debug information when * repository mapping fails due to enforcing strict dependency, therefore it's only recorded when - * we don't fallback to the requested repo name. + * we don't fall back to the requested repo name. */ @Nullable - abstract RepositoryName ownerRepo(); + public final RepositoryName ownerRepo() { + return ownerRepo; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof RepositoryMapping)) { + return false; + } + RepositoryMapping that = (RepositoryMapping) o; + return Objects.equal(entries, that.entries) && Objects.equal(ownerRepo, that.ownerRepo); + } + + @Override + public int hashCode() { + return Objects.hashCode(entries, ownerRepo); + } public static RepositoryMapping create( Map entries, RepositoryName ownerRepo) { - return createInternal( + return new RepositoryMapping( Preconditions.checkNotNull(entries), Preconditions.checkNotNull(ownerRepo)); } public static RepositoryMapping createAllowingFallback(Map entries) { - return createInternal(Preconditions.checkNotNull(entries), null); - } - - private static RepositoryMapping createInternal( - Map entries, RepositoryName ownerRepo) { - return new AutoValue_RepositoryMapping(ImmutableMap.copyOf(entries), ownerRepo); + return new RepositoryMapping(Preconditions.checkNotNull(entries), null); } /** @@ -70,7 +94,7 @@ private static RepositoryMapping createInternal( public RepositoryMapping withAdditionalMappings(Map additionalMappings) { HashMap allMappings = new HashMap<>(additionalMappings); allMappings.putAll(entries()); - return createInternal(allMappings, ownerRepo()); + return new RepositoryMapping(allMappings, ownerRepo()); } /** @@ -134,6 +158,26 @@ public RepositoryMapping composeWith(RepositoryMapping other) { RepositoryName mappedName = other.get(entry.getValue().getName()); entries.put(entry.getKey(), mappedName.isVisible() ? mappedName : entry.getValue()); } - return createInternal(entries, null); + return new RepositoryMapping(entries, null); + } + + /** + * Returns a new {@link RepositoryMapping} instance with identical contents, except that the + * inverse mapping is cached, causing {@link #getInverse} to be much more efficient. This is + * particularly important for the main repo mapping, as it's often used to generate display-form + * labels ({@link Label#getDisplayForm}). + */ + public RepositoryMapping withCachedInverseMap() { + var inverse = Maps.newHashMapWithExpectedSize(entries.size()); + for (Map.Entry entry : entries.entrySet()) { + inverse.putIfAbsent(entry.getValue(), entry.getKey()); + } + var inverseCopy = ImmutableMap.copyOf(inverse); + return new RepositoryMapping(entries, ownerRepo) { + @Override + public Optional getInverse(RepositoryName postMappingName) { + return Optional.ofNullable(inverseCopy.get(postMappingName)); + } + }; } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunction.java index 2d21acaf527e45..0925bc222804dd 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunction.java @@ -128,14 +128,17 @@ public SkyValue compute(SkyKey skyKey, Environment env) .withAdditionalMappings( ImmutableMap.of( externalPackageValue.getPackage().getWorkspaceName(), RepositoryName.MAIN)) - .withAdditionalMappings(additionalMappings); + .withAdditionalMappings(additionalMappings) + .withCachedInverseMap(); } // Try and see if this is a repo generated from a Bazel module. Optional mappingValue = computeForBazelModuleRepo(repositoryName, bazelDepGraphValue); if (mappingValue.isPresent()) { - return mappingValue.get(); + return repositoryName.isMain() + ? mappingValue.get().withCachedInverseMap() + : mappingValue.get(); } // Now try and see if this is a repo generated from a module extension. diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingValue.java index a6a0346b13e4b9..d32aea20728326 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingValue.java @@ -107,6 +107,17 @@ public final RepositoryMappingValue withAdditionalMappings(Map