[meta] Performance settings section

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: evanxd, Unassigned)

Tracking

(Blocks 1 bug, {meta})

unspecified
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [photon-preference])

Attachments

(1 attachment)

Reporter

Description

2 years ago
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
Reporter

Comment 1

2 years ago
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.
Flags: needinfo?(thsieh)
Reporter

Comment 2

2 years ago
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
Reporter

Comment 3

2 years ago
(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.
Reporter

Comment 4

2 years ago
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

2 years ago
(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.

Updated

2 years ago
Depends on: 1352069
(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?
Flags: needinfo?(mrbkap)
(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.
(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

2 years ago
(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.
(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.
Reporter

Comment 12

2 years ago
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
Reporter

Comment 13

2 years ago
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
> On Thu, Apr 13, 2017 at 4:42 PM, Gabor Krizsanits <gkrizsanits@mozilla.com> wrote:
> 
> Hi,
> 
>>    On Thu, Apr 13, 2017 at 6:09 AM, Fischer Liu <fliu@mozilla.com> 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.
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?
Flags: needinfo?(mrbkap)
Whiteboard: [photon-preference]
(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?
Flags: needinfo?(mrbkap)
Evan will work on this.
Assignee: nobody → evan
Flags: qe-verify-
Keywords: meta
(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.
Flags: needinfo?(mrbkap)
Reporter

Updated

2 years ago
Depends on: 1357348
Reporter

Updated

2 years ago
Depends on: 1357349
Reporter

Comment 19

2 years ago
Unassigned myself from this meta bug. I've taken the engineering bugs.
Assignee: evan → nobody
Reporter

Updated

2 years ago
Depends on: 1357316
Reporter

Updated

2 years ago
Summary: [meta] Performance Settings section → [meta] Performance settings section
Target Milestone: --- → Firefox 55
Reporter

Updated

2 years ago
Depends on: 1359735
Hi Evan,
Please see the spec of Performance section :)
https://mozilla.invisionapp.com/share/65BDUBWA8#/228017152_1-0_Cover_-_Home_-
Flags: needinfo?(thsieh)
Posted file Performance UX spec
Sorry, the link I gave was broken. Please use this new link instead.
Duplicate of this bug: 1329342
Reporter

Updated

2 years ago
Depends on: 1365210
Reporter

Updated

2 years ago
Depends on: 1367350
Reporter

Updated

2 years ago
Depends on: 1367959
No longer depends on: 1357349
No longer depends on: 1357316
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?
Flags: needinfo?(timdream)
No. We weren't going to do anything like that. The feature was simply a UI that exposes the pref.
Flags: needinfo?(timdream)
Close meta bug since all dependent bugs were completed.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.