Skip Merge-Modules When We Can #34231
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The driver will eventually use this to compute the input status of a merge-modules job.
Plumb the logic necessary to schedule merge-modules incrementally. This means that if any inputs jobs to merge-modules run, merge-modules is run. But, if they are all skipped, merge-modules will be skipped as well. This requires some light special-casing of the legacy driver's incremental job handling because it assumes in a few places it can always extract a swiftdeps file. This invariant will be further broken when the precompile step for bridging headers is skipped as well. rdar://65893400
|
Awesome. |
|
@swift-ci test |
davidungar
approved these changes
Oct 8, 2020
Comment on lines
+133
to
+149
| /// The status of an input known to the driver. These are used to affect | ||
| /// the scheduling decisions made during an incremental build. | ||
| /// | ||
| /// \Note The order of cases matters. They are ordered from least to | ||
| /// greatest impact on the incremental build schedule. | ||
| enum class Status { | ||
| /// The input to this job is up to date. | ||
| UpToDate, | ||
| NeedsCascadingBuild, | ||
| /// The input to this job has changed in a way that requires this job to | ||
| /// be rerun, but not in such a way that it requires a cascading rebuild. | ||
| NeedsNonCascadingBuild, | ||
| /// The input to this job has changed in a way that requires this job to | ||
| /// be rerun, and in such a way that all jobs dependent upon this one | ||
| /// must be scheduled as well. | ||
| NeedsCascadingBuild, | ||
| /// The input to this job was not known to the driver when it was last | ||
| /// run. |
There was a problem hiding this comment.
Fantastic! The only thing you could add would be that the two "needs" options are pure guesses.
Comment on lines
+461
to
+462
| assert(isa<MergeModuleJobAction>(FinishedCmd->getSource()) || | ||
| FinishedCmd->getCondition() == Job::Condition::Always); |
There was a problem hiding this comment.
As I read this, the assertion seems like it maybe should come before the comment, and also the assertion could use more of an explanation.
| @@ -915,6 +917,11 @@ namespace driver { | |||
| continue; | |||
| } | |||
|
|
|||
| if (isa<MergeModuleJobAction>(cmd->getSource())) { | |||
| assert(!mergeModulesJob && "multiple scheduled merge-modules jobs?"); | |||
There was a problem hiding this comment.
Nice explanation for the assertion.
Comment on lines
+946
to
+954
|
|
||
| // The merge-modules job is special: it *must* be scheduled if any other | ||
| // job has been scheduled because any other job can influence the | ||
| // structure of the resulting module. Additionally, the initial scheduling | ||
| // predicate above is only aware of intra-module changes. External | ||
| // dependencies changing *must* cause merge-modules to be scheduled. | ||
| if (!jobsToSchedule.empty() && mergeModulesJob) { | ||
| jobsToSchedule.insert(mergeModulesJob); | ||
| } |
There was a problem hiding this comment.
The additions to this function aren't too long, but the net effect has been to trigger my this-function-is-too-long-to-be-easily-understood neuron. I would love it if you could chop it up into smaller pieces, either in this PR, or in a subsequent one.
| @@ -1163,7 +1186,12 @@ namespace driver { | |||
| continue; | |||
| } | |||
|
|
|||
| // Can we run a cross-module incremental build at all? If not, fallback. | |||
| // Is this module out of date? If not, just keep searching. | |||
There was a problem hiding this comment.
Not your fault, but collectIncrementalExternallyDependentJobsFromDependencyGraph is too long for my comfort, too.
Comment on lines
+62
to
+67
| case CompileJobAction::InputInfo::Status::UpToDate: | ||
| return ""; | ||
| case CompileJobAction::InputInfo::NewlyAdded: | ||
| case CompileJobAction::InputInfo::NeedsCascadingBuild: | ||
| case CompileJobAction::InputInfo::Status::NewlyAdded: | ||
| case CompileJobAction::InputInfo::Status::NeedsCascadingBuild: | ||
| return "!dirty"; | ||
| case CompileJobAction::InputInfo::NeedsNonCascadingBuild: | ||
| case CompileJobAction::InputInfo::Status::NeedsNonCascadingBuild: |
There was a problem hiding this comment.
Nice to add the typing for Status
|
⛵ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Plumb the logic necessary to schedule merge-modules incrementally. This means that if any input jobs to merge-modules run, merge-modules is run. But, if they are all skipped, merge-modules will be skipped as well.
This requires some light special-casing of the legacy driver's incremental job handling because it assumes in a few places it can always extract a swiftdeps file. This invariant will be further broken when the precompile step for bridging headers is skipped as well.
rdar://65893400