From 7dd569f1b0166cbf651789d91ce192d8bde44267 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 10 Jan 2024 00:37:50 -0800 Subject: [PATCH] Add a profiler span for fetching repositories. 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 | 184 ++++++++++-------- 3 files changed, 103 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 5964a715a522c1..8013c96050e158 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/BUILD @@ -405,6 +405,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 5e880db394e198..92030f4fb0a173 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,108 +126,127 @@ 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 && starlarkSemantics.getBool(BuildLanguageOptions.ENABLE_WORKSPACE)) { - // 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) { - rule = null; + if (markerHash != null) { + return RepositoryDirectoryValue.builder().setPath(repoRoot).setDigest(markerHash).build(); + } } - } - if (rule == null) { - return new NoRepositoryDirectoryValue( - String.format("Repository '%s' is not defined", repositoryName.getCanonicalForm())); - } - 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())); + /* 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 (!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(); } + } - 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); + @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; } - if (markerHash != null) { - return RepositoryDirectoryValue.builder().setPath(repoRoot).setDigest(markerHash).build(); + if (value != BzlmodRepoRuleValue.REPO_RULE_NOT_FOUND_VALUE) { + return value.getRule(); } } - /* 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) { + if (starlarkSemantics.getBool(BuildLanguageOptions.ENABLE_WORKSPACE)) { + // fallback to look up the repository in the WORKSPACE file. + try { + return getRepoRuleFromWorkspace(repositoryName, env); + } catch (NoSuchRepositoryException e) { 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 (!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(); + return null; } @Nullable