Refactor `IncrementalCompilationState` to compute initial state before job-generation. #621

artemcm · 2021-04-30T20:34:41Z

In the near future, planning of PrecompileModuleDependenciesJobs, for explicit module dependencies' compilation jobs, will require access to the incremental state in order to incrementalize expensive actions such as dependency scanning.

This change refactors the creation of incremental compilation state in order to move computation of the initial state (dependency graph) to occur before job-generation. The first wave of jobs is then computed after job-generation, using the initial state, and the set of jobs in the plan, as input.

Separate InitialIncrementalStateComputer to run early, by the Driver, to compute the dependency graph.
Introduce FirstWaveComputer which the the constructor of the IncrementalCompilationContext uses to compute the mandatoryJobsInOrder set of jobs the executors must run.

davidungar

Testing, one two three

davidungar · 2021-05-04T04:15:50Z

Sources/SwiftDriver/IncrementalCompilation/IncrementalCompilationState.swift

  ) throws {
-    guard driver.shouldAttemptIncrementalCompilation else { return nil }


Not your doing, but in a prior PR, someone else moved shouldAttemptIncrementalCompilation into the driver, which diffused the knowledge about incrementality. It would be great to find a better place for it.

davidungar · 2021-05-04T04:27:50Z

Sources/SwiftDriver/IncrementalCompilation/InitialStateComputer.swift

+    guard shouldAttemptIncrementalCompilation else { return nil }
+
+    guard let outputFileMap = outputFileMap else {
+      diagnosticEngine.emit(.warning_incremental_requires_output_file_map)
+      return nil
+    }
+
+    let reporter : IncrementalCompilationState.Reporter?
+    if options.contains(.showIncremental) {
+      reporter = IncrementalCompilationState.Reporter(diagnosticEngine: diagnosticEngine,
+                                                      outputFileMap: outputFileMap)
+    } else {
+      reporter = nil
+    }

+    guard let buildRecordInfo = buildRecordInfo else {
+      reporter?.reportDisablingIncrementalBuild("no build record path")
+      return nil
+    }
+
+    // FIXME: This should work without an output file map. We should have
+    // another way to specify a build record and where to put intermediates.
+    let maybeBuildRecord =
+      buildRecordInfo.populateOutOfDateBuildRecord(inputFiles: inputFiles,
+                                                   reporter: reporter)
+


This code encapsulates knowledge about what incremental builds need: an output file map, a build record info, etc. How about moving it (or much of it) into the InitialStateComputer, either the constructor, or in a static function?

Agreed, it is now a static method on IncrementalCompilationState.

davidungar · 2021-05-04T04:33:16Z

Sources/SwiftDriver/IncrementalCompilation/InitialStateComputer.swift

+}
+
+/// Builds the `InitialState`
+/// Also bundles up an bunch of configuration info
 extension IncrementalCompilationState {

  public struct InitialStateComputer {


Not your doing, at all, but I really want to rename the InitialStateComputer. It's my name and a horrible one. "Initial" and "state" are way too generic, and "computer" isn't much better. What is it really doing? Looking at the inputs, constructing a graph. Is it setting up for an incremental build? IncrementalBuildSetup? InputChangeAndDependencyGraphConstructor? Can you come up with something good?

Tentatively going with IncrementalDependencyAndInputSetup, I think it better reflects what it is computing. 😬

davidungar · 2021-05-04T04:36:17Z

Sources/SwiftDriver/IncrementalCompilation/IncrementalCompilationState.swift

@@ -140,7 +118,6 @@ public final class IncrementalCompilationState {
 }

 // MARK: - Initial State
-
 extension IncrementalCompilationState {
  /// The initial state of an incremental compilation plan.
  @_spi(Testing) public struct InitialState {


While I'm kvetching, I'm wondering if this struct is pulling its own weight?

davidungar · 2021-05-04T04:41:19Z

Sources/SwiftDriver/Jobs/Planning.swift

@@ -136,6 +141,7 @@ extension Driver {
    )
  }

+  // Extract options relevant to incremental builds
  mutating func computeIncrementalOptions() -> IncrementalCompilationState.Options {


I hate to sound like a broken record, and not your fault, but computeIncrementalOptions is in the wrong place, too, wrt centralizing the knowledge about incremental builds.

Moved. Thanks for pointing it out.

davidungar · 2021-05-04T05:01:10Z

Sources/SwiftDriver/IncrementalCompilation/FirstWaveComputer.swift

+    moduleDependencyGraph.phase = .updatingAfterCompilation
+
+
+    let skippedInputs = computeSkippedCompilationInputs(


It feels like this computation belongs in the other place, the initial-wobulator-computer.

As we discussed in the chat: I very much would like to have moved this to the initial state setup, but the reliance of this code on the input->outputs mapping that is a part of compilation jobs prevents us from being able to do so with the current planning/jobs architecture.

davidungar · 2021-05-04T05:01:49Z

Sources/SwiftDriver/IncrementalCompilation/FirstWaveComputer.swift

+    return skippedInputs
+  }
+
+  private func sortByCommandLineOrder(_ inputs: Set<TypedVirtualPath>) -> [TypedVirtualPath] {


same here, feels like the wrong place.

davidungar · 2021-05-04T05:03:06Z

Sources/SwiftDriver/IncrementalCompilation/FirstWaveComputer.swift

+
+  /// Encapsulates information about an input the driver has determined has
+  /// changed in a way that requires an incremental rebuild.
+  struct ChangedInput {


wrong place?

davidungar · 2021-05-04T05:03:13Z

Sources/SwiftDriver/IncrementalCompilation/FirstWaveComputer.swift

+  }
+
+  // Find the inputs that have changed since last compilation, or were marked as needed a build
+  private func computeChangedInputs(


wrong place?

davidungar · 2021-05-04T05:04:21Z

Sources/SwiftDriver/IncrementalCompilation/InitialStateComputer.swift

-/// Also bundles up an bunch of configuration info
+// Initial incremental state computation
+extension Driver {
+  func computeInitialIncrementalState(options: IncrementalCompilationState.Options)


Doesn't feel right to put this in the Driver, not your fault, I know.

Agreed, moved to IncrementalCompilationState as a static.

artemcm · 2021-05-04T19:01:30Z

@swift-ci please test

artemcm · 2021-05-05T16:11:35Z

@davidungar, the current revision addresses the above comments.
Thanks for the feedback.

davidungar

Thank you. Overall much much better, IMO. Just a few small thoughts; see what you think.

davidungar · 2021-05-05T16:22:43Z

Sources/SwiftDriver/IncrementalCompilation/IncrementalCompilationState.swift

@@ -69,81 +69,48 @@ public final class IncrementalCompilationState {
  /// be protected by the confinement queue.
  private var skippedCompileGroups = [TypedVirtualPath: CompileJobGroup]()

-// MARK: - Creating IncrementalCompilationState if possible
+  // MARK: - Creating IncrementalCompilationState if possible
  /// Return nil if not compiling incrementally


Is this comment still correct?

Nope it got stale. Thanks for the catch.

davidungar · 2021-05-05T16:27:04Z

Sources/SwiftDriver/Jobs/Planning.swift

+    if let initialState = initialIncrementalState {
+      incrementalCompilationState =
+        try IncrementalCompilationState(driver: &self, jobsInPhases: jobsInPhases,
+                                        initialState: initialState)
+    } else {
+      incrementalCompilationState = nil
+    }


Nice! Here's an interesting Swift-style question that I wonder about myself:
Is the "if" construction more or less preferable than the "map" construction:
let incrementalCompilationState = initialIncrementalState.map {...}

I think it depends on whether the reader is comfortable/familiar with "map", but I don't know. Coming from C++, one would favor "if", coming from Haskel (which I don't know), one might favor "map"?

Feel free to choose either--just mentioning it.

davidungar · 2021-05-05T16:28:52Z

Sources/SwiftDriver/Jobs/Planning.swift

@@ -104,7 +137,8 @@ extension Driver {

    try addPrecompileModuleDependenciesJobs(addJob: addJobBeforeCompiles)


BTW, nice name!

davidungar · 2021-05-05T16:32:42Z

Sources/SwiftDriver/Jobs/Planning.swift

+      try IncrementalCompilationState.computeIncrementalStateForPlanning(driver: &self)
+
+    // Compute all jobs required for this module
+    let jobsInPhases = try computeBuildJobsInPhases()


Nit: computeBuildJobsInPhases can be misread as "compute and build the jobs (in phases)".
Maybe something like "computeJobsForPhasedStandardBuild" ??

BTW, I like how this reads with the middle factored out. Thank you!

Made the name more explanatory and updated the comment.

davidungar · 2021-05-05T16:35:20Z

Sources/SwiftDriver/Jobs/Planning.swift

+  }
+
+  /// Consturct all jobs required for this module
+  mutating func computeBuildJobsInPhases() throws -> JobsInPhases {


Good catch, changed.

davidungar · 2021-05-05T16:38:05Z

Sources/SwiftDriver/Jobs/Planning.swift

+    )
+  }
+
+  /// Consturct all jobs required for this module


What does the comment say that the name of the function does not? I would either put more info in the comment, or eliminate it.
Why is this function here? The comment could be:
"After computing the basic incremental state, plan a build as if it were not incremental, so that the plan can be modified for incrementality"
or some such.

davidungar · 2021-05-05T16:38:45Z

Tests/SwiftDriverTests/Helpers/MockingIncrementalCompilation.swift

+    return Self(options, outputFileMap,
+                BuildRecordInfo.mock(diagnosticsEngine, outputFileMap),
+                nil, nil, [], fileSystem, diagnosticsEngine)


davidungar · 2021-05-05T16:42:29Z

@davidungar, the current revision addresses the above comments.
Thanks for the feedback.

My pleasure!

davidungar

Much better! Just a few nits you can take or leave.

…e job-generation. In the near future, planning of `PrecompileModuleDependenciesJobs`, for explicit module dependencies, will require access to the incremental state in order to incrementalize expensive actions such as dependency scanning. This change refactors the creation of incremental compilation state in order to move computation of the initial state (dependency graph) to occur before job-generation. The first wave of jobs is then computed after job-generation, using the initial state, and the set of jobs in the plan, as input. - Separate `InitialIncrementalStateComputer` to run early, by the `Driver`, to compute the dependency graph and the set of changed inputs. Rename `InitialIncrementalStateComputer` into `IncrementalDependencyAndInputSetup`. - Introduce `FirstWaveComputer` which the constructor of the `IncrementalCompilationContext` uses to compute the `mandatoryJobsInOrder` set of jobs the executors *must* run.

artemcm · 2021-05-05T17:15:14Z

@swift-ci please test

artemcm requested review from davidungar and CodaFi April 30, 2021 20:34

davidungar reviewed May 4, 2021

View reviewed changes

artemcm force-pushed the IncrementalRefactorForExplicit branch from 259fc05 to c0e5cf3 Compare May 4, 2021 18:51

davidungar reviewed May 5, 2021

View reviewed changes

davidungar approved these changes May 5, 2021

View reviewed changes

artemcm force-pushed the IncrementalRefactorForExplicit branch from c0e5cf3 to ccc4024 Compare May 5, 2021 17:09

artemcm merged commit 8b1e46e into swiftlang:main May 5, 2021

artemcm mentioned this pull request May 6, 2021

[5.5] Refactor IncrementalCompilationState to compute initial state before job-generation. #634

Merged

This was referenced Nov 30, 2022

[Explicit Module Builds] Serialize and re-use the inter-module dependency graph as incremental build state #1230

Merged

Bring back [Explicit Module Builds] Serialize and re-use the inter-module dependency graph as incremental build state #1236

Merged

		) throws {
		guard driver.shouldAttemptIncrementalCompilation else { return nil }

		moduleDependencyGraph.phase = .updatingAfterCompilation


		let skippedInputs = computeSkippedCompilationInputs(

		@@ -104,7 +137,8 @@ extension Driver {

		try addPrecompileModuleDependenciesJobs(addJob: addJobBeforeCompiles)

Refactor IncrementalCompilationState to compute initial state before job-generation. #621

Refactor IncrementalCompilationState to compute initial state before job-generation. #621

Uh oh!

Conversation

artemcm commented Apr 30, 2021

Uh oh!

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

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!

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!

artemcm commented May 4, 2021

Uh oh!

artemcm commented May 5, 2021

Uh oh!

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

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 May 5, 2021

Uh oh!

davidungar left a comment

Choose a reason for hiding this comment

Refactor `IncrementalCompilationState` to compute initial state before job-generation. #621

Refactor `IncrementalCompilationState` to compute initial state before job-generation. #621