From 3cc99fbedc3fc51965b75b45058e35e8439ca576 Mon Sep 17 00:00:00 2001 From: Oli Date: Mon, 3 Oct 2022 11:26:43 -0700 Subject: [PATCH 1/3] Add support for per-dependencies and per-class compilation avoidance --- .../buildjar/JavaLibraryBuildRequest.java | 1 + .../build/buildjar/OptionsParser.java | 9 + .../build/buildjar/javac/plugins/BUILD | 2 + .../plugins/dependency/DependencyModule.java | 41 ++- .../dependency/StrictJavaDepsPlugin.java | 48 ++- .../build/lib/actions/ActionCacheChecker.java | 52 +++- .../google/devtools/build/lib/actions/BUILD | 3 + .../build/lib/actions/cache/ActionCache.java | 16 +- .../usage/ActionInputUsageTracker.java | 277 ++++++++++++++++++ .../lib/actions/usage/ClassUsageInfo.java | 77 +++++ .../usage/PreComputedMetadataValue.java | 61 ++++ .../build/lib/actions/usage/TrackingInfo.java | 45 +++ .../build/lib/actions/usage/Utils.java | 61 ++++ .../lib/buildtool/BuildRequestOptions.java | 33 +++ .../build/lib/buildtool/ExecutionTool.java | 8 + .../lib/rules/java/JavaCompilationHelper.java | 1 + .../lib/rules/java/JavaCompileAction.java | 1 + .../rules/java/JavaCompileActionBuilder.java | 12 + .../lib/rules/java/JavaConfiguration.java | 6 + .../build/lib/rules/java/JavaOptions.java | 14 + src/main/protobuf/deps.proto | 16 + .../lib/actions/ActionCacheCheckerTest.java | 6 + .../SequencedSkyframeExecutorTest.java | 6 + .../skyframe/TimestampBuilderTestCase.java | 10 +- src/test/shell/bazel/BUILD | 20 ++ .../bazel/bazel_java_compilation_avoidance.sh | 239 +++++++++++++++ 26 files changed, 1051 insertions(+), 14 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/actions/usage/ActionInputUsageTracker.java create mode 100644 src/main/java/com/google/devtools/build/lib/actions/usage/ClassUsageInfo.java create mode 100644 src/main/java/com/google/devtools/build/lib/actions/usage/PreComputedMetadataValue.java create mode 100644 src/main/java/com/google/devtools/build/lib/actions/usage/TrackingInfo.java create mode 100644 src/main/java/com/google/devtools/build/lib/actions/usage/Utils.java create mode 100755 src/test/shell/bazel/bazel_java_compilation_avoidance.sh diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/JavaLibraryBuildRequest.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/JavaLibraryBuildRequest.java index 2be7797a0088b0..8c02dcb1dc8603 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/JavaLibraryBuildRequest.java +++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/JavaLibraryBuildRequest.java @@ -159,6 +159,7 @@ public JavaLibraryBuildRequest( if (optionsParser.reduceClasspathMode() != OptionsParser.ReduceClasspathMode.NONE) { depsBuilder.setReduceClasspath(); } + depsBuilder.setUsageTrackerMode(optionsParser.usageTrackerMode()); if (optionsParser.getTargetLabel() != null) { depsBuilder.setTargetLabel(optionsParser.getTargetLabel()); } diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/OptionsParser.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/OptionsParser.java index 60d04e1a1760e1..27d710e9132848 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/OptionsParser.java +++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/OptionsParser.java @@ -67,6 +67,8 @@ public enum ReduceClasspathMode { */ private ReduceClasspathMode reduceClasspathMode = ReduceClasspathMode.NONE; + private boolean usageTrackerMode = false; + private int fullClasspathLength = -1; private int reducedClasspathLength = -1; @@ -156,6 +158,9 @@ private void processCommandlineArgs(Deque argQueue) throws InvalidComman case "--reduce_classpath_mode": reduceClasspathMode = ReduceClasspathMode.valueOf(getArgument(argQueue, arg)); break; + case "--experimental_track_class_usage": + usageTrackerMode = true; + break; case "--full_classpath_length": fullClasspathLength = Integer.parseInt(getArgument(argQueue, arg)); break; @@ -378,6 +383,10 @@ public ReduceClasspathMode reduceClasspathMode() { return reduceClasspathMode; } + public boolean usageTrackerMode() { + return usageTrackerMode; + } + public int fullClasspathLength() { return fullClasspathLength; } diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/BUILD b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/BUILD index bfa5768f132c97..6a7f77c66d2491 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/BUILD +++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/BUILD @@ -36,10 +36,12 @@ java_library( ":plugins", "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar:JarOwner", "//src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/statistics", + "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/protobuf:deps_java_proto", "//third_party:auto_value", "//third_party:guava", "//third_party:tomcat_annotations_api", + "//third_party/protobuf:protobuf_java", ], ) diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/DependencyModule.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/DependencyModule.java index d6917e96de8f64..736b72abb90659 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/DependencyModule.java +++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/DependencyModule.java @@ -24,6 +24,7 @@ import com.google.common.collect.Streams; import com.google.devtools.build.buildjar.JarOwner; import com.google.devtools.build.buildjar.javac.plugins.BlazeJavaCompilerPlugin; +import com.google.devtools.build.lib.view.proto.Deps; import com.google.devtools.build.lib.view.proto.Deps.Dependencies; import com.google.devtools.build.lib.view.proto.Deps.Dependency; import com.google.devtools.build.lib.view.proto.Deps.Dependency.Kind; @@ -76,12 +77,15 @@ public static enum StrictJavaDeps { private final FixTool fixDepsTool; private final ImmutableSet directJars; private final boolean strictClasspathMode; + private final boolean usageTrackerMode; private final Set depsArtifacts; private final String targetLabel; private final Path outputDepsProtoFile; private boolean hasMissingTargets; private final Map explicitDependenciesMap; private final Map implicitDependenciesMap; + + private final Map> usedClassesMap; private final ImmutableSet platformJars; Set requiredClasspath; private final FixMessage fixMessage; @@ -93,6 +97,7 @@ public static enum StrictJavaDeps { FixTool fixDepsTool, ImmutableSet directJars, boolean strictClasspathMode, + boolean usageTrackerMode, Set depsArtifacts, ImmutableSet platformJars, String targetLabel, @@ -103,11 +108,13 @@ public static enum StrictJavaDeps { this.fixDepsTool = fixDepsTool; this.directJars = directJars; this.strictClasspathMode = strictClasspathMode; + this.usageTrackerMode = usageTrackerMode; this.depsArtifacts = depsArtifacts; this.targetLabel = targetLabel; this.outputDepsProtoFile = outputDepsProtoFile; this.explicitDependenciesMap = new HashMap<>(); this.implicitDependenciesMap = new HashMap<>(); + this.usedClassesMap = new HashMap<>(); this.platformJars = platformJars; this.fixMessage = fixMessage; this.exemptGenerators = exemptGenerators; @@ -116,7 +123,7 @@ public static enum StrictJavaDeps { /** Returns a plugin to be enabled in the compiler. */ public BlazeJavaCompilerPlugin getPlugin() { - return new StrictJavaDepsPlugin(this); + return new StrictJavaDepsPlugin(this, usageTrackerMode); } /** @@ -162,11 +169,18 @@ Dependencies buildDependenciesProto( // Filter using the original classpath, to preserve ordering. for (Path entry : classpath) { if (explicitDependenciesMap.containsKey(entry)) { - deps.addDependency(explicitDependenciesMap.get(entry)); + Deps.Dependency d = explicitDependenciesMap.get(entry).toBuilder() + .addAllUsedClasses(usedClassesMap.getOrDefault(entry, Set.of())) + .build(); + deps.addDependency(d); } else if (implicitDependenciesMap.containsKey(entry)) { - deps.addDependency(implicitDependenciesMap.get(entry)); + Deps.Dependency d = implicitDependenciesMap.get(entry).toBuilder() + .addAllUsedClasses(usedClassesMap.getOrDefault(entry, Set.of())) + .build(); + deps.addDependency(d); } } + return deps.build(); } @@ -195,6 +209,10 @@ public Map getImplicitDependenciesMap() { return implicitDependenciesMap; } + public Map> getUsedClassesMap() { + return usedClassesMap; + } + /** Returns the jars in the platform classpath. */ public ImmutableSet getPlatformJars() { return platformJars; @@ -230,6 +248,11 @@ public boolean reduceClasspath() { return strictClasspathMode; } + /** Writes used classes information in deps file. */ + public boolean usageTrackerMode() { + return usageTrackerMode; + } + void setHasMissingTargets() { hasMissingTargets = true; } @@ -337,6 +360,7 @@ public static class Builder { private String targetLabel; private Path outputDepsProtoFile; private boolean strictClasspathMode = false; + private boolean usageTrackerMode = false; private FixMessage fixMessage = new DefaultFixMessage(); private final Set exemptGenerators = new LinkedHashSet<>(SJD_EXEMPT_PROCESSORS); @@ -370,6 +394,7 @@ public DependencyModule build() { fixDepsTool, directJars, strictClasspathMode, + usageTrackerMode, depsArtifacts, platformJars, targetLabel, @@ -456,6 +481,16 @@ public Builder setReduceClasspath() { return this; } + /** + * Set action input usage tracking behavior. Used for build time optimization via compilation avoidance. + * + * @return this Builder instance + */ + public Builder setUsageTrackerMode(boolean usageTrackerMode) { + this.usageTrackerMode = usageTrackerMode; + return this; + } + /** * Set the message to display when a missing indirect dependency is found. * diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/StrictJavaDepsPlugin.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/StrictJavaDepsPlugin.java index de1ad14a8acb49..5e0136b3f5974b 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/StrictJavaDepsPlugin.java +++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/StrictJavaDepsPlugin.java @@ -19,12 +19,15 @@ import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableSet; +import com.google.common.io.ByteStreams; import com.google.devtools.build.buildjar.JarOwner; import com.google.devtools.build.buildjar.javac.plugins.BlazeJavaCompilerPlugin; import com.google.devtools.build.buildjar.javac.plugins.dependency.DependencyModule.StrictJavaDeps; import com.google.devtools.build.buildjar.javac.statistics.BlazeJavacStatistics; +import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.view.proto.Deps; import com.google.devtools.build.lib.view.proto.Deps.Dependency; +import com.google.protobuf.ByteString; import com.sun.tools.javac.code.Flags; import com.sun.tools.javac.code.Kinds; import com.sun.tools.javac.code.Symbol; @@ -42,6 +45,7 @@ import com.sun.tools.javac.util.Log.WriterKind; import com.sun.tools.javac.util.Name; import java.io.IOException; +import java.io.InputStream; import java.io.PrintWriter; import java.io.UncheckedIOException; import java.lang.reflect.Method; @@ -59,6 +63,7 @@ import java.util.jar.Manifest; import javax.lang.model.element.AnnotationValue; import javax.lang.model.util.SimpleAnnotationValueVisitor8; +import javax.tools.FileObject; import javax.tools.JavaFileObject; /** @@ -76,6 +81,7 @@ public final class StrictJavaDepsPlugin extends BlazeJavaCompilerPlugin { private ImplicitDependencyExtractor implicitDependencyExtractor; private CheckingTreeScanner checkingTreeScanner; private final DependencyModule dependencyModule; + private final boolean usageTrackerMode; /** Marks seen compilation toplevels and their import sections */ private final Set toplevels; @@ -111,8 +117,9 @@ static SjdDiagnostic create(int pos, String message, JavaFileObject source) { * flagging that dependency. Also, we can check whether the direct dependencies were actually * necessary, i.e. if their associated jars were used at all for looking up class definitions. */ - public StrictJavaDepsPlugin(DependencyModule dependencyModule) { + public StrictJavaDepsPlugin(DependencyModule dependencyModule, boolean usageTrackerMode) { this.dependencyModule = dependencyModule; + this.usageTrackerMode = usageTrackerMode; toplevels = new HashSet<>(); trees = new HashSet<>(); missingTargets = new HashSet<>(); @@ -135,7 +142,7 @@ public void init( if (checkingTreeScanner == null) { Set platformJars = dependencyModule.getPlatformJars(); checkingTreeScanner = - new CheckingTreeScanner(dependencyModule, diagnostics, missingTargets, platformJars); + new CheckingTreeScanner(dependencyModule, diagnostics, missingTargets, platformJars, usageTrackerMode); context.put(CheckingTreeScanner.class, checkingTreeScanner); } } @@ -230,6 +237,8 @@ private static class CheckingTreeScanner extends TreeScanner { /** Collect seen direct dependencies and their associated information */ private final Map directDependenciesMap; + private final Map> usedClassesMap; + /** We only emit one warning/error per class symbol */ private final Set seenClasses = new HashSet<>(); @@ -240,6 +249,9 @@ private static class CheckingTreeScanner extends TreeScanner { /** The set of jars on the compilation bootclasspath. */ private final Set platformJars; + /** The action usage tracker mode. */ + private boolean usageTrackerMode; + /** The current source, for diagnostics. */ private JavaFileObject source = null; @@ -247,12 +259,15 @@ public CheckingTreeScanner( DependencyModule dependencyModule, List diagnostics, Set missingTargets, - Set platformJars) { + Set platformJars, + boolean usageTrackerMode) { this.directJars = dependencyModule.directJars(); this.diagnostics = diagnostics; this.missingTargets = missingTargets; this.directDependenciesMap = dependencyModule.getExplicitDependenciesMap(); + this.usedClassesMap = dependencyModule.getUsedClassesMap(); this.platformJars = platformJars; + this.usageTrackerMode = usageTrackerMode; } Set getSeenClasses() { @@ -270,6 +285,33 @@ private void checkTypeLiteral(JCTree node, Symbol sym) { // whether that jar was a direct dependency and error out otherwise. if (jarPath != null && seenClasses.add(sym.enclClass())) { collectExplicitDependency(jarPath, node, sym); + + // Track used classes + if (usageTrackerMode) { + if (!usedClassesMap.containsKey(jarPath)) { + usedClassesMap.put(jarPath, new HashSet()); + } + String internalPath = sym.enclClass().classfile.toString().split(":")[1]; + Deps.UsedClass usedClass = Deps.UsedClass.newBuilder() + .setFullyQualifiedName(sym.enclClass().fullname.toString()) + .setJarInternalPath(internalPath.substring(1, internalPath.length() - 1)) + .setHash(hashFile(sym.enclClass().classfile)) + .build(); + usedClassesMap.get(jarPath).add(usedClass); + } + } + } + + /* + * Generate Sha256 of input fileObject content. + */ + private static ByteString hashFile(FileObject fileObject) { + try { + InputStream stream = fileObject.openInputStream(); + byte[] targetArray = ByteStreams.toByteArray(stream); + return ByteString.copyFrom(DigestHashFunction.SHA256.getHashFunction().hashBytes(targetArray).asBytes()); + } catch (IOException ex) { + throw new RuntimeException("Failure to compute hash for " + fileObject, ex); } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java index 916cb1bfdd5673..339d04b22da97f 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java @@ -33,6 +33,8 @@ import com.google.devtools.build.lib.actions.cache.MetadataDigestUtils; import com.google.devtools.build.lib.actions.cache.MetadataHandler; import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics.MissReason; +import com.google.devtools.build.lib.actions.usage.ActionInputUsageTracker; +import com.google.devtools.build.lib.actions.usage.TrackingInfo; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; @@ -72,6 +74,7 @@ public class ActionCacheChecker { private final ActionKeyContext actionKeyContext; private final Predicate executionFilter; private final ArtifactResolver artifactResolver; + private final ActionInputUsageTracker actionInputUsageTracker; private final CacheConfig cacheConfig; @Nullable private final ActionCache actionCache; // Null when not enabled. @@ -105,12 +108,14 @@ public abstract static class Builder { public ActionCacheChecker( @Nullable ActionCache actionCache, ArtifactResolver artifactResolver, + ActionInputUsageTracker actionInputUsageTracker, ActionKeyContext actionKeyContext, Predicate executionFilter, @Nullable CacheConfig cacheConfig) { this.executionFilter = executionFilter; this.actionKeyContext = actionKeyContext; this.artifactResolver = artifactResolver; + this.actionInputUsageTracker = actionInputUsageTracker; this.cacheConfig = cacheConfig != null ? cacheConfig @@ -191,7 +196,7 @@ private static FileArtifactValue getCachedMetadata( * from {@code metadataHandler}. * @return true if at least one artifact has changed, false - otherwise. */ - private static boolean validateArtifacts( + private boolean validateArtifacts( ActionCache.Entry entry, Action action, NestedSet actionInputs, @@ -209,9 +214,31 @@ private static boolean validateArtifacts( mdMap.put(artifact.getExecPathString(), metadata); } } + for (Artifact artifact : actionInputs.toList()) { - mdMap.put(artifact.getExecPathString(), getMetadataMaybe(metadataHandler, artifact)); + // Request tracking info with fresh hashes from direct dependencies, since we want to compare + // these against the hashes that were used at previous compilation time. + TrackingInfo trackingInfo = actionInputUsageTracker.getTrackingInfo(action, artifact, true); + + if (trackingInfo.isUnused()) { + // Input artifact is not used, exclude it from action cache key computation + continue; + } else if (trackingInfo.tracksUsedClasses()) { + // A subset of input artifact is used, include individual used classes for action cache key computation + trackingInfo.getUsedClasses().forEach(usedClass -> + mdMap.put(usedClass.getInternalPath(), usedClass.getDependencyFileArtifactValue()) + ); + } else { + // Standard case, track input artifact itself + mdMap.put(artifact.getExecPathString(), getMetadataMaybe(metadataHandler, artifact)); + } } + + // Possibly output debug info about action, to help troubleshooting usage tracking if needed + if (actionInputUsageTracker.supportsInputTracking(action)) { + System.out.println(actionInputUsageTracker.dump(action)); + } + return !Arrays.equals(MetadataDigestUtils.fromMetadata(mdMap), entry.getFileDigest()); } @@ -466,6 +493,9 @@ public Token getTokenIfNeedToExecute( cachedOutputMetadata = loadCachedOutputMetadata(action, entry, metadataHandler); } + // Load tracking info from action output .jdeps file + actionInputUsageTracker.refreshInputTrackingInfo(action); + if (mustExecute( action, entry, @@ -583,6 +613,10 @@ public void updateActionCache( // This cache entry has already been updated by a shared action. We don't need to do it again. return; } + + // Action has been executed, and output .jdeps file has been re-generated, so we need to update tracking info. + actionInputUsageTracker.refreshInputTrackingInfo(action); + Map usedEnvironment = computeUsedEnv(action, clientEnv, remoteDefaultPlatformProperties); ActionCache.Entry entry = @@ -622,10 +656,20 @@ public void updateActionCache( : ImmutableSet.of(); for (Artifact input : action.getInputs().toList()) { + TrackingInfo trackingInfo = actionInputUsageTracker.getTrackingInfo(action, input, false); + entry.addInputFile( input.getExecPath(), getMetadataMaybe(metadataHandler, input), - /* saveExecPath= */ !excludePathsFromActionCache.contains(input)); + /* saveExecPath= */ !excludePathsFromActionCache.contains(input), + /* excludeFromDigest= */ trackingInfo.isUnused() || trackingInfo.tracksUsedClasses()); + + if (trackingInfo.tracksUsedClasses()) { + // A subset of input artifact is used, include individual used classes to action cache key computation + trackingInfo.getUsedClasses().forEach(usedClass -> + entry.addHash(usedClass.getInternalPath(), usedClass.getCompileTimeFileArtifactValue()) + ); + } } entry.getFileDigest(); actionCache.put(key, entry); @@ -743,7 +787,7 @@ private void checkMiddlemanAction( entry = new ActionCache.Entry("", ImmutableMap.of(), false); for (Artifact input : action.getInputs().toList()) { entry.addInputFile( - input.getExecPath(), getMetadataMaybe(metadataHandler, input), /*saveExecPath=*/ true); + input.getExecPath(), getMetadataMaybe(metadataHandler, input), /*saveExecPath=*/ true, false); } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/BUILD b/src/main/java/com/google/devtools/build/lib/actions/BUILD index 0fe4c4ffefe7fe..ae54c12da2dfd9 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BUILD +++ b/src/main/java/com/google/devtools/build/lib/actions/BUILD @@ -24,6 +24,7 @@ java_library( [ "*.java", "cache/*.java", + "usage/*.java", ], exclude = [ "ActionInput.java", @@ -75,6 +76,7 @@ java_library( ":middleman_type", ":package_roots", ":thread_state_receiver", + "//src/main/java/com/google/devtools/build/lib:build-request-options", "//src/main/java/com/google/devtools/build/lib/analysis/platform", "//src/main/java/com/google/devtools/build/lib/bugreport", "//src/main/java/com/google/devtools/build/lib/buildeventstream", @@ -119,6 +121,7 @@ java_library( "//src/main/java/com/google/devtools/common/options", "//src/main/java/net/starlark/java/eval", "//src/main/java/net/starlark/java/syntax", + "//src/main/protobuf:deps_java_proto", "//src/main/protobuf:action_cache_java_proto", "//src/main/protobuf:extra_actions_base_java_proto", "//src/main/protobuf:failure_details_java_proto", diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java b/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java index 94bc9fbf2407a8..9a9cc9dee0db4b 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java @@ -231,7 +231,7 @@ Map getOutputTrees() { /** Adds metadata of an input file */ public void addInputFile( - PathFragment relativePath, FileArtifactValue md, boolean saveExecPath) { + PathFragment relativePath, FileArtifactValue md, boolean saveExecPath, boolean excludeFromDigest) { checkState(mdMap != null); checkState(!isCorrupted()); checkState(digest == null); @@ -240,11 +240,21 @@ public void addInputFile( if (discoversInputs() && saveExecPath) { files.add(execPath); } - mdMap.put(execPath, md); + if (!excludeFromDigest) { + mdMap.put(execPath, md); + } + } + + public void addHash(String path, FileArtifactValue md) { + checkState(mdMap != null); + checkState(!isCorrupted()); + checkState(digest == null); + + mdMap.put(path, md); } public void addInputFile(PathFragment relativePath, FileArtifactValue md) { - addInputFile(relativePath, md, /*saveExecPath=*/ true); + addInputFile(relativePath, md, /*saveExecPath=*/ true, false); } /** diff --git a/src/main/java/com/google/devtools/build/lib/actions/usage/ActionInputUsageTracker.java b/src/main/java/com/google/devtools/build/lib/actions/usage/ActionInputUsageTracker.java new file mode 100644 index 00000000000000..1a6e9e2229bfd8 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/actions/usage/ActionInputUsageTracker.java @@ -0,0 +1,277 @@ +// Copyright 2014 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.actions.usage; + +import com.google.devtools.build.lib.actions.*; +import com.google.devtools.build.lib.buildtool.BuildRequestOptions.ActionInputUsageTrackerMode; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.view.proto.Deps; + +import javax.annotation.Nullable; +import java.io.FileNotFoundException; +import java.io.InputStream; +import java.util.*; +import java.util.function.Predicate; +import java.util.stream.Collectors; + +import static com.google.common.base.Preconditions.checkState; +import static com.google.devtools.build.lib.actions.usage.Utils.getHashFromJarEntry; +import static com.google.devtools.build.lib.actions.usage.Utils.getJDepsOutput; + +/** + * Class responsible for removing irrelevant inputs used for an action cache key computation. The goal here is to + * achieve incremental compilation avoidance by dismissing unused input. This way, any change to these input will + * cause computed action cache key to remain unchanged, leading to a local cache hit / skip of action re-execution. + * The list of unused input is generated by the action itself, during previous action execution, and stored to disk + * among action outputs (typically .jdeps file for Java/Kotlin compilation action). + *

+ * Action can also generate more granular usage data through used "classes". For these, changes not classes that are + * not specified as 'used' will similarly be removed from action cache key computation. + *

+ * Note of software design : instead of modifying Action/AbstractAction class itself (similarly to input discovery + * feature), we use composition via this separate class to keep the logic separated, and facilitate merging changes + * from Bazel master branch. + */ +public class ActionInputUsageTracker { + + /** + * Internal class tracking usage info for a given action. + */ + class UsageInfo { + private Set unusedArtifactPaths; + private Map> usedClassesMap; + + UsageInfo(Set unusedArtifactPaths, Map> usedClassesMap) { + this.unusedArtifactPaths = unusedArtifactPaths; + this.usedClassesMap = usedClassesMap; + } + } + + /** + * For now, only JVM-based rules tracks usage / provide .jdeps output artifact. + */ + private static final Set SUPPORTED_DEPENDENCY_TRACKING_MNEMONICS = Set.of("Javac", "KotlinCompile"); + private static final Set SUPPORTED_CLASS_TRACKING_MNEMONICS = Set.of("Javac", "KotlinCompile"); + + private static final boolean VERBOSE_MODE = true; + + private final ArtifactPathResolver pathResolver; + private final ActionInputUsageTrackerMode trackerMode; + private final Map trackerInfoMap; + + public ActionInputUsageTracker(ArtifactPathResolver pathResolver, ActionInputUsageTrackerMode trackerMode) { + this.pathResolver = pathResolver; + this.trackerMode = trackerMode; + this.trackerInfoMap = new HashMap<>(); + } + + /** + * Whether action input tracking is enabled or not. + */ + public boolean enabled() { + return this.trackerMode != ActionInputUsageTrackerMode.DISABLED; + } + + /** + * Whether action supports input tracking. + */ + public boolean supportsInputTracking(Action action) { + return enabled() && + action.getOwner().getLabel() != null && + action.getOwner().getLabel().getRepository().isMain() && + getJDepsOutput(action) != null && + SUPPORTED_DEPENDENCY_TRACKING_MNEMONICS.contains(action.getMnemonic()); + } + + /** + * Whether action supports classes input tracking. + */ + public boolean supportsClassTracking(Action action) { + return this.trackerMode == ActionInputUsageTrackerMode.UNUSED_CLASSES && + action.getOwner().getLabel() != null && + action.getOwner().getLabel().getRepository().isMain() && + getJDepsOutput(action) != null && + SUPPORTED_CLASS_TRACKING_MNEMONICS.contains(action.getMnemonic()); + } + + /** + * Refresh internal input tracking info from action .jdeps file. + */ + public void refreshInputTrackingInfo(Action action) { + if (!enabled() || !supportsInputTracking(action)) { + return; + } + + UsageInfo usageInfo = null; + try { + Artifact jdeps = getJDepsOutput(action); + Path output = pathResolver.toPath(jdeps); + InputStream input = output.getInputStream(); + Deps.Dependencies deps = Deps.Dependencies.parseFrom(input); + checkState(deps.getRuleLabel().equals(action.getOwner().getLabel().toString())); + + Set usedPaths = deps.getDependencyList().stream() + .filter(d -> d.getKind() == Deps.Dependency.Kind.EXPLICIT || d.getKind() == Deps.Dependency.Kind.IMPLICIT) + .map(d -> d.getPath()) + .collect(Collectors.toCollection(LinkedHashSet::new)); + Set unusedArtifactsPath = action.getInputs().toList().stream() + .filter(ActionInputUsageTracker::canArtifactBeUnused) + .map(d -> d.getExecPathString()) + .filter(Predicate.not(usedPaths::contains)) + .collect(Collectors.toCollection(LinkedHashSet::new)); + Map> usedClassesMap = deps.getDependencyList().stream() + .filter(d -> d.getKind() == Deps.Dependency.Kind.EXPLICIT || d.getKind() == Deps.Dependency.Kind.IMPLICIT) + .collect(Collectors.toMap( + d -> d.getPath(), + d -> d.getUsedClassesList().stream() + .map(ClassUsageInfo::create) + .collect(Collectors.toCollection(LinkedHashSet::new)))); + + usageInfo = new UsageInfo(unusedArtifactsPath, usedClassesMap); + } catch (FileNotFoundException fileNotFoundException) { + // Silently ignore, this could be a clean build + } catch (Exception exception) { + System.err.println("ActionInputUsageTracker: " + getKey(action) + " .jdeps file failed to load, ex=" + exception); + } + + trackerInfoMap.put(getKey(action), usageInfo); + } + + /** + * Get tracking information for the given action and input artifact. If includeDependencyHash is true, we include + * in the return object classes hash computed from action dependencies themselves (as opposed to the hash of classes + * that were used in the last action execution, which are always included). + */ + public TrackingInfo getTrackingInfo(Action action, Artifact artifact, boolean includeDependencyHash) { + boolean isUnused = supportsInputTracking(action) && isUnusedInput(action, artifact); + + Set usedClasses = null; + if (supportsClassTracking(action) && canArtifactTrackUsedClasses(artifact)) { + UsageInfo usageInfo = getUsageInfo(action); + if (usageInfo != null && + usageInfo.usedClassesMap != null && + usageInfo.usedClassesMap.getOrDefault(artifact.getExecPathString(), Collections.emptySet()).size() > 0) { + usedClasses = usageInfo.usedClassesMap.get(artifact.getExecPathString()); + if (includeDependencyHash) { + usedClasses = usedClasses.stream() + .map(d -> ClassUsageInfo.create( + d.getFullyQualifiedName(), + d.getInternalPath(), + d.getCompileTimeFileArtifactValue(), + new PreComputedMetadataValue(getHashFromJarEntry(artifact, d.getInternalPath())))) + .collect(Collectors.toCollection(LinkedHashSet::new)); + } + } + } + return new TrackingInfo(isUnused, usedClasses); + } + + /** + * For debugging purpose. + */ + public String dump(Action action) { + int unusedCount = 0; + int usedCount = 0; + int usedClasses = 0; + StringBuilder s = new StringBuilder("ActionInputUsageTracker: " + getKey(action) + " "); + for (Artifact input : action.getInputs().toList()) { + if (!canArtifactBeUnused(input)) { + continue; + } + boolean isUnused = supportsInputTracking(action) && isUnusedInput(action, input); + TrackingInfo trackingInfo = getTrackingInfo(action, input, false); + if (VERBOSE_MODE) { + s.append("\n"); + if (isUnused) { + s.append("\t(-) " + input.getExecPathString()); + } else { + s.append("\t(+) " + input.getExecPathString()); + if (trackingInfo.tracksUsedClasses()) { + s.append(" (" + trackingInfo.getUsedClasses().size() + " tracked classes)"); + } + } + } else { + if (isUnused) { + unusedCount++; + } else { + usedCount++; + if (trackingInfo.tracksUsedClasses()) { + usedClasses += trackingInfo.getUsedClasses().size(); + } + } + } + } + if (!VERBOSE_MODE) { + String usedClassesStr = supportsClassTracking(action) ? String.format(", %d tracked classes", usedClasses) : ""; + s.append(String.format("[%d used dep, %d unused dep%s]", usedCount, unusedCount, usedClassesStr)); + } + + return s.toString(); + } + + /** + * Returns the cache key used internally for a given action + */ + private String getKey(Action action) { + return action.getOwner().getLabel().toString() + "(" + action.getMnemonic() + ")"; + } + + /** + * Returns whether input artifact is used by action or not. + */ + private boolean isUnusedInput(Action action, Artifact input) { + // Now that jdeps contains additional info (list of used classes/shas), we don't want these to contribute to + // cache key, as this could cause undesired change of cache key. + String artifactExecPath = input.getExecPathString(); + if (artifactExecPath.endsWith(".jdeps")) { + return true; + } + + if (!supportsInputTracking(action)) { + return false; + } + + UsageInfo usageInfo = getUsageInfo(action); + return usageInfo != null && usageInfo.unusedArtifactPaths.contains(artifactExecPath); + } + + /** + * Returns UsageInfo for the given action. + */ + @Nullable + private UsageInfo getUsageInfo(Action action) { + return trackerInfoMap.getOrDefault(getKey(action), null); + } + + /** + * Returns whether artifact is eligible to be treated as unused. For JVM, compiling against + * ABI jars is pre-requisite, therefore we only support ijar/hjar/kotlin ABI artifacts. + */ + private static boolean canArtifactBeUnused(Artifact artifact) { + String artifactExecPath = artifact.getExecPathString(); + return artifactExecPath.endsWith("-ijar.jar") || + artifactExecPath.endsWith("-hjar.jar") || + artifactExecPath.endsWith(".abi.jar"); + } + + /** + * Returns whether input artifact can track classes or not. + */ + private static boolean canArtifactTrackUsedClasses(Artifact artifact) { + String artifactExecPath = artifact.getExecPathString(); + return artifactExecPath.endsWith("-ijar.jar") || + artifactExecPath.endsWith("-hjar.jar") || + artifactExecPath.endsWith(".abi.jar"); + } +} diff --git a/src/main/java/com/google/devtools/build/lib/actions/usage/ClassUsageInfo.java b/src/main/java/com/google/devtools/build/lib/actions/usage/ClassUsageInfo.java new file mode 100644 index 00000000000000..3e88c72b8926b8 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/actions/usage/ClassUsageInfo.java @@ -0,0 +1,77 @@ +// Copyright 2014 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.actions.usage; + +import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.view.proto.Deps; + +import javax.annotation.Nullable; + +/** + * Class holding info for an input artifact internal class being used by an action. + */ +public class ClassUsageInfo { + private String fullyQualifiedName; + private String internalPath; + private FileArtifactValue compileTimeFileArtifactValue; + @Nullable + private FileArtifactValue dependencyFileArtifactValue; + + private ClassUsageInfo() { + } + + static ClassUsageInfo create(Deps.UsedClass c) { + return create(c.getFullyQualifiedName(), + c.getJarInternalPath(), + new PreComputedMetadataValue(c.getHash().toByteArray()), + /* dependencyFileArtifactValue= */ null); + } + + static ClassUsageInfo create(String fullyQualifiedName, String internalPath, FileArtifactValue compileTimeFileArtifactValue, FileArtifactValue dependencyFileArtifactValue) { + ClassUsageInfo usageInfo = new ClassUsageInfo(); + usageInfo.fullyQualifiedName = fullyQualifiedName; + usageInfo.internalPath = internalPath; + usageInfo.compileTimeFileArtifactValue = compileTimeFileArtifactValue; + usageInfo.dependencyFileArtifactValue = dependencyFileArtifactValue; + return usageInfo; + } + + /** + * Fully qualified name of class being used. + */ + public String getFullyQualifiedName() { + return fullyQualifiedName; + } + + /** + * Path of .class file within compiled (abi) jar. + */ + public String getInternalPath() { + return internalPath; + } + + /** + * Hash of .class file as to when it was used during compilation. + */ + public FileArtifactValue getCompileTimeFileArtifactValue() { + return compileTimeFileArtifactValue; + } + + /** + * Hash of .class file as extracted by input artifact jar prior to compilation. + */ + public FileArtifactValue getDependencyFileArtifactValue() { + return dependencyFileArtifactValue; + } +} diff --git a/src/main/java/com/google/devtools/build/lib/actions/usage/PreComputedMetadataValue.java b/src/main/java/com/google/devtools/build/lib/actions/usage/PreComputedMetadataValue.java new file mode 100644 index 00000000000000..dfeeac6dbd22c3 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/actions/usage/PreComputedMetadataValue.java @@ -0,0 +1,61 @@ +// Copyright 2014 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.actions.usage; + +import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.FileContentsProxy; +import com.google.devtools.build.lib.actions.FileStateType; +import com.google.devtools.build.lib.vfs.Path; + +/** + * Implementation of FileArtifactValue using pre-computed hardcoded value. + */ +public final class PreComputedMetadataValue extends FileArtifactValue { + private byte[] digest; + + PreComputedMetadataValue(byte[] digest) { + this.digest = digest; + } + + @Override + public FileStateType getType() { + return FileStateType.REGULAR_FILE; + } + + @Override + public byte[] getDigest() { + return digest; + } + + @Override + public FileContentsProxy getContentsProxy() { + throw new UnsupportedOperationException(); + } + + @Override + public long getSize() { + return 0; + } + + @Override + public long getModifiedTime() { + return -1; + } + + @Override + public boolean wasModifiedSinceDigest(Path path) { + throw new UnsupportedOperationException( + "PreComputedMetadataValue doesn't support wasModifiedSinceDigest " + path.toString()); + } +} diff --git a/src/main/java/com/google/devtools/build/lib/actions/usage/TrackingInfo.java b/src/main/java/com/google/devtools/build/lib/actions/usage/TrackingInfo.java new file mode 100644 index 00000000000000..236dc32fb8856b --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/actions/usage/TrackingInfo.java @@ -0,0 +1,45 @@ +// Copyright 2014 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.actions.usage; + +import javax.annotation.Nullable; +import java.util.Set; + +/** + * Class holding tracking info for a given action/input artifact pair. + */ +public class TrackingInfo { + + private boolean isUnused; + + @Nullable + private Set usedClasses; + + public TrackingInfo(boolean isUnused, Set usedClasses) { + this.isUnused = isUnused; + this.usedClasses = usedClasses; + } + + public boolean isUnused() { + return isUnused; + } + + public boolean tracksUsedClasses() { + return usedClasses != null; + } + + public Set getUsedClasses() { + return usedClasses; + } +} diff --git a/src/main/java/com/google/devtools/build/lib/actions/usage/Utils.java b/src/main/java/com/google/devtools/build/lib/actions/usage/Utils.java new file mode 100644 index 00000000000000..3e845a92913d77 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/actions/usage/Utils.java @@ -0,0 +1,61 @@ +// Copyright 2014 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.actions.usage; + +import com.google.common.io.ByteStreams; +import com.google.devtools.build.lib.actions.Action; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.vfs.DigestHashFunction; + +import javax.annotation.Nullable; +import java.io.IOException; +import java.io.InputStream; +import java.util.List; +import java.util.jar.JarFile; +import java.util.stream.Collectors; +import java.util.zip.ZipEntry; + +/** + * Utility class for action input tracking. + */ +class Utils { + + /** + * Compute sha256 of the jar entry corresponding to provided path. + */ + static byte[] getHashFromJarEntry(Artifact artifact, String path) { + try (JarFile jarFile = new JarFile(artifact.getPath().getPathFile())) { + ZipEntry entry = jarFile.getEntry(path); + if (entry == null) { + return null; + } + InputStream stream = jarFile.getInputStream(entry); + byte[] targetArray = ByteStreams.toByteArray(stream); + return DigestHashFunction.SHA256.getHashFunction().hashBytes(targetArray).asBytes(); + } catch (IOException e) { + } + return new byte[0]; + } + + /** + * Get action .jdeps artifact, used to extract compilation tracking information from. + */ + @Nullable + static Artifact getJDepsOutput(Action action) { + List jdepsOutput = action.getOutputs().stream() + .filter(output -> output.getExecPathString().endsWith(".jdeps")) + .collect(Collectors.toList()); + return jdepsOutput.size() == 1 ? jdepsOutput.get(0) : null; + } +} diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java index 9d1d087b4eefee..cfad76c494695f 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java @@ -21,6 +21,7 @@ import com.google.devtools.build.lib.util.ResourceConverter; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.common.options.BoolOrEnumConverter; +import com.google.devtools.common.options.EnumConverter; import com.google.devtools.common.options.Converters; import com.google.devtools.common.options.Converters.CaffeineSpecConverter; import com.google.devtools.common.options.Converters.RangeConverter; @@ -409,6 +410,23 @@ public boolean useTopLevelTargetsForSymlinks() { help = "Whether to use the action cache") public boolean useActionCache; + @Option( + name = "experimental_action_input_usage_tracker", + converter = ActionInputUsageTrackerModeConverter.class, + defaultValue = "disabled", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = { + OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION, + OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS + }, + help = "This flag controls how Bazel tracks usage of action inputs (for rules that supports it, like Java " + + "and Kotlin), in order to achieve compilation avoidance and improve build time. Possible values:\n" + + " disabled (default): normal behavior, i.e no optimization.\n" + + " unused_dependencies: tracks unused input artifact. Changes to these won't cause action re-execution.\n" + + " unused_classes: tracks unused classes. Changes to these won't cause action re-execution.\n") + @Nullable + public ActionInputUsageTrackerMode experimentalActionInputUsageTrackerMode; + @Option( name = "experimental_action_cache_store_output_metadata", defaultValue = "false", @@ -621,4 +639,19 @@ enum ConvenienceSymlinksMode { /** Will not create or clean up any symlinks, but will record the symlinks. */ LOG_ONLY } + + public static class ActionInputUsageTrackerModeConverter extends EnumConverter { + public ActionInputUsageTrackerModeConverter() { + super(ActionInputUsageTrackerMode.class, "usage tracker mode"); + } + } + + public enum ActionInputUsageTrackerMode { + /** Disabled. */ + DISABLED, + /** Tracks unused input artifacts. */ + UNUSED_DEPENDENCIES, + /** Tracks unused classes within input artifacts. */ + UNUSED_CLASSES, + } } diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java index ec026c6933b717..52626aa71c68ef 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java @@ -31,7 +31,9 @@ import com.google.devtools.build.lib.actions.ActionExecutionStatusReporter; import com.google.devtools.build.lib.actions.ActionGraph; import com.google.devtools.build.lib.actions.ActionInputPrefetcher; +import com.google.devtools.build.lib.actions.usage.ActionInputUsageTracker; import com.google.devtools.build.lib.actions.ArtifactFactory; +import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.BuildFailedException; import com.google.devtools.build.lib.actions.DynamicStrategyRegistry; import com.google.devtools.build.lib.actions.Executor; @@ -905,12 +907,18 @@ private Builder createBuilder( Predicate executionFilter = CheckUpToDateFilter.fromOptions(request.getOptions(ExecutionOptions.class)); ArtifactFactory artifactFactory = env.getSkyframeBuildView().getArtifactFactory(); + ActionInputUsageTracker actionInputUsageTracker = new ActionInputUsageTracker( + ArtifactPathResolver.createPathResolver( + executor.getFileSystem(), + executor.getExecRoot()), + options.experimentalActionInputUsageTrackerMode); return new SkyframeBuilder( skyframeExecutor, env.getLocalResourceManager(), new ActionCacheChecker( actionCache, artifactFactory, + actionInputUsageTracker, skyframeExecutor.getActionKeyContext(), executionFilter, ActionCacheChecker.CacheConfig.builder() diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java index 7f94317e74677e..77cc3c5eb2a721 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java @@ -353,6 +353,7 @@ && getJavaConfiguration().experimentalEnableJspecify() builder.setExtraData(JavaCommon.computePerPackageData(ruleContext, javaToolchain)); builder.setStrictJavaDeps(attributes.getStrictJavaDeps()); builder.setFixDepsTool(getJavaConfiguration().getFixDepsTool()); + builder.setExperimentalTrackClassUsage(getJavaConfiguration().experimentalTrackClassUsage()); builder.setCompileTimeDependencyArtifacts(attributes.getCompileTimeDependencyArtifacts()); builder.setTargetLabel( attributes.getTargetLabel() == null ? label : attributes.getTargetLabel()); diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java index 725ea563eac527..f939ee343dc190 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java @@ -42,6 +42,7 @@ import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.ActionResult; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.BaseSpawn; import com.google.devtools.build.lib.actions.CommandAction; import com.google.devtools.build.lib.actions.CommandLine; diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java index 5aa6094071e305..da3b3a3265977a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java @@ -148,6 +148,7 @@ public void extend(ExtraActionInfo.Builder builder, ImmutableList argume private ImmutableList sourceJars = ImmutableList.of(); private StrictDepsMode strictJavaDeps = StrictDepsMode.ERROR; private String fixDepsTool = "add_dep"; + private boolean experimentalTrackClassUsage = false; private NestedSet directJars = NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER); private NestedSet compileTimeDependencyArtifacts = NestedSetBuilder.emptySet(Order.STABLE_ORDER); @@ -372,6 +373,9 @@ private CustomCommandLine buildParamFileContents( result.addExecPaths("--direct_dependencies", directJars); } result.add("--experimental_fix_deps_tool", fixDepsTool); + if (experimentalTrackClassUsage) { + result.add("--experimental_track_class_usage"); + } // Chose what artifact to pass to JavaBuilder, as input to jacoco instrumentation processor. if (coverageArtifact != null) { @@ -425,6 +429,14 @@ public JavaCompileActionBuilder setCompileTimeDependencyArtifacts( return this; } + /** + * Sets the class tracking mode. + */ + public JavaCompileActionBuilder setExperimentalTrackClassUsage(boolean experimentalTrackClassUsage) { + this.experimentalTrackClassUsage = experimentalTrackClassUsage; + return this; + } + public JavaCompileActionBuilder setJavacOpts(ImmutableList copts) { this.javacOpts = Preconditions.checkNotNull(copts); return this; diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java index 57020b7d1ee1c1..acb3021ac00391 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java @@ -85,6 +85,7 @@ public enum ImportDepsCheckingLevel { private final boolean allowRuntimeDepsOnNeverLink; private final JavaClasspathMode javaClasspath; private final boolean inmemoryJdepsFiles; + private final boolean experimentalTrackClassUsage; private final ImmutableList defaultJvmFlags; private final StrictDepsMode strictJavaDeps; private final String fixDepsTool; @@ -122,6 +123,7 @@ public JavaConfiguration(BuildOptions buildOptions) throws InvalidConfigurationE javaOptions.javaDeps || javaOptions.javaClasspath != JavaClasspathMode.OFF; this.javaClasspath = javaOptions.javaClasspath; this.inmemoryJdepsFiles = javaOptions.inmemoryJdepsFiles; + this.experimentalTrackClassUsage = javaOptions.experimentalTrackClassUsage; this.defaultJvmFlags = ImmutableList.copyOf(javaOptions.jvmOpts); this.strictJavaDeps = javaOptions.strictJavaDeps; this.fixDepsTool = javaOptions.fixDepsTool; @@ -247,6 +249,10 @@ public boolean inmemoryJdepsFiles() { return inmemoryJdepsFiles; } + public boolean experimentalTrackClassUsage() { + return experimentalTrackClassUsage; + } + @Override public ImmutableList getDefaultJvmFlags() { return defaultJvmFlags; diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java index 375f3694452592..7575ee997dd2a6 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java @@ -209,6 +209,19 @@ public ImportDepsCheckingLevelConverter() { + "written to disk.") public boolean inmemoryJdepsFiles; + @Option( + name = "experimental_track_class_usage", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.BUILD_TIME_OPTIMIZATION, + effectTags = { + OptionEffectTag.LOADING_AND_ANALYSIS, + OptionEffectTag.EXECUTION, + OptionEffectTag.AFFECTS_OUTPUTS + }, + metadataTags = {OptionMetadataTag.EXPERIMENTAL}, + help = "If enabled, produced .jdeps output file will contains granular used classes information.") + public boolean experimentalTrackClassUsage; + @Option( name = "java_debug", defaultValue = "null", @@ -626,6 +639,7 @@ public FragmentOptions getHost() { host.javaDeps = javaDeps; host.javaClasspath = javaClasspath; host.inmemoryJdepsFiles = inmemoryJdepsFiles; + host.experimentalTrackClassUsage = experimentalTrackClassUsage; host.strictJavaDeps = strictJavaDeps; host.fixDepsTool = fixDepsTool; diff --git a/src/main/protobuf/deps.proto b/src/main/protobuf/deps.proto index 45c3c53cad8457..1a14602f390cfb 100644 --- a/src/main/protobuf/deps.proto +++ b/src/main/protobuf/deps.proto @@ -48,6 +48,22 @@ message Dependency { // Source file locations: compilers can pinpoint the uses of a dependency. repeated SourceLocation location = 3; + + // List of used classes. + repeated UsedClass usedClasses = 4; +} + +message UsedClass { + + // Class fully qualified name + required string fullyQualifiedName = 1; + + // Path within jar to .class file + required string jarInternalPath = 2; + + // Hash of the corresponding .class bytecode. + required bytes hash = 3; + } // Top-level message found in .deps artifacts diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java index 536d0866e32373..646644f1e1d80b 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java @@ -39,11 +39,13 @@ import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics; import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics.MissDetail; import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics.MissReason; +import com.google.devtools.build.lib.actions.usage.ActionInputUsageTracker; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.actions.util.ActionsTestUtil.FakeArtifactResolverBase; import com.google.devtools.build.lib.actions.util.ActionsTestUtil.FakeMetadataHandlerBase; import com.google.devtools.build.lib.actions.util.ActionsTestUtil.MissDetailsBuilder; import com.google.devtools.build.lib.actions.util.ActionsTestUtil.NullAction; +import com.google.devtools.build.lib.buildtool.BuildRequestOptions; import com.google.devtools.build.lib.clock.Clock; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; @@ -107,6 +109,10 @@ private ActionCacheChecker createActionCacheChecker(boolean storeOutputMetadata) return new ActionCacheChecker( cache, new FakeArtifactResolverBase(), + new ActionInputUsageTracker( + ArtifactPathResolver.IDENTITY, + BuildRequestOptions.ActionInputUsageTrackerMode.DISABLED + ), new ActionKeyContext(), action -> true, ActionCacheChecker.CacheConfig.builder() diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java index ff6567a0888e68..c6cc6b1563b764 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java @@ -54,6 +54,7 @@ import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ArtifactOwner; +import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.ArtifactRoot.RootType; import com.google.devtools.build.lib.actions.BasicActionLookupValue; @@ -63,6 +64,7 @@ import com.google.devtools.build.lib.actions.MiddlemanType; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.actions.ResourceManager; +import com.google.devtools.build.lib.actions.usage.ActionInputUsageTracker; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.actions.util.DummyExecutor; import com.google.devtools.build.lib.actions.util.InjectedActionLookupKey; @@ -738,6 +740,10 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext) new ActionCacheChecker( AMNESIAC_CACHE, new ActionsTestUtil.FakeArtifactResolverBase(), + new ActionInputUsageTracker( + ArtifactPathResolver.IDENTITY, + BuildRequestOptions.ActionInputUsageTrackerMode.DISABLED + ), new ActionKeyContext(), Predicates.alwaysTrue(), /*cacheConfig=*/ null); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java index 016fcd91f618c7..f8b8f3ad7f3c38 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java @@ -40,6 +40,7 @@ import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; +import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.ArtifactRoot.RootType; import com.google.devtools.build.lib.actions.BasicActionLookupValue; @@ -54,6 +55,7 @@ import com.google.devtools.build.lib.actions.cache.ActionCache; import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics; import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics.MissReason; +import com.google.devtools.build.lib.actions.usage.ActionInputUsageTracker; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.actions.util.DummyExecutor; import com.google.devtools.build.lib.actions.util.InjectedActionLookupKey; @@ -365,7 +367,13 @@ public void buildArtifacts( executor, options, new ActionCacheChecker( - actionCache, null, actionKeyContext, ALWAYS_EXECUTE_FILTER, null), + actionCache, + null, + new ActionInputUsageTracker(ArtifactPathResolver.IDENTITY, BuildRequestOptions.ActionInputUsageTrackerMode.DISABLED), + actionKeyContext, + ALWAYS_EXECUTE_FILTER, + null + ), /*outputService=*/ null, /*incrementalAnalysis=*/ true); skyframeActionExecutor.setActionExecutionProgressReportingObjects( diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD index a72684d6430307..f9b8913ea2dd73 100644 --- a/src/test/shell/bazel/BUILD +++ b/src/test/shell/bazel/BUILD @@ -214,6 +214,26 @@ sh_test( exec_compatible_with = ["//:highcpu_machine"], ) +sh_test( + name = "bazel_java_compilation_avoidance_test", + size = "large", + timeout = "eternal", + srcs = ["bazel_java_compilation_avoidance.sh"], + args = [ + # java_tools zips to test + "src/java_tools.zip", + "src/java_tools_prebuilt.zip", + ], + data = [ + ":test-deps", + "//src:java_tools_prebuilt_zip", + "//src:java_tools_zip", + "//src/test/shell/bazel/testdata:jdk_http_archives_filegroup", + "@bazel_tools//tools/bash/runfiles", + ], + exec_compatible_with = ["//:highcpu_machine"], +) + JAVA_VERSIONS = ("11", "17") JAVA_VERSIONS_COVERAGE = ("11", "17") diff --git a/src/test/shell/bazel/bazel_java_compilation_avoidance.sh b/src/test/shell/bazel/bazel_java_compilation_avoidance.sh new file mode 100755 index 00000000000000..34a4276b25b101 --- /dev/null +++ b/src/test/shell/bazel/bazel_java_compilation_avoidance.sh @@ -0,0 +1,239 @@ +#!/bin/bash +# +# Copyright 2016 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# Tests the examples provided in Bazel +# + +# --- begin runfiles.bash initialization --- +if [[ ! -d "${RUNFILES_DIR:-/dev/null}" && ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + if [[ -f "$0.runfiles_manifest" ]]; then + export RUNFILES_MANIFEST_FILE="$0.runfiles_manifest" + elif [[ -f "$0.runfiles/MANIFEST" ]]; then + export RUNFILES_MANIFEST_FILE="$0.runfiles/MANIFEST" + elif [[ -f "$0.runfiles/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + export RUNFILES_DIR="$0.runfiles" + fi +fi +if [[ -f "${RUNFILES_DIR:-/dev/null}/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + source "${RUNFILES_DIR}/bazel_tools/tools/bash/runfiles/runfiles.bash" +elif [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + source "$(grep -m1 "^bazel_tools/tools/bash/runfiles/runfiles.bash " \ + "$RUNFILES_MANIFEST_FILE" | cut -d ' ' -f 2-)" +else + echo >&2 "ERROR: cannot find @bazel_tools//tools/bash/runfiles:runfiles.bash" + exit 1 +fi +# --- end runfiles.bash initialization --- + +source "$(rlocation "io_bazel/src/test/shell/integration_test_setup.sh")" \ + || { echo "integration_test_setup.sh not found!" >&2; exit 1; } + +case "$(uname -s | tr [:upper:] [:lower:])" in +msys*|mingw*|cygwin*) + declare -r is_windows=true + ;; +*) + declare -r is_windows=false + ;; +esac + +if "$is_windows"; then + export MSYS_NO_PATHCONV=1 + export MSYS2_ARG_CONV_EXCL="*" +fi + +JAVA_TOOLCHAIN="@bazel_tools//tools/jdk:toolchain" + +JAVA_TOOLS_ZIP="$1"; shift +if [[ "${JAVA_TOOLS_ZIP}" != "released" ]]; then + if [[ "${JAVA_TOOLS_ZIP}" == file* ]]; then + JAVA_TOOLS_ZIP_FILE_URL="${JAVA_TOOLS_ZIP}" + elif "$is_windows"; then + JAVA_TOOLS_ZIP_FILE_URL="file:///$(rlocation io_bazel/$JAVA_TOOLS_ZIP)" + else + JAVA_TOOLS_ZIP_FILE_URL="file://$(rlocation io_bazel/$JAVA_TOOLS_ZIP)" + fi +fi +JAVA_TOOLS_ZIP_FILE_URL=${JAVA_TOOLS_ZIP_FILE_URL:-} + +JAVA_TOOLS_PREBUILT_ZIP="$1"; shift +if [[ "${JAVA_TOOLS_PREBUILT_ZIP}" != "released" ]]; then + if [[ "${JAVA_TOOLS_PREBUILT_ZIP}" == file* ]]; then + JAVA_TOOLS_PREBUILT_ZIP_FILE_URL="${JAVA_TOOLS_PREBUILT_ZIP}" + elif "$is_windows"; then + JAVA_TOOLS_PREBUILT_ZIP_FILE_URL="file:///$(rlocation io_bazel/$JAVA_TOOLS_PREBUILT_ZIP)" + else + JAVA_TOOLS_PREBUILT_ZIP_FILE_URL="file://$(rlocation io_bazel/$JAVA_TOOLS_PREBUILT_ZIP)" + fi + # Remove the repo overrides that are set up for some Bazel CI workers. + inplace-sed "/override_repository=remote_java_tools=/d" "$TEST_TMPDIR/bazelrc" + inplace-sed "/override_repository=remote_java_tools_linux=/d" "$TEST_TMPDIR/bazelrc" + inplace-sed "/override_repository=remote_java_tools_windows=/d" "$TEST_TMPDIR/bazelrc" + inplace-sed "/override_repository=remote_java_tools_darwin=/d" "$TEST_TMPDIR/bazelrc" +fi +JAVA_TOOLS_PREBUILT_ZIP_FILE_URL=${JAVA_TOOLS_PREBUILT_ZIP_FILE_URL:-} + +if [[ $# -gt 0 ]]; then + JAVA_LANGUAGE_VERSION="$1"; shift + add_to_bazelrc "build --java_language_version=${JAVA_LANGUAGE_VERSION}" + add_to_bazelrc "build --tool_java_language_version=${JAVA_LANGUAGE_VERSION}" +fi + + +if [[ $# -gt 0 ]]; then + JAVA_RUNTIME_VERSION="$1"; shift + add_to_bazelrc "build --java_runtime_version=${JAVA_RUNTIME_VERSION}" + add_to_bazelrc "build --tool_java_runtime_version=${JAVA_RUNTIME_VERSION}" + if [[ "${JAVA_RUNTIME_VERSION}" == 8 ]]; then + JAVA_TOOLCHAIN="@bazel_tools//tools/jdk:toolchain_java8" + elif [[ "${JAVA_RUNTIME_VERSION}" == 11 ]]; then + JAVA_TOOLCHAIN="@bazel_tools//tools/jdk:toolchain_java11" + else + JAVA_TOOLCHAIN="@bazel_tools//tools/jdk:toolchain_jdk_${JAVA_RUNTIME_VERSION}" + fi +fi + +export TESTENV_DONT_BAZEL_CLEAN=1 + +function set_up() { + cat >>WORKSPACE <>WORKSPACE <> WORKSPACE +} + +function tear_down() { + rm -rf "$(bazel info bazel-bin)/java" +} + +function write_project_files() { + mkdir -p java/libA + cat >java/libA/BUILD <java/libA/A.java <java/libB/BUILD <java/libB/B.java <java/libC/BUILD <java/libC/Used.java <java/libC/Unused.java < ABI change of C will *not* recompile A. +function test_unused_java_dependency() { + write_project_files + bazel build //java/libA &>"${TEST_log}" --nojava_header_compilation --experimental_action_input_usage_tracker=unused_dependencies || fail "Expected to build" + + inplace-sed "s/myVar/myVar_/g" "java/libC/Used.java" + bazel build //java/libA &>"${TEST_log}" --nojava_header_compilation --experimental_action_input_usage_tracker=unused_dependencies || fail "Expected to build" + expect_log " processes: .* 1 worker" +} + +# Java lib B depends on Java lib C, and uses classes from B. +# -> ABI change of C will recompile B. +function test_used_java_dependency() { + write_project_files + bazel build //java/libB &>"${TEST_log}" --nojava_header_compilation --experimental_action_input_usage_tracker=unused_dependencies || fail "Expected to build" + + inplace-sed "s/myVar/myVar_/g" "java/libC/Used.java" + bazel build //java/libB &>"${TEST_log}" --nojava_header_compilation --experimental_action_input_usage_tracker=unused_dependencies || fail "Expected to build" + expect_log " processes: .* 2 worker" +} + +# Java lib B depends on Java lib C, and uses class 'UsedClass' from Java lib C. +# -> ABI change to class 'UsedClass' will recompile B. +function test_used_java_class() { + write_project_files + bazel build //java/libB &>"${TEST_log}" --nojava_header_compilation --experimental_action_input_usage_tracker=unused_classes --experimental_track_class_usage || fail "Expected to build" + + inplace-sed "s/myVar/myVar_/g" "java/libC/Used.java" + bazel build //java/libB &>"${TEST_log}" --nojava_header_compilation --experimental_action_input_usage_tracker=unused_classes --experimental_track_class_usage || fail "Expected to build" + expect_log " processes: .* 2 worker" +} + +# Java lib B depends on Java lib C, and use class 'UsedClass' from Java lib C. +# -> ABI change to class 'UnusedClass' from C will *not* recompile B. +function test_unused_java_class() { + write_project_files + bazel build //java/libB &>"${TEST_log}" --nojava_header_compilation --experimental_action_input_usage_tracker=unused_classes --experimental_track_class_usage || fail "Expected to build" + + inplace-sed "s/myVar/myVar_/g" "java/libC/Unused.java" + bazel build //java/libB &>"${TEST_log}" --nojava_header_compilation --experimental_action_input_usage_tracker=unused_classes --experimental_track_class_usage || fail "Expected to build" + expect_log " processes: .* 1 worker" +} + +run_suite "Java compilation avoidance integration tests" \ No newline at end of file From 793c62f8ace50db8014c55d6ee58ff1e428f586f Mon Sep 17 00:00:00 2001 From: Oli Date: Wed, 5 Oct 2022 16:35:27 -0700 Subject: [PATCH 2/3] Fix concurency issue when populating jdeps content --- .../lib/actions/usage/ActionInputUsageTracker.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/usage/ActionInputUsageTracker.java b/src/main/java/com/google/devtools/build/lib/actions/usage/ActionInputUsageTracker.java index 1a6e9e2229bfd8..cabb91aae256d3 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/usage/ActionInputUsageTracker.java +++ b/src/main/java/com/google/devtools/build/lib/actions/usage/ActionInputUsageTracker.java @@ -22,6 +22,7 @@ import java.io.FileNotFoundException; import java.io.InputStream; import java.util.*; +import java.util.concurrent.ConcurrentHashMap; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -64,7 +65,7 @@ class UsageInfo { private static final Set SUPPORTED_DEPENDENCY_TRACKING_MNEMONICS = Set.of("Javac", "KotlinCompile"); private static final Set SUPPORTED_CLASS_TRACKING_MNEMONICS = Set.of("Javac", "KotlinCompile"); - private static final boolean VERBOSE_MODE = true; + private static final boolean VERBOSE_MODE = false; private final ArtifactPathResolver pathResolver; private final ActionInputUsageTrackerMode trackerMode; @@ -73,7 +74,7 @@ class UsageInfo { public ActionInputUsageTracker(ArtifactPathResolver pathResolver, ActionInputUsageTrackerMode trackerMode) { this.pathResolver = pathResolver; this.trackerMode = trackerMode; - this.trackerInfoMap = new HashMap<>(); + this.trackerInfoMap = new ConcurrentHashMap<>(); } /** @@ -231,6 +232,10 @@ private String getKey(Action action) { * Returns whether input artifact is used by action or not. */ private boolean isUnusedInput(Action action, Artifact input) { + if (!supportsInputTracking(action)) { + return false; + } + // Now that jdeps contains additional info (list of used classes/shas), we don't want these to contribute to // cache key, as this could cause undesired change of cache key. String artifactExecPath = input.getExecPathString(); @@ -238,10 +243,6 @@ private boolean isUnusedInput(Action action, Artifact input) { return true; } - if (!supportsInputTracking(action)) { - return false; - } - UsageInfo usageInfo = getUsageInfo(action); return usageInfo != null && usageInfo.unusedArtifactPaths.contains(artifactExecPath); } From 20812ba74159678deafdb68ba6ed035f381545be Mon Sep 17 00:00:00 2001 From: Oli Date: Thu, 13 Oct 2022 10:08:46 -0700 Subject: [PATCH 3/3] Fix NPE inserting null value in ConcurrentHashMap per code-review feedback --- .../build/lib/actions/usage/ActionInputUsageTracker.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/usage/ActionInputUsageTracker.java b/src/main/java/com/google/devtools/build/lib/actions/usage/ActionInputUsageTracker.java index cabb91aae256d3..f153d9afc90bd3 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/usage/ActionInputUsageTracker.java +++ b/src/main/java/com/google/devtools/build/lib/actions/usage/ActionInputUsageTracker.java @@ -146,7 +146,9 @@ public void refreshInputTrackingInfo(Action action) { System.err.println("ActionInputUsageTracker: " + getKey(action) + " .jdeps file failed to load, ex=" + exception); } - trackerInfoMap.put(getKey(action), usageInfo); + if (usageInfo != null) { + trackerInfoMap.put(getKey(action), usageInfo); + } } /**