Skip to content

[Incremental] Dependency fixes in preparation for fine-grained dependencies #29009

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

Jump to bottom
Merged
merged 5 commits into from
Jan 9, 2020
Merged

[Incremental] Dependency fixes in preparation for fine-grained dependencies #29009

merged 5 commits into from
Jan 9, 2020

Conversation

davidungar
Copy link
Contributor

@davidungar davidungar commented Jan 4, 2020 •
edited
Loading

  1. Only emit dot files for fine-grained dependencies when -emit-fine-grained-dependency-source-file-dot-files is specified
  2. Better comments, esp. for fine-grained dependencies
  3. Restructuring of fine-grained dependencies to support unit testing
  4. Added unit test for fine-grained dependencies
  5. Bug fixes for fine-grained dependencies
  6. Batch sorting refactoring.
  7. Better fine-grained dependency printing
  8. Added -disable-fine-grained-dependencies to frontend and driver
  9. Cleanups to driver to better support fine-grained-dependencies

@davidungar davidungar force-pushed the fine-grained-fixes-post-rb branch from 7875e45 to ea57166 Compare January 4, 2020 22:04
David Ungar added 2 commits January 4, 2020 14:37
Fine-grained and driver fixes.
6fffe72 
Restructure fine-grained-dependencies to enable unit testing

Get frontend to emit correct swiftdeps file (fine-grained when needed) and only emit dot file for -emit-fine-grained-dependency-sourcefile-dot-files

Use deterministic order for more information outputs.

Set EnableFineGrainedDependencies consistently in frontend.

Tolerate errors that result in null getExtendedNominal()

Fix memory issue by removing node everywhere.

Break up print routine

Be more verbose so it will compile on Linux.

Sort batchable jobs, too.
Add fine-grained unit test
3126142
@davidungar davidungar force-pushed the fine-grained-fixes-post-rb branch from ea57166 to 3126142 Compare January 4, 2020 22:37
@davidungar
Copy link
Contributor Author

@swift-ci please smoke test

@davidungar davidungar requested a review from CodaFi January 4, 2020 22:43
@davidungar
Copy link
Contributor Author

@swift-ci please smoke test

@davidungar davidungar force-pushed the fine-grained-fixes-post-rb branch from eeeb631 to eb4ad14 Compare January 5, 2020 00:48
@davidungar
Copy link
Contributor Author

@swift-ci please smoke test linux platform

1 similar comment
@davidungar
Copy link
Contributor Author

@swift-ci please smoke test linux platform

@davidungar davidungar force-pushed the fine-grained-fixes-post-rb branch from eb4ad14 to 41cbcfd Compare January 5, 2020 01:40
@davidungar
Copy link
Contributor Author

@swift-ci please smoke test linux platform

@davidungar
Copy link
Contributor Author

@swift-ci please smoke test os x platform

@compnerd
Copy link
Member

compnerd commented Jan 5, 2020

@swift-ci please test windows platform

@CodaFi
Copy link
Contributor

CodaFi commented Jan 5, 2020

Reviewing this in two passes. First, nits and surface level stuff.

CodaFi
CodaFi reviewed Jan 5, 2020
View reviewed changes
include/swift/Driver/Action.h Outdated
auto Inputs = getInputs();
const InputAction *IA = nullptr;
for (auto const *I : Inputs) {
if (auto const *S = dyn_cast<InputAction>(I)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: The nesting here is pretty deep. This can be refactored into a straight-line loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure how to fix it, without a compactMap operation, but I have taken a stab at rewriting it for clarity.

CodaFi
CodaFi reviewed Jan 5, 2020
View reviewed changes
lib/AST/FineGrainedDependencies.cpp Outdated
// But, if an arc is added for this, then *any* change that causes
// a same-named interface to be dirty will dirty this implementation,
// even if that interface is in another file.
// So, make the interface->implementation arc implicit.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting design point. Can you expand on why this is for my edification?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I've rewritten the comment to clarify the point. Thanks.

CodaFi
CodaFi reviewed Jan 5, 2020
View reviewed changes
lib/Driver/FineGrainedDependencyDriverGraph.cpp
@@ -404,7 +438,8 @@ void ModuleDepGraph::emitDotFileForJob(DiagnosticEngine &diags,

void ModuleDepGraph::emitDotFile(DiagnosticEngine &diags, StringRef baseName) {
unsigned seqNo = dotFileSequenceNumber[baseName]++;
std::string fullName = baseName.str() + "." + std::to_string(seqNo) + ".dot";
std::string fullName =
baseName.str() + "-post-integration." + std::to_string(seqNo) + ".dot";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this debugging code?

Copy link
Contributor Author

@davidungar davidungar Jan 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure what you are asking for here. The dot file facility is for debugging and visualization, as I tried to comment at the method declarations. What would be clearer? (I'm not crazy about the fact that one has to go find the declaration in order to get the scoop on the definition, but I've been told that that's how it is in this world.) There is a single comment there for a group of declarations. Is that the problem? Seems redundant to repeat it, but that would help the option-click gesture.

@CodaFi
Copy link
Contributor

CodaFi commented Jan 5, 2020

Alright, first pass is through. Will come back to you with more detailed feedback later.

@davidungar
Copy link
Contributor Author

Alright, first pass is through. Will come back to you with more detailed feedback later.

Thank you very very much for such a quick review! I'll address these comments (and the test failures ASAP).

@davidungar
Copy link
Contributor Author

@swift-ci please smoke test linux platform

CodaFi
CodaFi approved these changes Jan 9, 2020
View reviewed changes
Copy link
Contributor

@CodaFi CodaFi left a comment

Choose a reason for hiding this comment

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

No further comments.

@davidungar
Copy link
Contributor Author

@swift-ci please smoke test os x platform

@davidungar
Copy link
Contributor Author

@CodaFi Thanks!

@davidungar
Copy link
Contributor Author

@swift-ci please smoke test os x platform

@davidungar davidungar merged commit 939034a into swiftlang:master Jan 9, 2020
@compnerd
Copy link
Member

@davidungar @CodaFi: this broke the windows bots. The bots have been red for a while now: https://ci-external.swift.org/job/oss-swift-windows-x86_64/2398/

Please test the windows platform when making changes like this, there seems to be a pattern of the recent driver work breaking the Windows build. In the mean time, Im going to generate a revert for this change.

@compnerd compnerd mentioned this pull request Jan 10, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CodaFi CodaFi CodaFi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@davidungar @compnerd @CodaFi