Skip to content

[Incremental] Harness immutability to make inputDependencySourceMap robust #675

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 5 commits into from
Jun 9, 2021
Merged

[Incremental] Harness immutability to make inputDependencySourceMap robust #675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac5301f
Fix tests to prepare for immutable map
Jun 5, 2021
426c0b9
do not mutate
Jun 5, 2021
b6cc8ec
Remove inputDependencySourceMap from priors
Jun 5, 2021
f6ae539
Fix comment for InputDependencySourceMap.init
Jun 7, 2021
4887e02
Remove superfluous white space.
Jun 9, 2021
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
18 changes: 14 additions & 4 deletions Sources/SwiftDriver/IncrementalCompilation/BidirectionalMap.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,6 @@

/// Like a two-way dictionary
///
// FIXME: The current use of this abstraction in the driver is
// fundamentally broken. This data structure should be retired ASAP.
// See the extended FIXME in its use in
// `ModuleDependencyGraph.inputDependencySourceMap`
public struct BidirectionalMap<T1: Hashable, T2: Hashable>: Equatable, Sequence {
private var map1: [T1: T2] = [:]
private var map2: [T2: T1] = [:]
Expand Down Expand Up @@ -74,6 +70,20 @@ public struct BidirectionalMap<T1: Hashable, T2: Hashable>: Equatable, Sequence
public func contains(key: T2) -> Bool {
map2.keys.contains(key)
}

public mutating func updateValue(_ newValue: T2, forKey key: T1) -> T2? {
let oldValue = map1.updateValue(newValue, forKey: key)
_ = oldValue.map {map2.removeValue(forKey: $0)}
map2[newValue] = key
return oldValue
}
public mutating func updateValue(_ newValue: T1, forKey key: T2) -> T1? {
let oldValue = map2.updateValue(newValue, forKey: key)
_ = oldValue.map {map1.removeValue(forKey: $0)}
map1[newValue] = key
return oldValue
}

public mutating func removeValue(forKey t1: T1) {
if let t2 = map1[t1] {
map2.removeValue(forKey: t2)
Expand Down
2 changes: 1 addition & 1 deletion Sources/SwiftDriver/IncrementalCompilation/DirectAndTransitiveCollections.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public struct Directly {}
public struct InvalidatedSet<ClosureLevel, Element: Hashable>: Sequence {
var contents: Set<Element>

init(_ s: Set<Element> = Set()) {
@_spi(Testing) public init(_ s: Set<Element> = Set()) {
self.contents = s
}
init<Elements: Sequence>(_ elements: Elements)
Expand Down
10 changes: 2 additions & 8 deletions Sources/SwiftDriver/IncrementalCompilation/IncrementalDependencyAndInputSetup.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,6 @@ extension IncrementalCompilationState.IncrementalDependencyAndInputSetup {
else {
return buildInitialGraphFromSwiftDepsAndCollectInputsInvalidatedByChangedExternals()
}
guard graph.populateInputDependencySourceMap(for: .inputsAddedSincePriors) else {
return nil
}
graph.dotFileWriter?.write(graph)

// Any externals not already in graph must be additions which should trigger
Expand All @@ -255,13 +252,10 @@ extension IncrementalCompilationState.IncrementalDependencyAndInputSetup {
private func buildInitialGraphFromSwiftDepsAndCollectInputsInvalidatedByChangedExternals()
-> (ModuleDependencyGraph, TransitivelyInvalidatedInputSet)?
{
let graph = ModuleDependencyGraph(self, .buildingWithoutAPrior)
assert(outputFileMap.onlySourceFilesHaveSwiftDeps())

guard graph.populateInputDependencySourceMap(for: .buildingFromSwiftDeps) else {
guard let graph = ModuleDependencyGraph(self, .buildingWithoutAPrior)
else {
return nil
}

var inputsInvalidatedByChangedExternals = TransitivelyInvalidatedInputSet()
for input in sourceFiles.currentInOrder {
guard let invalidatedInputs =
Expand Down
129 changes: 17 additions & 112 deletions Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import SwiftOptions
@_spi(Testing) public var nodeFinder = NodeFinder()

/// Maps input files (e.g. .swift) to and from the DependencySource object.
@_spi(Testing) public private(set) var inputDependencySourceMap = InputDependencySourceMap()
@_spi(Testing) public let inputDependencySourceMap: InputDependencySourceMap

// The set of paths to external dependencies known to be in the graph
public internal(set) var fingerprintedExternalDependencies = Set<FingerprintedExternalDependency>()
Expand All @@ -43,7 +43,7 @@ import SwiftOptions
/// Minimize the number of file system modification-time queries.
private var externalDependencyModTimeCache = [ExternalDependency: Bool]()

public init(_ info: IncrementalCompilationState.IncrementalDependencyAndInputSetup,
public init?(_ info: IncrementalCompilationState.IncrementalDependencyAndInputSetup,
_ phase: Phase
) {
self.info = info
Expand All @@ -52,6 +52,11 @@ import SwiftOptions
: nil
self.phase = phase
self.creationPhase = phase
guard let inputDependencySourceMap = InputDependencySourceMap(info)
else {
return nil
}
self.inputDependencySourceMap = inputDependencySourceMap
}

@_spi(Testing) public func sourceRequired(for input: TypedVirtualPath,
Expand Down Expand Up @@ -188,46 +193,9 @@ extension ModuleDependencyGraph {
return nodeFinder.findNodes(for: source).map {!$0.isEmpty}
?? false
}

/// Returns: false on error
func populateInputDependencySourceMap(
`for` purpose: InputDependencySourceMap.AdditionPurpose
) -> Bool {
let ofm = info.outputFileMap
let diags = info.diagnosticEngine
var allFound = true
for input in info.inputFiles {
if let source = ofm.dependencySource(for: input, diagnosticEngine: diags) {
inputDependencySourceMap.addEntry(input, source, for: purpose)
} else {
// Don't break in order to report all failures.
allFound = false
}
}
return allFound
}
}
extension OutputFileMap {
fileprivate func dependencySource(
for sourceFile: TypedVirtualPath,
diagnosticEngine: DiagnosticsEngine
) -> DependencySource? {
assert(sourceFile.type == FileType.swift)
guard let swiftDepsPath = existingOutput(inputFile: sourceFile.fileHandle,
outputType: .swiftDeps)
else {
// The legacy driver fails silently here.
diagnosticEngine.emit(
.remarkDisabled("\(sourceFile.file.basename) has no swiftDeps file in the output file map")
)
return nil
}
assert(VirtualPath.lookup(swiftDepsPath).extension == FileType.swiftDeps.rawValue)
let typedSwiftDepsFile = TypedVirtualPath(file: swiftDepsPath, type: .swiftDeps)
return DependencySource(typedSwiftDepsFile)
}
}


// MARK: - Scheduling the 2nd wave
extension ModuleDependencyGraph {
/// After `source` has been compiled, figure out what other source files need compiling.
Expand Down Expand Up @@ -489,7 +457,6 @@ extension ModuleDependencyGraph {
case useIDNode = 4
case externalDepNode = 5
case identifierNode = 6
case mapNode = 7

/// The human-readable name of this record.
///
Expand All @@ -510,8 +477,6 @@ extension ModuleDependencyGraph {
return "EXTERNAL_DEP_NODE"
case .identifierNode:
return "IDENTIFIER_NODE"
case .mapNode:
return "MAP_NODE"
}
}
}
Expand Down Expand Up @@ -563,12 +528,15 @@ extension ModuleDependencyGraph {
private var identifiers: [String] = [""]
private var currentDefKey: DependencyKey? = nil
private var nodeUses: [(DependencyKey, Int)] = []
private var inputDependencySourceMap: [(TypedVirtualPath, DependencySource)] = []
public private(set) var allNodes: [Node] = []

init(_ info: IncrementalCompilationState.IncrementalDependencyAndInputSetup) {
init?(_ info: IncrementalCompilationState.IncrementalDependencyAndInputSetup) {
self.fileSystem = info.fileSystem
self.graph = ModuleDependencyGraph(info, .updatingFromAPrior)
guard let graph = ModuleDependencyGraph(info, .updatingFromAPrior)
else {
return nil
}
self.graph = graph
}

func finalizeGraph() -> ModuleDependencyGraph {
Expand All @@ -577,11 +545,6 @@ extension ModuleDependencyGraph {
.record(def: dependencyKey, use: self.allNodes[useID])
assert(isNewUse, "Duplicate use def-use arc in graph?")
}
for (input, dependencySource) in inputDependencySourceMap {
graph.inputDependencySourceMap.addEntry(input,
dependencySource,
for: .readingPriors)
}
return self.graph
}

Expand Down Expand Up @@ -671,27 +634,6 @@ extension ModuleDependencyGraph {
throw ReadError.malformedDependsOnRecord
}
self.nodeUses.append( (key, Int(record.fields[0])) )
case .mapNode:
Copy link
Contributor

Choose a reason for hiding this comment

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

This tells me that we are no longer reading in swiftdeps files from serialized priors. I'm lacking context here to understand the implications of this, where were these deserialized files used?

They are one of the examples that made the SourceMap not-truly-bijective, is it the case that they never led to correct behavior? Or if they resulted in another behavior, where were we relying on it that we now choose not to?

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 I understand the confusion here.

The intention of this change is to no longer serialize and deserialize the inputDependencySourceMap. Serialized mapNodes were only used for that purpose. We still read in ModuleDependencyGraphs from the priors.

Yes, serializing the map and then deserialized it created confusion because the deserialized version might be inconsistent with the info from the OutputFileMap. This PR is premised on the believe that it is the OutputFileMap that should be the source of truth.

Does this explanation satisfy? Have I missed something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that makes sense, thank you. And it also makes sense for the output-file-map to be the canonical source of truth. Probably so much so that I am wondering why we serialized the inputDependencySourceMap in the first place, hence the confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for validating the notion of output-file-map as canonical. Wrt to serializing the inputDependencySourceMap I have to plead mea culpa. It seemed like a good idea at the time because the inputDependencySourceMap was being dynamically build as swiftdeps files were processed. When @CodaFi added the prior deserialization code, IIRC, there was a moment when we had deserialized priors with an empty map. So I (wrongly) fixed it by serializing the inputDependencySourceMap. My mistake, in conjuction with the build system's proclivity to change file names when a C++ file and Swift file had the same basename-without-extension, led to hilarity ensuing. @CodaFi got to the bottom of things there, and identified the problems with that design.

Later, I realized that the output-file-map should just be the truth, and that, consequently, the inputDependencySourceMap could be both immutable and a bijection. I hope I'm right this time!

guard record.fields.count == 2,
record.fields[0] < identifiers.count,
record.fields[1] < identifiers.count
else {
throw ReadError.malformedModuleDepGraphNodeRecord
}
let inputPathString = identifiers[Int(record.fields[0])]
let dependencySourcePathString = identifiers[Int(record.fields[1])]
let inputHandle = try VirtualPath.intern(path: inputPathString)
let inputPath = VirtualPath.lookup(inputHandle)
let dependencySourceHandle = try VirtualPath.intern(path: dependencySourcePathString)
let dependencySourcePath = VirtualPath.lookup(dependencySourceHandle)
guard inputPath.extension == FileType.swift.rawValue,
dependencySourcePath.extension == FileType.swiftDeps.rawValue,
let dependencySource = DependencySource(dependencySourceHandle)
else {
throw ReadError.malformedMapRecord
}
let input = TypedVirtualPath(file: inputHandle, type: .swift)
inputDependencySourceMap.append((input, dependencySource))
case .externalDepNode:
guard record.fields.count == 2,
record.fields[0] < identifiers.count,
Expand All @@ -717,7 +659,9 @@ extension ModuleDependencyGraph {
}
}

var visitor = Visitor(info)
guard var visitor = Visitor(info) else {
return nil
}
try Bitcode.read(bytes: data, using: &visitor)
guard let major = visitor.majorVersion,
let minor = visitor.minorVersion,
Expand Down Expand Up @@ -815,7 +759,6 @@ extension ModuleDependencyGraph {
self.emitRecordID(.useIDNode)
self.emitRecordID(.externalDepNode)
self.emitRecordID(.identifierNode)
self.emitRecordID(.mapNode)
}
}

Expand Down Expand Up @@ -877,11 +820,6 @@ extension ModuleDependencyGraph {
}
}

graph.inputDependencySourceMap.enumerateToSerializePriors { input, dependencySource in
self.addIdentifier(input.file.name)
self.addIdentifier(dependencySource.file.name)
}

for edF in graph.fingerprintedExternalDependencies {
self.addIdentifier(edF.externalDependency.fileName)
}
Expand Down Expand Up @@ -953,13 +891,6 @@ extension ModuleDependencyGraph {
// identifier data
.blob
])
self.abbreviate(.mapNode, [
.literal(RecordID.mapNode.rawValue),
// input name
.vbr(chunkBitWidth: 13),
// dependencySource name
.vbr(chunkBitWidth: 13),
])
}

private func abbreviate(
Expand Down Expand Up @@ -1022,15 +953,6 @@ extension ModuleDependencyGraph {
}
}
}
graph.inputDependencySourceMap.enumerateToSerializePriors {
input, dependencySource in
serializer.stream.writeRecord(serializer.abbreviations[.mapNode]!) {
$0.append(RecordID.mapNode)
$0.append(serializer.lookupIdentifierCode(for: input.file.name))
$0.append(serializer.lookupIdentifierCode(for: dependencySource.file.name))
}
}

for fingerprintedExternalDependency in graph.fingerprintedExternalDependencies {
serializer.stream.writeRecord(serializer.abbreviations[.externalDepNode]!, {
$0.append(RecordID.externalDepNode)
Expand Down Expand Up @@ -1126,7 +1048,6 @@ fileprivate extension DependencyKey.Designator {
extension ModuleDependencyGraph {
func matches(_ other: ModuleDependencyGraph) -> Bool {
guard nodeFinder.matches(other.nodeFinder),
inputDependencySourceMap.matches(other.inputDependencySourceMap),
fingerprintedExternalDependencies.matches(other.fingerprintedExternalDependencies)
else {
return false
Expand All @@ -1141,24 +1062,8 @@ extension Set where Element == ModuleDependencyGraph.Node {
}
}

extension InputDependencySourceMap {
fileprivate func matches(_ other: Self) -> Bool {
self == other
}
}

extension Set where Element == FingerprintedExternalDependency {
fileprivate func matches(_ other: Self) -> Bool {
self == other
}
}

/// This should be in a test file, but addMapEntry should be private.
extension ModuleDependencyGraph {
@_spi(Testing) public func mockMapEntry(
_ mockInput: TypedVirtualPath,
_ mockDependencySource: DependencySource
) {
inputDependencySourceMap.addEntry(mockInput, mockDependencySource, for: .mocking)
}
}
Loading