292569 - Add support for #pragma once to headers

Status: RESOLVED WONTFIX

(Firefox Build System :: General, enhancement, P3)

Description

[Justin Wood (:Callek)]

This should theoretically decrease build-time on Windows (msvc) build, and on
any other compiler that designs in 'feature support' for msvc's |#pragma once|

Bsmedberg expressed interest in accepting a patch for this, wtc also expressed
interest in terms of "if this helps build time"

This includes patching xpidl to output .h files with a #pragma once as well.

This bug being committed will be pending build-time analysis once completed.

Comment 1

[Justin Wood (:Callek)]

tsk verified for me that #pragma once also works on gcc at least on OSX 10.3, I
imagine that it also works on other iterations of gcc;  Noting that here so I do
not forget.

Comment 2

[Benjamin Smedberg]

We are going to need some sort of configure test for this.

Comment 3

[Chase Phillips]

A write-up that people could find useful:
http://gamearchitect.net/Articles/ExperimentsWithIncludes.html

Comment 4

[Chase Phillips]

Don't mean to bug spam but wanted to point out a follow-up article that could be
equally useful:

"Even More Experiments with Includes"
http://www.gamesfromwithin.com/articles/0501/000067.html

Comment 5

[Justin Wood (:Callek)]

I'm not currently working on this, if someone else could come up with a configure check for the feature, I might have time to investigate further.

Comment 6

[Justin Wood (:Callek)]

Ted, Taras, or Ben, 

any chance one of you could give me information on how to instrument the Test of build-time win/fail/other if I choose to try this approach.

Comment 7

[(not currently active) Ted Mielczarek]

All I've ever done is "time make -f client.mk" multiple times.

Comment 8

[Justin Wood (:Callek)]

I added |#pragma once| to nsCOMPtr.h and nsISupportsBase.h and sent off to try.

On our try infra, this did not error out any of the builds. Do we still care if this has a configure check, or can I unconditionally add it (pending actual testing of build speedup)?

Comment 9

[Justin Wood (:Callek)]

Attachment: xpidl changes

If anyone wishes to time just these [minimal] changes for me on win, I'd be happy. (Clobber builds is probably where this will make a difference)

Comment 10

[Justin Wood (:Callek)]

Attachment: larger changes

This is the larger changes I ended up profiling with. I disabled Virus Scan, exited IRC and all browser windows, basically killed every timing hurting app on my machine I could think of [easily could have missed some]

Ran 4 clobbers without patch, 4 do-nothing-depends without patch.
Then repeat with patch.

I ran with pymake, Firefox Opt, tests enabled; included the configure duration in my benchmarking.

I am omitting the dep results below.

The results [raw]:
Without Path:
* Clobber:
*  * 4642 seconds
*  * 4788 seconds
*  * 4909 seconds
*  * 4905 seconds

With Patch:
* Clobber:
*  * 4642 seconds
*  * 4715 seconds
*  * 5599 seconds
*  * 5925 seconds

So either I *did* have something running unintentionally causing the ODD increase in the later runs [with patch], OR the noise is such that we can't make any real determination. But either way this does not appear to be a significant win on my end at least.

Comment 11

[Justin Wood (:Callek)]

Ben/Ted, given my results above, I think we might as well _either_ take the work I did [largest omissions in my test of #pragma once are NSPR/NSS and mozalloc.h]

*or* resolve this closed as WONTFIX.

I can do more fine grained testing in an effort to yeild the expected results if you also wish, but I am beginning to think the supposed win is not as large as I once suspected.

[Many of the xpcom files use external header ifdef guards anyway]

Comment 12

[(not currently active) Ted Mielczarek]

Let's just get the patch reviewed and landed. It's a pretty simple thing, so even if it has a very small benefit it seems worth doing.

Comment 13

[Justin Wood (:Callek)]

Comment on attachment 461973 [details] [diff] [review]
larger changes

Ok Ted, since you want to take the work I did so far, lets go for it.

(I'm not sure if we want/need ben's review for the xpidl stuff).

Requesting feedback from bz since I very gingerly chose some headers I *think* are used a lot of places across content/layout/ etc. So if he has any ideas of a few additional headers to add to this patch, I can also do so.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #13)
> Requesting feedback from bz since I very gingerly chose some headers I *think*
> are used a lot of places across content/layout/ etc. So if he has any ideas of
> a few additional headers to add to this patch, I can also do so.

nsCOMPtr.h please!

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 461973 [details] [diff] [review]
larger changes

I'm not going to trawl through the patch looking for the list of headers involved, sorry..

If you get our core XPCOM headers and some common interfaces, that's good enough by me.

Comment 16

[Justin Wood (:Callek)]

Comment on attachment 461973 [details] [diff] [review]
larger changes

Even though this touches many files should be completely safe. Has passed tryserver, and the patch I land will include khuey's recommendation for nsCOMPtr.h

Comment 17

[Wan-Teh Chang]

Comment on attachment 461973 [details] [diff] [review]
larger changes

r=wtc.  Thanks.

It's worth calling out in your commit comment two
nontrivial changes in this changeset:
xpcom/idl-parser/header.py
xpcom/typelib/xpidl/xpidl_header.c

I would take the opportunity to remove the extra
blank line at the top of these files:
xpcom/string/public/nsString.h
xpcom/string/public/nsStringBuffer.h

Comment 18

[Wan-Teh Chang]

Note this recent thread on the chromium-dev discussion group:
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thread/53791dfe33d00bd9?pli=1

Just FYI.  I am not familiar with #pragma once...

Comment 19

[Justin Wood (:Callek)]

(In reply to comment #17)
> Comment on attachment 461973 [details] [diff] [review]
> larger changes
> 
> r=wtc.  Thanks.

O hey, so if this is wanted for NSPR/NSS I can do that too [I'll do the large*r* changes for more headers in another bug(s)]

> It's worth calling out in your commit comment two
> nontrivial changes in this changeset:

will do.

> I would take the opportunity to remove the extra
> blank line at the top of these files:

Sure, why not.

Comment 20

[Benjamin Smedberg]

Please wait until after we branch.

Comment 21

[Mitchell Field [:Mitch]]

Attachment: IPDL (v1)

The code I removed for adding a first newline seemed unnecessary in the generated headers I inspected.

Comment 22

[Mitchell Field [:Mitch]]

Attachment: XPIDL (v1)

Sorry, the previous patch's comments were meant for this patch. (Extraneous newline).

Comment 23

[Mitchell Field [:Mitch]]

Attachment: Quickstubs (v1)

Comment 24

[Mitchell Field [:Mitch]]

Attachment: MFBT (v1)

Comment 25

[Mitchell Field [:Mitch]]

Attachment: XPCOM (v1)

This is as much of XPCOM that I could add "#pragma once" to without it failing to build or test.

Comment 26

[Mitchell Field [:Mitch]]

The above patches seemed to pass TryServer except for known intermittent failures:
https://tbpl.mozilla.org/?tree=Try&rev=3f1f50e9dcbb

Comment 27

[Doug Turner (:dougt)]

Comment on attachment 586799 [details] [diff] [review]
XPCOM (v1)

over to benjamin.

Comment 28

[Jeff Walden [:Waldo]]

Comment on attachment 586798 [details] [diff] [review]
MFBT (v1)

Review of attachment 586798 [details] [diff] [review]:
-----------------------------------------------------------------

I guess as compilers are guaranteed to ignore pragmas they don't recognize, this can't hurt.  Still a little leery of it, tho.

Comment 29

[Jason Orendorff [:jorendorff]]

Comment on attachment 586796 [details] [diff] [review]
Quickstubs (v1)

Yup! Don't take this as a general endorsement of "#pragma once" (which I don't have an informed opinion about) but yeah, the patch is the right thing, if we want that in each header.

Comment 30

[Benjamin Smedberg]

Comment on attachment 586799 [details] [diff] [review]
XPCOM (v1)

Please don't change dmd.h, which is copied from valgrind.

Is your goal here to add #pragma once to every non-special header in the tree? Are you doing this with a script? If so, can I just review the list of files being modified?

This should probably be announced to dev.platform before landing.

Comment 31

[Mitchell Field [:Mitch]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #30)
> Comment on attachment 586799 [details] [diff] [review]
> XPCOM (v1)
> 
> Please don't change dmd.h, which is copied from valgrind.
> 
> Is your goal here to add #pragma once to every non-special header in the
> tree? Are you doing this with a script? If so, can I just review the list of
> files being modified?
> 
> This should probably be announced to dev.platform before landing.

Without any solid data to go on (which would be great), I guessed that XPCOM headers would be oft-included. I'm doing this all manually but I'll provide diffstat output if you'd like.

Comment 32

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 586795 [details] [diff] [review]
XPIDL (v1)

Review of attachment 586795 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/idl-parser/header.py
@@ -208,4 @@
>      for inc in idl.includes():
> -        if not foundinc:
> -            foundinc = True
> -            fd.write('\n')

What are these changes for?

Comment 33

[Mitchell Field [:Mitch]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #32)
> Comment on attachment 586795 [details] [diff] [review]
> XPIDL (v1)
> 
> Review of attachment 586795 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/idl-parser/header.py
> @@ -208,4 @@
> >      for inc in idl.includes():
> > -        if not foundinc:
> > -            foundinc = True
> > -            fd.write('\n')
> 
> What are these changes for?


I botched the attachment comments.

> The code I removed for adding a first newline seemed unnecessary in the
> generated headers I inspected.
> The code I removed for adding a first newline seemed unnecessary in the
> generated headers I inspected.

Comment 34

[Mitchell Field [:Mitch]]

Since there are thousands of headers I think it makes sense to cherrypick directories. Does that sound like an okay plan?

Comment 35

[Ludovic Hirlimann [:Usul]]

(In reply to Mitchell Field [:Mitch] from comment #34)
> Since there are thousands of headers I think it makes sense to cherrypick
> directories. Does that sound like an okay plan?

Callek ?

Comment 36

[Justin Wood (:Callek)]

(In reply to Ludovic Hirlimann [:Usul] from comment #35)
> (In reply to Mitchell Field [:Mitch] from comment #34)
> > Since there are thousands of headers I think it makes sense to cherrypick
> > directories. Does that sound like an okay plan?
> 
> Callek ?

I have no problem with it, but I'm not a reviewer in m-c for this stuff.

Comment 37

[Benjamin Smedberg]

Comment on attachment 586799 [details] [diff] [review]
XPCOM (v1)

Clearing review request until this is announced to m.d.platform.

Comment 38

[Benoit Girard (:BenWa)]

I was looking into this bug apparently compilers, gcc at least, will do include guard optimization:
http://gcc.gnu.org/onlinedocs/cppinternals/Guard-Macros.html

Comment 39

[(no longer active)]

I hear coop is looking at making builds faster these days.

Comment 40

[Chris Cooper [:coop] (he/him)]

To followup comment 38, from https://en.wikipedia.org/wiki/Pragma_once#Advantages_and_disadvantages:

"Common compilers such as GCC, Clang, and EDG-based compilers include special speedup code to recognize and optimize the handling of include guards, and thus little or no speedup benefit is obtained from the use of #pragma once."

Has anyone verified that there's an actual speedup to be gained here any more? I'm recommending WONTFIX here.

Comment 41

[Benoit Girard (:BenWa)]

I don't think this will improve our build time. Just clean up our headers.

Comment 42

[(no longer active)]

I brought this up on dev.platform recently, and it turns out that we can't do this: <https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.platform/PgDjWw3xp8k>