1354473 - [meta] Performance settings section

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement)

Description

[Evan Tseng [:evanxd]]

We would like to add the Performance Settings section into Preferences. This is the draft UX spec[1].

[1]: https://mozilla.invisionapp.com/share/BWADX1YR4#/screens/218291590

Comment 1

[Evan Tseng [:evanxd]]

Hi Tina,

Could you post the design spec in this Bug once we have it? This is our meta bug for the Performance Settings in Preferences.

Thanks.

Comment 2

[Evan Tseng [:evanxd]]

We can configure the page fetching with the `network.prefetch-next` about:config flag [1] and the content process count with the `dom.ipc.processCount.*` about:config flag [2] (Thanks Fischer).

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1329342#c10
[2]: https://bugzilla.mozilla.org/show_bug.cgi?id=1329342#c12

Comment 3

[Evan Tseng [:evanxd]]

(In reply to Evan Tseng [:evanxd] from comment #1)
> Hi Tina,
> 
> Could you post the design spec in this Bug once we have it? This is our meta
> bug for the Performance Settings in Preferences.
> 
> Thanks.

Just a reminder for us. We will also send our design spec to Preference code module owners or reviewers (Jared and Gijs) and get the feedback from them.

Comment 4

[Evan Tseng [:evanxd]]

Currently, we have few about:config flags about animation, which are
- browser.download.animateNotifications
- browser.fullscreen.animate
- browser.tabs.animate
- browser.panorama.animate_zoom
- browser.preferences.animateFadeIn
- dom.animations-api.element-animate.enabled
- layout.animated-image-layers.enabled

We also need to figure out what items are about "Use UI animations" in the design spec.

Comment 5

[:Gijs (he/him)]

(In reply to Evan Tseng [:evanxd] from comment #4)
> Currently, we have few about:config flags about animation, which are
> - browser.panorama.animate_zoom

This doesn't exist anymore. Also, see bug 1352069 for discussion about having a single pref for this.

Comment 6

[Justin Dolske [:Dolske]]

(In reply to Evan Tseng [:evanxd] from comment #4)

> - dom.animations-api.element-animate.enabled
> - layout.animated-image-layers.enabled

These two prefs have more to do with platform features than UI animations, and so shouldn't be part of this page. (But this is a moot point if this ends up just inheriting the single new pref from bug 1352069).


Also, it would be good to have someone from platform weigh in on if exposing the number of content processes to the user is a good idea or not. The classic problem with this kind of thing is that users often can't make well-educated decisions about the tradeoffs involved, and so they're just as likely to make things worse than better. [But I'm glad the the UX mock does at least try to explain things.] Also, I thought I saw some discussion that > 4(?) processes starts to get bad, so I'm not sure if the upper limit here is too high.

Blake - you have any insight/opinion here?

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Justin Dolske [:Dolske] from comment #6)
> Also, it would be good to have someone from platform weigh in on if exposing
> the number of content processes to the user is a good idea or not. The
> classic problem with this kind of thing is that users often can't make
> well-educated decisions about the tradeoffs involved, and so they're just as
> likely to make things worse than better. [But I'm glad the the UX mock does
> at least try to explain things.] Also, I thought I saw some discussion that
> > 4(?) processes starts to get bad, so I'm not sure if the upper limit here
> is too high.
> 
> Blake - you have any insight/opinion here?

Thanks :Dolske, we really need this input from Gecko engineering. Tina's WIP call for either a slider or a input[number] allowing user adjust the count. If we need to put an upper limit here it's definitely possible (and, user who want to overcome that can continue to abuse Firefox through about:config). It would be very helpful if we need a upper limit on the UI & the upper limit to set.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

* It would be very helpful *to know*

Comment 9

[Fischer [:Fischer]]

(In reply to :Gijs from comment #5)
> (In reply to Evan Tseng [:evanxd] from comment #4)
> > Currently, we have few about:config flags about animation, which are
> > - browser.panorama.animate_zoom
> 
> This doesn't exist anymore. Also, see bug 1352069 for discussion about
> having a single pref for this.
Thanks for the info.
As the comment from Jim in Bug 1352069[1], the master UI animation pref is expecting to have a patch early this week for review, probably will be landed in this or the next week.
We can have that master UI animation pref in Performance section.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1352069#c7

Comment 10

[Greg]

(In reply to Justin Dolske [:Dolske] from comment #6)
> Also, it would be good to have someone from platform weigh in on if exposing
> the number of content processes to the user is a good idea or not. The
> classic problem with this kind of thing is that users often can't make
> well-educated decisions about the tradeoffs involved, and so they're just as
> likely to make things worse than better. [But I'm glad the the UX mock does
> at least try to explain things.] Also, I thought I saw some discussion that
> > 4(?) processes starts to get bad, so I'm not sure if the upper limit here
> is too high.
> 

I'm looking to get the same speed and responsiveness in Firefox that Google Chrome offers right now. With the idea that this will eventually be possible in Firefox, how about listing a few options: 2 processes | 10 processes | 1-process-per-tab

User picks which he/she wants. I will be picking 1-process-per-tab to get Google Chrome responsiveness. I've got plenty of RAM and don't care if lots of RAM is used. I'd rather get more responsiveness (no lag) than keep RAM usage down.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Greg from comment #10)
> User picks which he/she wants. I will be picking 1-process-per-tab to get
> Google Chrome responsiveness. I've got plenty of RAM and don't care if lots
> of RAM is used. I'd rather get more responsiveness (no lag) than keep RAM
> usage down.

You can already do that today with about:config, after dismiss the warning. The intention of a pref option is to make most power user aware of the option and play with a few relatively harmless choices. 1-process-per-tab* shouldn't be offered without a lengthy and wordy description, thus are excluded from the current UI proposal.

* 1-process-per-tab has been a myth; the precise description is one-process-per-connected-origin.

Comment 12

[Evan Tseng [:evanxd]]

Let's make a conclusion for Comment 4,

Based on Comment 5 and Comment 6, we will configure the below about:config flags when user toggle "Use UI animations" in the spec[1]
- browser.download.animateNotifications
- browser.fullscreen.animate
- browser.tabs.animate
- browser.preferences.animateFadeIn

And after traced and investigated code, we can make the above animation flags validated WITHOUT restarting Firefox.

[1]: https://mozilla.invisionapp.com/share/BWADX1YR4#/screens/218291590

Comment 13

[Evan Tseng [:evanxd]]

After investigated the `dom.ipc.processCount` about:config flag and traced code, we don't need to restart Firefox to validate a new `dom.ipc.processCount` setting. Gecko reads the flag in runtime (source code [1]).

[1]: http://searchfox.org/mozilla-central/source/dom/ipc/ContentParent.cpp#713-735

Comment 14

[Fischer [:Fischer]]

> On Thu, Apr 13, 2017 at 4:42 PM, Gabor Krizsanits <[e-mail]> wrote:
> 
> Hi,
> 
>>    On Thu, Apr 13, 2017 at 6:09 AM, Fischer Liu <[e-mail]> wrote:
>> 
>>    We are using dom.ipc.processCount to limit the count of process.
>>    After updating dom.ipc.processCount, do we still need to restart Firefox?
> 
> It depends. It will prevent any new processes to be launched that would go beyond the new limit right away, but it will not shut down any existing processes if the current number is above the limit. For that you must either close tabs or restart the browser.
>     
> 
>>    As I know in PreallocatedProcessManagerImpl::AllocateNow[2] and
>>    PreallocatedProcessManagerImpl::RereadPrefs, there would read the pref to
>>    check the max count to allow creation or close process.
>> 
>>    [1] https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/dom/ipc/PreallocatedProcessManager.cpp#143
>>    [2]https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/dom/ipc/PreallocatedProcessManager.cpp#200
> 
> The preallocated process manager is not enabled yet on any channels by default, I'm still struggling with some failing tests and other issues >with it. Hopefully that'll be fixed soon (Bug 1341008), I wish I had more time for it... Once it's turned on, it will detect the change of the pref and shut down the preallocated process if necessary just like you described.
>  
After checking with the e10s team member, Gabor, until the preallocated process manager is done when decreasing the dom.ipc.processCount it would still require to close tab or restart firefox to close any existing process.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

It looks like the bits of this bug that deal with e10s multi need to be coordinated with bug 1329342. 

The thing about the 4 process "limit" is that's where our overall memory usage passes Chrome for a given workload. So, up to 4 processes, we won't regress memory usage past them and after that we will. So, it's not a "hard" limit per se. Also, our testing and bugfixing right now is all geared toward getting 4 content processes out the door. With more processes, it's possible that we'll hit either new bugs or more races. How much QA are we going to spend testing and verifying that the experience with more processes is up to snuff?

Comment 16

[Fischer [:Fischer]]

(In reply to Blake Kaplan (:mrbkap) from comment #15)
> It looks like the bits of this bug that deal with e10s multi need to be
> coordinated with bug 1329342. 
> 
> The thing about the 4 process "limit" is that's where our overall memory
> usage passes Chrome for a given workload. So, up to 4 processes, we won't
> regress memory usage past them and after that we will. So, it's not a "hard" limit per se.
So does this with 5 or more processes, under a given workload, there could see we are using more memory than chrome?

> Also, our testing and bugfixing right now is all geared toward
> getting 4 content processes out the door. With more processes, it's possible
> that we'll hit either new bugs or more races. How much QA are we going to
> spend testing and verifying that the experience with more processes is up to
> snuff?
In Bug 1329342, it is having some intention to expose e10s configs.
So is it that having dom.ipc.processCount able to be set to 5 or more(up to 7) would not be recommended since likely to hit some new bugs and not enough tests around that now?

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Evan will work on this.

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Fischer [:Fischer] from comment #16)
> (In reply to Blake Kaplan (:mrbkap) from comment #15)
> > It looks like the bits of this bug that deal with e10s multi need to be
> > coordinated with bug 1329342. 
> > 
> > The thing about the 4 process "limit" is that's where our overall memory
> > usage passes Chrome for a given workload. So, up to 4 processes, we won't
> > regress memory usage past them and after that we will. So, it's not a "hard" limit per se.
> So does this with 5 or more processes, under a given workload, there could
> see we are using more memory than chrome?

The same question was asked elsewhere and this is what's given (and proof that we can safely expose ~8 process config to users):
http://www.erahm.org/2017/03/10/are-they-slim-yet-round-2/
 
> > Also, our testing and bugfixing right now is all geared toward
> > getting 4 content processes out the door. With more processes, it's possible
> > that we'll hit either new bugs or more races. How much QA are we going to
> > spend testing and verifying that the experience with more processes is up to
> > snuff?
> In Bug 1329342, it is having some intention to expose e10s configs.
> So is it that having dom.ipc.processCount able to be set to 5 or more(up to
> 7) would not be recommended since likely to hit some new bugs and not enough
> tests around that now?

It probably doesn't make sense to ask engineers to confirm "there will be no more bugs" :). But yeah, whether or not the risk is manageable is a legit question. I will raise this to the larger group on the mailing list.

Comment 19

[Evan Tseng [:evanxd]]

Unassigned myself from this meta bug. I've taken the engineering bugs.

Comment 20

[Tina Hsieh[:Tina_Hsieh] UX]

Hi Evan,
Please see the spec of Performance section :)
https://mozilla.invisionapp.com/share/65BDUBWA8#/228017152_1-0_Cover_-_Home_-

Comment 21

[Tina Hsieh[:Tina_Hsieh] UX]

Attachment: Performance UX spec

Sorry, the link I gave was broken. Please use this new link instead.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Hey timdream,

Is the "recommended performance settings" supposed to throttle back dom.ipc.processCount based on how much RAM the user has? I ask, because I've got an about:support from a user with dom.ipc.processCount set to 4, and they've only got 1GB of RAM to work with. Was throttling back content processes based on RAM something we were going to do?

Comment 24

[Tim Guan-tin Chien [:timdream] (please needinfo)]

No. We weren't going to do anything like that. The feature was simply a UI that exposes the pref.

Comment 25

[Ricky Chien [:rickychien] (inactive)]

Close meta bug since all dependent bugs were completed.