Open Bug 1277309 Opened 8 years ago Updated 2 years ago

system header wrappers should include #ifdef/#endif include guards

Categories

(Firefox Build System :: General, defect)

defect

Tracking

(firefox49 affected)

Tracking Status
firefox49 --- affected

People

(Reporter: froydnj, Unassigned)

Details

I was looking at a .ii file for something else today and I noticed this:

# Measure the number of includes of something underneath dist/system_wrappers.
@nightcrawler:~/Documents$ grep dist/system_wrappers Unified_cpp_javaplugin0.ii |egrep '" 1' |f 3 |sort |uniq -c | f 1|addup
327

# Measure the total number of unique includes from above
@nightcrawler:~/Documents$ grep dist/system_wrappers Unified_cpp_javaplugin0.ii |egrep '" 1' |f 3 |sort |uniq -c | wc -l
72

So that's ~250 inclusions where we're reading the system wrapper off the disk for no gain.  (I know GCC, at least, pattern-matches the header #ifdef/#endif include guard idiom and doesn't re-read if a header that's already been #include'd is #included'd again.  I think clang might do something similar.)  Assuming this is semi-representative of other C++ files in the tree, 250 useless filesystem fiddlings times 5000+ files equals what looks like a lot of wasted work.

We should at least add the guards and see if compilation time is noticeably impacted; I think we should add the guards for completeness's sake regardless.
Is it worth revisiting whether we still need the system wrappers in the first place? Do other major projects do something similar or just us?
Does it actually matter? In practice, since the system header has already been included, you're not /actually/ hitting the disk thanks to caching.

(In reply to Michael Shal [:mshal] from comment #1)
> Is it worth revisiting whether we still need the system wrappers in the
> first place?

We do. They allow much more things to have hidden visibility than -fvisibility=hidden allows.
> (In reply to Michael Shal [:mshal] from comment #1)
> > Is it worth revisiting whether we still need the system wrappers in the
> > first place?
> 
> We do. They allow much more things to have hidden visibility than
> -fvisibility=hidden allows.

And does that still result in a noticeable performance boost and/or executable size reduction? That seems to be the benefit claimed by bug 273336, but that was in 2004, so maybe it's no longer relevant. Is there some other reason we would want more things with hidden visibility aside from this?
Product: Core → Firefox Build System
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.