Batch mode driver work (almost usable?) #14525

graydon · 2018-02-10T10:54:05Z

This includes the remaining batch mode driver components from the prototype cleaned up to the point where I think they're credible to land.

It's very lightly fiddled-with; I wouldn't even say tested yet. But it's also disabled by default in its disabled state, appears not to perturb tests (at least on linux -- checking macos now). I just wanted to get all this in place so David has something he can play with (since it appears I might get pulled off again to do other things for another few days next week).

It's also missing, of course, support for writing OFMs to frontends, or randomizing batch partition, or probably a dozen forms of failure management I haven't covered yet.

graydon · 2018-02-10T10:54:31Z

@swift-ci please test

jrose-apple · 2018-02-12T22:51:21Z

lib/Driver/Job.cpp

+  assert(&DerivedOutputMap == &other.DerivedOutputMap);
+  Inputs.append(other.Inputs.begin(),
+                other.Inputs.end());
+  types::forAllTypes([&](types::ID Type) {


SmallSet has an iterator-based insert which will be equivalent.

Yes, as a sink; but it doesn't have an accessor to give you iterator(s) as a source!

Oh, bah! Okay then.

That said, if you don't care about adding to an existing map, you could just do a copy-assignment.

In this case we're doing a merge, the moral equivalent of +=, so I do care about the existing entries (if I'm understanding your comment correctly?)

I understand that the operation is written as +=, but at the only place you use it it'll be empty or identical anyway, right?

Oh, fair point. I'll modify it to assert that.

jrose-apple

This is not as intrusive as I feared! Some comments so far, but mostly this looks like it will work, while not getting in the way of non-batch mode.

jrose-apple · 2018-02-12T22:51:57Z

include/swift/Driver/ToolChain.h

@@ -166,6 +166,27 @@ class ToolChain {
                                    std::unique_ptr<CommandOutput> output,
                                    const OutputInfo &OI) const;

+  /// Return true iff the input \c Job \p A is an acceptable candidate for
+  /// batching together into a BatchJob, via a call to \c constructBathJob. This


Very relaxing typo: constructBathJob.

Hah, fixed.

jrose-apple · 2018-02-12T22:52:37Z

lib/Driver/ToolChain.cpp

@@ -150,6 +151,150 @@ types::ID ToolChain::lookupTypeForExtension(StringRef Ext) const {
  return types::lookupTypeForExtension(Ext);
 }

+// Return a _single_ TY_Swift InputAction, if one exists;
+// if 0 or >1 such inputs exist, return nullptr.


Doc comment?

Sure I guess? (do static functions in implementation files get docs extracted to anywhere useful?)

Code completion and Quick Help will pick them up in Xcode.

jrose-apple · 2018-02-12T22:54:15Z

lib/Driver/ToolChain.cpp

+    return false;
+
+  // Check Jobs have same executable.
+  if (strcmp(A->getExecutable(), B->getExecutable()) != 0)


I don't think CompileJobActions will ever be allowed to pick different executables, so maybe this can just be an assertion.

jrose-apple · 2018-02-12T22:54:59Z

include/swift/Driver/ToolChain.h

+  /// This is true when each job independently satisfies \c jobIsBatchable, and the
+  /// jobs have identical executables, output types and environments (i.e. they are
+  /// identical aside from their inputs).
+  bool jobsAreBatchCombinable(const Job *A, const Job *B) const;


I think it makes sense to require that this be an equivalence relation, so as not to (supposedly) need quadratic checking at the use site.

Hm, fair; I think it technically is one already, I was just describing it as a pairwise relation out of paranoia. I'll change the doc comment to reflect the reality.

jrose-apple · 2018-02-12T22:55:34Z

lib/Driver/ToolChain.cpp

+  if (strcmp(A->getExecutable(), B->getExecutable()) != 0)
+    return false;
+
+  // Check Jobs are making the same kind of output.


Arguably they have to be making the same supplementary outputs too.

Good catch, fixed.

jrose-apple · 2018-02-12T23:21:49Z

lib/Driver/Compilation.cpp

+        if (Comp.ShowJobLifecycle)
+          llvm::outs() << "Batch job finished: "
+                       << LogJob(FinishedCmd) << "\n";
+        auto const *B = static_cast<const BatchJob *>(FinishedCmd);


I just realized Job needs a virtual destructor if BatchJob is going to contain a SmallVector.

Whoops, fixed!

jrose-apple · 2018-02-12T23:25:45Z

lib/Driver/Compilation.cpp

@@ -410,6 +449,7 @@ namespace driver {
      SmallVector<const Job *, 16> Dependents;
      if (Comp.getIncrementalBuildEnabled()) {
        reloadAndRemarkDeps(FinishedCmd, ReturnCode, Dependents);
+        formBatchJobsAndAddPendingJobsToTaskQueue();


I had to go figure out how reloadAndRemarkDeps could schedule new jobs. Maybe there should be a comment around that. (My fault, not yours.)

Fixing. Though .. there's something a little hinky here that I was kinda papering over and I'm still uncomfortable with: we should pick up the extra pending jobs and requeue them in the outer loop (runTaskQueueToCompletion); I'm going to take another look and see if I can figure out why were weren't (there's a single test that fails without one of these seemingly-redundant formBatchJobsAndAddPendingJobsToTaskQueue calls).

Oh gosh, ok, found the underlying issue here and it's both quite subtle and would have been pretty serious if I'd let it go.

(I put a couple comments on the reload path anyways, but it's not the culprit)

jrose-apple · 2018-02-12T23:32:59Z

lib/Driver/ToolChain.cpp

+  // with a CompileJobAction and JobContext that differ only slightly.
+
+  if (jobs.size() == 0)
+    return nullptr;


If it's 1, worth early-exiting with the single Job?

Hm, I don't think so; but I do think we should guard against getting to this path with any number of jobs < 2. I'll do a little refactoring in the caller and put an assert in here.

Eh actually this just moves ugliness in the logic elsewhere, I'm going to leave it as-is.

graydon · 2018-02-15T08:38:46Z

@swift-ci please test

graydon · 2018-02-15T08:46:44Z

@jrose-apple pushed update with review commentary addressed (I think!), with particular emphasis on 91aa7e4 which was actually quite a serious problem -- those seemingly-redundant calls to formBatchJobsAndAddPendingJobsToTaskQueue were not-very-thoroughly compensating for PendingExecution jobs being dropped on the floor in the outer loop -- but I believe I have set it right now. Added a bunch of commentary; let me know if it's not clear.

(The remaining changes are mostly tidying, commenting and splitting functions up as sensibly requested by @davidungar; again, I don't really know that the batching code works at the moment since there isn't frontend code quite ready to connect it to, and it's disabled by default anyways; but it seems about as logically reasonable as it was in its previous iteration which we did manage to test in prototype form, and it at least compiles?)

swift-ci · 2018-02-15T09:55:47Z

Build failed
Swift Test OS X Platform
Git Sha - c93497e4677a681362c04b5b808616a6d5324c04

jrose-apple · 2018-02-15T16:54:18Z

include/swift/Driver/Compilation.h

@@ -17,6 +17,7 @@
 #ifndef SWIFT_DRIVER_COMPILATION_H
 #define SWIFT_DRIVER_COMPILATION_H

+#include "swift/Driver/Driver.h"


Nitpick: this include seems to be unnecessary.

It's needed to get the complete type for driver::OutputInfo.

Oh, I didn't realize that didn't have its own header. Carry on.

jrose-apple · 2018-02-15T17:09:26Z

lib/Driver/Compilation.cpp

+      auto const &TC = Comp.getToolChain();
+      if (TC.jobsAreBatchCombinable(Comp, Cmd, CurrentBatch[0]))
+        return true;
+      return CurrentBatch.size() < TargetBatchSize;


It looks like these conditions are backwards: you're doing an OR rather than an AND check here.

Ah good catch.

jrose-apple · 2018-02-15T17:11:34Z

lib/Driver/Job.cpp

@@ -181,6 +193,8 @@ CommandOutput::dump() const {
  llvm::errs() << '\n';
 }

+Job::~Job() {


Nitpick: You can write this as = default for the same effect.

jrose-apple · 2018-02-15T17:12:42Z

lib/Driver/ToolChain.cpp

+ToolChain::jobsAreBatchCombinable(const Compilation &C,
+                                  const Job *A, const Job *B) const {
+  return (jobIsBatchable(C, A) &&
+          jobIsBatchable(C, B) &&


To avoid redundant checking, you could assert these conditions instead.

Sure, fixed.

jrose-apple · 2018-02-15T17:19:04Z

lib/Driver/Job.cpp

+  assert(&DerivedOutputMap == &other.DerivedOutputMap);
+  Inputs.append(other.Inputs.begin(),
+                other.Inputs.end());
+  types::forAllTypes([&](types::ID Type) {


That said, if you don't care about adding to an existing map, you could just do a copy-assignment.

…Types.

graydon · 2018-02-15T23:40:53Z

@swift-ci please test

swift-ci · 2018-02-15T23:41:23Z

Build failed
Swift Test OS X Platform
Git Sha - c93497e4677a681362c04b5b808616a6d5324c04

graydon · 2018-02-15T23:41:33Z

@jrose-apple fixed issues discussed so far, squashed and rebased.

swift-ci · 2018-02-15T23:42:17Z

Build failed
Swift Test Linux Platform
Git Sha - c93497e4677a681362c04b5b808616a6d5324c04

jrose-apple · 2018-02-15T23:55:53Z

lib/Driver/Compilation.cpp

+        // there are, we need to continue trying to make progress on the
+        // TaskQueue before we start marking deferred jobs as skipped, below.
+        if (!PendingExecution.empty() && Result == 0) {
+          formBatchJobsAndAddPendingJobsToTaskQueue();


I suppose if this still bugs you another option is to put it at the top of the loop.

Before the call to TaskQueue::execute? I .. am pretty sure that wouldn't work.

(It doesn't bug me enough to carry on fiddling it further; I think there are about 2 variants that work right and they're similarly confusing. One has two calls, the other needs more explanation, shrug)

Yeah, it would also require changing the loop's controlling condition. This is fine.

graydon requested review from davidungar and jrose-apple February 10, 2018 10:54

jrose-apple reviewed Feb 12, 2018

View reviewed changes

jrose-apple reviewed Feb 15, 2018

View reviewed changes

[BatchMode] Add virtual destructor to Job.

6132f18

graydon force-pushed the batch-mode-driver-work branch from c93497e to 394c5a9 Compare February 15, 2018 23:19

graydon added 7 commits February 15, 2018 15:37

[BatchMode] Add CommandOutput::addOutputs and hasSameAdditionalOutput…

83aeb39

…Types.

[BatchMode] Pass OutputInfo to Compilation on construction.

a2c7187

[BatchMode] Make Compilation::TheToolChain available to clients.

798f43c

[BatchMode] Add batch-mode support methods to ToolChain.

2bf7a79

[BatchMode] Store -{enable,disable}-batch-mode as a flag in Compilation.

3d9ce0a

[BatchMode] Add driver::PerformJobsState logic for BatchJobs.

57fe9ee

[BatchMode] Fix a bug in DummyTaskQueue uncovered by recent change.

7686a5a

graydon force-pushed the batch-mode-driver-work branch from 394c5a9 to 7686a5a Compare February 15, 2018 23:39

jrose-apple approved these changes Feb 15, 2018

View reviewed changes

graydon merged commit 15e8441 into swiftlang:master Feb 16, 2018

Batch mode driver work (almost usable?) #14525

Batch mode driver work (almost usable?) #14525

Uh oh!

Conversation

graydon commented Feb 10, 2018

Uh oh!

graydon commented Feb 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graydon commented Feb 15, 2018

Uh oh!

graydon commented Feb 15, 2018

Uh oh!

swift-ci commented Feb 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment