Skip to content

Use a Confinement Queue to Protect IncrementalCompilationState #565

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.

Sign up for GitHub

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 1 commit into from
Mar 30, 2021
Merged

Use a Confinement Queue to Protect IncrementalCompilationState #565

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
84 changes: 59 additions & 25 deletions Sources/SwiftDriver/IncrementalCompilation/IncrementalCompilationState.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,25 @@ import TSCBasic
import Foundation
import SwiftOptions

public class IncrementalCompilationState {
/// An instance of `IncrementalCompilationState` encapsulates the data necessary
/// to make incremental build scheduling decisions.
///
/// The primary form of interaction with the incremental compilation state is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice comment! May I offer a suggestion? "The foo is a bar" can usually be strengthened with active voice--who is doing what to whom? Also, here I would be tempted to mention the first wave, too. Something like:

"When performing an incremental compilation, the driver must determine which source files much be compiled and which may be skipped. This determination is complicated by the fact that one compilation may reveal the need to perform another compilation if, for instance, the user has changed a declaration in one file that another (unchanged) file, depends upon. These decisions are made here."

Then follow up with the "how":

"Additionally the IncrementalCompilationState must account for the batching of compilations into jobs."

And a chronology:

"It starts with the planning process, which creates the jobs for a non-incremental compilation and passes them of to the InitialCompilationState constructor, which builds the ModuleDependencyGraph and initializes two key fields: mandatoryJobsInOrder, and jobsAfterCompiles. The driver will then schedule the mandatory jobs, which are always needed. This phase is also called the first wave.

Next, as each job finishes, collectJobsDiscoveredToBeNeededAfterFinishing will discover additional jobs that must be run. collectJobsDiscoveredToBeNeededAfterFinishing is also called after each of these jobs. This phase is called the "second wave". When there is no longer any possibility of requiring additional jobs, the driver will run jobsAfterCompiles."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would make for better information in a document that describes the implementation details of the incremental build.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a document and a pointer is good, but that proposal does not answer my objection:
What bothers me is the general practice if comments that merely summarize the "what" without much, if any, of the "why". The comment, as is, feels like a "what" summary, with very little why.

Why do we have comments in the code at all? To help future maintainers. The most vital help is that which cannot be understood from reading the code, especially non-local information. I don't mean to single this PR out; many comments I read in the whole llvm/swift-compiler community fail this test.

Sure, what I wrote may feel like a bit much, but as it stands--no such document as you propose exists as of this writing--your comment could be more helpful by adding a little more of the nonobvious, nonlocal information.

However, I won't block the PR for this. I will, however, unresolve the conversation.

/// using it as an oracle to discover the jobs to execute as the incremental
/// build progresses. After a job completes, call
/// `IncrementalCompilationState.collectJobsDiscoveredToBeNeededAfterFinishing(job:)`
/// to both update the incremental state and recieve an array of jobs that
/// need to be executed in response.
///
/// Jobs become "unstuck" as their inputs become available, or may be discovered
/// by this class as fresh dependency information is integrated.
///
/// Threading Considerations
/// ========================
///
/// The public API surface of this class is thread safe, but not re-entrant.
/// FIXME: This should be an actor.
public final class IncrementalCompilationState {

/// The oracle for deciding what depends on what. Applies to this whole module.
@_spi(Testing) public let moduleDependencyGraph: ModuleDependencyGraph
Expand All @@ -25,15 +43,24 @@ public class IncrementalCompilationState {
/// Already batched, and in order of input files.
public let mandatoryJobsInOrder: [Job]

/// Jobs to run *after* the last compile, for instance, link-editing.
public let jobsAfterCompiles: [Job]

/// A high-priority confinement queue that must be used to protect the incremental compilation state.
private let confinementQueue: DispatchQueue = DispatchQueue(label: "com.apple.swift-driver.incremental-compilation-state", qos: .userInteractive)

/// Sadly, has to be `var` for formBatchedJobs
///
/// After initialization, mutating accesses to the driver must be protected by
/// the confinement queue.
private var driver: Driver

/// Keyed by primary input. As required compilations are discovered after the first wave, these shrink.
///
/// This state is modified during the incremental build. All accesses must
/// be protected by the confinement queue.
private var skippedCompileGroups = [TypedVirtualPath: CompileJobGroup]()

/// Jobs to run *after* the last compile, for instance, link-editing.
public let jobsAfterCompiles: [Job]

// MARK: - Creating IncrementalCompilationState if possible
/// Return nil if not compiling incrementally
init?(
Expand Down Expand Up @@ -124,10 +151,6 @@ extension IncrementalCompilationState {
/// for the first wave to execute.
let mandatoryJobsInOrder: [Job]
}




}

extension Driver {
Expand Down Expand Up @@ -199,18 +222,20 @@ extension IncrementalCompilationState {
/// Careful: job may not be primary.
public func collectJobsDiscoveredToBeNeededAfterFinishing(
job finishedJob: Job) throws -> [Job]? {
// Find and deal with inputs that now need to be compiled
let invalidatedInputs = collectInputsInvalidatedByRunning(finishedJob)
assert(Set(invalidatedInputs).isDisjoint(with: finishedJob.primaryInputs),
"Primaries should not overlap secondaries.")

if let reporter = self.reporter {
for input in invalidatedInputs {
reporter.report(
"Queuing because of dependencies discovered later:", input)
return try self.confinementQueue.sync {
// Find and deal with inputs that now need to be compiled
let invalidatedInputs = collectInputsInvalidatedByRunning(finishedJob)
assert(Set(invalidatedInputs).isDisjoint(with: finishedJob.primaryInputs),
"Primaries should not overlap secondaries.")

if let reporter = self.reporter {
for input in invalidatedInputs {
reporter.report(
"Queuing because of dependencies discovered later:", input)
}
}
return try getJobs(for: invalidatedInputs)
}
return try getJobs(for: invalidatedInputs)
}

/// Needed for API compatibility, `result` will be ignored
Expand All @@ -222,6 +247,7 @@ extension IncrementalCompilationState {

/// After `job` finished find out which inputs must compiled that were not known to need compilation before
private func collectInputsInvalidatedByRunning(_ job: Job)-> Set<TypedVirtualPath> {
dispatchPrecondition(condition: .onQueue(self.confinementQueue))
guard job.kind == .compile else {
return Set<TypedVirtualPath>()
}
Expand All @@ -231,8 +257,10 @@ extension IncrementalCompilationState {
.subtracting(job.primaryInputs) // have already compiled these
}

private func collectInputsInvalidated(byCompiling input: TypedVirtualPath)
-> TransitivelyInvalidatedInputSet {
private func collectInputsInvalidated(
byCompiling input: TypedVirtualPath
) -> TransitivelyInvalidatedInputSet {
dispatchPrecondition(condition: .onQueue(self.confinementQueue))
if let found = moduleDependencyGraph.collectInputsRequiringCompilation(byCompiling: input) {
return found
}
Expand All @@ -242,8 +270,10 @@ extension IncrementalCompilationState {
}

/// Find the jobs that now must be run that were not originally known to be needed.
private func getJobs(for invalidatedInputs: Set<TypedVirtualPath>
private func getJobs(
for invalidatedInputs: Set<TypedVirtualPath>
) throws -> [Job] {
dispatchPrecondition(condition: .onQueue(self.confinementQueue))
let unbatched = invalidatedInputs.flatMap { input -> [Job] in
if let group = skippedCompileGroups.removeValue(forKey: input) {
let primaryInputs = group.compileJob.primaryInputs
Expand All @@ -264,12 +294,16 @@ extension IncrementalCompilationState {
// MARK: - After the build
extension IncrementalCompilationState {
var skippedCompilationInputs: Set<TypedVirtualPath> {
Set(skippedCompileGroups.keys)
return self.confinementQueue.sync {
Set(skippedCompileGroups.keys)
}
}
public var skippedJobs: [Job] {
skippedCompileGroups.values
.sorted {$0.primaryInput.file.name < $1.primaryInput.file.name}
.flatMap {$0.allJobs()}
return self.confinementQueue.sync {
skippedCompileGroups.values
.sorted {$0.primaryInput.file.name < $1.primaryInput.file.name}
.flatMap {$0.allJobs()}
}
}
}

Expand Down