[Incremental] Do incremental builds #329

davidungar · 2020-10-22T23:14:47Z

EDIT: It has been redone. It returns all jobs from the planning method to preserve compatibility, but the run function uses a side door for incremental execution.

Here is a large PR (sorry!) that performs incremental builds (per our discussion the other day). It isn't optimal: it uses rebatching, and it uses llbuild in waves. It does not assume only two waves, though. Probably a bit rough in spots--all feedback welcome!
If you want to see it work, try testIncremental.

davidungar · 2020-10-22T23:15:16Z

@swift-ci please test

cltnschlosser · 2020-10-22T23:52:14Z

Sources/SwiftDriver/Driver/Driver.swift

- if jobs.contains(where: { $0.requiresInPlaceExecution })
- // Only one job and no cleanup required
- || (jobs.count == 1 && !parsedOptions.hasArgument(.parseableOutput)) {
+ if jobs.contains(where: { $0.requiresInPlaceExecution }) {


This is the c++ driver logic. I believe there are lit tests that verify this optimization happens. If this significantly complicates testing it's probably still worth not having the optimization, but there will likely be a lit test regression.

// If we don't have to do any cleanup work, just exec the subprocess. if (Level < OutputLevel::Parseable && !ShowDriverTimeCompilation && (SaveTemps || TempFilePaths.empty()) && CompilationRecordPath.empty() && Jobs.size() == 1) { return performSingleCommand(Jobs.front().get()); }

Thanks. Yes, it does complicate testing. We could add a boolean to pass in when creating the driver to disable this, just for testing. Or could fix the lit tests. What's best?

Been a while since I've contributed to swift-driver, but I try to keep up on the changes. So I haven't run the lit tests in a while, but I think it's worth running them with and without this to see what the differences are first. @owenv Might have thoughts too as he has been continuing to update swift-driver to pass more lit tests.

There aren't any lit tests for this behavior afaik, but we should try to keep it around. I don't think this change breaks anything outright, but, for example, -dump-ast disables color output because it assumes output is being redirected to a file if the frontend is launched as a subprocess. I think it does need to be disabled anytime a build record needs to be read or written.

cltnschlosser · 2020-10-22T23:59:01Z

Sources/SwiftDriver/Incremental Compilation/BuildRecordInfo.swift

- func populateOutOfDateBuildRecord() -> BuildRecord? {
+ func populateOutOfDateBuildRecord(
+ inputFiles: [TypedVirtualPath],
+ failed: (String) -> Void


Should these be Diagnostic.Messages? Seems like Strings will make it harder to support localization of diagnostics.

Can probably wait until this project has a plan for localization.

I tried to ask folks about this issue, but missed you, sorry! The advantages of using diagnostics are that it is much easier to test, and it won't end up messing up the xcodebuild logs. The disadvantage I had thought of was the incompatibility. I hadn't though of localization. What do you think?

Probably safe to just leave it as is for now, seeing as there is no plan for localization currently.

cltnschlosser · 2020-10-23T00:10:54Z

Sources/SwiftDriver/Incremental Compilation/IncrementalCompilationState.swift

+ .subtracting(firstWaveInputs)
 for skippedInput in skippedInputs.sorted(by: {$0.file.name < $1.file.name}) {
- reportIncrementalQueuing("Skipping:", skippedInput.file)
+ reportIncrementalDecision?("Skipping:", skippedInput)


Can do if let check for reportIncrementalDecision before the loop so the sort and iteration is skipped when it's nil.

Thanks! Good catch. I'll fix it.

Thanks! I'll fix it.

cltnschlosser · 2020-10-23T00:15:06Z

Sources/SwiftDriver/Incremental Compilation/IncrementalCompilationState.swift

+ func addPostCompileJobs(_ jobs: [Job]) {
+ for job in jobs {
+ for input in job.primaryInputs {
+ reportIncrementalDecision?("Delaying pending discovering delayed dependencies", input)


if let reportIncrementalDecision to skip loop if nil

Thanks, fixed!

cltnschlosser · 2020-10-23T00:16:03Z

Sources/SwiftDriver/Incremental Compilation/IncrementalCompilationState.swift

+
+ let secondWaveInputs = collectInputsToCompileIn2ndWave(job)
+ for input in secondWaveInputs {
+ reportIncrementalDecision?(


Can skip loop if nil (I wonder if the optimizer catches this?)

Thanks, fixed!

Yes, would be nice to just let the optimizer deal with it.

cltnschlosser · 2020-10-23T00:17:52Z

Sources/SwiftDriver/Incremental Compilation/IncrementalCompilationState.swift

+
+ private func rememberJobFor2ndWave(_ j: Job) {
+ for input in j.primaryInputs {
+ reportIncrementalDecision?("Scheduling for 2nd wave", input)


Can skip loop if nil

Thanks fixed

davidungar · 2020-10-23T16:48:55Z

@cltnschlosser I'm very grateful you put in the time to look at this large PR so quickly! Thank you. I'll get to your specific comments in a bit.

davidungar · 2020-10-23T19:37:01Z

@swift-ci please test

davidungar · 2020-10-23T20:08:18Z

@swift-ci please test

davidungar · 2020-10-23T22:09:31Z

@cltnschlosser I'm very grateful you put in the time to look at this large PR so quickly! Thank you. I'll get to your specific comments in a bit.

OK, I now believe I've addressed the points that I can. Thank again!

owenv

Sorry, I haven't had a chance to look over this in detail yet, I'll try to set aside some time this weekend. I added a couple initial comments. It's really exciting to see this working!

owenv · 2020-10-24T01:27:38Z

Sources/SwiftDriver/Driver/Driver.swift

- if jobs.contains(where: { $0.requiresInPlaceExecution })
- // Only one job and no cleanup required
- || (jobs.count == 1 && !parsedOptions.hasArgument(.parseableOutput)) {
+ if jobs.contains(where: { $0.requiresInPlaceExecution }) {


There aren't any lit tests for this behavior afaik, but we should try to keep it around. I don't think this change breaks anything outright, but, for example, -dump-ast disables color output because it assumes output is being redirected to a file if the frontend is launched as a subprocess. I think it does need to be disabled anytime a build record needs to be read or written.

owenv · 2020-10-24T02:24:35Z

Sources/SwiftDriver/Jobs/Planning.swift

+ let swiftInputFiles = inputFiles.filter { inputFile in
+ inputFile.type.isPartOfSwiftCompilation
 }


This change seems to break using the driver as a linker, because object files passed on the command line are no longer added to the set of linkerInputs. It seems to be causing the Driver/unknown-inputs.swift lit test to fail. Other than that, I don't see any regressions in the lit tests

owenv · 2020-10-24T17:39:20Z

Sources/SwiftDriver/Jobs/Planning.swift

+ var skippedCompileJobs = [Job]()
+ func addSkippedCompileJob(_ j: Job) {
+ skippedCompileJobs.append(j)
+ }

 try addPrecompileModuleDependenciesJobs(addJob: addJob)
 try addPrecompileBridgingHeaderJob(addJob: addJob)
 try addEmitModuleJob(addJob: addJob)

- let linkerInputs = try addJobsFeedingLinker(addJob: addJob)
+ let linkerInputs = try addJobsFeedingLinker(
+ addJob: addJob,
+ addSkippedCompileJob: addSkippedCompileJob)
+
+ var postCompileJobs = [Job]()
+ func addPostCompileJob(_ j: Job) { postCompileJobs.append(j) }
 try addLinkAndPostLinkJobs(linkerInputs: linkerInputs,
 debugInfo: debugInfo,
- addJob: addJob)
- return jobs
+ addJob: skippedCompileJobs.isEmpty
+ ? addJob
+ : addPostCompileJob)
+
+ incrementalCompilationState?.addSkippedCompileJobs(skippedCompileJobs)
+ incrementalCompilationState?.addPostCompileJobs(postCompileJobs)
+ return try formBatchedJobs(jobs)


I've been thinking about this a bit more, and I think we should try to avoid explicitly partitioning jobs into first wave, potentially-skippable and link/post-link during the planning stage. In the short term, it complicates the SPM implementation, which expects a complete, static build graph from Driver.planBuild, and in the long term I think it will make it harder to move more of the scheduling and synchronization out of Driver.run and into the llbuild-based executor.

What do you think about adding a potentiallySkippable flag to Job instead? Then, based on the new flag and Job.kind, Driver.run can do this partitioning just before execution and update the incremental state then. SPM (which always passes -incremental afaict) should keep working in that case because planBuild will continue to return the full set of jobs, and true incremental behavior would be 'opt-in' because clients would need to look at the potentiallySkippable flag and schedule jobs appropriately.

Edit: Alternatively, maybe Driver.planBuild could return a new BuildPlan type which contains the partitioning.

Very interesting! Let me think about it for a while. At first glance, I'm not sure that that flag is appropriate for inclusion in the job. We could get the same effect by having the planning phase include all the jobs, and the run method could consult the incrementalBuildState. Does that sound reasonable?

Also, someday, we'll have to enhance the relationship between SPM and the driver, if we ever want SPM to have incremental capability. But that's a task for a later time, IMO.

I also need to go back into my notes to see how to run the lit tests, etc., on swift-driver.

I very much appreciate the time and thought you're putting in. Thank you!

Very interesting! Let me think about it for a while. At first glance, I'm not sure that that flag is appropriate for inclusion in the job. We could get the same effect by having the planning phase include all the jobs, and the run method could consult the incrementalBuildState. Does that sound reasonable?

That's fair, and your proposed solution sounds good to me overall! I think as long as planBuild is returning all of the jobs in some form rather than only registering some of them with the incremental state, my concerns are mostly addressed.

Also, I haven't tested this change with SPM at all, but it might not be a bad idea to patch it so that it doesn't pass -incremental when using the integrated driver before landing this change.

@owenv OK, I've addressed the issues. Would you be interested in taking another look? If you're interested in the current state of -driver-show-incremental or -driver-show-job-lifecycle check out testIncremental().

This looks good to me, thanks! I'm probably not the best person to review the batching changes, but I didn't notice any issues except the one I mentioned above where something like swiftc test.o no longer produces any jobs.

Thank you, @owenv . Nice catches!

OK, I think it's fixed now. Tx again.

1. planBuild returns all jobs; run consults IncrementalBuildState. (for swiftpm compatibility) 2. Use job groups instead of jobs to accommodate bitcode emission. 3. Avoid potential race by keeping postCompile jobs separate

davidungar · 2020-10-25T23:26:19Z

@swift-ci please test

davidungar · 2020-10-26T23:16:03Z

@swift-ci please test

davidungar · 2020-10-27T00:40:47Z

@swift-ci please test linux platform

davidungar · 2020-10-27T03:19:22Z

@swift-ci please test linux

davidungar · 2020-10-27T03:32:48Z

@swift-ci please test macos platform

artemcm

I'm happy with how this looks!
Excited to see how it evolves and about making SwiftPM do something similar, in-tandem.

artemcm · 2020-10-27T15:25:11Z

Sources/SwiftDriver/Incremental Compilation/SynchronizedQueue.swift

@@ -0,0 +1,64 @@
+//===--------------- JobQueue.swift - Incremental -----------===//


This filename got stale.

artemcm · 2020-10-27T15:25:23Z

Sources/SwiftDriver/Incremental Compilation/SynchronizedQueue.swift

+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2019 Apple Inc. and the Swift project authors


Thank you, @artemcm, for both of these catches and most of all, for taking the time to look over this large PR!

davidungar · 2020-10-27T16:09:30Z

@swift-ci please test

David Ungar added 5 commits October 21, 2020 17:48

remove minor optimization that interfered with testing

952e1e3

Remove bogus assertion

48fed3a

Add descriptionForLifecycle to Job

1b962d3

Fix "inputs no longer used" message

375a866

Runs a simple incremental test.

28cd56a

davidungar requested review from CodaFi, artemcm, nkcsgexi and owenv October 22, 2020 23:15

cltnschlosser reviewed Oct 23, 2020

View reviewed changes

David Ungar added 3 commits October 23, 2020 12:10

loop fixes

20ae9a3

Update CMakeLists.txt

af8b377

Regenerate tests for Linux

71ec88a

Fix Linux

beb1630

davidungar requested a review from DougGregor October 23, 2020 20:12

owenv reviewed Oct 24, 2020

View reviewed changes

David Ungar added 5 commits October 24, 2020 15:05

Several changes:

bfa05e3

1. planBuild returns all jobs; run consults IncrementalBuildState. (for swiftpm compatibility) 2. Use job groups instead of jobs to accommodate bitcode emission. 3. Avoid potential race by keeping postCompile jobs separate

fixed testIncremental

2fdc410

Restore in-place optimization modulo writing build record

35f1105

factor out and fix performTheBuild

a5cd698

Don't print lifecycle messages for road not taken

3530aba

linux fix

78083de

Linux?

b40ebeb

Fix spelling of "queueing" and try for Linux

0ccbea3

artemcm approved these changes Oct 27, 2020

View reviewed changes

Fix date and filename

c535c3d

davidungar merged commit a3f1e28 into swiftlang:main Oct 27, 2020

artemcm mentioned this pull request Oct 27, 2020

Revert "[Incremental] Do incremental builds" #333

Merged

davidungar mentioned this pull request Oct 27, 2020

[Incremental] Redoing do incremental builds #334

Merged

		@@ -0,0 +1,64 @@
		//===--------------- JobQueue.swift - Incremental -----------===//

Uh oh!

[Incremental] Do incremental builds #329

[Incremental] Do incremental builds #329

Uh oh!

Conversation

davidungar commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

davidungar commented Oct 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidungar commented Oct 23, 2020

davidungar commented Oct 23, 2020

davidungar commented Oct 23, 2020

davidungar commented Oct 23, 2020

owenv left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owenv Oct 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidungar commented Oct 25, 2020

davidungar commented Oct 26, 2020

davidungar commented Oct 27, 2020

davidungar commented Oct 27, 2020

davidungar commented Oct 27, 2020

artemcm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidungar commented Oct 27, 2020

Labels

4 participants

davidungar commented Oct 22, 2020 •

edited

Loading

owenv left a comment •

edited

Loading

owenv Oct 24, 2020 •

edited

Loading