PGO builds should be passing -cgthreads:num_cores

RESOLVED FIXED in Firefox 47

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: ted, Assigned: ted)

Tracking

unspecified
mozilla47

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment)

Our Windows PGO builds in automation are happening on c3.2xlarge instances, which have 8 cores. Visual C++ has this cgthreads linker flag which defaults to 4:
https://msdn.microsoft.com/en-us/library/dn631956.aspx

We should make it pass -cgthreads:numcores to make sure it's using all possible cores (the max value you can pass is 8, though):
https://msdn.microsoft.com/en-us/library/dn631956.aspx
Comment on attachment 8723137 [details]
MozReview Request: bug 1250971 - pass cgthreads with number of cores for Windows PGO builds. r?gps

https://reviewboard.mozilla.org/r/36377/#review32937

LGTM
Attachment #8723137 - Flags: review?(gps) → review+
Comment on attachment 8723137 [details]
MozReview Request: bug 1250971 - pass cgthreads with number of cores for Windows PGO builds. r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36377/diff/1-2/
Attachment #8723137 - Flags: review+
Comment on attachment 8723137 [details]
MozReview Request: bug 1250971 - pass cgthreads with number of cores for Windows PGO builds. r?gps

https://reviewboard.mozilla.org/r/36377/#review33101

::: configure.in:2208
(Diff revision 2)
> -        PROFILE_USE_LDFLAGS="-LTCG:PGUPDATE"
> +        PROFILE_USE_LDFLAGS="-LTCG:PGUPDATE $cgthreads"

https://msdn.microsoft.com/en-us/library/dn631956%28v=vs.120%29.aspx says /CGTHREADS is a cl.exe option. So this should be on a CFLAGS not a LDFLAGS variable, right?
It's also a linker option:
https://msdn.microsoft.com/en-us/library/dn655038.aspx

...which makes sense in this case because it's link.exe doing the code generation in the LTCG case.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> It's also a linker option:
> https://msdn.microsoft.com/en-us/library/dn655038.aspx
> 
> ...which makes sense in this case because it's link.exe doing the code
> generation in the LTCG case.

Oh. I was getting warnings during the first pass about /cgthreads not being a supported linker option.
Ugh, apparently the linker option wants a colon before the number, so /cgthreads:N, where the compiler option does not, which is ridiculous.
I tested /cgthreads:8 on my Windows desktop with VS2013 and it provided a very minimal wall time benefit over /PogoSafeMode of ~20s over 87 minutes. That's statistically insignificant, especially when you consider the sample size is 1. Looking at the CPU usage, /cgthreads:8 appears to saturate 4+4 cores for 100-150s in PGO pass 2 in areas where the default of /cgthreads:4 is only saturating 4 cores. However, that extra 4 cores are the hyperthreaded cores, which history says add up to 25% core each.

At least on my machine, increasing /cgthreads doesn't appear to add that much benefit :/

I think we should still move forward with this, however, as it makes sense to default to giving the linker access to as much processor as possible.

Comment 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1278fd4daced
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.