Only load depsfile if not dirty [Fix #2666] by moritzx22 · Pull Request #2680 · ninja-build/ninja

moritzx22 · 2025-10-12T17:19:11Z

This Merge request is related to #2666.

This pull request proposes to load the depsfile only if it is not dirty.
For a more detailed description, see comment #2666

moritzx22 · 2025-10-12T17:24:42Z

running the example from #2666 with the proposed solution, reports no cycle and builds as expected

...
// change the cpp files
$ ninja
[6/6] Linking CXX static library libhasmodules.a
$ ninja
ninja: no work to do.

the solution does work for the build from #2666 but it is still a draft.

mathstuf

I think the test suite leak fix can be its own PR. Can a test case for the compelling scenario be added?

src/build_test.cc

moritzx22 · 2025-10-21T16:44:55Z

think the test suite leak fix can be its own PR

new PR created: #2684. I will keep this PR unchanged, until PR2684 is merged and a rebase can be done.

moritzx22 · 2025-10-22T17:48:13Z

Can a test case for the compelling scenario be added?

One cycle test for the depfile has been added. A similar test for dyndep is pending.
Numerous tests still fail because this PR does change some basic rules, ninja is designed to and this is reflected in the test suite.

moritzx22 · 2025-10-25T16:53:05Z

The dyndep issue is none. The dyndep file is already only loaded if it is not dirty. The respective commit has been removed.

moritzx22 · 2025-11-22T16:26:12Z

Changes in the recent push

more mature implementation
- restat functionality is corrected
- dependencies are only checked once
- runtime performance optimization
unit tests have been changed to comply with the new depsfile loading
- unit tests runs without failing in Linux
- one Windows only test does still fail
tests with builds like llvm reported the expected behavior

moritzx22 · 2025-11-22T18:12:02Z

ninja -t missingdeps

console output

$ninja -t missingdeps
... There might be build flakiness if any of the targets listed above
are built alone, or not late enough, in a clean output directory.

This essentially means that issues can occur if a depsfile is not loaded because it has not yet been generated. With this merge request, the condition is extended: if the corresponding target is considered dirty due to the manifest, the depsfile will also not be loaded.

As a result, this PR may increase the likelihood of build flakiness when missingdeps are present.
Note: well‑designed Ninja builds should not produce missing dependencies.

At the time of writing, I do not see any additional negative impact introduced by the conceptual change in this PR.

mathstuf

Looks OK to me, but someone else should also review (I did it mainly from the test cases).

src/graph.h

src/graph.cc

src/missing_deps.cc

moritzx22 · 2025-12-15T19:17:20Z

rebased to master

moritzx22 · 2026-03-22T17:36:11Z

rebased to master

moritzx22 · 2026-04-03T10:12:16Z

Rebased to master in previous push.

digit-google

This PR is impossible to review properly as a stack of 8 commits with what looks like random changes with unclear commit messages, please squash / rebase this into something that is simpler to review. For example:

one commit to add the constness changes + the cp-deps test rule implementation.
second commit to change the LoadDepXXX() signatures with proper documentation of all new parameters, preferably without changing the implementation yet.
a third commit that changes the implementation to change the behavior / fix the bug and modify the tests accordingly.

Each commit should have a clear commit message explaining its purpose and why things are changed in a certain way. I'll add some inline comments too.

digit-google · 2026-04-03T14:09:40Z

src/graph.cc

+namespace {
+
+/// execute hash only once in lifetime of object and only on request
+struct hashCommand {


Please follow existing coding conventions, i.e. struct/class names should use PascalCase (hashCommand -> HashCommand), and member variables should use trailing underscore (valid -> valid_). Moreover, call this LazyEdgeCommandHash for clarity.

digit-google · 2026-04-03T14:12:13Z

src/graph.cc

+
+/// class is similar to a pointer of BuildLog::LogEntry
+/// additionally the LookupByOutput is cached for performance reasons
+class LogEntryCache {


nit: Name this CachedLogEntry for clarity since this is not a cache.

digit-google · 2026-04-03T14:13:43Z

src/graph.cc

+ public:
+  LogEntryCache(){};
+
+  operator bool() const { return entry_; }


Explain what this corresponds to and when it is safe to call, since it never looks at evaluated_, the meaning of the result value is ambiguous. Consider replacing this with is_valid() for clarity, bool operators can lead to surprising bugs.

renamed to is_valid()

example usage for clarity

CachedLogEntry cached; if(cached.is_valid()) cached->foo(); // nullptr cached.LookupByOutput(build_log, output); // assign value if(cached.is_valid()) cached->foo(); // call foo() cached.LookupByOutput(build_log, output); // already cached if(cached.is_valid()) cached->foo(); // call foo() // a raw pointer instead BuildLog::LogEntry* entry = nullptr; if(entry) entry->foo(); // nullptr entry = build_log.LookupByOutput(output); if(entry.is_valid()) entry->foo(); // call foo() entry = build_log.LookupByOutput(output); // second call if(entry.is_valid()) entry->foo(); // call foo()

digit-google · 2026-04-03T14:17:25Z

src/graph.cc

+  BuildLog::LogEntry* entry_ = nullptr;
+};
+
+bool LogEntryCache::LookupByOutput(const BuildLog* buildLog, const Node* output) {


This interface is ambiguous, because the function could in theory be called with different |output| values and will only return a result corresponding to the first call. You could implement something similar without a dedicated LogEntryCache class with a simple std::map<const Node*, const BuildLog::LogEntry*> instead inside RecomputeOutputsDirty_, which would be simpler / clearer.

Here are the reasons behind introducing this new class:

Requirements

Cache the result of LookupByOutput.

Store only a single pointer and a single boolean per cache entry.

Keep the cache in contiguous memory (std::vector) for compiler‑friendly access patterns.

Allocate the memory only once (vector sized in the constructor).

Allow lookup of the cached value for a given output at effectively zero cost.

RecomputeOutputsDirty is performance‑critical.
Its worst‑case scenario is a clean build, where no early exits occur and every output must be visited. RecomputeOutputsDirty will be invoked twice in this situation because the depfile is loaded here. This is exactly where the cache provides the most benefit.

To achieve this performance, the class relies on strict usage assumptions:
LookupByOutput must always be called with the same parameters for a given instance. Debug assertions enforce this, and the assumptions are documented in the code. These constraints allow the implementation to remain efficient.

RecomputeOutputsDirtyCache ensures these assumptions hold. It selects the correct output and its associated cache entry for processing. The CachedLogEntry type is defined in the private section to prevent accidental misuse outside the intended context.

A std::map could also be used to implement the cache, and it would likely be simpler to write, but I expect its performance to be worse, especially for clean builds.

Please advise.

digit-google · 2026-04-03T14:18:31Z

src/graph.cc

+}
+
+/// performance optimized to recompute the outputs
+class RecomputeOutputsDirty_ {


Please do not use trailing underscores in class names (or even inside them). Call this RecomputeOutputsDirtyCache instead, or something similar. Also consider moving changes related to performance optimizations to their own commit so they can be reviewed more easily.

digit-google · 2026-04-03T14:19:00Z

src/graph.cc

+/// performance optimized to recompute the outputs
+class RecomputeOutputsDirty_ {
+ public:
+  RecomputeOutputsDirty_(BuildLog* buildLog, OptionalExplanations& explanations,


coding style: please use snake_case for variable / member identifiers (buildLog -> build_log)

digit-google · 2026-04-03T14:19:24Z

src/graph.cc

+                         Edge* edge)
+      : buildLog_(buildLog), explanations_(explanations), edge_(edge),
+        LogEntry_(edge->outputs_.size()) {}
+  bool all(const Node* most_recent_input);


nit: document what these methods do.

digit-google · 2026-04-03T14:23:01Z

src/graph.cc

+  return false;
+}
+
+// disable warning for windows


Explain why these are needed exactly.

MSVC warns (and errors) on constructs like:

if (false) { // do some stuff }

This is suppressed. Anyhow this is obsolete with the change to c++17 and the use of if constexpr

This should go as a comment inside the source code, so that future maintainers now how / when to keep this.

sorry for not being clear. In the latest push it looks like

if constexpr (false) { // do some stuff }

and no warning or error is reported by MSVC anymore. The disable warning stuff has been removed.

digit-google · 2026-04-03T14:23:48Z

src/graph.cc

+  assert(FIRSTRUN || !(cond)); /* NOLINT */ \
+  if (FIRSTRUN && (cond))      /* NOLINT */
+
+template <bool FIRSTRUN>


I strongly recommend to getting rid of the template parameter, and adding a simple first_run function parameter instead.

The template parameter is constexpr, which gives the compiler the best opportunity to optimize the code. Conceptually, the template function represents two distinct functions and helps avoid code duplication. The runtime if is replaced with if constexpr in the next push, so the unused branch is removed entirely at compile time.

There are essentially three design options:

A template function

Uses a constexpr template parameter to generate two optimized code paths without duplication.

Cleanly separates the regular function parameters from the compile‑time selection parameter.

A regular function with a runtime parameter

Simpler interface

Two separate functions

Maximum clarity, but duplicates code.

Please restate your preference.
If you still recommend avoiding the template parameter, I’d appreciate some more detail on why the template approach is undesirable in this context.

Note: The if is changed to if constexpr in the macro.

digit-google · 2026-04-03T14:38:22Z

src/graph.h

  //                          or out of date).
-  bool LoadDeps(Edge* edge, std::string* err);
+  bool LoadDeps(Edge* edge, std::string* err,
+                std::array<std::size_t, 2>* input_range = nullptr);


Please update the documentation to explain the purpose of this new input_range parameter. Consider using a simple struct InputRange { size_t start; size_t end; } definition to make this easier to read and understand. Clarify that this is an optional output parameter.

replaced std::array with:

struct InputView { std::size_t offset_begin = 0; std::size_t offset_end = 0; };

The original conditional was removed to improve performance.
The Call that previously used that conditional now uses a dummy object.
This path(missingdeps) isn't performance‑critical, so using a dummy is acceptable.

digit-google · 2026-04-03T14:45:03Z

src/graph.cc

+      return false;
+  }
+
+  const auto input_end = edge->inputs_.cend() - input_range[1];


Is this computation correct here? Can you clarify the meaning of input_range[1]? From the name "range" it can be assumed that this would be the position of the first item after the range, but in this case, you would use input_end = edge->inputs_.cbegin() + input_range[1] instead.

If the value is a count instead, "input_span" might be a better name, but the computation would be input_end = edge->inputs_.cbegin() + input_range[0] + input_range[1] so I am puzzled as to what this code does.

digit-google · 2026-04-03T14:45:48Z

src/graph.cc

  // Load output mtimes so we can compare them to the most recent input below.
-  for (Node* o : edge->outputs_) {
+  for (vector<Node*>::iterator o = edge->outputs_.begin();
+       o != edge->outputs_.end(); ++o) {


nit: Why regress here when the original code was perfectly fine?

digit-google · 2026-04-03T14:47:20Z

src/graph.cc

+  // if an rebuild is necessary the deps log is outdated for this target
+  if (!edge->deps_loaded_ && !dirty) {
+    // This is our first encounter with this edge.  Load discovered deps.
+    std::array<std::size_t, 2> newLinks{ 0, 0 };


nit: newLinks doesn't mean anything here. Use something more specific here. new_deps maybe?

Or more precisely new_deps_range

renamed to new_deps

digit-google · 2026-04-03T14:50:50Z

src/graph.cc

  }

+  if (input_range)
+    (*input_range)[1] = std::distance(implicit_dep, edge->inputs_.end());


oh, so you are storing the distance from the last implicit input to the end of the array. This data structure definitely is not a range. Consider storing the distance from the start for clarity.

This data structure represents a subset of a container.
The usual approach starting from index 0 has a drawback:
the default‑constructed view should represent the entire container, which requires knowing its size.

In this context it would look like:

if (!RecomputeEdgesInputsDirty(node, InputView(), most_recent_input, dirty, stack, validation_nodes, err)) // would need to become if (!RecomputeEdgesInputsDirty(node, InputView{0, node->in_edge()->inputs_.size()}, most_recent_input, dirty, stack, validation_nodes, err))

graph.cc#L482

I can implement the change, and it can certainly be expressed more cleanly than in the example above. However, in this particular context the change makes the code a bit more complicated. Before proceeding, I’d appreciate your advice.

Second, there is

This data structure represents a subset of a container. The usual approach starting from index 0 has a drawback: the default‑constructed view should represent the entire container, which requires knowing its size.

Technically, this is neither a "view" nor a "subset" as these terms usually refer to objects that can be used directly to access individual items. This is not the case here: you just have a pair of numbers, whose interpretation requires additional information (in this case the exact and unmodified inputs_ array they refer to). Hence using something like "range" in the name makes more sense. Another option is to store the Edge pointer in the data structure (or at least a pointer to its edge->inputs_ array).

If you prefer a non-conventional layout / interpretation for the values, I strongly recommend making a custom class with human-friendly accessors to properly document its purpose and simplify its usage. E.g.

struct EdgeInputsRange { /// Create new instance covering all |edge| inputs. EdgeInputsRange(const Edge* edge); /// Create instance covering the [start_pos..end_pos) interval of |edge| inputs. EdgeInputsRange(const Edge* edge, size_t start_pos, size_t end_pos); size_t start_pos() const; size_t end_pos() const; private: ... };

In this context it would look like:

if (!RecomputeEdgesInputsDirty(node, InputView(), most_recent_input, dirty, stack, validation_nodes, err)) // would need to become if (!RecomputeEdgesInputsDirty(node, InputView{0, node->in_edge()->inputs_.size()}, most_recent_input, dirty, stack, validation_nodes, err))

graph.cc#L482

I can implement the change, and it can certainly be expressed more cleanly than in the example above. However, in this particular context the change makes the code a bit more complicated. Before proceeding, I’d appreciate your advice.

This change adjusts the internal order of the load output mtimes step and the step that recomputes the dirty state of inputs. The modification does not alter any functional behavior. The reordering improves internal consistency and prepares the code for upcoming changes.

moritzx22 · 2026-04-07T14:22:51Z

This PR is impossible to review properly as a stack of 8 commits with what looks like random changes with unclear commit messages, please squash / rebase this into something that is simpler to review. ...

True. I will reorder and clean up the commits so that each one has a clear purpose and is easier to understand. Only the final commit introduces a functional change. All earlier commits are refactoring or cleanup and keep current master behavior. After restructuring the history, this separation will be clearer and the review much simpler. Most of the other comments will be incorporated as well.

This change refactors internal parts of the code without altering functional behavior. It prepares the implementation for a future update in which dependencies will be loaded only when inputs are not marked dirty. The sequence in which only a subset of inputs can be specified to be processed will matter for upcoming changes to depfile loading. A new helper function 'RecomputeEdgesInputsDirty' has been introduced for clarity. The function can be specified to visit only a subset of an edge’s inputs as well.

Replace DependencyScan::RecomputeOutputDirty with the new RecomputeOutputsDirtyCache helper class to centralize the logic for determining whether edge outputs are dirty. The new cache‑aware implementation avoids redundant work, improves readability, and prepares the codebase for upcoming changes to load depfiles only when nodes are not dirty. Build‑log lookups are now cached, and command hash computation is performed lazily to improve performance. This commit introduces no functional changes.

This change updates the logic so that the depfile is loaded only when no output node is not dirty based on manifest and dyndep inputs (excluding the depfile itself). If the outputs are already scheduled to be regenerated, loading the depfile is unnecessary, since its only purpose is to trigger regeneration of the outputs — which is already guaranteed. Avoiding the use of a potentially outdated depfile prevents incorrect cycle detection and ensures that stale depfiles are no longer added to the graph.

moritzx22 marked this pull request as draft October 12, 2025 17:24

moritzx22 force-pushed the fix2666 branch 2 times, most recently from bb1200c to f6320d7 Compare October 19, 2025 12:21

mathstuf suggested changes Oct 20, 2025

View reviewed changes

src/build_test.cc Outdated Show resolved Hide resolved

moritzx22 force-pushed the fix2666 branch from f6320d7 to c07a13d Compare October 20, 2025 20:01

moritzx22 mentioned this pull request Oct 21, 2025

Fix unit test crash, if test fails #2684

Merged

moritzx22 force-pushed the fix2666 branch 2 times, most recently from e42469e to e06a6b6 Compare October 25, 2025 16:49

moritzx22 changed the title ~~Only load deps and dyndeps if not dirty [Fix #2666]~~ Only load depsfile if not dirty [Fix #2666] Oct 25, 2025

moritzx22 force-pushed the fix2666 branch from e06a6b6 to b8ab02c Compare October 27, 2025 18:30

moritzx22 force-pushed the fix2666 branch from b8ab02c to 993e5ce Compare November 22, 2025 15:50

moritzx22 force-pushed the fix2666 branch from 993e5ce to 2cef958 Compare November 22, 2025 17:17

moritzx22 marked this pull request as ready for review November 22, 2025 18:21

moritzx22 requested a review from mathstuf November 22, 2025 18:22

mathstuf reviewed Dec 8, 2025

View reviewed changes

src/graph.h Outdated Show resolved Hide resolved

src/graph.cc Outdated Show resolved Hide resolved

moritzx22 commented Dec 10, 2025

View reviewed changes

src/graph.cc Outdated Show resolved Hide resolved

moritzx22 commented Dec 15, 2025

View reviewed changes

src/missing_deps.cc Outdated Show resolved Hide resolved

moritzx22 force-pushed the fix2666 branch from 05d6ad7 to 67ad894 Compare December 15, 2025 19:02

moritzx22 force-pushed the fix2666 branch 5 times, most recently from e1e20a8 to d234784 Compare December 29, 2025 18:51

moritzx22 force-pushed the fix2666 branch 4 times, most recently from 140ec6c to 8af8118 Compare January 1, 2026 14:51

moritzx22 force-pushed the fix2666 branch from 8af8118 to 87c9a41 Compare March 22, 2026 17:26

moritzx22 force-pushed the fix2666 branch from 87c9a41 to 3fda509 Compare April 3, 2026 10:01

digit-google suggested changes Apr 3, 2026

View reviewed changes

digit-google reviewed Apr 3, 2026

View reviewed changes

moritzx22 force-pushed the fix2666 branch from 3fda509 to 7caaddd Compare April 7, 2026 14:24

moritzx22 added 3 commits April 7, 2026 16:33

moritzx22 force-pushed the fix2666 branch from 7caaddd to 0070603 Compare April 7, 2026 14:33

Conversation

moritzx22 commented Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moritzx22 commented Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mathstuf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

moritzx22 commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moritzx22 commented Oct 22, 2025

Uh oh!

moritzx22 commented Oct 25, 2025

Uh oh!

moritzx22 commented Nov 22, 2025

Changes in the recent push

Uh oh!

moritzx22 commented Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ninja -t missingdeps

Uh oh!

mathstuf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moritzx22 commented Dec 15, 2025

Uh oh!

moritzx22 commented Mar 22, 2026

Uh oh!

moritzx22 commented Apr 3, 2026

Uh oh!

digit-google 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!

Choose a reason for hiding this comment

moritzx22 commented Oct 12, 2025 •

edited

Loading

moritzx22 commented Oct 12, 2025 •

edited

Loading

moritzx22 commented Oct 21, 2025 •

edited

Loading

moritzx22 commented Nov 22, 2025 •

edited

Loading

moritzx22 Apr 7, 2026 •

edited

Loading