1445089 - Revert workaround for a VS2017 <15.6 constexpr pointer math bug

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P3)

Description

[Masatoshi Kimura [:emk]]

We are going to require VS 2017 15.6.

Comment 1

[Ralph Giles (:rillian)]

Bryce, do you mind reverting the original patch once bug 1424281 lands?

Comment 2

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Sure thing. Looks like that's just happened, so I'll throw this on my immediate queue and try have it sorted in the next few days.

Comment 3

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bf780078be7ea7066e88fb697842076a8f6030e

Comment 4

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1445089 - Revert workaround for a VS2017 <15.6 constexpr pointer math bug.

Bug 1408695 introduced a workaround in HTMLTrackElement for a bug in VS2017 <
15.6. Bug 1424281 bumps our minimum support versions so the workaround can be
removed.

Review commit: https://reviewboard.mozilla.org/r/228096/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228096/

Comment 5

[(Away)]

Comment on attachment 8959213 [details]
Bug 1445089 - Revert workaround for a VS2017 <15.6 constexpr pointer math bug.

https://reviewboard.mozilla.org/r/228096/#review234002

I don't expect any problems, but you may want to wait a few more days for 15.6 to stick, just to avoid tempting fate.

Comment 6

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8959213 [details]
Bug 1445089 - Revert workaround for a VS2017 <15.6 constexpr pointer math bug.

https://reviewboard.mozilla.org/r/228096/#review234002

Appreciate the headsup. Will let it bake, have set a reminder for next Tuesday for landing.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92532463d99c
Revert workaround for a VS2017 <15.6 constexpr pointer math bug. r=dmajor

Comment 8

[Cosmin Sabou [:CosminS]]

Backed out changeset 92532463d99c (bug 1445089) for web platform test failures on HTMLTrackElement/kind.html.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&fromchange=92532463d99c16b6abd0ea70ec1df59f1883a59e&tochange=46de8c37c991cf779339cb38b14bd7fc0ce6a2b6

Failures log: https://treeherder.mozilla.org/logviewer.html#?job_id=169293254&repo=autoland&lineNumber=7056

Backout link: https://hg.mozilla.org/integration/autoland/rev/46de8c37c991cf779339cb38b14bd7fc0ce6a2b6

Comment 9

[Cosmin Sabou [:CosminS]]

Updated push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=92532463d99c16b6abd0ea70ec1df59f1883a59e&tochange=46de8c37c991cf779339cb38b14bd7fc0ce6a2b6&filter-searchStr=wpt&selectedJob=169293254

Comment 10

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Original bug mentioned mochitests should pick this up, but it looks like it's the web platform tests which catch this.

Looking more it seems MSVC mainline releases are not yet handling this. Running the test case from the original bug and building central I'm seeing the same issues, tested with:

- 15.6.2 with cl version 19.13.26128
- 15.6.3 with cl version 19.13.26129

Since we're supporting 19.13.26128 in tree I think we're going to have to park this until we get another bump before we can try this again.

Comment 11

[(Away)]

Honestly I think we've already put in more effort than this deserves. The constexpr->const reduction on this single tiny array is such a miniscule inefficency, we could leave it that way forever and not a single cpu would ever notice.

Comment 12

[Masatoshi Kimura [:emk]]

MS claims that this bug was fixed since VS2017 15.6 Preview 4 (a.k.a. 19.13.26125.1):
https://developercommunity.visualstudio.com/content/problem/60059/code-generation-problem-with-constexpr.html
We should file another bug report if this is not fixed yet. Or fix bug 1443590.

(In reply to David Major [:dmajor] from comment #11)
> Honestly I think we've already put in more effort than this deserves. The
> constexpr->const reduction on this single tiny array is such a miniscule
> inefficency, we could leave it that way forever and not a single cpu would
> ever notice.

Although this workaround is negligible, this bug might affect some more complex use case such as bug 1411469.

Comment 13

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

I'll hold onto this and look into it some more when I have a moment. If it turns out MS have fixed the other cases but not ours I can file a bug with them and update this bug. This doesn't seem to be causing us any pain if it stays as is for some time longer though.

Comment 14

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Bug created, let's see where it goes: https://developercommunity.visualstudio.com/content/problem/223146/constexpr-code-gneration-bug.html

Comment 15

[Chris Peterson [:cpeterson]]

Unassigning bugs assigned to Bryce because he no longer works at Mozilla.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Given that we don't support MSVC at all anymore, there should be nothing from blocking this. And Try agrees:
https://treeherder.mozilla.org/jobs?repo=try&revision=eb88424469b54bbff3504ffbd5e3c7017e378dac

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bug 1445089 - Revert workaround for a VS2017 <15.6 constexpr pointer math bug. r=alwu

Bug 1408695 introduced a workaround in HTMLTrackElement for a bug in VS2017 <
15.6. We don't support MSVC at all anymore, so this issue should be moot even
if it was fixed upstream since.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ca81485a950
Revert workaround for a VS2017 <15.6 constexpr pointer math bug. r=alwu

Comment 19

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/8ca81485a950