Skip to content

Commit

Permalink
Improve creation of Label and RepositoryName instances.
Browse files Browse the repository at this point in the history 
In `Label#getRelativeWithRemapping`, when the current label is in the main repository, default the new label to the main repo as well. This saves the intermediate step of creating (and interning) a garbage `Label` with the default repository, only for `resolveRepositoryRelative` to subsequently create another `Label` in the main repo.

In `RepositoryName#create`, optimize for the common repository names (default and main) by skipping the interner cache and other unnecessary checks.

PiperOrigin-RevId: 381857421
  • Loading branch information
@justinhorvitz @copybara-github
justinhorvitz authored and copybara-github committed Jun 28, 2021
1 parent d5d3d9b commit 46aff0e
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 47 deletions.
57 changes: 25 additions & 32 deletions src/main/java/com/google/devtools/build/lib/cmdline/Label.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public final class Label
* Package names that aren't made relative to the current repository because they mean special
* things to Bazel.
*/
public static final ImmutableSet<PathFragment> ABSOLUTE_PACKAGE_NAMES =
private static final ImmutableSet<PathFragment> ABSOLUTE_PACKAGE_NAMES =
ImmutableSet.of(
// Used for select
PathFragment.create("conditions"),
Expand Down Expand Up @@ -101,8 +101,7 @@ public final class Label
public static Label parseAbsolute(
String absName, ImmutableMap<RepositoryName, RepositoryName> repositoryMapping)
throws LabelSyntaxException {
return parseAbsolute(
absName, /* defaultToMain= */ true, repositoryMapping);
return parseAbsolute(absName, /*defaultToMain=*/ true, repositoryMapping);
}

/**
Expand Down Expand Up @@ -153,23 +152,16 @@ public static Label parseAbsolute(
labelParts.getPackageName(), labelParts.getTargetName(), repo, repositoryMapping);
PathFragment packageFragment = pkgId.getPackageFragment();
if (repo.isEmpty() && ABSOLUTE_PACKAGE_NAMES.contains(packageFragment)) {
pkgId =
PackageIdentifier.create(getGlobalRepoName("@", repositoryMapping), packageFragment);
RepositoryName globalRepo =
repositoryMapping.getOrDefault(RepositoryName.MAIN, RepositoryName.MAIN);
pkgId = PackageIdentifier.create(globalRepo, packageFragment);
}
return create(pkgId, labelParts.getTargetName());
} catch (BadLabelException e) {
throw new LabelSyntaxException(e.getMessage());
}
}

private static RepositoryName getGlobalRepoName(
String repo, ImmutableMap<RepositoryName, RepositoryName> repositoryMapping)
throws LabelSyntaxException {
Preconditions.checkNotNull(repositoryMapping);
RepositoryName repoName = RepositoryName.create(repo);
return repositoryMapping.getOrDefault(repoName, repoName);
}

/**
* Alternate factory method for Labels from absolute strings. This is a convenience method for
* cases when a Label needs to be initialized statically, so the declared exception is
Expand Down Expand Up @@ -299,12 +291,10 @@ private static PackageIdentifier validatePackageName(
String repo,
ImmutableMap<RepositoryName, RepositoryName> repositoryMapping)
throws LabelSyntaxException {
String error = null;
try {
return PackageIdentifier.parse(packageIdentifier, repo, repositoryMapping);
} catch (LabelSyntaxException e) {
error = e.getMessage();
error = "invalid package name '" + packageIdentifier + "': " + error;
String error = "invalid package name '" + packageIdentifier + "': " + e.getMessage();
// This check is just for a more helpful error message
// i.e. valid target name, invalid package name, colon-free label form
// used => probably they meant "//foo:bar.c" not "//foo/bar.c".
Expand Down Expand Up @@ -556,7 +546,7 @@ public interface HasRepoMapping {
* {@code //wiz:quux} relative to {@code //foo/bar:baz} is {@code //wiz:quux};
* {@code @repo//foo:bar} relative to anything will be {@code @repo//foo:bar} if {@code @repo} is
* not in {@code repositoryMapping} but will be {@code @other_repo//foo:bar} if there is an entry
* {@code @repo -> @other_repo} in {@code repositoryMapping}
* {@code @repo -> @other_repo} in {@code repositoryMapping}.
*
* @param relName the relative label name; must be non-empty
* @param repositoryMapping the map of local repository names in external repository to global
Expand All @@ -566,12 +556,15 @@ public Label getRelativeWithRemapping(
String relName, ImmutableMap<RepositoryName, RepositoryName> repositoryMapping)
throws LabelSyntaxException {
Preconditions.checkNotNull(repositoryMapping);
if (relName.length() == 0) {
if (relName.isEmpty()) {
throw new LabelSyntaxException("empty package-relative label");
}

if (LabelValidator.isAbsolute(relName)) {
return resolveRepositoryRelative(parseAbsolute(relName, false, repositoryMapping));
// If this label is in the main repository, default the new label to the main repo as well to
// save resolveRepositoryRelative some work.
return resolveRepositoryRelative(
parseAbsolute(relName, /*defaultToMain=*/ getRepository().isMain(), repositoryMapping));
} else if (relName.equals(":")) {
throw new LabelSyntaxException("':' is not a valid package-relative label");
} else if (relName.charAt(0) == ':') {
Expand All @@ -594,17 +587,17 @@ public Label resolveRepositoryRelative(Label relative) {
if (packageIdentifier.getRepository().isDefault()
|| !relative.packageIdentifier.getRepository().isDefault()) {
return relative;
} else {
try {
return create(
PackageIdentifier.create(
packageIdentifier.getRepository(), relative.getPackageFragment()),
relative.getName());
} catch (LabelSyntaxException e) {
// We are creating the new label from an existing one which is guaranteed to be valid, so
// this can't happen
throw new IllegalStateException(e);
}
}

try {
return create(
PackageIdentifier.create(
packageIdentifier.getRepository(), relative.getPackageFragment()),
relative.name);
} catch (LabelSyntaxException e) {
// We are creating the new label from an existing one which is guaranteed to be valid, so this
// can't happen.
throw new IllegalStateException(e);
}
}

Expand All @@ -622,7 +615,7 @@ public int hashCode() {
* Specialization of {@link Arrays#hashCode()} that does not require constructing a 2-element
* array.
*/
private static final int hashCode(Object obj1, Object obj2) {
private static int hashCode(Object obj1, Object obj2) {
int result = 31 + (obj1 == null ? 0 : obj1.hashCode());
return 31 * result + (obj2 == null ? 0 : obj2.hashCode());
}
Expand Down Expand Up @@ -673,7 +666,7 @@ public static String print(@Nullable Label label) {
*/
public static PathFragment getContainingDirectory(Label label) {
PathFragment pkg = label.getPackageFragment();
String name = label.getName();
String name = label.name;
if (name.equals(".")) {
return pkg;
}
Expand Down
30 changes: 15 additions & 15 deletions src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,14 @@
/** A human-readable name for the repository. */
@AutoCodec
public final class RepositoryName implements Serializable {

static final String DEFAULT_REPOSITORY = "";
@SerializationConstant public static final RepositoryName DEFAULT;
@SerializationConstant public static final RepositoryName MAIN;

@SerializationConstant
public static final RepositoryName DEFAULT = new RepositoryName(DEFAULT_REPOSITORY);

@SerializationConstant public static final RepositoryName MAIN = new RepositoryName("@");

private static final Pattern VALID_REPO_NAME = Pattern.compile("@[\\w\\-.]*");

/** Helper for serializing {@link RepositoryName}. */
Expand Down Expand Up @@ -95,22 +100,19 @@ private Object writeReplace() {
return new RepositoryName(StringCanonicalizer.intern(name));
});

static {
try {
DEFAULT = RepositoryName.create(RepositoryName.DEFAULT_REPOSITORY);
MAIN = RepositoryName.create("@");
} catch (LabelSyntaxException e) {
throw new IllegalStateException(e);
}
}

/**
* Makes sure that name is a valid repository name and creates a new RepositoryName using it.
*
* @throws LabelSyntaxException if the name is invalid
*/
@AutoCodec.Instantiator
public static RepositoryName create(String name) throws LabelSyntaxException {
if (name.isEmpty()) {
return DEFAULT;
}
if (name.equals("@")) {
return MAIN;
}
try {
return repositoryNameCache.get(name);
} catch (CompletionException e) {
Expand Down Expand Up @@ -164,11 +166,9 @@ private RepositoryName(String name) {
this.name = name;
}

/**
* Performs validity checking. Returns null on success, an error message otherwise.
*/
/** Performs validity checking. Returns null on success, an error message otherwise. */
static String validate(String name) {
if (name.isEmpty()) {
if (name.isEmpty() || name.equals("@")) {
return null;
}

Expand Down

0 comments on commit 46aff0e

Please sign in to comment.