[Incremental] Harness immutability to make `inputDependencySourceMap` robust #675

davidungar · 2021-05-24T05:26:03Z

Make the inputDependencyMap immutable, solely reflecting the OutputFileMap.
Do not include it in the priors.

In a previous PR, @CodaFi uncovered and explained issues with the `inputDependencySourceMap:

   // FIXME: The map between swiftdeps and swift files is absolutely *not*`
   // a bijection. In particular, more than one swiftdeps file can be encountered
   // in the course of deserializing priors *and* reading the output file map
   // *and* re-reading swiftdeps files after frontends complete
   // that correspond to the same swift file. These cause two problems:
   // - overwrites in this data structure that lose data and
   // - cache misses in `getInput(for:)` that cause the incremental build to
   // turn over when e.g. entries in the output file map change. This should be
   // replaced by a multi-map from swift files to dependency sources,
   // and a regular map from dependency sources to swift files -
   // since that direction really is one-to-one.

This PR solves the problem in a simpler way. The key insight is that the output file map contains the information linking a source file to the swiftdeps file for any build. Thus, the output file map should just determine the entries in the inputDependencySourceMap, just as it did before the introduction of incremental imports. Then the map in question does become a true bijection. It can also be made immutable, which greatly eases reasoning about the code.

It will also make it easy to cull removed file nodes from priors in a future PR.

davidungar · 2021-05-24T05:26:12Z

@swift-ci please test

CodaFi · 2021-05-27T18:32:21Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/Tracer.swift

@@ -121,7 +121,8 @@ extension ModuleDependencyGraph.Tracer {
        path.compactMap { node in
          node.dependencySource.map {
            source in
-            graph.inputDependencySourceMap[source].map { input in
+            graph.inputDependencySourceMap.input(ifKnownFor: source).map {
+              input in


Please remove this change

Is this request still relevant now that the map has been turned into its own, domain-specific type?

CodaFi · 2021-05-27T18:32:52Z

Tests/SwiftDriverTests/Helpers/MockingIncrementalCompilation.swift

+
+
+extension OutputFileMap {
+  fileprivate static func mock(maxIndex: Int) -> Self {


Nit: Whitespace here is quite weird

Is the white space there still weird? I hope I've fixed it.

davidungar · 2021-06-05T02:30:50Z

@swift-ci please test

davidungar · 2021-06-07T19:16:06Z

@CodaFi Would you mind taking a look at this? Because you uncovered and explored the issues before with this map, you'll be a great reviewer to assess the correctness of this change. Thanks!

davidungar · 2021-06-07T20:22:53Z

@swift-ci please test

davidungar · 2021-06-08T15:35:01Z

@CodaFi Any word on when you might have time for this? Thanks!

artemcm

On the surface, this change seems like and improvement to me, and the problems Robert outlined shouldn't be possible with this data-structure being immutable. It seems reasonable to have the outputFileMap be the canonical source of dependencies for each input and it simplifies the logic quite a bit.

I'd still really like @CodaFi to weigh in because I'm less familiar with the scenarios that previously led to conflict and whether we were relying on serialized priors containing dependency sources in a way I'm not familiar with. And whether that could have implications on incremental build performance.

artemcm · 2021-06-09T16:33:56Z

Sources/SwiftDriver/IncrementalCompilation/IncrementalDependencyAndInputSetup.swift

      return nil
    }
-
+    


Nit: trailing whitespace.

Nice catch! I'll fix it.

artemcm · 2021-06-09T16:43:44Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift

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


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?

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?

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.

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!

davidungar · 2021-06-09T18:05:45Z

On the surface, this change seems like and improvement to me, and the problems Robert outlined shouldn't be possible with this data-structure being immutable. It seems reasonable to have the outputFileMap be the canonical source of dependencies for each input and it simplifies the logic quite a bit.

I'd still really like @CodaFi to weigh in because I'm less familiar with the scenarios that previously led to conflict and whether we were relying on serialized priors containing dependency sources in a way I'm not familiar with. And whether that could have implications on incremental build performance.

Thank you for your review. Yes, it would still be ideal for @CodaFi to weigh in. As I haven't received any indication of how long it will be till he can review this PR, I plan to fix the white-space issue and merge.

davidungar · 2021-06-09T18:08:45Z

@swift-ci please test

[Incremental] Harness immutability to make `inputDependencySourceMap` robust # Conflicts: # Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift

davidungar · 2021-06-11T18:46:52Z

rdar://77998890

davidungar force-pushed the after-replace-bijection branch from 3cae5b2 to 2d81f55 Compare May 25, 2021 17:59

CodaFi reviewed May 27, 2021

View reviewed changes

CodaFi mentioned this pull request May 27, 2021

Revert #664 #681

Merged

davidungar force-pushed the after-replace-bijection branch 4 times, most recently from 11c4f8c to 9b3e681 Compare June 5, 2021 02:19

davidungar mentioned this pull request Jun 6, 2021

[DNM, Incremental] Fix problem of priors injecting nodes for removed inputs into the graph. #703

Closed

David Ungar added 3 commits June 7, 2021 09:38

Fix tests to prepare for immutable map

ac5301f

do not mutate

426c0b9

Remove inputDependencySourceMap from priors

b6cc8ec

davidungar force-pushed the after-replace-bijection branch from 7d88e76 to b6cc8ec Compare June 7, 2021 16:38

davidungar changed the title ~~[DNM] Testing a new approach to inputDependencyMap~~ [Incremental] Harness immutability to make inputDependencyMap immutable Jun 7, 2021

davidungar changed the title ~~[Incremental] Harness immutability to make inputDependencyMap immutable~~ [Incremental] Harness immutability to make inputDependencySourceMap immutable Jun 7, 2021

Fix comment for InputDependencySourceMap.init

f6ae539

davidungar requested a review from artemcm June 8, 2021 15:27

davidungar changed the title ~~[Incremental] Harness immutability to make inputDependencySourceMap immutable~~ [Incremental] Harness immutability to make inputDependencySourceMap robust Jun 8, 2021

artemcm approved these changes Jun 9, 2021

View reviewed changes

Remove superfluous white space.

4887e02

davidungar merged commit f32e8e1 into swiftlang:main Jun 9, 2021

davidungar mentioned this pull request Jun 11, 2021

[5.5, Incremental] Harness immutability to make inputDependencySourceMap robust #712

Closed

davidungar deleted the after-replace-bijection branch July 2, 2021 22:10



		extension OutputFileMap {
		fileprivate static func mock(maxIndex: Int) -> Self {

[Incremental] Harness immutability to make inputDependencySourceMap robust #675

[Incremental] Harness immutability to make inputDependencySourceMap robust #675

Uh oh!

Conversation

davidungar commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidungar commented May 24, 2021

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!

davidungar commented Jun 5, 2021

Uh oh!

davidungar commented Jun 7, 2021

Uh oh!

davidungar commented Jun 7, 2021

Uh oh!

davidungar commented Jun 8, 2021

Uh oh!

artemcm 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!

davidungar commented Jun 9, 2021

Uh oh!

davidungar commented Jun 9, 2021

Uh oh!

davidungar commented Jun 11, 2021

Uh oh!

Uh oh!

[Incremental] Harness immutability to make `inputDependencySourceMap` robust #675

[Incremental] Harness immutability to make `inputDependencySourceMap` robust #675

davidungar commented May 24, 2021 •

edited

Loading