[Driver] NFC: Added some comments around incremental compilation. #20823

davidungar · 2018-11-28T05:28:38Z

Based on my own investigation, and conversations with Jordan, have added some comments. Zero changes to actual code. Pay particular attention to the comment for InitialOutOfDateCommands, where (if I read the code right) I had to reverse the sense of a comment. Either the code or the comment was wrong before.

Azoy · 2018-11-28T14:18:38Z

include/swift/Driver/Job.h

@@ -230,9 +230,17 @@ class CommandOutput {
 class Job {
 public:
  enum class Condition {
+    // There was no input map, or the map makred this Job as dirty


Typo: marked

jrose-apple

Thanks for doing this!

jrose-apple · 2018-11-28T17:03:32Z

include/swift/Driver/DependencyGraph.h

+  ///
+  /// "Marked" does NOT mean that any successors in the graph have been
+  /// processed. The traversal routines use "Visited" to avoid endless
+  /// "recursion".


This last bit doesn't seem like it belongs here, since "Visited" isn't persistent state.

Reworded and moved to markTransitive.

jrose-apple · 2018-11-28T17:04:04Z

include/swift/Driver/DependencyGraph.h

+  /// Assume that the starting node is "cascading"; that all dependencies must
+  /// be dirty if the start is dirty. Therefore, mark the start. For each
+  /// visited node, add it to visited, and mark it if it cascades. The start
+  /// node is NOT added to visited.


There's no comment here because there is a comment on the templated version. Can you keep them all together?

Yes! Moved this comment and added an attempt at a cross-reference.

jrose-apple · 2018-11-28T17:04:43Z

include/swift/Driver/Job.h

    CheckDependencies,
+    // Pessimal: run no matter what.


This is actually not the worst case; a newly-added file doesn't necessarily cascade.

Clarified, thanks.

jrose-apple · 2018-11-28T17:06:31Z

include/swift/Driver/Job.h

    Always,
+    // The input changed, or this job was private or [it was not dirty but
+    // primary output was missing].


Let's stick to "non-cascading" as the terminology, but even then it's something more like "this job was scheduled as non-cascading in the last build but didn't get to run".

Changed the term "private". I didn't want to remove the lower-level predicate, so I added your sentence to the comment.

jrose-apple · 2018-11-28T17:07:55Z

include/swift/Driver/Job.h

@@ -230,9 +230,17 @@ class CommandOutput {
 class Job {
 public:
  enum class Condition {
+    // There was no input map, or the map makred this Job as dirty


I don't think it's obvious what "input map" means here. Maybe something like "we have no information about any previous build"?

Good catch! Fixed it to say both.

jrose-apple · 2018-11-28T17:08:31Z

lib/Driver/Compilation.cpp

@@ -206,8 +206,9 @@ namespace driver {
    /// Jobs that incremental-mode has decided it can skip.
    CommandSet DeferredCommands;

-    /// Jobs in the initial set with Condition::Always, or lacking existing
+    /// Jobs in the initial set with Condition::Always, and having an existing
    /// .swiftdeps files.


This change seems incorrect. NewlyAdded files would also show up here (at least they should).

On second thought, maybe not. Maybe the variable name just needs to change.

Here is the code from Compilation.cpp:

case Job::Condition::Always: if (Comp.getIncrementalBuildEnabled() && !DependenciesFile.empty()) { // Ensure dependents will get recompiled. InitialOutOfDateCommands.push_back(Cmd); // Mark this job as cascading. DepGraph.markIntransitive(Cmd); } LLVM_FALLTHROUGH;

Please look at the code above and see if the code is wrong, or I'm reading it wrong, or what? Am I missing another place where jobs get added?

Oops, didn't see your second thought, messages crossed in progress. Happy to change the name if you like. Let me know.

Yeah, given that this is the only place this gets set, I suggest "InitialCascadingCommands" or something.

OK, will do!

jrose-apple · 2018-11-28T17:09:32Z

lib/Driver/Compilation.cpp

+    /// It would probably be safe and simpler to markTransitive on the start
+    /// nodes in the "Always" condition from the start instead of using
+    /// markIntransitive and having later functions call markTransitive. That
+    /// way markInstransitive would be an implemenation detail.


This does not belong in a doc comment because it's not relevant to callers. Maybe it belongs next to markIntransitive instead?

(Also typo: "implemenation".)

Great! Done.

davidungar · 2018-11-28T18:45:00Z

@jrose-apple Thank you for take the time to look at these comments. So helpful to get a review!

jrose-apple · 2018-11-29T17:31:34Z

include/swift/Driver/DependencyGraph.h

@@ -121,6 +121,13 @@ class DependencyGraphImpl {
  llvm::StringMap<std::pair<std::vector<DependencyEntryTy>, DependencyMaskTy>> Dependencies;

  /// The set of marked nodes.
+  /// "Marked" means that everything provided by this node (i.e. Job) is


Nitpick: Can you put an extra newline in here? We're formatting our comments as Doxygen (even if we never actually run the generator) and so the first "paragraph" is taken as a short summary.

jrose-apple · 2018-11-29T17:33:24Z

include/swift/Driver/DependencyGraph.h

@@ -121,6 +121,13 @@ class DependencyGraphImpl {
  llvm::StringMap<std::pair<std::vector<DependencyEntryTy>, DependencyMaskTy>> Dependencies;

  /// The set of marked nodes.
+  /// "Marked" means that everything provided by this node (i.e. Job) is
+  /// dirty. Thus any file using any of these provides must be recompiled.
+  /// (Only non-private entities are output as provides.)


This particular note doesn't really belong here. The dependency graph (and the driver in general) doesn't know about what in a file gives rise to links in the graph, and if the logic did output private entities as "provides", they'd be marked differently somehow anyway.

Hard to figure out where to put it, as there is no central declaration I found besides this one.
However, I've moved it into reloadAndRemarkDeps. If you have a better idea, please let me know!

jrose-apple · 2018-11-29T17:34:25Z

include/swift/Driver/DependencyGraph.h

+  /// (Only non-private entities are output as provides.)
+  /// In other words, this Job "cascades"; the need to recompile it causes other
+  /// recompilations. It is possible that the current code marks things that do
+  /// not need to be marked. Nothing would break if that were the case.


"Nothing would break" seems a bit too generic; the effect of over-marking is unnecessary recompilation.

jrose-apple · 2018-11-29T17:36:57Z

include/swift/Driver/DependencyGraph.h

+  ///
+  /// Do not confused "Marked" with "Visited".
+  /// "Marked" does NOT influence the traversal. The traversal routines use
+  /// "Visited" to avoid endless "recursion".


This isn't correct; already-marked nodes do not end up in visited, and it's not just to avoid endless recursion. I got that wrong.

(Also, another Doxygen note: parameters can be referred to inline with \p visited.)

Good catch! Fixed.

jrose-apple · 2018-11-29T17:38:50Z

include/swift/Driver/Job.h

@@ -230,9 +230,20 @@ class CommandOutput {
 class Job {
 public:
  enum class Condition {
+    // There was no information about the previous build (i.e., an input map),
+    // or the map marked this Job as dirty
+    // but the input didn't change.


This isn't correct; the job being marked as "dirty + cascading" takes priority over whether or not the input changed. (At least, I hope it does.)

Thank you. Fixed!

include/swift/Driver/Job.h

jrose-apple · 2018-11-29T17:40:52Z

include/swift/Driver/Job.h

    CheckDependencies,
+    // Pessimal: Run no matter what (but may or may not cascade).


I don't think "pessimal" is appropriate. As mentioned before, this is usually equivalent to RunWithoutCascading; it just means we don't compare against a previous swiftdeps file.

jrose-apple · 2018-11-29T17:41:44Z

lib/Driver/Compilation.cpp

@@ -206,8 +206,9 @@ namespace driver {
    /// Jobs that incremental-mode has decided it can skip.
    CommandSet DeferredCommands;

-    /// Jobs in the initial set with Condition::Always, or lacking existing
+    /// Jobs in the initial set with Condition::Always, and having an existing
    /// .swiftdeps files.


Yeah, given that this is the only place this gets set, I suggest "InitialCascadingCommands" or something.

jrose-apple · 2018-11-29T17:42:29Z

lib/Driver/Compilation.cpp

+            // It would probably be safe and simpler to markTransitive on the
+            // start nodes in the "Always" condition from the start instead of
+            // using markIntransitive and having later functions call
+            // markTransitive. That way markInstransitive would be an


Typo: "markInstransitive". Also, it's probably worth saying saying that it would be an implementation detail of DependencyGraph, since the comment is no longer in DependencyGraph.

Thanks. Fixed.

davidungar · 2018-12-10T17:33:51Z

@swift-ci please smoke test

davidungar · 2018-12-10T17:34:46Z

@jrose-apple @graydon I'd like to land this if it's OK with you. Any more suggestions? TIA.

jrose-apple · 2018-12-10T21:26:20Z

include/swift/Driver/DependencyGraph.h

+  /// Assumes that the starting node is "cascading"; that all dependencies must
+  /// be dirty if the start is dirty. Therefore, mark the start. For each
+  /// visited node, add it to visited, and mark it if it cascades. The start
+  /// node is NOT added to visited.


In this graph model, nodes aren't cascading or not; edges are. (Also, please be consistent about use of \p visited!)

Good catch on "visited!" I've fixed that.

jrose-apple · 2018-12-10T21:27:50Z

include/swift/Driver/Job.h

+    // build
+    // but didn't get to run.
+    // The scheduled-but-didn't-run condition is detected when the job was not
+    // dirty but its primary output was missing.


This isn't the only such case, so I'm not sure why you're calling it out here.

Seemed to be worthwhile for the sake of clarity. I can delete it if you prefer. (Not exactly sure what the antecedent for "this" was in your comment.)

I keep doing that. "the job is not dirty but the primary output is missing" is one case where RunWithoutCascading can happen, but it's not the only one.

Thanks! I get it and removed that sentence.

jrose-apple · 2018-12-10T21:53:57Z

include/swift/Driver/DependencyGraph.h

-  /// be dirty if the start is dirty. Therefore, mark the start. For each
-  /// visited node, add it to visited, and mark it if it cascades. The start
-  /// node is NOT added to visited.
+  /// Assumes that some edge into the starting node is "cascading."


I like this better, but since no such edge has to actually exist maybe there's still another way to phrase it?

Good point! I've reworded.

davidungar · 2018-12-14T19:28:48Z

@jrose-apple @graydon Thanks for your comments. I'd like to land this. Anything else before I do?

jrose-apple

Ah, yep! Looks good.

davidungar · 2018-12-15T06:24:58Z

Thanks, @jrose-apple . I'll go ahead and test and land, but if @graydon has any more, I'll put it in in another PR.

davidungar · 2018-12-15T06:25:12Z

@swift-ci please smoke test

Added some comments around incremental compilation.

4c61354

davidungar requested review from graydon and jrose-apple November 28, 2018 05:28

Azoy reviewed Nov 28, 2018

View reviewed changes

jrose-apple reviewed Nov 28, 2018

View reviewed changes

Improvements thanks to Jordan’s feedback.

6823b93

jrose-apple reviewed Nov 29, 2018

View reviewed changes

More fixes.

3463451

jrose-apple reviewed Dec 10, 2018

View reviewed changes

Fixes

f2b5bf7

jrose-apple reviewed Dec 10, 2018

View reviewed changes

David Ungar added 2 commits December 10, 2018 13:56

Removed confusing sentence.

c273a61

Reword.

6e31cf1

jrose-apple approved these changes Dec 14, 2018

View reviewed changes

davidungar merged commit 7ae7038 into swiftlang:master Dec 15, 2018

davidungar deleted the master-with-comments branch June 14, 2019 18:03

		CheckDependencies,
		// Pessimal: Run no matter what (but may or may not cascade).

[Driver] NFC: Added some comments around incremental compilation. #20823

[Driver] NFC: Added some comments around incremental compilation. #20823

Uh oh!

Conversation

davidungar commented Nov 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple 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!

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

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 28, 2018

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!

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!