We will be requiring CPU SSE support to run Firefox. We will need to send CPU features as part of the update URL/ping so the update server can refuse to serve updates to clients lacking required CPU features. I'm not sure what all CPU features we should report. In the future, it is conceivable we could send highly optimized builds (e.g. SSE4 and AVX optimized) to clients that support them. That decision is obviously for another bug. But if we are smart about what CPU features we send, we could open that door today.

(In reply to Gregory Szorc [:gps] from comment #0) > We will be requiring CPU SSE support to run Firefox. > > We will need to send CPU features as part of the update URL/ping so the > update server can refuse to serve updates to clients lacking required CPU > features. > > I'm not sure what all CPU features we should report. In the future, it is > conceivable we could send highly optimized builds (e.g. SSE4 and AVX > optimized) to clients that support them. That decision is obviously for > another bug. But if we are smart about what CPU features we send, we could > open that door today. The most obvious place to put this when thinking about the possibility of unique binaries by different cpu targets, would be in the %BUILD_TARGET% field. However, the update server needs to know about all valid build targets, so each one we add now needs to be aliased in our tools. This isn't difficult, but having a bunch of essentially unused build targets could be confusing. Another option could be a new field more similar to %OS_VERSION%. Doing this would let us do SSE deprecation without any extra aliasing, but wouldn't help serve unique builds by CPU feature in the future. There's no reason we couldn't do the latter now and switch to the former later AFAIK.

Will this be needed for all Firefox supported OS's?

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #2) > Will this be needed for all Firefox supported OS's? Good question. Windows for sure. I'm not sure about OS X. It's quite possible that all Macs supported by Firefox 49+/OS X 10.9+ are guaranteed to have SSE. But I'm not certain of this. I assume Linux is also impacted. However, I'm guessing that population is so small that it may be tempting to ignore that platform. We should run the numbers.

bhearsum, since BUILD_TARGET describes the build and not the local system running the application I don't think that would be an appropriate place to put this information. I could see adding it to OS_VERSION and just making that more generic. It could be changed to OS_INFO and be a bucket for this information. We could also go with a new field if you prefer and return "unknown" for the time being for OS's that don't have this implemented. gps, instead of going with a __cpuid implementation I think I'd prefer going with IsProcessorFeaturePresent implemented in js-ctypes. SSE4 can't be detected at this time and if / when the day comes that it is needed it can be implemented using __cpuid. https://msdn.microsoft.com/en-us/library/windows/desktop/ms724482(v=vs.85).aspx The current implementation I have with js-ctypes returns the first instruction set supported out of SSE3, SSE2, SSE, and MMX in the update url as shown below. https://aus5.mozilla.org/update/3/Firefox/48.0a2/20160511192050/WINNT_x86-msvc-x64/en-US/rs-test/Windows_NT%2010.0.0.0%20(x64)%20(SSE3)/default/default/update.xml?force=1

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #4) > bhearsum, since BUILD_TARGET describes the build and not the local system > running the application I don't think that would be an appropriate place to > put this information. I could see adding it to OS_VERSION and just making > that more generic. It could be changed to OS_INFO and be a bucket for this > information. We could also go with a new field if you prefer and return > "unknown" for the time being for OS's that don't have this implemented. For the current use case (deprecation), I agree that BUILD_TARGET is a bad place. If we ever look at serving people separate binaries by CPU feature, I think it may be (because product+channel+build target+locale = unique stream of builds). As for now, OS_VERSION is totally fine for me, but I do think we should change the name at the same time to avoid confusion. I can't think of a reason why adding a new field would be better than renaming OS_VERSION.

rstrong: advanced CPU detection support is scope bloat. So I think going with IsProcessorFeaturePresent is fine. I only wrote the bit in comment #0 about SSE4 and AVX so we could future proof ourselves if the effort was small.

(In reply to Ben Hearsum (:bhearsum) from comment #6) > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #4) > > bhearsum, since BUILD_TARGET describes the build and not the local system > > running the application I don't think that would be an appropriate place to > > put this information. I could see adding it to OS_VERSION and just making > > that more generic. It could be changed to OS_INFO and be a bucket for this > > information. We could also go with a new field if you prefer and return > > "unknown" for the time being for OS's that don't have this implemented. > > For the current use case (deprecation), I agree that BUILD_TARGET is a bad > place. If we ever look at serving people separate binaries by CPU feature, I > think it may be (because product+channel+build target+locale = unique stream > of builds). So, BUILD_TARGET in this case should include what the app's build specific feature is (just for reference and not part of this bug). For example, if the build was an SSE2 build this would be included in BUILD_TARGET. The OS_INFO field would include what the client supported. For example, the OS_INFO field would include SSE2 for SSE2 capable systems, SSE3 for SSE3 capable systems, etc. > As for now, OS_VERSION is totally fine for me, but I do think we should > change the name at the same time to avoid confusion. I can't think of a > reason why adding a new field would be better than renaming OS_VERSION. So, the OS_VERSION field which will become the OS_INFO field contains on my system Windows_NT 10.0.0.0 (x64) How would you like the new value represented? The current patch has it as Windows_NT 10.0.0.0 (x64) (SSE3) but I just did that to verify the patch worked. Perhaps the following? Windows_NT 10.0.0.0 (x64,SSE3) Note to self: OS_VERSION is used by the add-ons manager so all apps are going to have to change to OS_INFO instead of repurposing OS_VERSION.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #8) > (In reply to Ben Hearsum (:bhearsum) from comment #6) > > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > > comment #4) > > > bhearsum, since BUILD_TARGET describes the build and not the local system > > > running the application I don't think that would be an appropriate place to > > > put this information. I could see adding it to OS_VERSION and just making > > > that more generic. It could be changed to OS_INFO and be a bucket for this > > > information. We could also go with a new field if you prefer and return > > > "unknown" for the time being for OS's that don't have this implemented. > > > > For the current use case (deprecation), I agree that BUILD_TARGET is a bad > > place. If we ever look at serving people separate binaries by CPU feature, I > > think it may be (because product+channel+build target+locale = unique stream > > of builds). > So, BUILD_TARGET in this case should include what the app's build specific > feature is (just for reference and not part of this bug). For example, if > the build was an SSE2 build this would be included in BUILD_TARGET. The > OS_INFO field would include what the client supported. For example, the > OS_INFO field would include SSE2 for SSE2 capable systems, SSE3 for SSE3 > capable systems, etc. We might want to take this part elsewhere, or stash it for some point in the future, but BUILD_TARGET started to include more than just target information when we added target arch and _running_ arch for Mac a long time ago (and more recently for Windows). This was important, because the old AUS server didn't let us serve people specific builds based on their OS_VERSION. This is similarly true in Balrog - OS_VERSION let's us do some filtering, but it's not really designed for helping delivering a series of builds over time. If you want to chat about this more let me know...otherwise I'll leave it at this for now since it's not relevant to the problem at hand. > > As for now, OS_VERSION is totally fine for me, but I do think we should > > change the name at the same time to avoid confusion. I can't think of a > > reason why adding a new field would be better than renaming OS_VERSION. > So, the OS_VERSION field which will become the OS_INFO field contains on my > system > Windows_NT 10.0.0.0 (x64) > > How would you like the new value represented? The current patch has it as > Windows_NT 10.0.0.0 (x64) (SSE3) but I just did that to verify the patch > worked. Perhaps the following? > Windows_NT 10.0.0.0 (x64,SSE3) I think the first one (SSE3) would be better, but anything where each individual feature has a known leading and trailing character should be fine. Eg: the second would meet this if it was (x64,SSE3,). OS_VERSION is a simple substring match on the server, so it can sometimes be hard to isolate individual parts of it, eg with: Windows_NT 10.0.0.0 (x64,SSE3,SSE2,SSE) ...we may have trouble just matching "SSE". This is a bit of a contrived example, but especially if the order of the feature is not guaranteed it could make matching a lot trickier.

(In reply to Ben Hearsum (:bhearsum) from comment #9) > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > comment #8) > > (In reply to Ben Hearsum (:bhearsum) from comment #6) > > > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from > > > comment #4) > > > > bhearsum, since BUILD_TARGET describes the build and not the local system > > > > running the application I don't think that would be an appropriate place to > > > > put this information. I could see adding it to OS_VERSION and just making > > > > that more generic. It could be changed to OS_INFO and be a bucket for this > > > > information. We could also go with a new field if you prefer and return > > > > "unknown" for the time being for OS's that don't have this implemented. > > > > > > For the current use case (deprecation), I agree that BUILD_TARGET is a bad > > > place. If we ever look at serving people separate binaries by CPU feature, I > > > think it may be (because product+channel+build target+locale = unique stream > > > of builds). > > So, BUILD_TARGET in this case should include what the app's build specific > > feature is (just for reference and not part of this bug). For example, if > > the build was an SSE2 build this would be included in BUILD_TARGET. The > > OS_INFO field would include what the client supported. For example, the > > OS_INFO field would include SSE2 for SSE2 capable systems, SSE3 for SSE3 > > capable systems, etc. > > We might want to take this part elsewhere, or stash it for some point in the > future, but BUILD_TARGET started to include more than just target > information when we added target arch and _running_ arch for Mac a long time > ago (and more recently for Windows). This was important, because the old AUS > server didn't let us serve people specific builds based on their OS_VERSION. > This is similarly true in Balrog - OS_VERSION let's us do some filtering, > but it's not really designed for helping delivering a series of builds over > time. If you want to chat about this more let me know...otherwise I'll leave > it at this for now since it's not relevant to the problem at hand. Agreed though iirc they were added to BUILD_TARGET to simplify things for AUS. > > > As for now, OS_VERSION is totally fine for me, but I do think we should > > > change the name at the same time to avoid confusion. I can't think of a > > > reason why adding a new field would be better than renaming OS_VERSION. > > So, the OS_VERSION field which will become the OS_INFO field contains on my > > system > > Windows_NT 10.0.0.0 (x64) > > > > How would you like the new value represented? The current patch has it as > > Windows_NT 10.0.0.0 (x64) (SSE3) but I just did that to verify the patch > > worked. Perhaps the following? > > Windows_NT 10.0.0.0 (x64,SSE3) > > I think the first one (SSE3) would be better, but anything where each > individual feature has a known leading and trailing character should be > fine. Eg: the second would meet this if it was (x64,SSE3,). > > OS_VERSION is a simple substring match on the server, so it can sometimes be > hard to isolate individual parts of it, eg with: > Windows_NT 10.0.0.0 (x64,SSE3,SSE2,SSE) > > ...we may have trouble just matching "SSE". This is a bit of a contrived > example, but especially if the order of the feature is not guaranteed it > could make matching a lot trickier. It is sounding like we should use BUILD_TARGET and make that a more generic field instead of using OS_VERSION. I just want to make sure that it is added to the final location now instead of having to do rework later.

Note: just talked with bsmedberg and he said we are going with a minimum of SSE2.

(In reply to Gregory Szorc [:gps] from comment #3) > I'm not sure about OS X. It's quite possible that all Macs supported by > Firefox 49+/OS X 10.9+ are guaranteed to have SSE. But I'm not certain of > this. Yes, this is true.

What's about adding a field called SYSTEM_INFO? This would be more generic and I can imagine that this would be someday interesting for other things that depends not on the OS (GFX, GPU?).

SYSTEM_INFO is already a structure in the Windows world and I'd prefer not using it to prevent confusion with it. The naming is the simple part and we'll come up with something.

Ben, I went with adding it right after %OS_VERSION% https://aus5.mozilla.org/update/4/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%SYSTEM_CAPABILITIES%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml The url sent on my system is https://aus5.mozilla.org/update/4/Firefox/49.0a1/20160519113608/WINNT_x86-msvc-x64/en-US/rs-test/Windows_NT%2010.0.0.0%20(x64)/SSE3/default/default/update.xml?force=1 If there is no value then the value in the url will be unknown. If there was an error getting the value then the value in the url will be error. For systems that don't send any information yet in that field the value will be NA.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #15) > Created attachment 8754495 [details] [diff] [review] > patch rev1 > > Ben, I went with adding it right after %OS_VERSION% > https://aus5.mozilla.org/update/4/%PRODUCT%/%VERSION%/%BUILD_ID%/ > %BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%SYSTEM_CAPABILITIES%/ > %DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml > > The url sent on my system is > https://aus5.mozilla.org/update/4/Firefox/49.0a1/20160519113608/WINNT_x86- > msvc-x64/en-US/rs-test/Windows_NT%2010.0.0.0%20(x64)/SSE3/default/default/ > update.xml?force=1 I think you'll want to use /update/6. 4 is taken by Fennec (https://dxr.mozilla.org/mozilla-central/source/mobile/android/app/mobile.js#576), and 5 by B2G (https://dxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#533). Looks good to me otherwise though - we'll need to coordinate a change to Balrog before this can land.

Ben, will the balrog change happen in bug 1271762 or a different bug?

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #18) > Ben, will the balrog change happen in bug 1271762 or a different bug? Let's use bug 1274374 for that.

[Tracking Requested - why for this release]: This is needed in Firefox 48 and Firefox ESR 45 to support requiring SSE2 in Firefox 49 and Firefox ESR 52.

Ben, could you give a heads up regarding the balrog changes for aus2-community.mozilla.org which SeaMonkey uses and update.instantbird.org which Instantbird uses?

Comment on attachment 8754499 [details] [diff] [review] patch rev2 Review of attachment 8754499 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #22) > Ben, could you give a heads up regarding the balrog changes for > aus2-community.mozilla.org which SeaMonkey uses and update.instantbird.org > which Instantbird uses? I spoke with Florian about this for Instabird on IRC, and I'm told ewong is the right person to deal with this for SeaMonkey. It's worth noting that they don't *have* to opt-in to this. They could choose to stick with /update/3 if they don't feel they have enough non-SSE2 users to worry about.

Tracking as it is critical that we have a way to ping the update server

Splitting out the app update changes from the Firefox pref change so I can get the app update changes landed before the balrog changes. Then each app just has to change a pref.

Felipe, this patch won't land until after the balrog changes land in bug 1274374

Comment on attachment 8754953 [details] [diff] [review] Firefox patch Wrong patch

Felipe, this patch won't land until after the balrog changes land in bug 1274374

Filed: bug 1274723 for Instantbird bug 1274722 for SeaMonkey bug 1274721 for Thunderbird

Comment on attachment 8754952 [details] [diff] [review] App update patch Landed this patch on fx-team. Leave open for landing the Firefox patch.

Just checked nightly by adding the app.update.url.override pref so it contained %SYSTEM_CAPABILITIES% and it was replaced by SSE3. We should be good to go as soon as bug 1274374 is fixed.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #35) > Just checked nightly by adding the app.update.url.override pref so it > contained %SYSTEM_CAPABILITIES% and it was replaced by SSE3. We should be > good to go as soon as bug 1274374 is fixed. I hope to have this in production by the end of the week, fyi.

Pushed by rstrong@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/25321494921c Firefox preference patch - app update patch for adding CPU features/detection to update URL. r=felipc

Comment on attachment 8754952 [details] [diff] [review] App update patch Approval Request Comment [Feature/regressing bug #]: Bug 1186064 [User impact if declined]: I believe the release drivers know the impact better than I do. :) [Describe test coverage new/current, TreeHerder]: There is a test in the tree and both bhearsum and myself verified that this works. [Risks and why]: None known besides typical risks with code changes. [String/UUID change made/needed]: None.

Comment on attachment 8754955 [details] [diff] [review] Firefox patch Approval Request Comment [Feature/regressing bug #]: Bug 1186064 [User impact if declined]: I believe the release drivers know the impact better than I do. :) [Describe test coverage new/current, TreeHerder]: There is a test in the tree and both bhearsum and myself verified that this works. [Risks and why]: None known besides typical risks with code changes. [String/UUID change made/needed]: None.

Comment on attachment 8754952 [details] [diff] [review] App update patch [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is needed so we don't update users to a version that won't run on their system. User impact if declined: I believe the release drivers know the impact better than I do. :) Fix Landed on Version: 49 Risk to taking this patch (and alternatives if risky): None known besides typical risks with code changes. String or UUID changes made by this patch: None.

Comment on attachment 8754955 [details] [diff] [review] Firefox patch [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is needed so we don't update users to a version that won't run on their system. User impact if declined: I believe the release drivers know the impact better than I do. :) Fix Landed on Version: 49 Risk to taking this patch (and alternatives if risky): None known besides typical risks with code changes. String or UUID changes made by this patch: None.

It sounds like this should land in the ESR version that ships along with 48, so, ESR 45.3.0, and not for the ESR that's about to ship (45.2.0). Is that correct?

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #42) > It sounds like this should land in the ESR version that ships along with 48, > so, ESR 45.3.0, and not for the ESR that's about to ship (45.2.0). Is that > correct? Unless one of the minor releases of ESR 45 is going to require SSE2 this needs to ship before ESR 52. It would be a good thing to ship it as soon as safely possible before ESR 52 though so older clients can update directly to ESR 52 without having to update to an intermediate version first to get these patches.

(In reply to Gregory Szorc [:gps] from comment #3) > Good question. > > Windows for sure. > > I'm not sure about OS X. It's quite possible that all Macs supported by > Firefox 49+/OS X 10.9+ are guaranteed to have SSE. But I'm not certain of > this. > > I assume Linux is also impacted. However, I'm guessing that population is so > small that it may be tempting to ignore that platform. We should run the > numbers. I talked to bsmedberg about this yesterday. Based on his comments in dev.platform[1], we can enable this on all platforms. 1. https://groups.google.com/d/msg/mozilla.dev.platform/v0QAe2olnH0/TivwVmhWOQAJ

Filed Bug 1277359 for Linux Bug 1277361 for Mac

Can I get aurora approval so this doesn't have to be uplifted to beta? Thanks!

Comment on attachment 8754955 [details] [diff] [review] Firefox patch Taking it, thanks

remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/f77d5675d8f8 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/9b770efcfb3f

remote: https://hg.mozilla.org/releases/mozilla-esr45/rev/8b7be062b622 remote: https://hg.mozilla.org/releases/mozilla-esr45/rev/572d40904f85

I was able to run the following successfully on the release branch hg update FIREFOX_47_0_2_RELBRANCH hg graft f77d5675d8f8 hg graft 9b770efcfb3f I'll also build, run tests, and manually verify just to make sure.

I've visually inspected the files and all looks good. I'm having some trouble with the build environment (with and without the grafts so it is unrelated to the patches)... will try to get a build going and run the tests.

I pushed these changes to FIREFOX_47_0_2_RELBRANCH: https://treeherder.mozilla.org/#/jobs?repo=mozilla-release&revision=d7f98b92fdeca9a662206667c68ec41a1476bdb7 remote: https://hg.mozilla.org/releases/mozilla-release/rev/e1e47decdca318da61d55cddc16fc453d180c141 remote: https://hg.mozilla.org/releases/mozilla-release/rev/d7f98b92fdeca9a662206667c68ec41a1476bdb7