803276 - Disable typed array move() operator for specific channels

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

Over in bug 786386, I have to remember to re-disable a feature in beta after every uplift, until it gets added to a spec someday. This is suboptimal because it requires me to remember to do it and it happens at an unpredictable time during the development cycle so beta testers will see the feature for some random percentage of the beta cycle before it disappears.

I would like to propose that part of the uplift process be to update a preprocessor macro that indicates the stage (final, beta, aurora, development) of the release process. (This way actually dherman's idea, but I like it.) That way I could do something like

  #if defined(RELEASE_STAGE_FINAL) || defined(RELEASE_STAGE_BETA)
  # undef ENABLE_MY_UNSPECCED_APIS
  #else
  # define ENABLE_MY_UNSPECCED_APIS
  #endif

or somewhat less error-prone would be to make it numeric so it would be:

  #if RELEASE_STAGE >= RELEASE_STAGE_BETA

Comment 1

[Nick Thomas [:nthomas] (UTC+12)]

I'd suggest using the update channel as other places in the tree do, but that would only be defined for non-dep builds (eg nightlies, beta & release in the release automation).

Comment 2

[Steve Fink [:sfink] [:s:]]

(In reply to Nick Thomas [:nthomas] from comment #1)
> I'd suggest using the update channel as other places in the tree do, but
> that would only be defined for non-dep builds (eg nightlies, beta & release
> in the release automation).

Thanks, I hadn't known about that, and it's pretty close. But for something like this, I'm leery of borrowing the meaning of something else.

Are all beta builds done as nightlies? That would make it a little better.

The major risk for the specific use I'm talking about is that someone commits JS code that uses the controlled feature. It would work until it hit beta, and even then, it would work for try pushes. Or rather, it would work for try pushes if it was actually modified in the tree as part of the uplift process; if it was something set by the build infrastructure, it might or might not be set for try pushes. Which seems risky.

Comment 3

[Alex Keybl [:akeybl]]

I think Gavin will be able to make a recommendation of the best practice for doing this. Gavin, we're trying to prevent the backout in bug 803288 with each new release.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Generally people just use the update channel for this, as Nick suggested (which is set in the mozconfigs used by the MoCo builders). We could add #defines based on source repository (similar to how SOURCE_REPO works), or based on the presence of "a1"/"a2" in version.txt (updated as part of the uplift process), but I don't think we've historically seen the need for it - update channel has been "good enough".

I'm not sure I like the idea of doing this more in general. The risks of bustage only being exposed on merge days as you describe are real, and we've seen multiple such problems in the past (e.g. bug 787253). But I'm not sure how to weigh those against the benefits of having Nightly/Aurora-only testing of the new feature, because I'm not sure how much useful feedback that's getting you.

Comment 5

[Alex Keybl [:akeybl]]

Steven - let's either complete this bug prior to the next merge, or do the backout in bug 803288 for all branches. Please weigh the risks that Gavin has called out, since if this is not really necessary to be in the trees, we should just back out of all branches.

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Disable ArrayBufferView.prototype.move on release channels

I think I do want to keep this enabled for non-beta, non-release builds. It's mainly meant as a trail for emscripten optimization, and will be joined by other similar features. If we need to create custom builds every time emscripten wants to do timing comparisons, it just won't happen.

dmandelin - cf http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/Makefile.in#137

Comment 7

[David Mandelin [:dmandelin]]

Comment on attachment 679743 [details] [diff] [review]
Disable ArrayBufferView.prototype.move on release channels

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

And thanks for adding the ability to change the update channel in the build, too.

Comment 8

[David Mandelin [:dmandelin]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #4)
> Generally people just use the update channel for this, as Nick suggested
> (which is set in the mozconfigs used by the MoCo builders). We could add
> #defines based on source repository (similar to how SOURCE_REPO works), or
> based on the presence of "a1"/"a2" in version.txt (updated as part of the
> uplift process), but I don't think we've historically seen the need for it -
> update channel has been "good enough".
> 
> I'm not sure I like the idea of doing this more in general. The risks of
> bustage only being exposed on merge days as you describe are real, and we've
> seen multiple such problems in the past (e.g. bug 787253). But I'm not sure
> how to weigh those against the benefits of having Nightly/Aurora-only
> testing of the new feature, because I'm not sure how much useful feedback
> that's getting you.

What options do we have? I also thought that having a bunch of stuff get disabled automatically on merge day could cause bustage. But it seems like we really do need to test some features for a while in aurora only. And disabling after the beta merge seems worse--stuff that's not ready "leaks out" into beta. 

One idea would be to designate the end of aurora as "pre-beta" and turn things off then. Or if we have the existing automated turnoff, we could do that in a separate set of TB (or even try) builds a little before the merge, to try to detect turnoff bustage early.

Comment 9

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f4f4eec3b452

Comment 10

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/f4f4eec3b452

Comment 11

[Steve Fink [:sfink] [:s:]]

Comment on attachment 679743 [details] [diff] [review]
Disable ArrayBufferView.prototype.move on release channels

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 730873
User impact if declined: none
Testing completed (on m-c, etc.): on m-c since yesterday
Risk to taking this patch (and alternatives if risky): very low. This only changes around how some experimental features are controlled. It does not change any behavior.
String or UUID changes made by this patch: none

I do not intend to request approval-beta. It's already disabled there manually. This is the permanent fix going forward, so that the experimental features will automatically be disabled on beta, release, and esr.

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 679743 [details] [diff] [review]
Disable ArrayBufferView.prototype.move on release channels

[Triage Comment]
Approving for Aurora instead of tracking bug 803288 for multiple releases. Thanks Steve.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c5d3109c0dcd

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to David Mandelin [:dmandelin] from comment #8)
> One idea would be to designate the end of aurora as "pre-beta" and turn
> things off then. Or if we have the existing automated turnoff, we could do
> that in a separate set of TB (or even try) builds a little before the merge,
> to try to detect turnoff bustage early.

This probably isn't necessary, unless we start having things be release-only. Bustage at the aurora->beta merge makes merge days more annoying, but it's not in any time-sensitive release path.