From 1b440b31cb9ae5ea1f24c62374e7be04c02477fb Mon Sep 17 00:00:00 2001 From: lberki Date: Thu, 11 Jan 2024 14:33:45 +0100 Subject: [PATCH] [7.1.0] Add a profiler span for fetching repositories. (#20852) This seems superfluous because we already have one for their Starlark implementation function, but sometimes significant amount of time is spent in RepositoryFunction just to determine that the repository is in fact up-to-date. In this case, it's useful to see in which repository the time is spent. RELNOTES: None. PiperOrigin-RevId: 597165396 Change-Id: I2c327450459a41dc7eec6ec2ac89c186cb43b34f --- .../build/lib/profiler/ProfilerTask.java | 1 + .../com/google/devtools/build/lib/rules/BUILD | 1 + .../RepositoryDelegatorFunction.java | 183 ++++++++++-------- 3 files changed, 102 insertions(+), 83 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/profiler/ProfilerTask.java b/src/main/java/com/google/devtools/build/lib/profiler/ProfilerTask.java index 1c17751576329c..2024b49cfe2ce9 100644 --- a/src/main/java/com/google/devtools/build/lib/profiler/ProfilerTask.java +++ b/src/main/java/com/google/devtools/build/lib/profiler/ProfilerTask.java @@ -98,6 +98,7 @@ public enum ProfilerTask { PRESSURE_STALL_MEMORY("Memory pressure stall level"), CONFLICT_CHECK("Conflict checking"), DYNAMIC_LOCK("Acquiring dynamic execution output lock", Threshold.FIFTY_MILLIS), + REPOSITORY_FETCH("Fetching repository"), UNKNOWN("Unknown event"); private static class Threshold { diff --git a/src/main/java/com/google/devtools/build/lib/rules/BUILD b/src/main/java/com/google/devtools/build/lib/rules/BUILD index c2c9fe435594cf..381968d6481fb8 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/BUILD @@ -404,6 +404,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/packages/semantics", + "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/repository:external_package_exception", "//src/main/java/com/google/devtools/build/lib/repository:external_package_helper", "//src/main/java/com/google/devtools/build/lib/repository:external_rule_not_found_exception", diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java index 756476d263349f..c4c7274519ece4 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java @@ -30,6 +30,9 @@ import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleFormatter; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; +import com.google.devtools.build.lib.profiler.Profiler; +import com.google.devtools.build.lib.profiler.ProfilerTask; +import com.google.devtools.build.lib.profiler.SilentCloseable; import com.google.devtools.build.lib.repository.ExternalPackageException; import com.google.devtools.build.lib.repository.ExternalPackageHelper; import com.google.devtools.build.lib.repository.ExternalRuleNotFoundException; @@ -115,10 +118,6 @@ public RepositoryDelegatorFunction( @Override public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException, RepositoryFunctionException { - StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env); - if (starlarkSemantics == null) { - return null; - } RepositoryName repositoryName = (RepositoryName) skyKey.argument(); if (!repositoryName.isVisible()) { return new NoRepositoryDirectoryValue( @@ -127,105 +126,123 @@ public SkyValue compute(SkyKey skyKey, Environment env) repositoryName.getName(), repositoryName.getOwnerRepoDisplayString())); } - Path repoRoot = - RepositoryFunction.getExternalRepositoryDirectory(directories) - .getRelative(repositoryName.getName()); - Map overrides = REPOSITORY_OVERRIDES.get(env); - if (Preconditions.checkNotNull(overrides).containsKey(repositoryName)) { - DigestWriter.clearMarkerFile(directories, repositoryName); - return setupOverride(overrides.get(repositoryName), env, repoRoot, repositoryName.getName()); - } + try (SilentCloseable c = + Profiler.instance().profile(ProfilerTask.REPOSITORY_FETCH, repositoryName.toString())) { + StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env); + if (starlarkSemantics == null) { + return null; + } + Path repoRoot = + RepositoryFunction.getExternalRepositoryDirectory(directories) + .getRelative(repositoryName.getName()); + Map overrides = REPOSITORY_OVERRIDES.get(env); + if (Preconditions.checkNotNull(overrides).containsKey(repositoryName)) { + DigestWriter.clearMarkerFile(directories, repositoryName); + return setupOverride( + overrides.get(repositoryName), env, repoRoot, repositoryName.getName()); + } - Rule rule = null; - if (starlarkSemantics.getBool(BuildLanguageOptions.ENABLE_BZLMOD)) { - // Tries to get a repository rule instance from Bzlmod generated repos. - SkyKey key = BzlmodRepoRuleValue.key(repositoryName); - BzlmodRepoRuleValue value = (BzlmodRepoRuleValue) env.getValue(key); + Rule rule = getRepositoryRule(env, repositoryName, starlarkSemantics); if (env.valuesMissing()) { return null; } - if (value != BzlmodRepoRuleValue.REPO_RULE_NOT_FOUND_VALUE) { - rule = value.getRule(); + + if (rule == null) { + return new NoRepositoryDirectoryValue( + String.format("Repository '%s' is not defined", repositoryName.getCanonicalForm())); } - } - if (rule == null) { - // fallback to look up the repository in the WORKSPACE file. - try { - rule = getRepoRuleFromWorkspace(repositoryName, env); + RepositoryFunction handler = getHandler(rule); + if (handler == null) { + // If we refer to a non repository rule then the repository does not exist. + return new NoRepositoryDirectoryValue( + String.format("'%s' is not a repository rule", repositoryName.getCanonicalForm())); + } + + DigestWriter digestWriter = new DigestWriter(directories, repositoryName, rule); + if (shouldUseCachedRepos(env, handler, repoRoot, rule)) { + // Make sure marker file is up-to-date; correctly describes the current repository state + byte[] markerHash = digestWriter.areRepositoryAndMarkerFileConsistent(handler, env); if (env.valuesMissing()) { return null; } - } catch (NoSuchRepositoryException e) { - return new NoRepositoryDirectoryValue( - String.format("Repository '%s' is not defined", repositoryName.getCanonicalForm())); + if (markerHash != null) { + return RepositoryDirectoryValue.builder().setPath(repoRoot).setDigest(markerHash).build(); + } } - } - - RepositoryFunction handler = getHandler(rule); - if (handler == null) { - // If we refer to a non repository rule then the repository does not exist. - return new NoRepositoryDirectoryValue( - String.format("'%s' is not a repository rule", repositoryName.getCanonicalForm())); - } - DigestWriter digestWriter = new DigestWriter(directories, repositoryName, rule); - if (shouldUseCachedRepos(env, handler, repoRoot, rule)) { - // Make sure marker file is up-to-date; correctly describes the current repository state - byte[] markerHash = digestWriter.areRepositoryAndMarkerFileConsistent(handler, env); - if (env.valuesMissing()) { - return null; + /* At this point: This is a force fetch, a local repository, OR The repository cache is old or + didn't exist. In any of those cases, we initiate the fetching process UNLESS this is offline + mode (fetching is disabled) */ + if (isFetch.get()) { + // Fetching a repository is a long-running operation that can easily be interrupted. If it + // is and the marker file exists on disk, a new call of this method may treat this + // repository as valid even though it is in an inconsistent state. Clear the marker file and + // only recreate it after fetching is done to prevent this scenario. + DigestWriter.clearMarkerFile(directories, repositoryName); + RepositoryDirectoryValue.Builder builder = + fetchRepository(skyKey, repoRoot, env, digestWriter.getMarkerData(), handler, rule); + if (builder == null) { + return null; + } + // No new Skyframe dependencies must be added between calling the repository implementation + // and writing the marker file because if they aren't computed, it would cause a Skyframe + // restart thus calling the possibly very slow (networking, decompression...) fetch() + // operation again. So we write the marker file here immediately. + byte[] digest = digestWriter.writeMarkerFile(); + return builder.setDigest(digest).build(); } - if (markerHash != null) { - return RepositoryDirectoryValue.builder().setPath(repoRoot).setDigest(markerHash).build(); + + if (!repoRoot.exists()) { + // The repository isn't on the file system, there is nothing we can do. + throw new RepositoryFunctionException( + new IOException( + "to fix, run\n\tbazel fetch //...\nExternal repository " + + repositoryName + + " not found and fetching repositories is disabled."), + Transience.TRANSIENT); } + + // Try to build with whatever is on the file system and emit a warning. + env.getListener() + .handle( + Event.warn( + rule.getLocation(), + String.format( + "External repository '%s' is not up-to-date and fetching is disabled. To" + + " update, run the build without the '--nofetch' command line option.", + rule.getName()))); + + return RepositoryDirectoryValue.builder() + .setPath(repoRoot) + .setFetchingDelayed() + .setDigest(new Fingerprint().digestAndReset()) + .build(); } + } - /* At this point: This is a force fetch, a local repository, OR The repository cache is old or - didn't exist. In any of those cases, we initiate the fetching process UNLESS this is offline - mode (fetching is disabled) */ - if (isFetch.get()) { - // Fetching a repository is a long-running operation that can easily be interrupted. If it is - // and the marker file exists on disk, a new call of this method may treat this repository as - // valid even though it is in an inconsistent state. Clear the marker file and only recreate - // it after fetching is done to prevent this scenario. - DigestWriter.clearMarkerFile(directories, repositoryName); - RepositoryDirectoryValue.Builder builder = - fetchRepository(skyKey, repoRoot, env, digestWriter.getMarkerData(), handler, rule); - if (builder == null) { + @Nullable + private Rule getRepositoryRule( + Environment env, RepositoryName repositoryName, StarlarkSemantics starlarkSemantics) + throws InterruptedException, RepositoryFunctionException { + if (starlarkSemantics.getBool(BuildLanguageOptions.ENABLE_BZLMOD)) { + // Tries to get a repository rule instance from Bzlmod generated repos. + SkyKey key = BzlmodRepoRuleValue.key(repositoryName); + BzlmodRepoRuleValue value = (BzlmodRepoRuleValue) env.getValue(key); + if (env.valuesMissing()) { return null; } - // No new Skyframe dependencies must be added between calling the repository implementation - // and writing the marker file because if they aren't computed, it would cause a Skyframe - // restart thus calling the possibly very slow (networking, decompression...) fetch() - // operation again. So we write the marker file here immediately. - byte[] digest = digestWriter.writeMarkerFile(); - return builder.setDigest(digest).build(); + if (value != BzlmodRepoRuleValue.REPO_RULE_NOT_FOUND_VALUE) { + return value.getRule(); + } } - if (!repoRoot.exists()) { - // The repository isn't on the file system, there is nothing we can do. - throw new RepositoryFunctionException( - new IOException( - "to fix, run\n\tbazel fetch //...\nExternal repository " - + repositoryName - + " not found and fetching repositories is disabled."), - Transience.TRANSIENT); + // fallback to look up the repository in the WORKSPACE file. + try { + return getRepoRuleFromWorkspace(repositoryName, env); + } catch (NoSuchRepositoryException e) { + return null; } - - // Try to build with whatever is on the file system and emit a warning. - env.getListener() - .handle(Event.warn(rule.getLocation(), - String.format( - "External repository '%s' is not up-to-date and fetching is disabled. To update, " - + "run the build without the '--nofetch' command line option.", - rule.getName()))); - - return RepositoryDirectoryValue.builder() - .setPath(repoRoot) - .setFetchingDelayed() - .setDigest(new Fingerprint().digestAndReset()) - .build(); } @Nullable