Use ThreadLocalRandom for include scanning shuffling. LegacyIncludeScanner previously used a shared java.util.Random(88) instance to shuffle header inclusions. Under high concurrency, concurrent calls to Random.nextInt() caused severe CAS contention on its internal AtomicLong seed. Replacing it with ThreadLocalRandom.current() eliminates lock/CAS contention and restores inclusion shuffling across both sync and async execution. PiperOrigin-RevId: 972478655 Change-Id: I265baebdd12b28b7a9cc74f632da54a06c31af9f
diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScannerSupplier.java b/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScannerSupplier.java index 2dc0118..fbc245a 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScannerSupplier.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScannerSupplier.java
@@ -86,7 +86,6 @@ public IncludeScannerSupplier( BlazeDirectories directories, ExecutorService includePool, - boolean shouldShuffle, ArtifactFactory artifactFactory, Supplier<SpawnIncludeScanner> spawnIncludeScannerSupplier, Path execRoot) { @@ -109,7 +108,6 @@ new LegacyIncludeScanner( includeParser, includePool, - shouldShuffle, includeParseCache, pathCache, key.quoteIncludePaths,
diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningModule.java b/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningModule.java index 9a2d247..2df4724 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningModule.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/IncludeScanningModule.java
@@ -33,7 +33,6 @@ import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.platform.PlatformInfo; import com.google.devtools.build.lib.buildtool.BuildRequest; -import com.google.devtools.build.lib.buildtool.BuildRequestOptions; import com.google.devtools.build.lib.concurrent.ExecutorUtil; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadHostile; import com.google.devtools.build.lib.exec.ExecutorBuilder; @@ -196,7 +195,6 @@ SwigIncludeScanner scanner = new SwigIncludeScanner( includePool.get(), - shouldShuffle(env), spawnScannerSupplier.get(), cache, swigIncludePaths, @@ -347,7 +345,6 @@ new IncludeScannerSupplier( env.getDirectories(), includePool, - shouldShuffle(env), env.getSkyframeBuildView().getArtifactFactory(), spawnScannerSupplier, env.getExecRoot()); @@ -356,15 +353,4 @@ spawnScannerSupplier.get().setInMemoryOutput(options.getInMemoryIncludesFiles()); } } - - private static boolean useAsyncExecution(CommandEnvironment env) { - var buildRequestOptions = env.getOptions().getOptions(BuildRequestOptions.class); - return buildRequestOptions != null && buildRequestOptions.getUseAsyncExecution(); - } - - private static boolean shouldShuffle(CommandEnvironment env) { - // Don't shuffle if using virtual threads, otherwise it introduces high CPU regression on - // machines with large number of cores. - return !useAsyncExecution(env); - } }
diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/LegacyIncludeScanner.java b/src/main/java/com/google/devtools/build/lib/includescanning/LegacyIncludeScanner.java index 3fd1b4a..cbfcd90 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/LegacyIncludeScanner.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/LegacyIncludeScanner.java
@@ -50,13 +50,13 @@ import java.util.Iterator; import java.util.List; import java.util.Objects; -import java.util.Random; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Future; +import java.util.concurrent.ThreadLocalRandom; import java.util.function.Supplier; import javax.annotation.Nullable; @@ -444,11 +444,6 @@ private final PathExistenceCache pathCache; private final ExecutorService includePool; - private final boolean shouldShuffle; - - // We are using this Random just for shuffling, so keep the order deterministic by hardcoding - // the seed. - private static final Random CONSTANT_SEED_RANDOM = new Random(88); /** * Constructs a new IncludeScanner @@ -462,7 +457,6 @@ LegacyIncludeScanner( IncludeParser parser, ExecutorService includePool, - boolean shouldShuffle, ConcurrentMap<Artifact, ListenableFuture<Collection<Inclusion>>> cache, PathExistenceCache pathCache, List<PathFragment> quoteIncludePaths, @@ -474,7 +468,6 @@ Supplier<SpawnIncludeScanner> spawnIncludeScannerSupplier) { this.parser = parser; this.includePool = includePool; - this.shouldShuffle = shouldShuffle; this.fileParseCache = cache; this.pathCache = pathCache; this.artifactFactory = Preconditions.checkNotNull(artifactFactory); @@ -816,21 +809,15 @@ } } - Collection<Inclusion> maybeShuffledInclusions; - if (shouldShuffle) { - // Shuffle the inclusions to get better parallelism. See b/62200470. - List<Inclusion> shuffledInclusions = new ArrayList<>(inclusions); - Collections.shuffle(shuffledInclusions, CONSTANT_SEED_RANDOM); - maybeShuffledInclusions = shuffledInclusions; - } else { - maybeShuffledInclusions = inclusions; - } + // Shuffle the inclusions to get better parallelism. See b/62200470. + List<Inclusion> shuffledInclusions = new ArrayList<>(inclusions); + Collections.shuffle(shuffledInclusions, ThreadLocalRandom.current()); // For each inclusion: get or locate its target file & recursively process IncludeScannerHelper helper = new IncludeScannerHelper(includePaths, quoteIncludePaths, source); PathFragment parent = source.getExecPath().getParentDirectory(); - for (Inclusion inclusion : maybeShuffledInclusions) { + for (Inclusion inclusion : shuffledInclusions) { findAndProcess( helper.createInclusionWithContext(inclusion, contextPathPos, contextKind), source,
diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/SwigIncludeScanner.java b/src/main/java/com/google/devtools/build/lib/includescanning/SwigIncludeScanner.java index 223444f..35ac97b 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/SwigIncludeScanner.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/SwigIncludeScanner.java
@@ -40,7 +40,6 @@ */ public SwigIncludeScanner( ExecutorService includePool, - boolean shouldShuffle, SpawnIncludeScanner spawnIncludeScanner, ConcurrentMap<Artifact, ListenableFuture<Collection<Inclusion>>> cache, List<PathFragment> includePaths, @@ -50,7 +49,6 @@ super( new SwigIncludeParser(), includePool, - shouldShuffle, cache, new PathExistenceCache(execRoot, artifactFactory), /* quoteIncludePaths= */ ImmutableList.of(),