[Incremental] Interleave waves by adding llbuild rules dynamically #351

davidungar · 2020-11-07T05:31:26Z

Redo the interaction between llbuild and IncrementalBuildState in order to allow 2nd wave compilations to interleave with first wave compilations. As each first wave job finishes, add rules for discovered 2nd wave jobs.

davidungar · 2020-11-07T05:31:43Z

@swift-ci please test

davidungar · 2020-11-07T05:59:40Z

@swift-ci please test

davidungar · 2020-11-07T06:03:32Z

@swift-ci please test

davidungar · 2020-11-07T06:27:11Z

@swift-ci please test

davidungar · 2020-11-07T07:04:41Z

@swift-ci please test

owenv

This looks like a really great improvement! I don't know enough about llbuild best practices to meaningfully review the changes to MultiJobExecutor, but I didn't see any issues there. This PR will need a simultaneous update to SPM, but it'll be a really small change

owenv · 2020-11-07T21:15:04Z

Sources/SwiftDriver/Execution/DriverExecutor.swift

  func execute(jobs: [Job],
+               incrementalCompilationState: IncrementalCompilationState?,


This will require a corresponding (but small) change to SPMSwiftDriverExecutor in the SPM repo so that it still conforms to the protocol. Right now it's implementation of this requirement just fatalErrors because it only uses it's executor for dependency scanning during the planning step

Thanks! I haven't been into that repo, but will check it out.

Correction: haven't been there lately

owenv · 2020-11-07T21:26:01Z

Sources/SwiftDriver/Driver/Driver.swift

-    guard let postCompileJobs = incrementalCompilationState.postCompileJobs
-    else {
-      fatalError("planning must have finished by now")
+    var numParallelJobs: Int {


tiny nit: I think the intent would be a little clearer if this was just a constant

Thanks! Nice catch.

davidungar · 2020-11-08T00:00:58Z

This looks like a really great improvement! I don't know enough about llbuild best practices to meaningfully review the changes to MultiJobExecutor, but I didn't see any issues there. This PR will need a simultaneous update to SPM, but it'll be a really small change

Thank you. As always your help is much appreciated.

davidungar · 2020-11-08T00:01:16Z

@swift-ci please test

davidungar · 2020-11-08T02:18:03Z

Will need to be synchronized with swiftlang/swift-package-manager#3033

CodaFi · 2020-11-10T17:58:51Z

Just for my edification, let's clarify some terminology:

Primary Jobs
Secondary Jobs
Tertiary Jobs

It looks like primary jobs are the initial jobs gathered by files with mod time changes, external deps modified, etc. Secondary jobs are gathered during the swiftdeps reload step after a job finishes. Tertiary jobs are sort of a mystery to me.

davidungar · 2020-11-10T18:54:38Z

Just for my edification, let's clarify some terminology:

Primary Jobs

Secondary Jobs

Tertiary Jobs

It looks like primary jobs are the initial jobs gathered by files with mod time changes, external deps modified, etc. Secondary jobs are gathered during the swiftdeps reload step after a job finishes. Tertiary jobs are sort of a mystery to me.

Thanks much for looking at this! I will add some comments. You are absolutely right. Tertiary jobs include linking, etc. These are the jobs that must wait until all primary and secondary jobs finish. I'd be happy to brainstorm better names, if these seem to be obscure.

davidungar · 2020-11-10T19:12:12Z

@swift-ci please test

artemcm

This looks good to me! The code is quite a bit easier to follow now, too, versus the previous approach. 👍
It would be nice to have someone who knows llbuild a bit more to have a look.

artemcm · 2020-11-10T20:56:40Z

Sources/SwiftDriver/Incremental Compilation/IncrementalCompilationState.swift

+    if finishedJob.kind == .compile {
+      finishedJob.primaryInputs.forEach {
+        if pendingInputs.remove($0) == nil {
+          fatalError("should have been pending")


This error could be a little bit more descriptive and show which input is involved.

Good point! I'll fix it.

artemcm · 2020-11-10T21:03:44Z

Sources/SwiftDriver/Jobs/Planning.swift

-    incrementalCompilationState?.addPreOrCompileJobGroups(preAndCompileJobGroups)
-    incrementalCompilationState?.addPostCompileJobs(postCompileJobs)
+    incrementalCompilationState?.addPrimaryOrSkippedJobGroups(preAndCompileJobGroups)
+    incrementalCompilationState?.addTertiaryJobs(postCompileJobs)


This isn't a change request, but rather my 2¢: I preferred the name postCompileJobs, as it is completely self-explanatory. Is there a semantic difference that necessitated the change?

This sort of thing is another reason I appreciate you and @CodaFi taking the time to look at this PR. It's a great sanity check on clarity. I'll change it to the other kind of name.

CodaFi

The cleanup here is quite nice. I agree with Artem's assessment though considering even reading the comments that were there were not sufficient for me to grasp the naming change for the abstractions here.

cltnschlosser · 2020-11-10T23:26:51Z

Sources/SwiftDriver/Incremental Compilation/IncrementalCompilationState.swift

+
+  /// All the  compilation job (groups) known to be required, which have not finished yet.
+  /// (Changes as jobs complete.)
+  private var unfinishedPrimaryJobs = Set<Job>()


Getting this correct is obviously the first step, but after that might want to look at how a Set performs. #103 Was able to get potentially huge performance wins in the past by not doing Job equality checks (or hashing jobs). This is something that scales with the command line and the test cases used as a benchmark for that pull request had a non-practically large number of parameters, but similar concerns may apply here.

If we have to, we can probably use a similar approach where we use an index in the primaryJobsInOrder array as a job ID instead of using the full job itself.

Yes, that's a valid concern. I'm reluctant to go for indices unless completely necessary because then the invariants spread out a bit. But I will take a look at the code again with an eye peeled for performance issues.

And I appreciate your time and trouble here!

Yes, this complexity is N log N when in incremental mode. But the driver is reading and integrating the .swiftdeps file for each job, so it's unclear. Something to watch out for, though, yes.

cltnschlosser · 2020-11-10T23:49:56Z

Sources/SwiftDriverExecution/MultiJobExecutor.swift

-      producerMap: [VirtualPath: Int],
-      jobs: [Job],
+      nonincrementalJobs: [Job],
+      incrementalCompilationState: IncrementalCompilationState?,


Seems as though this could be described as

enum { case normal([Job]) case incremental(IncrementalCompilationState) }

Not sure if that helps readability or cleans up the api, but it's definitely more explicit (Naming could be significantly better)

Yes, I had thought of it. IIRC, there is an error case where the nonicrementalJobs are needed, even in the incremental case. I'll go back and look again. IIRC, in case of serious compiler failure, for instance no .swiftdeps written out by a frontend job, the legacy driver attempts to compile every input. But I want to double-check that. And then there is the question of preserving that behavior or not, if I'm right.

OK, did it!

cltnschlosser · 2020-11-10T23:55:28Z

Sources/SwiftDriverExecution/MultiJobExecutor.swift

+    private static func addProducts(of job: Job, index: Int, to producerMap: inout [VirtualPath: Int]) {
+      for output in job.outputs {
+        if let _ = producerMap.updateValue(index, forKey: output.file) {
+          fatalError("multiple producers for output \(output): \(job) \(producerMap[output.file]!)")


Suggested change

fatalError("multiple producers for output \(output): \(job) \(producerMap[output.file]!)")

fatalError("multiple producers for output \(output): \(job) \(jobs[producerMap[output.file]!])")

If you want direct comparison.

Ah, yes--great catch!! Thank you.

cltnschlosser · 2020-11-11T00:10:20Z

Sources/SwiftDriverExecution/MultiJobExecutor.swift

+    /// Records the task build engine for the `ExecuteAllJobs` rule (and task) so that when a
+    /// primary job finishes, and secondaries are discovered, inputs can be added to that rule for
+    /// any required secondary. Set only once.
+    private(set) var executeAllJobsTaskBuildEngine: LLTaskBuildEngine? = nil


Does this need to be weak?

Could well be! Thank you again. I'll look at it.

I think I like nilling it out in the deinit for Context better. What do you think?

Does that even work if there is a cycle? If there was a cycle deinit for Context wouldn't be called?

Just thinking out if there is even a cycle here:
Context -> Engine -> Job -> Context
I think it should be fine because the Engine should release the job when it's done, right?

Oh looks like it's actually
Engine -> Delegate -> Context -> Engine
Those are all strong links

davidungar · 2020-11-11T04:36:30Z

The cleanup here is quite nice. I agree with Artem's assessment though considering even reading the comments that were there were not sufficient for me to grasp the naming change for the abstractions here.

Thank you--I really appreciate the time and thought of you and all the other reviewers. I'll go for the other names.

davidungar · 2020-11-12T19:15:23Z

@swift-ci please test

davidungar · 2020-11-13T01:15:25Z

@swift-ci please test

Interleave waves

d405e35

davidungar requested review from owenv, cltnschlosser, artemcm and CodaFi November 7, 2020 05:32

Fix Linux tests

2d768a4

davidungar force-pushed the incremental-11-6 branch from a85792d to 2d768a4 Compare November 7, 2020 06:02

Fix >1 tertiary

313d277

David Ungar added 2 commits November 6, 2020 22:54

Revise planning

d631b4d

fine tune for Linux

f7ce02b

owenv reviewed Nov 7, 2020

View reviewed changes

Fix a nit

0720715

davidungar mentioned this pull request Nov 8, 2020

[Incremental] Update DriverExecutor protocol to be consistent with swift-driver swiftlang/swift-package-manager#3033

Merged

BenchR267 self-requested a review November 9, 2020 07:50

Add comments to clarify primary, secondary, tertiary.

9fafb09

davidungar requested a review from nkcsgexi November 10, 2020 19:02

artemcm approved these changes Nov 10, 2020

View reviewed changes

CodaFi approved these changes Nov 10, 2020

View reviewed changes

cltnschlosser reviewed Nov 11, 2020

View reviewed changes

David Ungar added 9 commits November 11, 2020 19:42

Restore clearer names

9a78db7

better fatal error message

edf8f9a

Use enum into Executor

af7c037

finish name change

836a183

better error message and break a cycle

166603b

privatization

009aa5c

batch 1st wave

b115874

WIP batching and spelling

d575017

Batch and test batching of 1st wave

f63f70b

davidungar merged commit 07d23c0 into swiftlang:main Nov 12, 2020

davidungar deleted the incremental-11-6 branch January 29, 2021 17:34

		func execute(jobs: [Job],
		incrementalCompilationState: IncrementalCompilationState?,

	fatalError("multiple producers for output \(output): \(job) \(producerMap[output.file]!)")
	fatalError("multiple producers for output \(output): \(job) \(jobs[producerMap[output.file]!])")

[Incremental] Interleave waves by adding llbuild rules dynamically #351

[Incremental] Interleave waves by adding llbuild rules dynamically #351

Uh oh!

Conversation

davidungar commented Nov 7, 2020

Uh oh!

davidungar commented Nov 7, 2020

Uh oh!

davidungar commented Nov 7, 2020

Uh oh!

davidungar commented Nov 7, 2020

Uh oh!

davidungar commented Nov 7, 2020

Uh oh!

davidungar commented Nov 7, 2020

Uh oh!

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

davidungar commented Nov 8, 2020

Uh oh!

davidungar commented Nov 8, 2020

Uh oh!

davidungar commented Nov 8, 2020

Uh oh!

CodaFi commented Nov 10, 2020

Uh oh!

davidungar commented Nov 10, 2020

Uh oh!

davidungar commented Nov 10, 2020

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!

CodaFi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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

davidungar Nov 11, 2020 •

edited

Loading