Closed Bug 292569 Opened 16 years ago Closed 8 years ago

Add support for #pragma once to headers

Categories

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

x86
All
enhancement

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: Callek, Assigned: Mitch)

Details

(Whiteboard: [buildfaster:?])

Attachments

(6 files, 1 obsolete file)

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.
Assignee: nobody → bugspam.Callek
Severity: normal → enhancement
Priority: -- → P3
Target Milestone: --- → mozilla1.9alpha
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.
We are going to need some sort of configure test for this.
A write-up that people could find useful:
http://gamearchitect.net/Articles/ExperimentsWithIncludes.html
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
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.
Assignee: bugspam.Callek → nobody
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.
All I've ever done is "time make -f client.mk" multiple times.
Target Milestone: mozilla1.9alpha1 → ---
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)?
Attached patch xpidl changes (obsolete) — Splinter Review
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)
Attached patch larger changesSplinter Review
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.
Attachment #451223 - Attachment is obsolete: true
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]
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 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.
Attachment #461973 - Flags: review?(ted.mielczarek)
Attachment #461973 - Flags: feedback?(bzbarsky)
(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 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.
Attachment #461973 - Flags: feedback?(bzbarsky)
Attachment #461973 - Flags: review?(ted.mielczarek) → review+
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
Attachment #461973 - Flags: approval2.0?
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
Attachment #461973 - Flags: review+
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...
(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.
Assignee: nobody → bugspam.Callek
Please wait until after we branch.
Attachment #461973 - Flags: approval2.0? → approval2.0-
Attached patch IPDL (v1)Splinter Review
The code I removed for adding a first newline seemed unnecessary in the generated headers I inspected.
Attachment #586794 - Flags: review?
Attached patch XPIDL (v1)Splinter Review
Sorry, the previous patch's comments were meant for this patch. (Extraneous newline).
Attachment #586795 - Flags: review?
Attachment #586794 - Flags: review? → review?(jones.chris.g)
Attachment #586795 - Flags: review? → review?(khuey)
Attached patch Quickstubs (v1)Splinter Review
Attachment #586796 - Flags: review?(jorendorff)
Attached patch MFBT (v1)Splinter Review
Attached patch XPCOM (v1)Splinter Review
This is as much of XPCOM that I could add "#pragma once" to without it failing to build or test.
Attachment #586799 - Flags: review?
Attachment #586798 - Flags: review?(jwalden+bmo)
Attachment #586799 - Flags: review? → review?(doug.turner)
The above patches seemed to pass TryServer except for known intermittent failures:
https://tbpl.mozilla.org/?tree=Try&rev=3f1f50e9dcbb
Comment on attachment 586799 [details] [diff] [review]
XPCOM (v1)

over to benjamin.
Attachment #586799 - Flags: review?(doug.turner) → review?(benjamin)
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.
Attachment #586798 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #586796 - Flags: review?(jorendorff) → review+
Attachment #586794 - Flags: review?(jones.chris.g) → review+
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.
(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 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?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Assignee: bugspam.Callek → mitchell.field
Since there are thousands of headers I think it makes sense to cherrypick directories. Does that sound like an okay plan?
(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 ?
(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 on attachment 586799 [details] [diff] [review]
XPCOM (v1)

Clearing review request until this is announced to m.d.platform.
Attachment #586799 - Flags: review?(benjamin)
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
I hear coop is looking at making builds faster these days.
Whiteboard: [buildfaster:?]
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.
I don't think this will improve our build time. Just clean up our headers.
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>
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.