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)
Firefox Build System
General
Tracking
(firefox49 affected)
NEW
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.
Comment 1•8 years ago
|
||
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?
Comment 2•8 years ago
|
||
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.
Comment 3•8 years ago
|
||
> (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?
Updated•6 years ago
|
Product: Core → Firefox Build System
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•