Add support for concatenated depfiles #258

Closed
wants to merge 5 commits into
from

Projects

None yet

7 participants

@PetrWolf
Contributor

This adds a new feature in ninja - the ability to read dependency information from "depfile groups", which are text files created by simple concatenation of several depfiles.

This is to speed up ninja start-up time, especially on Windows, where reading large amount of "little" depfiles is inefficient.

Note that this also includes a refactoring of Edge::StatIfNecessary, because I needed to separate the notion of a stat-ed and visited node.

For more info, see the commit in asciidoc for high-level description (478d765) and the following e-mail threads on previous discussions:

  • .d files on Windows again
  • Building ninja with MSVC

Please, raise questions and comments!

@usovalx

Shouldn't it be "foo.o.d"?
And if it should, why the tests works?

Owner

The test is ok, but is misleading. I will update it.

To answer your question - with this patch, ninja uses timestamps of the output files, not the plain depfiles, and compares the newest of them against the timestamp on the depfile_group file.
So if timestamp(foo.o) > timestamp(group.D), ninja will use the depfile associated with foo.o.

@usovalx

Shouldn't it use IsVisited check here?
IsVisited code looks a bit strange now, especially given that you have to manually toggle it later in the code.

Owner

This is intentional. In this patch, I separated two different pieces of information - the mtime and the state of the edge. Before, mtime was used as an indicator for both.

So here, the "if" construct simply works as a cache, preventing us from going repeatedly "to the disk" with a stat operation on the same file. It has no relation to the state of the edge whatsoever.

The Is/SetVisited(), on the other hand, is a typical get/set pair to manipulate the state of the edge. I believe it is good to be explicit about changing a state of the edge, rather then using the StatIfNecessary function for that.

FYI, the driver for this change was the need to stat a file without changing the state of the edge in depfile_group_up_to_date(). The original design did not allow that, thus the refactoring.

@usovalx

Superfluous copy of the string.

Owner

Good point. Let me find a better way

@usovalx
Contributor
usovalx commented Mar 20, 2012

A more generic issue -- it seems that this code is tailored to the very specific use-case.
The magical handling of "use consolidated or individual depfiles depending on which one is newer" fails the least surprise test.

If I will use some alternative way to generate consolidated depfiles (for example using dependency scanner in cmake) it will end up unnecessarily stat()ing a lot of .d files which aren't supposed to be there at all.

@PetrWolf
Contributor

Hi Alex, thank you for your comments!

You are right, this is explicitly supporting only one use case. One that was discussed previously and which is very straigthforward to use.

The "magical handling" is indeed the most tricky part of the implementation. But from the outside, I believe it behaves correctly - you'd naturaly expect ninja to use the most up-to-date dependency information. Actually, I would be surprised if it behaved differently.

Surely enough, there are many other ways to create or handle consolidated depfiles. If you'd prefer a different one, please explain what would be the difference/benefit. I'll be happy to discuss that. Yet I don't think ninja should be too flexible in this respect - better keep it strict and explicit.

Regarding the unnecessary stating, I am not sure I follow. If you don't use "depfile_group", ninja will behave like the patch was not there at all. And if you do use it, ninja still won't stat any additional files - only the files, which it would stat anyway (the outpust and the aggregated depfiles, which are assumed to be part of the build graph).

Looking forward to your answers.

@usovalx
Contributor
usovalx commented Mar 21, 2012

It seems that significant part of my comment was due to misunderstanding of how exactly it works.

I'm trying to use cmake/ninja generator and was looking at consolidated depfiles as one of the ways to track dependencies which aren't directly visible to the compiler (like dependencies between input files of the codegen tool). But this case isn't easy to fit into the current scheme, and I have a different (though somewhat ugly) workaround for it already.

One thing worth considering -- whether it is feasible to support just the depfile_group option (without corresponding depfile) -- this might me just good enough to make them usable for things like codegen dependencies.

I also don't really understand what your code does in Edge::depfile_group_up_to_date() w.r.t. restat, but that's because I'm new to ninja code & don't really understand what restat does.

@PetrWolf
Contributor

The depfile_group feature proposed here does not bring any new information to the build system. It is just making the read opeartions during ninja startup more efficient.

The tricky part is, that there is no easy way to keep the concatenated depfiles always up to date. Unlike normal depfiles, which are created as a native byproduct by the build tools (e.g. gcc or swig), the concatenation needs to be managed in a separate build step. So it might happen that a single file gets compiled, thus creating a new depfile, but the information is not included in the concatenated one (e.g. because of a build failure elsewhere, or the user stopping the build manually). This is the reason, why ninja needs to compare timestamps before deciding which depfile to use and also a reason, why depfile_group can't be used without depfile.

The restat related code in depfile_group_up_to_date() is there just because the assumptions of the above-mentioned timestamp check are not good enough for restat and an additional check (based on ninja_log) is needed.

@evmar
Collaborator
evmar commented Apr 11, 2012

I spent some time looking through this. I like the general design of having a cache mapping paths to dependency lists.

It seems that by having two ways to specify depfiles, we now end up stat'ing even more files that we would without the merged dependency info.

I appreciate that it's possible for the build to be interrupted before the merged deplist is created, but I wonder if you could just rely on the mtime of the output file and the deplist to judge whether we're in that scenario. For example, say I build foo.o and merge its output into merged.d once.
foo.o has mtime 1, merged.d has mtime 2.

Now I build again, foo gets mtime 3. If I interrupt right here, I'll have merged.d with an mtime less than foo.o, which indicates that it is out of date. In that situation we could then recover in various ways. (We currently don't recover well from a similar scenario: if you have foo.o but then delete foo.o.d, we just pretend there's no dependency info for foo.o; perhaps it would make more sense to require a rebuild in that case.)

What do you think? You've clearly thought about this longer than I have.

@sgraham
Contributor
sgraham commented Apr 11, 2012

I just wanted to mention my branch here: https://github.com/sgraham/ninja/blob/deplist/src/dep_database.cc.

Quick summary: the depdatabase is simply a (conceptual) map<path, deplist>, where 'deplist' is the binary format from Evan's deplist branch (which is just conceptually vector representing the .h files a .c depends on). The depdatabase is stored as a mmap'd file, which the showIncludes helpers mmap and update. More detail in the .cc file.

Possible benefits:

  • simpler (?) access/update without the .ninja writer required to add merge steps, and one canonical source for dependency information
  • I suspect faster access even compared to merged D files because of no actual filesystem access (except for delayed write-back). Unfortunately I don't have hard timing data to offer a comparison.

Anyway, it's still just a sketch, but I just thought I would mention it for consideration and comparison.

@qhuo
Contributor
qhuo commented Apr 12, 2012

Martin, I think the patch is doing what you wrote when deciding which depfile to use. If I read Edge::depfile_group_up_to_date() correct, it is comparing the timestamp of the output file(s) against the depfile_group's. Furthermore, the result of the stat-ing operation is cached, so that the output file(s) are not stat'ed again later when deciding whether they need to be rebuilt. So overall, the stat calls are roughly the same as before this change. Did I miss more file stat calls?

I agree with your statement regarding the situation of depfile against object file. I think this is a common problem of current ninja as well as make. The solution is to just be honest, and declare the depfile along with .o file as the outputs of the compiler. I believe this can be done by the ninja (makefile) generator without changing ninja (make) itself.

@evmar
Collaborator
evmar commented Apr 13, 2012

My thinking was this: before we'd stat every depfile, but now we stat every depfile and additional some depfile_group files. So it strictly increases the number of stats.

But I reread it more carefully. It seems the logic is: stat the output, stat the depfile group. If the times match up we're ok, otherwise we stat the inputs to the depfile group. So while it's possible to do more stats, it's not guaranteed to happen. Sorry for my confusion.

@qhuo
Contributor
qhuo commented Apr 13, 2012

Afraid that I am still a bit confused. Perhaps you mean file reads instead of stats in some of the places? Apologies for being pedantic, but the cost of these two operations are hugely different.

@PetrWolf
Contributor

Hi all, thanks for the comments and sorry for a delay in discussion - I was on holiday.

The only added stat operations (unless I made a mistake) are those related to the merged depfiles, which become part of the build graph. All other stat operations are cached, so the order in which they happen have changed, but not the number. That drove the refactoring of StatIfNecessary btw.

While I am in favour of further development here (be it a dep database or a depfile/mtime daemon), that should not prevent this from being merged in, as it is a working and quite well tested (we've been using in for moths) solution. If a better approach is estabilished, I'll be happy to deprecate or fully remove this feature then.

@philipcraig
Contributor

Hi Evan,

What are next steps for this pull request?

@nico
Collaborator
nico commented Jul 25, 2013

Is this still needed now that depsmode exists?

@evmar
Collaborator
evmar commented Nov 11, 2015

We think this is no longer necessary due to the depslog. Please reopen if we're wrong!

@evmar evmar closed this Nov 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment