PGO builds should be passing -cgthreads:num_cores

RESOLVED FIXED in Firefox 47

Status

()

Core
Build Config
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ted, Assigned: ted)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
Created attachment 8723137 [details]
MozReview Request: bug 1250971 - pass cgthreads with number of cores for Windows PGO builds. r?gps

Review commit: https://reviewboard.mozilla.org/r/36377/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36377/
Attachment #8723137 - Flags: review?(gps)

Comment 3

2 years ago
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+
(Assignee)

Comment 4

2 years ago
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/

Updated

2 years ago
Attachment #8723137 - Flags: review+

Comment 6

2 years ago
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?
(Assignee)

Comment 7

2 years ago
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.

Comment 8

2 years ago
(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.
(Assignee)

Comment 9

2 years ago
Ugh, apparently the linker option wants a colon before the number, so /cgthreads:N, where the compiler option does not, which is ridiculous.
(Assignee)

Comment 10

2 years ago
Wasting so much machine time on try this week:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7984984348c5
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.
(Assignee)

Comment 12

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1278fd4daced632d061da07be72d62cf709c9ded
bug 1250971 - pass cgthreads with number of cores for Windows PGO builds. r=gps

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1278fd4daced
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.