Store the total number of runs per test Instead of recomputing the runs per test everywhere it's needed, compute it once and store it in the TestTargetExecutionSettings. This is purely a cleanup change and should have no impact on semantics. This should not affect memory consumption, because object size is a multiple of 8, and there were previously 11 4-byte fields (assuming compressed oops) in this class. PiperOrigin-RevId: 262096308
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java index 0022330..4991d83 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
@@ -232,6 +232,9 @@ Artifact collectCoverageScript = null; TreeMap<String, String> extraTestEnv = new TreeMap<>(); + int runsPerTest = + config.getFragment(TestConfiguration.class).getRunsPerTestForLabel(ruleContext.getLabel()); + TestTargetExecutionSettings executionSettings; if (collectCodeCoverage) { collectCoverageScript = ruleContext.getHostPrerequisiteArtifact("$collect_coverage_script"); @@ -302,16 +305,23 @@ Artifact instrumentedFileManifest = InstrumentedFileManifestAction.getInstrumentedFileManifest(ruleContext, instrumentedFiles.getInstrumentedFiles(), metadataFiles); - executionSettings = new TestTargetExecutionSettings(ruleContext, runfilesSupport, - executable, instrumentedFileManifest, shards); + executionSettings = + new TestTargetExecutionSettings( + ruleContext, + runfilesSupport, + executable, + instrumentedFileManifest, + shards, + runsPerTest); inputsBuilder.add(instrumentedFileManifest); // TODO(ulfjack): Is this even ever set? If yes, does this cost us a lot of memory? for (Pair<String, String> coverageEnvEntry : instrumentedFiles.getCoverageEnvironment()) { extraTestEnv.put(coverageEnvEntry.getFirst(), coverageEnvEntry.getSecond()); } } else { - executionSettings = new TestTargetExecutionSettings(ruleContext, runfilesSupport, - executable, null, shards); + executionSettings = + new TestTargetExecutionSettings( + ruleContext, runfilesSupport, executable, null, shards, runsPerTest); } extraTestEnv.putAll(extraEnv); @@ -323,9 +333,6 @@ } } - int runsPerTest = - config.getFragment(TestConfiguration.class).getRunsPerTestForLabel(ruleContext.getLabel()); - Iterable<Artifact> inputs = inputsBuilder.build(); int shardRuns = (shards > 0 ? shards : 1); List<Artifact.DerivedArtifact> results =
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java index f3f98c2..85c2cbe 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
@@ -342,7 +342,7 @@ fp.addInt(shardNum); fp.addInt(executionSettings.getTotalShards()); fp.addInt(runNumber); - fp.addInt(testConfiguration.getRunsPerTestForLabel(getOwner().getLabel())); + fp.addInt(executionSettings.getTotalRuns()); fp.addInt(configuration.isCodeCoverageEnabled() ? 1 : 0); fp.addStringMap(getExecutionInfo()); } @@ -386,7 +386,7 @@ testConfiguration.cacheTestResults(), readCacheStatus(), testProperties.isExternal(), - testConfiguration.getRunsPerTestForLabel(getOwner().getLabel())); + executionSettings.getTotalRuns()); } @VisibleForTesting @@ -510,8 +510,7 @@ // When we run test multiple times, set different TEST_RANDOM_SEED values for each run. // Don't override any previous setting. - if (testConfiguration.getRunsPerTestForLabel(getOwner().getLabel()) > 1 - && !env.containsKey("TEST_RANDOM_SEED")) { + if (executionSettings.getTotalRuns() > 1 && !env.containsKey("TEST_RANDOM_SEED")) { env.put("TEST_RANDOM_SEED", Integer.toString(getRunNumber() + 1)); } @@ -581,7 +580,7 @@ public String getTestSuffix() { int totalShards = executionSettings.getTotalShards(); // Use a 1-based index for user friendliness. - int runsPerTest = testConfiguration.getRunsPerTestForLabel(getOwner().getLabel()); + int runsPerTest = executionSettings.getTotalRuns(); if (totalShards > 1 && runsPerTest > 1) { return String.format( "(shard %d of %d, run %d of %d)", shardNum + 1, totalShards, runNumber + 1, runsPerTest);
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetExecutionSettings.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetExecutionSettings.java index 3277d11..29f5a48 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetExecutionSettings.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetExecutionSettings.java
@@ -39,6 +39,7 @@ private final CommandLine testArguments; private final String testFilter; private final int totalShards; + private final int totalRuns; private final RunUnder runUnder; private final Artifact runUnderExecutable; private final Artifact executable; @@ -48,8 +49,13 @@ private final Artifact runfilesInputManifest; private final Artifact instrumentedFileManifest; - TestTargetExecutionSettings(RuleContext ruleContext, RunfilesSupport runfilesSupport, - Artifact executable, Artifact instrumentedFileManifest, int shards) { + TestTargetExecutionSettings( + RuleContext ruleContext, + RunfilesSupport runfilesSupport, + Artifact executable, + Artifact instrumentedFileManifest, + int shards, + int runs) { Preconditions.checkArgument(TargetUtils.isTestRule(ruleContext.getRule())); Preconditions.checkArgument(shards >= 0); BuildConfiguration config = ruleContext.getConfiguration(); @@ -60,6 +66,7 @@ CommandLine.concat(targetArgs, ImmutableList.copyOf(testConfig.getTestArguments())); totalShards = shards; + totalRuns = runs; runUnder = config.getRunUnder(); runUnderExecutable = getRunUnderExecutable(ruleContext); @@ -96,6 +103,10 @@ return totalShards; } + public int getTotalRuns() { + return totalRuns; + } + public RunUnder getRunUnder() { return runUnder; }