Closed Bug 1445105 Opened 2 years ago Closed 2 years ago

Remove old MSVC de-optimizations working around bustage in unsupported versions

Categories

(Firefox Build System :: General, enhancement)

3 Branch
Unspecified
Windows
enhancement
Not set

Tracking

(firefox-esr52 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: RyanVM, Assigned: RyanVM)

References

Details

Attachments

(1 file)

We have a number of de-optimizations of functions working around MSVC bustage in versions which are no longer reported. Hopefully we can remove them without issue now.
See Also: → 1393189
OS: Unspecified → Windows
Duplicate of this bug: 1445550
The one from bug 1167883 scares me a lot because I don't see any good way of testing it besides "remove it and see what breaks" and hope we manage to actually trace any resulting issues back to this bug. I think I'll leave it out of the main patch for now and then do an extra Try push with it included to see if there's any observable perf difference worth making us care more.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #2)
> The one from bug 1167883 scares me a lot because I don't see any good way of
> testing it besides "remove it and see what breaks" and hope we manage to
> actually trace any resulting issues back to this bug. I think I'll leave it
> out of the main patch for now and then do an extra Try push with it included
> to see if there's any observable perf difference worth making us care more.

No obvious test failures from including this on top of the others, but no obvious Talos changes either.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #2)
> The one from bug 1167883 scares me a lot because I don't see any good way of
> testing it besides "remove it and see what breaks" and hope we manage to
> actually trace any resulting issues back to this bug. I think I'll leave it
> out of the main patch for now

Yeah that one was not really a compiler correctness bug, more "PGO inlines too much, resulting in huge stack frames", so keeping that one SGTM.
I hit this pretty hard on Try with no obvious issues:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=72cc372f78249083fd2f8e7b2477abf448a63847

Note that I retriggered the decision task so that I would get multiple builds w/ tests rather than retriggering the tests on the same build each time (to look for any sort of intermittent miscompilation issues). All the oranges there are known intermittents.

As noted in the commit message, this effectively reverts the following 4 bugs: 703135, 977538, 1274450, 1403220. Per comment 4, I left bug 1167883 alone since that one seemed like more trouble than it was worth.

Jan, I'm requesting your feedback RE: bug 977538. I landed the testcase from that bug as a crashtest and everything appears to be OK (and I was able to reproduce that crash still with builds from the affected timeframe), but I wanted to make sure you didn't have any lingering concerns from reverting that change either.
Attachment #8960182 - Flags: review?(dmajor)
Attachment #8960182 - Flags: feedback?(jdemooij)
Comment on attachment 8960182 [details] [diff] [review]
remove MSVC de-optimizations that don't appear to be necessary anymore

Review of attachment 8960182 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this.

It's hard to say if/when Microsoft fixed the real bug, but (according to blog posts) they made a lot of changes to their compiler and it's 4 years later so if we can't repro it probably got fixed at some point.
Attachment #8960182 - Flags: feedback?(jdemooij) → feedback+
Comment on attachment 8960182 [details] [diff] [review]
remove MSVC de-optimizations that don't appear to be necessary anymore

Review of attachment 8960182 [details] [diff] [review]:
-----------------------------------------------------------------

You may want to advise the sheriffs about this landing, in case some of these bugs are context-dependent and don't show up until tickled by something a hundred pushes later.
Attachment #8960182 - Flags: review?(dmajor) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34a7d2c7e73e
Remove various MSVC de-optimizations used to work around compiler bugs which are no longer needed. r=dmajor
(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)
> Created attachment 8960182 [details] [diff] [review]
> remove MSVC de-optimizations that don't appear to be necessary anymore
> 
> I hit this pretty hard on Try with no obvious issues:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=72cc372f78249083fd2f8e7b2477abf448a63847
> 
> Note that I retriggered the decision task so that I would get multiple
> builds w/ tests rather than retriggering the tests on the same build each
> time (to look for any sort of intermittent miscompilation issues). All the
> oranges there are known intermittents.
> 
> As noted in the commit message, this effectively reverts the following 4
> bugs: 703135, 977538, 1274450, 1403220. Per comment 4, I left bug 1167883
> alone since that one seemed like more trouble than it was worth.
> 
> Jan, I'm requesting your feedback RE: bug 977538. I landed the testcase from
> that bug as a crashtest and everything appears to be OK (and I was able to
> reproduce that crash still with builds from the affected timeframe), but I
> wanted to make sure you didn't have any lingering concerns from reverting
> that change either.

quick question: did you undo the changes from bug 791214 ? (wondering because the patch didnt have this specific  bug number)
(In reply to Mayank Bansal from comment #10)
> quick question: did you undo the changes from bug 791214 ? (wondering
> because the patch didnt have this specific  bug number)

Said changes were already removed before I ever had a chance to ;)
https://hg.mozilla.org/mozilla-central/rev/34a7d2c7e73e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
No longer blocks: VS15.6
Depends on: VS15.6
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.