Skip to content

Skip Merge-Modules When We Can #34231

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 7 commits into from
Oct 8, 2020
Merged

Skip Merge-Modules When We Can #34231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
703eece
[NFC] Extract ModuleInputs as a Separate Utility
CodaFi Oct 7, 2020
25978e1
[NFC] Add InputInfo::makeNeedsCascadingRebuild
CodaFi Oct 7, 2020
83f5162
[NFC] Reorder InputInfo::Status In Terms of Impact on the Incremental…
CodaFi Oct 7, 2020
d29dcf8
[NFC] Upgrade CompileJobAction::InputInfo::Status to an enum class
CodaFi Oct 7, 2020
84f9ba3
Use ModuleInputs instead of a Vector to build MergeModules
CodaFi Oct 7, 2020
a81cf93
[NFC] Promote MergeModuleJobAction to an IncrementalJobAction
CodaFi Oct 7, 2020
cecb7c8
Schedule MergeModules Incrementally
CodaFi Oct 7, 2020
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
36 changes: 27 additions & 9 deletions include/swift/Driver/Action.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,23 +130,40 @@ class JobAction : public Action {
class IncrementalJobAction : public JobAction {
public:
struct InputInfo {
enum Status {
/// 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.
Comment on lines +133 to +149
Copy link
Contributor

Choose a reason for hiding this comment

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

Fantastic! The only thing you could add would be that the two "needs" options are pure guesses.

NewlyAdded
};

public:
Status status = UpToDate;
Status status = Status::UpToDate;
llvm::sys::TimePoint<> previousModTime;

InputInfo() = default;
InputInfo(Status stat, llvm::sys::TimePoint<> time)
: status(stat), previousModTime(time) {}

static InputInfo makeNewlyAdded() {
return InputInfo(Status::NewlyAdded, llvm::sys::TimePoint<>::max());
return {Status::NewlyAdded, llvm::sys::TimePoint<>::max()};
}

static InputInfo makeNeedsCascadingRebuild() {
return {Status::NeedsCascadingBuild, llvm::sys::TimePoint<>::min()};
}
};

Expand All @@ -166,7 +183,8 @@ class IncrementalJobAction : public JobAction {

public:
static bool classof(const Action *A) {
return A->getKind() == Action::Kind::CompileJob;
return A->getKind() == Action::Kind::CompileJob ||
A->getKind() == Action::Kind::MergeModuleJob;
}
};

Expand Down Expand Up @@ -263,12 +281,12 @@ class REPLJobAction : public JobAction {
}
};

class MergeModuleJobAction : public JobAction {
class MergeModuleJobAction : public IncrementalJobAction {
virtual void anchor() override;
public:
MergeModuleJobAction(ArrayRef<const Action *> Inputs)
: JobAction(Action::Kind::MergeModuleJob, Inputs,
file_types::TY_SwiftModuleFile) {}
MergeModuleJobAction(ArrayRef<const Action *> Inputs, InputInfo input)
: IncrementalJobAction(Action::Kind::MergeModuleJob, Inputs,
file_types::TY_SwiftModuleFile, input) {}

static bool classof(const Action *A) {
return A->getKind() == Action::Kind::MergeModuleJob;
Expand Down
40 changes: 34 additions & 6 deletions lib/Driver/Compilation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ namespace driver {

std::vector<const Job*>
reloadAndRemarkDeps(const Job *FinishedCmd, int ReturnCode,
const bool forRanges) {
const bool forRanges) {
const CommandOutput &Output = FinishedCmd->getOutput();
StringRef DependenciesFile =
Output.getAdditionalOutputForType(file_types::TY_SwiftDeps);
Expand All @@ -458,7 +458,8 @@ namespace driver {
// coarse dependencies that always affect downstream nodes), but we're
// not using either of those right now, and this logic should probably
// be revisited when we are.
assert(FinishedCmd->getCondition() == Job::Condition::Always);
assert(isa<MergeModuleJobAction>(FinishedCmd->getSource()) ||
FinishedCmd->getCondition() == Job::Condition::Always);
Comment on lines +461 to +462
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return {};
}
const bool compileExitedNormally =
Expand Down Expand Up @@ -907,6 +908,7 @@ namespace driver {
return everyIncrementalJob;
};

const Job *mergeModulesJob = nullptr;
CommandSet jobsToSchedule;
CommandSet initialCascadingCommands;
for (const Job *cmd : Comp.getJobs()) {
Expand All @@ -915,6 +917,11 @@ namespace driver {
continue;
}

if (isa<MergeModuleJobAction>(cmd->getSource())) {
assert(!mergeModulesJob && "multiple scheduled merge-modules jobs?");
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice explanation for the assertion.

mergeModulesJob = cmd;
}

const Optional<std::pair<bool, bool>> shouldSchedAndIsCascading =
computeShouldInitiallyScheduleJobAndDependendents(cmd, forRanges);
if (!shouldSchedAndIsCascading)
Expand All @@ -936,6 +943,15 @@ namespace driver {
collectIncrementalExternallyDependentJobsFromDependencyGraph(
forRanges))
jobsToSchedule.insert(cmd);

// 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);
}
Comment on lines +946 to +954
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return jobsToSchedule;
}

Expand Down Expand Up @@ -1031,6 +1047,13 @@ namespace driver {
/// But returns None if there was a dependency read error.
Optional<std::pair<Job::Condition, bool>>
loadDependenciesAndComputeCondition(const Job *const Cmd, bool forRanges) {
// merge-modules Jobs do not have .swiftdeps files associated with them,
// however, their compilation condition is computed as a function of their
// inputs, so their condition can be used as normal.
if (isa<MergeModuleJobAction>(Cmd->getSource())) {
return std::make_pair(Cmd->getCondition(), true);
}

// Try to load the dependencies file for this job. If there isn't one, we
// always have to run the job, but it doesn't affect any other jobs. If
// there should be one but it's not present or can't be loaded, we have to
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not your fault, but collectIncrementalExternallyDependentJobsFromDependencyGraph is too long for my comfort, too.

if (Comp.getLastBuildTime() >= depStatus.getLastModificationTime())
continue;

// Can we run a cross-module incremental build at all?
// If not, fall back.
if (!Comp.getEnableCrossModuleIncrementalBuild()) {
fallbackToExternalBehavior(external);
continue;
Expand Down Expand Up @@ -1608,8 +1636,8 @@ namespace driver {
CompileJobAction::InputInfo info;
info.previousModTime = entry.first->getInputModTime();
info.status = entry.second ?
CompileJobAction::InputInfo::NeedsCascadingBuild :
CompileJobAction::InputInfo::NeedsNonCascadingBuild;
CompileJobAction::InputInfo::Status::NeedsCascadingBuild :
CompileJobAction::InputInfo::Status::NeedsNonCascadingBuild;
inputs[&inputFile->getInputArg()] = info;
}
}
Expand All @@ -1626,7 +1654,7 @@ namespace driver {

CompileJobAction::InputInfo info;
info.previousModTime = entry->getInputModTime();
info.status = CompileJobAction::InputInfo::UpToDate;
info.status = CompileJobAction::InputInfo::Status::UpToDate;
inputs[&inputFile->getInputArg()] = info;
}
}
Expand Down
18 changes: 9 additions & 9 deletions lib/Driver/CompilationRecord.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,12 @@ inline static StringRef getName(TopLevelKey Key) {
inline static StringRef
getIdentifierForInputInfoStatus(CompileJobAction::InputInfo::Status Status) {
switch (Status) {
case CompileJobAction::InputInfo::UpToDate:
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:
Comment on lines +62 to +67
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice to add the typing for Status

return "!private";
}

Expand All @@ -76,11 +76,11 @@ getIdentifierForInputInfoStatus(CompileJobAction::InputInfo::Status Status) {
/// compilation record file (.swiftdeps file).
inline static Optional<CompileJobAction::InputInfo::Status>
getInfoStatusForIdentifier(StringRef Identifier) {
return llvm::StringSwitch<Optional<
CompileJobAction::InputInfo::Status>>(Identifier)
.Case("", CompileJobAction::InputInfo::UpToDate)
.Case("!dirty", CompileJobAction::InputInfo::NeedsCascadingBuild)
.Case("!private", CompileJobAction::InputInfo::NeedsNonCascadingBuild)
using InputStatus = CompileJobAction::InputInfo::Status;
return llvm::StringSwitch<Optional<InputStatus>>(Identifier)
.Case("", InputStatus::UpToDate)
.Case("!dirty", InputStatus::NeedsCascadingBuild)
.Case("!private", InputStatus::NeedsNonCascadingBuild)
.Default(None);
}

Expand Down
Loading