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)

36 Branch
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: away, Unassigned)

References

Details

Attachments

(2 files)

Once the blocking bugs are fixed, we can use PGOPTIMIZE instead of the PGUPDATE hack, and we don't need the -wd lines.
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)
Attached patch NSPR configuresSplinter Review
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 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 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.
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 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+
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
(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
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)
(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)
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.
https://hg.mozilla.org/mozilla-central/rev/14f75afaad14
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Target Milestone: mozilla37 → mozilla36
Depends on: 1106174
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
Resolution: FIXED → ---
Target Milestone: mozilla36 → ---
Assignee: dmajor → nobody
Product: Core → Firefox Build System

Cleaning up some bug dashboards. The affected code here no longer exists after the transition to clang-cl.

Status: REOPENED → RESOLVED
Closed: 9 years ago5 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.