Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions include/swift/Driver/Compilation.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -298,8 +293,6 @@ class Compilation {
return EnableBatchMode;
}

bool getForceOneBatchRepartition() const { return ForceOneBatchRepartition; }

bool getContinueBuildingAfterErrors() const {
return ContinueBuildingAfterErrors;
}
Expand Down
3 changes: 0 additions & 3 deletions include/swift/Option/Options.td
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
65 changes: 9 additions & 56 deletions lib/Driver/Compilation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -136,7 +135,6 @@ Compilation::Compilation(DiagnosticEngine &Diags,
BatchSeed(BatchSeed),
BatchCount(BatchCount),
BatchSizeLimit(BatchSizeLimit),
ForceOneBatchRepartition(ForceOneBatchRepartition),
SaveTemps(SaveTemps),
ShowDriverTimeCompilation(ShowDriverTimeCompilation),
Stats(std::move(StatsReporter)),
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
3 changes: 0 additions & 3 deletions lib/Driver/Driver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -939,7 +937,6 @@ Driver::buildCompilation(const ToolChain &TC,
DriverBatchSeed,
DriverBatchCount,
DriverBatchSizeLimit,
DriverForceOneBatchRepartition,
SaveTemps,
ShowDriverTimeCompilation,
std::move(StatsReporter));
Expand Down
12 changes: 12 additions & 0 deletions lib/Driver/ToolChain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
10 changes: 0 additions & 10 deletions test/Driver/batch_mode_force_one_batch_repartition.swift

This file was deleted.

28 changes: 0 additions & 28 deletions validation-test/Driver/batch_mode_overlong_argv.swift

This file was deleted.