I did an IWYU pass on gfx/layers. It was not as much fun as I thought it would be - the process is far from automatic and getting things building again afterwards is tedious. IWYU is a tool for tidying up include lists. We generally get a longer include list but a smaller transitive set of included files. It makes it much clearer what is getting included where and why. That should prevent the horrendous bugs we sometimes get with include spaghetti. It also makes it easier to split header files and clearer where we should target out energies for reducing the include overheads in our builds. The changes improved build times by about 12.5%. To completely rebuild gfx/layers with Clang (j1) went from 4m05s to 3m33s (user+sys, averaged over four runs, max difference in runs was only 3s, so very consistent). There has been a similar effort in JS (bug 888768, bug 634839). See https://bugzilla.mozilla.org/show_bug.cgi?id=634839#c18 and bug 558723 for a discussion on the aesthetics of longer include lists.

I did not cover the d3d directories since Clang does not work on Windows. Also a few small MacOS and Windows only files are not covered. There were a couple of define-heavy files IWYU choked on, some of these (e.g., LayerManagerOGLProgram.*) I persuaded it to work with. Some (e.g., ShadowLayerUtils.h) I could not. But pretty much everything is done.

I did something similar last year and we went from 70% of files in the tree including Layers.h to about 2%. Sadly I think that number has crept up since then.

(In reply to David Zbarsky (:dzbarsky) from comment #2) > I did something similar last year and we went from 70% of files in the tree > including Layers.h to about 2%. Sadly I think that number has crept up > since then. According to the list in bug 901132, Layers.h is (transitively) included by 2090 non-h files. That is rather a lot and we should try and reduce that number. Hopefully this work makes it easier to do that.

Note that this work doesn't affect which layers files are included by files in the rest of the tree, it is about which files are included by layers files.

I know. Filed bug 903819.

> The changes improved build times by about 12.5%. To completely rebuild > gfx/layers with Clang (j1) went from 4m05s to 3m33s (user+sys, averaged over > four runs, max difference in runs was only 3s, so very consistent). Wow. That's a bigger improvement than I saw in js/src/. Please report your experience in the recent dev-platform thread about build times!

I also tried measuring with j12, but here the times were much more unreliable. Still a decent improvement though - before: around 55s, after: 48s, so still around 12.5%, but here there is enough variation in the times (up to 4s) that I am not so confident in the significance.

Sorry to ask for review on such a big and boring patch. I was thinking of splitting it up, but I figured it wouldn't actually make it any easier and this way it is easier to grep for stuff.

sorry for the email churn, noticed some TODOs I hadn't taken care of.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f82224bb8a3 https://hg.mozilla.org/integration/mozilla-inbound/rev/d393cfeb5dbf

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/cb38092d519d for Android reftest failures (though it being Android, one of the possibilities is that it needed a clobber and actually built something rather different than what it ought to have built).

Apparently not clobber, I was setting a clobber for an unrelated reason so I retriggered on your push, and the clobbered build still has the same failures.

Not sure if this bug is the best venue to ask this, but is there a guide somewhere for setting up the IWUY environment so that others can try to do the same for other parts of the tree?

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #15) > Not sure if this bug is the best venue to ask this, but is there a guide > somewhere for setting up the IWUY environment so that others can try to do > the same for other parts of the tree? maybe bug634839 ?

(In reply to comment #16) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #15) > > Not sure if this bug is the best venue to ask this, but is there a guide > > somewhere for setting up the IWUY environment so that others can try to do > > the same for other parts of the tree? > > maybe bug634839 ? Yeah well, I was kind of hoping for a shorter version. That bug is full of information but it's very hard to follow if you wanna set this up locally for the first time.

Ehsan, I just wrote https://blog.mozilla.org/nnethercote/2013/08/13/using-include-what-you-use/, which hopefully will help.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #15) > Not sure if this bug is the best venue to ask this, but is there a guide > somewhere for setting up the IWUY environment so that others can try to do > the same for other parts of the tree? I'm not sure what you mean by environment. I just built clang with IWYU and then ran it. It is imperfect because you have to correct a lot manually, but I fear IWYU will never be perfect for code like layers with lots of platform dependent stuff.

It's worth noting that I had decent success doing obvious pruning of things from header files, without IWYU. I'm sure IWYU is faster in the long run, though.

> It's worth noting that I had decent success doing obvious pruning of things > from header files, without IWYU. I'm sure IWYU is faster in the long run, > though. I've done both. It's nicer with IWYU. In particular, having a reference that (mostly correctly) tells you not only which headers are required but also *why* is very helpful.

I'm not a fan of checking in the include comments that IWYU generates. They will quickly become out of date. This is something we can generate up-to-date information by re-running IWYU. We might include Foo.h for class Bar today but later we may start using class X from Foo.h as well and that comment wont be updated.

(In reply to comment #18) > Ehsan, I just wrote > https://blog.mozilla.org/nnethercote/2013/08/13/using-include-what-you-use/, > which hopefully will help. Thanks, Nick!

(In reply to comment #22) > I'm not a fan of checking in the include comments that IWYU generates. They > will quickly become out of date. So do #include statements added without IWYU. > This is something we can generate up-to-date > information by re-running IWYU. Yes. > We might include Foo.h for class Bar today but > later we may start using class X from Foo.h as well and that comment wont be > updated. I think that's OK. This is a general problem with code comments. When in doubt, trust the code!

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #24) > (In reply to comment #22) > I think that's OK. This is a general problem with code comments. When in > doubt, trust the code! I think our manual #include comments are also a mistake and should be r-. Some code comments are useful and less likely to go out of date by accident unlike #include comments. Also non #include comments convey information that is not evident from the code and impossible to machine generate. This is very the opposite with IWYU. I think these comments are going to quickly just become noise and we should avoid noisy comments. Also what happens when IWYU is ran again when includes dependencies have changed? Will the comments stack?

(In reply to comment #25) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #24) > > (In reply to comment #22) > > I think that's OK. This is a general problem with code comments. When in > > doubt, trust the code! > > I think our manual #include comments are also a mistake and should be r-. Some > code comments are useful and less likely to go out of date by accident unlike > #include comments. Also non #include comments convey information that is not > evident from the code and impossible to machine generate. This is very the > opposite with IWYU. I think these comments are going to quickly just become > noise and we should avoid noisy comments. I disagree, but we don't need to convince each other. You can advocate for not adding comments in gfx, if it's cool with other peers of that component. :-) > Also what happens when IWYU is ran again when includes dependencies have > changed? Will the comments stack? I think it updates them.

I agree that the include comments seem prone to rot, and don't really add much value.

(In reply to Benoit Girard (:BenWa) from comment #22) > I'm not a fan of checking in the include comments that IWYU generates. I'm kind of conflicted about the comments. At first I thought they were pretty ugly, but you do get used to them. > They > will quickly become out of date. This is something we can generate > up-to-date information by re-running IWYU. The trouble is IWYU is a long way from automatic at the moment. Running IWYU is a little bit painful and I predict that many people will not want to do it. Having the comments, even if they become out of date allows you to more easily keep include info up to date - if someone removes some code or wants to remove an include it is quickly apparent what include or code is relevant (this is not always obvious from the header name). That lowers the bar for keeping in the IWYU style, and for further work on reducing include uses. From my personal experience fixing up errors after running IWYU, I found the comments very valuable. We might include Foo.h for class > Bar today but later we may start using class X from Foo.h as well and that > comment wont be updated. I agree that this rot is a problem. But we will learn to take them with a pinch of salt in files with a lot of traffic. In fact, noticing that the comments are wrong is probably a good sign that it is time to run IWYU again. So, I agree that the 'noise' factor/rot is a downside. But I think there is an upside here, and it is worth tolerating the latter for the former.

(In reply to Nick Cameron [:nrc] from comment #28) > The trouble is IWYU is a long way from automatic at the moment. Running IWYU > is a little bit painful and I predict that many people will not want to do > it. Last time I ran it I found copied the shell commands from an earlier IWYU bugzilla bug (from njn I believe) and once the build was done everything worked perfectly. If that too hard I think we can host precompiled binaries and/or have a script/mach command to do that for you we can maintain. I rather make it easier to run IWYU then pollute our codebase with these. > > So, I agree that the 'noise' factor/rot is a downside. But I think there is > an upside here, and it is worth tolerating the latter for the former. Perhaps, I don't want to bikeshed on something that's subjective. But if I had my way we would make IWYU easier to run via a binaries/script/mach and keep the codebase clean, win-win.

> Last time I ran it I found copied the shell commands from an earlier IWYU > bugzilla bug (from njn I believe) and once the build was done everything > worked perfectly. Then you were lucky. IWYU is just wrong a small fraction of the time, e.g. it regularly says you can remove headers that you cannot. I too was conflicted about whether to include the comments from IWYU. The fact that they rot is arguably a good thing, because if you have this: #include "foo.h" // for Foo and the file doesn't use Foo, there's a good chance you can remove it. In the end I didn't add the comments in SpiderMonkey because I only removed the removable headers; I didn't bother adding all the suggested headers. But I can see the arguments for both sides. Aryeh included the comments when he did editor/ in bug 772807.

(In reply to Nicholas Nethercote [:njn] from comment #30) > > Last time I ran it I found copied the shell commands from an earlier IWYU > > bugzilla bug (from njn I believe) and once the build was done everything > > worked perfectly. > > Then you were lucky. IWYU is just wrong a small fraction of the time, e.g. > it regularly says you can remove headers that you cannot. > I meant compiling the IWYU binary.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=e6ad8b2f4b95

(In reply to Nicholas Nethercote [:njn] from comment #30) > > Last time I ran it I found copied the shell commands from an earlier IWYU > > bugzilla bug (from njn I believe) and once the build was done everything > > worked perfectly. > > Then you were lucky. IWYU is just wrong a small fraction of the time, e.g. > it regularly says you can remove headers that you cannot. > Yep, this is where the effort goes in doing this. > I too was conflicted about whether to include the comments from IWYU. The > fact that they rot is arguably a good thing, because if you have this: > > #include "foo.h" // for Foo > > and the file doesn't use Foo, there's a good chance you can remove it. > Yes, I think this is an advantage. > In the end I didn't add the comments in SpiderMonkey because I only removed > the removable headers; I didn't bother adding all the suggested headers. > But I can see the arguments for both sides. Aryeh included the comments > when he did editor/ in bug 772807. Another advantage is that it looks different, and hopefully people will think to try to keep the includes up to date. If the list just gets longer there is not such a useful queue to IWYU.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c73cdccbe8c9 https://hg.mozilla.org/integration/mozilla-inbound/rev/24c814a25a79

https://hg.mozilla.org/mozilla-central/rev/c73cdccbe8c9 https://hg.mozilla.org/mozilla-central/rev/24c814a25a79