[meta] Performance settings section

NEW
Unassigned

Status

()

Firefox
Preferences
21 days ago
20 hours ago

People

(Reporter: evanxd, Unassigned)

Tracking

(Depends on: 5 bugs, Blocks: 1 bug, {meta})

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

Firefox Tracking Flags

(firefox55 affected)

Details

(Whiteboard: [photon-preference])

Attachments

(1 attachment)

(Reporter)

Description

21 days 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

21 days 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

21 days 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

21 days 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

21 days 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.
(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

18 days 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.
* It would be very helpful *to know*

Comment 9

17 days ago
(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

15 days 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

15 days 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

15 days 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

Comment 14

14 days ago
> 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)

Updated

14 days ago
Blocks: 1356269
Whiteboard: [photon-preference]

Comment 16

14 days ago
(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
status-firefox55: --- → affected

Updated

14 days ago
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

10 days ago
Depends on: 1357348
(Reporter)

Updated

10 days ago
Depends on: 1357349
(Reporter)

Comment 19

10 days ago
Unassigned myself from this meta bug. I've taken the engineering bugs.
Assignee: evan → nobody
(Reporter)

Updated

9 days ago
Depends on: 1357316
(Reporter)

Updated

9 days ago
Summary: [meta] Performance Settings section → [meta] Performance settings section

Updated

4 days ago
Target Milestone: --- → Firefox 55
(Reporter)

Updated

2 days 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)
Created attachment 8862289 [details]
Performance UX spec

Sorry, the link I gave was broken. Please use this new link instead.
You need to log in before you can comment on or make changes to this bug.