Skip to content

[Driver] Address Brian G's feedback on 848b3eb. #4046

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 1 commit into from
Aug 6, 2016
Merged

[Driver] Address Brian G's feedback on 848b3eb. #4046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
113 changes: 64 additions & 49 deletions lib/Driver/Compilation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,57 +464,72 @@ int Compilation::performJobsImpl() {
const CommandOutput &Output = FinishedCmd->getOutput();
StringRef DependenciesFile =
Output.getAdditionalOutputForType(types::TY_SwiftDeps);
if (!DependenciesFile.empty() &&
(ReturnCode == EXIT_SUCCESS || ReturnCode == EXIT_FAILURE)) {
bool wasCascading = DepGraph.isMarked(FinishedCmd);

switch (DepGraph.loadFromPath(FinishedCmd, DependenciesFile)) {
case DependencyGraphImpl::LoadResult::HadError:
if (ReturnCode == EXIT_SUCCESS) {
disableIncrementalBuild();
for (const Job *Cmd : DeferredCommands)
scheduleCommandIfNecessaryAndPossible(Cmd);
DeferredCommands.clear();
Dependents.clear();
} // else, let the next build handle it.
break;
case DependencyGraphImpl::LoadResult::UpToDate:
if (!wasCascading)
break;
SWIFT_FALLTHROUGH;
case DependencyGraphImpl::LoadResult::AffectsDownstream:
DepGraph.markTransitive(Dependents, FinishedCmd);
break;
}
if (DependenciesFile.empty()) {
// If this job doesn't track dependencies, it must always be run.
// Note: In theory CheckDependencies makes sense as well (for a leaf
// node in the dependency graph), and maybe even NewlyAdded (for very
// 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);
} else {
// If there's a crash, assume the worst.
switch (FinishedCmd->getCondition()) {
case Job::Condition::NewlyAdded:
// The job won't be treated as newly added next time. Conservatively
// mark it as affecting other jobs, because some of them may have
// completed already.
DepGraph.markTransitive(Dependents, FinishedCmd);
break;
case Job::Condition::Always:
// This applies to non-incremental tasks as well, but any incremental
// task that shows up here has already been marked.
break;
case Job::Condition::RunWithoutCascading:
// If this file changed, it might have been a non-cascading change and
// it might not. Unfortunately, the interface hash has been updated or
// compromised, so we don't actually know anymore; we have to
// conservatively assume the changes could affect other files.
DepGraph.markTransitive(Dependents, FinishedCmd);
break;
case Job::Condition::CheckDependencies:
// If the only reason we're running this is because something else
// changed, then we can trust the dependency graph as to whether it's
// a cascading or non-cascading change. That is, if whatever /caused/
// the error isn't supposed to affect other files, and whatever
// /fixes/ the error isn't supposed to affect other files, then
// there's no need to recompile any other inputs. If either of those
// are false, we /do/ need to recompile other inputs.
break;
// If we have a dependency file /and/ the frontend task exited normally,
// we can be discerning about what downstream files to rebuild.
if (ReturnCode == EXIT_SUCCESS || ReturnCode == EXIT_FAILURE) {
bool wasCascading = DepGraph.isMarked(FinishedCmd);

switch (DepGraph.loadFromPath(FinishedCmd, DependenciesFile)) {
case DependencyGraphImpl::LoadResult::HadError:
if (ReturnCode == EXIT_SUCCESS) {
disableIncrementalBuild();
for (const Job *Cmd : DeferredCommands)
scheduleCommandIfNecessaryAndPossible(Cmd);
DeferredCommands.clear();
Dependents.clear();
} // else, let the next build handle it.
break;
case DependencyGraphImpl::LoadResult::UpToDate:
if (!wasCascading)
break;
SWIFT_FALLTHROUGH;
case DependencyGraphImpl::LoadResult::AffectsDownstream:
DepGraph.markTransitive(Dependents, FinishedCmd);
break;
}
} else {
// If there's an abnormal exit (a crash), assume the worst.
switch (FinishedCmd->getCondition()) {
case Job::Condition::NewlyAdded:
// The job won't be treated as newly added next time. Conservatively
// mark it as affecting other jobs, because some of them may have
// completed already.
DepGraph.markTransitive(Dependents, FinishedCmd);
break;
case Job::Condition::Always:
// Any incremental task that shows up here has already been marked;
// we didn't need to wait for it to finish to start downstream
// tasks.
assert(DepGraph.isMarked(FinishedCmd));
break;
case Job::Condition::RunWithoutCascading:
// If this file changed, it might have been a non-cascading change
// and it might not. Unfortunately, the interface hash has been
// updated or compromised, so we don't actually know anymore; we
// have to conservatively assume the changes could affect other
// files.
DepGraph.markTransitive(Dependents, FinishedCmd);
break;
case Job::Condition::CheckDependencies:
// If the only reason we're running this is because something else
// changed, then we can trust the dependency graph as to whether
// it's a cascading or non-cascading change. That is, if whatever
// /caused/ the error isn't supposed to affect other files, and
// whatever /fixes/ the error isn't supposed to affect other files,
// then there's no need to recompile any other inputs. If either of
// those are false, we /do/ need to recompile other inputs.
break;
}
}
}
}
Expand Down
22 changes: 11 additions & 11 deletions lib/Driver/Driver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1612,20 +1612,13 @@ handleCompileJobCondition(Job *J, CompileJobAction::InputInfo inputInfo,
}

Job::Condition condition;
if (!hasValidModTime || J->getInputModTime() != inputInfo.previousModTime) {
if (alwaysRebuildDependents ||
inputInfo.status == CompileJobAction::InputInfo::NeedsCascadingBuild) {
condition = Job::Condition::Always;
} else {
condition = Job::Condition::RunWithoutCascading;
}
} else {
if (hasValidModTime && J->getInputModTime() == inputInfo.previousModTime) {
switch (inputInfo.status) {
case CompileJobAction::InputInfo::UpToDate:
if (!llvm::sys::fs::exists(J->getOutput().getPrimaryOutputFilename()))
condition = Job::Condition::RunWithoutCascading;
else
if (llvm::sys::fs::exists(J->getOutput().getPrimaryOutputFilename()))
condition = Job::Condition::CheckDependencies;
else
condition = Job::Condition::RunWithoutCascading;
break;
case CompileJobAction::InputInfo::NeedsCascadingBuild:
condition = Job::Condition::Always;
Expand All @@ -1636,6 +1629,13 @@ handleCompileJobCondition(Job *J, CompileJobAction::InputInfo inputInfo,
case CompileJobAction::InputInfo::NewlyAdded:
llvm_unreachable("handled above");
}
} else {
if (alwaysRebuildDependents ||
inputInfo.status == CompileJobAction::InputInfo::NeedsCascadingBuild) {
condition = Job::Condition::Always;
} else {
condition = Job::Condition::RunWithoutCascading;
}
}

J->setCondition(condition);
Expand Down
4 changes: 2 additions & 2 deletions test/Driver/Dependencies/Inputs/update-dependencies-bad.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,5 @@
else:
sys.exit(129)

dir = os.path.dirname(os.path.abspath(__file__))
execfile(os.path.join(dir, "update-dependencies.py"))
execDir = os.path.dirname(os.path.abspath(__file__))
execfile(os.path.join(execDir, "update-dependencies.py"))