[BatchMode] Sort batch inputs rather than asserting; they are not always sorted. #15531
Conversation
|
@swift-ci please test |
lib/Driver/ToolChain.cpp
Outdated
| @@ -319,7 +310,9 @@ ToolChain::constructBatchJob(ArrayRef<const Job *> jobs, | |||
| if (jobs.empty()) | |||
| return nullptr; | |||
|
|
|||
| assert(jobsAreSubsequenceOfCompilationInputs(jobs, C)); | |||
| llvm::SmallVector<const Job *, 16> sortedJobs; | |||
There was a problem hiding this comment.
Nitpick: no need for llvm::.
lib/Driver/ToolChain.cpp
Outdated
| assert(jobsAreSubsequenceOfCompilationInputs(jobs, C)); | ||
| llvm::SmallVector<const Job *, 16> sortedJobs; | ||
| sortJobsToMatchCompilationInputs(jobs, sortedJobs, C); | ||
| jobs = sortedJobs; |
There was a problem hiding this comment.
@davidungar would probably prefer you just use sortedJobs everywhere else; if you want to keep the diff small, you could rename the parameter to unsortedJobs instead.
There was a problem hiding this comment.
Yes, @jrose-apple , you read my mind!
lib/Driver/ToolChain.cpp
Outdated
| Compilation &C) { | ||
| llvm::SmallVector<const Job *, 16> sortedJobs; | ||
| static void sortJobsToMatchCompilationInputs(ArrayRef<const Job *> jobs, | ||
| llvm::SmallVectorImpl<const Job *> &sortedJobs, |
There was a problem hiding this comment.
Nitpick: no llvm::. I'd also consider just having this take an already-populated SmallVectorImpl (or a MutableArrayRef) and sorting it.
There was a problem hiding this comment.
It's not really a sort (in the sense of a thing you can adapt an in-place sorting algorithm to easily) so much as it's a "build a new vector in the order dictated by the input files list". But I'll fix the names anyways :)
There was a problem hiding this comment.
Ah, yes, I suppose not.
There was a problem hiding this comment.
That's a nice function name. Is there a comment anywhere explaining why we want the order to match the inputs?
|
Build failed |
graydon
force-pushed
the
sort-jobs-dont-assert
branch
from
ed6c599 to
e4bc944
Compare
|
@jrose-apple updated with test and comments addressed |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
lib/Driver/ToolChain.cpp
Outdated
| @@ -30,6 +30,7 @@ | |||
|
|
|||
| using namespace swift; | |||
| using namespace swift::driver; | |||
| using namespace llvm; | |||
There was a problem hiding this comment.
Ah, please don't do this. My comments last time were specific to SmallVector and SmallVectorImpl, two types used commonly enough that we import them into the swift namespace in swift/Basic/LLVM.h. That's not a fixed list (in general, it's fine to add more llvm/ADT/ types), but we don't want all LLVM things going unqualified.
graydon
force-pushed
the
sort-jobs-dont-assert
branch
from
e4bc944 to
b677adb
Compare
|
@jrose-apple pushed update with llvm:: back where it ought to be, and @davidungar's request for an explanatory comment added to the sort routine. |
lib/Driver/ToolChain.cpp
Outdated
| Compilation &C) { | ||
| llvm::SmallVector<const Job *, 16> sortedJobs; | ||
| /// Due to SR-631 / rdar://20535574, the success or failure of a Swift compilation | ||
| /// is currently sensitive to e order in which files are processed, in terms of |
There was a problem hiding this comment.
Typo: "e order". I also think citing a specific SR / Radar isn't so great here—that's the one we know about, but we'd want to do experiments to make sure there wasn't any order-dependency left in the non-incremental parts of the work.
Sorry for all this nitpicking!
There was a problem hiding this comment.
@jrose-apple S'ok, as I've said before, you're picky but I can't remember a nit you've picked that didn't need picking; better than my sloppy reviewing style! And being subject to it is a good exercise in cultivating equanimity anyway: the PR will land when it's ready, and not before :)
Meanwhile: pushed a fix.
|
@graydon Thanks for the clarification in the comment. It will help me someday! |
graydon
force-pushed
the
sort-jobs-dont-assert
branch
from
b677adb to
af538f1
Compare
|
@swift-ci please test and merge |
1 similar comment
|
@swift-ci please test and merge |
Some jobs sent to batch mode's helper function for batching are queued by out-of-order actions like flushed deferred jobs, or jobs discovered by traversing the dependency graph. These are not in an order that's a subsequence of the top-level compilation input sequence. Rather than asserting that they are, as we did before, we change to just forcibly sorting the jobs into the right order.