Merge pull request #19802 from graydon/rdar-43955209-breaking-up-is-hard-to-do

[Driver] <rdar://43955209> Remove obsolete/fragile batch mode 'repartitioning' code.

Author: graydon

include/swift/Driver/Compilation.h

@@ -180,10 +180,6 @@ class Compilation {
   /// by \c BatchCount.
   const Optional<unsigned> BatchSizeLimit;
 
-  /// In order to test repartitioning, set to true if
-  /// -driver-force-one-batch-repartition is present.
-  const bool ForceOneBatchRepartition = false;
-
   /// True if temporary files should not be deleted.
   const bool SaveTemps;
 
@@ -236,7 +232,6 @@ class Compilation {
               unsigned BatchSeed = 0,
               Optional<unsigned> BatchCount = None,
               Optional<unsigned> BatchSizeLimit = None,
-              bool ForceOneBatchRepartition = false,
               bool SaveTemps = false,
               bool ShowDriverTimeCompilation = false,
               std::unique_ptr<UnifiedStatsReporter> Stats = nullptr);
@@ -298,8 +293,6 @@ class Compilation {
     return EnableBatchMode;
   }
 
-  bool getForceOneBatchRepartition() const { return ForceOneBatchRepartition; }
-
   bool getContinueBuildingAfterErrors() const {
     return ContinueBuildingAfterErrors;
   }

include/swift/Option/Options.td

@@ -116,9 +116,6 @@ def driver_batch_count : Separate<["-"], "driver-batch-count">,
 def driver_batch_size_limit : Separate<["-"], "driver-batch-size-limit">,
   InternalDebugOpt,
   HelpText<"Use the given number as the upper limit on dynamic batch-mode partition size">;
-def driver_force_one_batch_repartition : Flag<["-"], "driver-force-one-batch-repartition">,
-  InternalDebugOpt,
-  HelpText<"Force one batch repartitioning for testing">;
 
 def driver_force_response_files : Flag<["-"], "driver-force-response-files">,
   InternalDebugOpt,

lib/Driver/Compilation.cpp

@@ -115,7 +115,6 @@ Compilation::Compilation(DiagnosticEngine &Diags,
                          unsigned BatchSeed,
                          Optional<unsigned> BatchCount,
                          Optional<unsigned> BatchSizeLimit,
-                         bool ForceOneBatchRepartition,
                          bool SaveTemps,
                          bool ShowDriverTimeCompilation,
                          std::unique_ptr<UnifiedStatsReporter> StatsReporter)
@@ -136,7 +135,6 @@ Compilation::Compilation(DiagnosticEngine &Diags,
     BatchSeed(BatchSeed),
     BatchCount(BatchCount),
     BatchSizeLimit(BatchSizeLimit),
-    ForceOneBatchRepartition(ForceOneBatchRepartition),
     SaveTemps(SaveTemps),
     ShowDriverTimeCompilation(ShowDriverTimeCompilation),
     Stats(std::move(StatsReporter)),
@@ -886,42 +884,6 @@ namespace driver {
         });
     }
 
-    // Due to the multiplication of the number of additional files and the
-    // number of files in a batch, it's pretty easy to construct too-long
-    // command lines here, which will then fail to exec. We address this crudely
-    // by re-forming batches with a finer partition when we overflow.
-    //
-    // Now that we're passing OutputFileMaps to frontends, this should never
-    // happen, but keep this as insurance, because the decision to pass output
-    // file maps cannot know the exact length of the command line, so may
-    // possibly fail to use the OutputFileMap.
-    //
-    // In order to be able to exercise as much of the code paths as possible,
-    // take a flag to force a retry, but only once.
-    bool shouldRetryWithMorePartitions(std::vector<const Job *> const &Batches,
-                                       bool &PretendTheCommandLineIsTooLongOnce,
-                                       size_t &NumPartitions) {
-
-      // Stop rebatching if we can't subdivide batches any further.
-      if (NumPartitions > PendingExecution.size())
-        return false;
-
-      for (auto const *B : Batches) {
-        if (!llvm::sys::commandLineFitsWithinSystemLimits(B->getExecutable(),
-                                                          B->getArguments()) ||
-            PretendTheCommandLineIsTooLongOnce) {
-          PretendTheCommandLineIsTooLongOnce = false;
-          // To avoid redoing the batch loop too many times, repartition pretty
-          // aggressively by doubling partition count / halving size.
-          NumPartitions *= 2;
-          LLVM_DEBUG(llvm::dbgs()
-                     << "Should have used a supplementary output file map.\n");
-          return true;
-        }
-      }
-      return false;
-    }
-
     // Selects the number of partitions based on the user-provided batch
     // count and/or the number of parallel tasks we can run, subject to a
     // fixed per-batch safety cap, to avoid overcommitting memory.
@@ -1049,28 +1011,19 @@ namespace driver {
       size_t NumPartitions = pickNumberOfPartitions();
       CommandSetVector Batchable, NonBatchable;
       std::vector<const Job *> Batches;
-      bool PretendTheCommandLineIsTooLongOnce =
-          Comp.getForceOneBatchRepartition();
-      do {
-        // We might be restarting loop; clear these before proceeding.
-        Batchable.clear();
-        NonBatchable.clear();
-        Batches.clear();
 
-        // Split the batchable from non-batchable pending jobs.
-        getPendingBatchableJobs(Batchable, NonBatchable);
+      // Split the batchable from non-batchable pending jobs.
+      getPendingBatchableJobs(Batchable, NonBatchable);
 
-        // Partition the batchable jobs into sets.
-        BatchPartition Partition(NumPartitions);
-        partitionIntoBatches(Batchable.takeVector(), Partition);
+      // Partition the batchable jobs into sets.
+      BatchPartition Partition(NumPartitions);
+      partitionIntoBatches(Batchable.takeVector(), Partition);
 
-        // Construct a BatchJob from each batch in the partition.
-        for (auto const &Batch : Partition) {
-          formBatchJobFromPartitionBatch(Batches, Batch);
-        }
+      // Construct a BatchJob from each batch in the partition.
+      for (auto const &Batch : Partition) {
+        formBatchJobFromPartitionBatch(Batches, Batch);
+      }
 
-      } while (shouldRetryWithMorePartitions(
-          Batches, PretendTheCommandLineIsTooLongOnce, NumPartitions));
       PendingExecution.clear();
 
       // Save batches so we can locate and decompose them on task-exit.

lib/Driver/Driver.cpp

@@ -915,8 +915,6 @@ Driver::buildCompilation(const ToolChain &TC,
     const Optional<unsigned> DriverBatchCount = getDriverBatchCount(*ArgList, Diags);
     const Optional<unsigned> DriverBatchSizeLimit =
       getDriverBatchSizeLimit(*ArgList, Diags);
-    const bool DriverForceOneBatchRepartition =
-    ArgList->hasArg(options::OPT_driver_force_one_batch_repartition);
     const bool SaveTemps = ArgList->hasArg(options::OPT_save_temps);
     const bool ShowDriverTimeCompilation =
         ArgList->hasArg(options::OPT_driver_time_compilation);
@@ -939,7 +937,6 @@ Driver::buildCompilation(const ToolChain &TC,
         DriverBatchSeed,
         DriverBatchCount,
         DriverBatchSizeLimit,
-        DriverForceOneBatchRepartition,
         SaveTemps,
         ShowDriverTimeCompilation,
         std::move(StatsReporter));

lib/Driver/ToolChain.cpp

@@ -344,6 +344,18 @@ ToolChain::constructBatchJob(ArrayRef<const Job *> unsortedJobs,
   JobContext context{C, inputJobs.getArrayRef(), inputActions.getArrayRef(),
                      *output, OI};
   auto invocationInfo = constructInvocation(*batchCJA, context);
+  // Batch mode can produce quite long command lines; in almost every case these
+  // will trigger use of supplementary output file maps, but in some rare corner
+  // cases (very few files, very long paths) they might not. However, in those
+  // cases we _should_ degrade to using response files to pass arguments to the
+  // frontend, which is done automatically by code elsewhere.
+  //
+  // The `allowsResponseFiles` flag on the `invocationInfo` we have here exists
+  // only to model external tools that don't know about response files, such as
+  // platform linkers; when talking to the frontend (which we control!) it
+  // should always be true. But double check with an assert here in case someone
+  // failed to set it in `constructInvocation`.
+  assert(invocationInfo.allowsResponseFiles);
   return llvm::make_unique<BatchJob>(
       *batchCJA, inputJobs.takeVector(), std::move(output), executablePath,
       std::move(invocationInfo.Arguments),