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

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

(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.

Comment 4

[Jan de Mooij [:jandem]]

(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.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Attachment: 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.

Comment 6

[Jan de Mooij [:jandem]]

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.

Comment 7

[(Away)]

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.

Comment 8

[(Away)]

And if you want to spend even more time on this :-) maybe take a look through these?

https://searchfox.org/mozilla-central/search?q=miscompil&case=true&regexp=false&path=
https://dxr.mozilla.org/mozilla-central/search?q=msvc%20bug

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Mayank Bansal]

(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)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

(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 ;)

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/34a7d2c7e73e