Closed
Bug 1095103
Opened 10 years ago
Closed 5 years ago
Remove Windows PGO flag hacks that are no longer necessary
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
WORKSFORME
People
(Reporter: away, Unassigned)
References
Details
Attachments
(2 files)
3.12 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
2.06 KB,
patch
|
wtc
:
review+
|
Details | Diff | Splinter Review |
Once the blocking bugs are fixed, we can use PGOPTIMIZE instead of the PGUPDATE hack, and we don't need the -wd lines.
Comment 1•10 years ago
|
||
Thanks for doing this! I gave up on this during the initial slog to get PGO enabled back in the Firefox 3 release cycle, and we just never revisited it.
I am curious whether the optimizer works better with PGOPTIMIZE. Maybe PGUPDATE is more conservative because the compiler assumes it's not seeing the full picture? Probably not but worth a try. Cleanup is good anyway :)
Assignee: nobody → dmajor
Attachment #8518428 -
Flags: review?(mh+mozilla)
Wan-Teh, what's your stance on compiler requirement? For FF36, VS2013 is implicitly required for PGO builds due to the huge memory requirements. If NSS wants to allow older compilers for PGO then maybe this patch could do a version check. Or we could just not land the patch, the current situation is probably not hurting anything.
Attachment #8518435 -
Flags: review?(wtc)
Comment 5•10 years ago
|
||
Comment on attachment 8518435 [details] [diff] [review] NSPR configures Review of attachment 8518435 [details] [diff] [review]: ----------------------------------------------------------------- r=wtc. 1. This is an NSPR patch, not an NSS patch. 2. I don't know of any NSS or NSPR developer who builds NSS/NSPR with PGO. So I think it should be fine to only consider Mozilla's compiler requirements. You just need to make sure this will work with the compilers used for all the currently supported Firefox ESRs, because occasionally we need to update the NSPR or NSS in an ESR.
Attachment #8518435 -
Flags: review?(wtc) → review+
Comment 6•10 years ago
|
||
Comment on attachment 8518435 [details] [diff] [review] NSPR configures David: I forgot to ask: please advise if I should check in this patch now, given that this could potentially be merged to a Firefox ESR branch.
Attachment #8518435 -
Attachment description: NSS configures → NSPR configures
Ah sorry, I get the NS- names confused sometimes :) Am I understanding correctly, that you sometimes merge from NSPR trunk to ESR branches? If that is the case, then we should wait with this patch until VS2013 reaches all the branches. The plan is for VS2013 to ride the FF36 train.
Comment 8•10 years ago
|
||
Actually, it doesn't matter either way. The LDFLAGS from nspr are not used when linking nspr on windows, because we actually don't use the nspr build system to link it.
Comment 9•10 years ago
|
||
Comment on attachment 8518428 [details] [diff] [review] Top-level and JS configures Review of attachment 8518428 [details] [diff] [review]: ----------------------------------------------------------------- Yay for removing old cruft.
Attachment #8518428 -
Flags: review?(mh+mozilla) → review+
Comment 10•10 years ago
|
||
I think this can cause LNK1270 linker error if we produce more than one .pgi file each module: invalid file; timestamp does not match file linked with /LTCG:PGINSTRUMENT
Comment 11•10 years ago
|
||
(In reply to xunxun from comment #10) > I think this can cause LNK1270 linker error if we produce more than one .pgi > file each module: > > invalid file; timestamp does not match file linked with /LTCG:PGINSTRUMENT change *.pgi to *.pgc
Reporter | ||
Comment 12•10 years ago
|
||
I don't understand your comment. Does this patch break your build? (Note that you need to pick up the fixes for all the blocking bugs before applying this patch, including bug 1094566 which is not yet landed) If you are still broken, please describe your build configuration.
Flags: needinfo?(xunxun1982)
Comment 13•10 years ago
|
||
(In reply to David Major [:dmajor] (UTC+13) from comment #12) > I don't understand your comment. Does this patch break your build? (Note > that you need to pick up the fixes for all the blocking bugs before applying > this patch, including bug 1094566 which is not yet landed) If you are still > broken, please describe your build configuration. I haven't tried, but I think this may have some issues (I suspect). You can do some easy tests, using any test.cpp as an example: cl /O2 /GL test.cpp /link /ltcg:pgi run test.exe more times, this will generate some *.pgc if you use cl /O2 /GL test.cpp /link /ltcg:pgo this will cause LNK1270 linker error if you use cl /O2 /GL test.cpp /link /ltcg:pgu all work well.
Flags: needinfo?(xunxun1982)
Reporter | ||
Comment 14•10 years ago
|
||
I do see the failure that you described with those command lines. But it doesn't affect the Firefox build. I don't think we should be blocked by this.
Reporter | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/14f75afaad14
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/14f75afaad14
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Updated•9 years ago
|
Target Milestone: mozilla37 → mozilla36
Comment 17•9 years ago
|
||
Backed out from Aurora for causing bug 1106174. https://hg.mozilla.org/releases/mozilla-aurora/rev/828a534b9653
status-firefox36:
--- → wontfix
status-firefox37:
--- → fixed
Comment 18•9 years ago
|
||
Bug 1106174 isn't showing signs of being fixed any time soon. Backed out. https://hg.mozilla.org/integration/mozilla-inbound/rev/8091cd611acd
Status: RESOLVED → REOPENED
status-firefox36:
wontfix → ---
status-firefox37:
fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla36 → ---
Comment 19•9 years ago
|
||
Merge of backout: https://hg.mozilla.org/mozilla-central/rev/8091cd611acd
Updated•6 years ago
|
Product: Core → Firefox Build System
Reporter | ||
Comment 20•5 years ago
|
||
Cleaning up some bug dashboards. The affected code here no longer exists after the transition to clang-cl.
Status: REOPENED → RESOLVED
Closed: 9 years ago → 5 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•