903816 - include-what-you-use for gfx/layers

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nick Cameron [:nrc]]

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.

Comment 1

[Nick Cameron [:nrc]]

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.

Comment 2

[David Zbarsky (:dzbarsky)]

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.

Comment 3

[Nick Cameron [:nrc]]

(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.

Comment 4

[Nick Cameron [:nrc]]

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.

Comment 5

[David Zbarsky (:dzbarsky)]

I know.  Filed bug 903819.

Comment 6

[Nicholas Nethercote [inactive]]

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

Comment 7

[Nick Cameron [:nrc]]

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.

Comment 8

[Nick Cameron [:nrc]]

Attachment: the meat

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.

Comment 9

[Nick Cameron [:nrc]]

Attachment: fixups outside of gfx/layers

Comment 10

[Nick Cameron [:nrc]]

Attachment: the meat

Comment 11

[Nick Cameron [:nrc]]

Attachment: fixups outside of gfx/layers

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

Comment 12

[Nick Cameron [:nrc]]

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

Comment 13

[Phil Ringnalda (:philor)]

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).

Comment 14

[Phil Ringnalda (:philor)]

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.

Comment 15

[(no longer active)]

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?

Comment 16

[[e-mail]]

(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 ?

Comment 17

[(no longer active)]

(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.

Comment 18

[Nicholas Nethercote [inactive]]

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

Comment 19

[Nick Cameron [:nrc]]

(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.

Comment 20

[Kelsey Gilbert [:jgilbert]]

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.

Comment 21

[Nicholas Nethercote [inactive]]

> 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.

Comment 22

[Benoit Girard (:BenWa)]

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.

Comment 23

[(no longer active)]

(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!

Comment 24

[(no longer active)]

(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!

Comment 25

[Benoit Girard (:BenWa)]

(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?

Comment 26

[(no longer active)]

(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.

Comment 27

[Kelsey Gilbert [:jgilbert]]

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

Comment 28

[Nick Cameron [:nrc]]

(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.

Comment 29

[Benoit Girard (:BenWa)]

(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.

Comment 30

[Nicholas Nethercote [inactive]]

> 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.

Comment 31

[Benoit Girard (:BenWa)]

(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.

Comment 32

[Nick Cameron [:nrc]]

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

Comment 33

[Nick Cameron [:nrc]]

(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.

Comment 34

[Nick Cameron [:nrc]]

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

Comment 35

[Ed Morley [:emorley]]

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