plugin-container not set to background cgroup

RESOLVED FIXED in Firefox 8

Status

Fennec Graveyard
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: kevin.t.bowen, Assigned: blassey)

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 8
All
Android
Dependency tree / graph

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build ID: 20110602121842

Steps to reproduce:

Android uses the linux cgroups feature to constrain background apps' cpu usage. Normally, when a user app is moved to the background, its process is automatically moved to the bg_non_interactive cgroup. This happens correctly for the main fennec process, but the plugin-container process remains in the foreground cgroup throughout its entire lifecyle, regardless of the app's visibility status. This allows it to make unconstrained use of the cpu while running in the background, degrading system performance.
(Reporter)

Updated

6 years ago
OS: Other → Android

Updated

6 years ago
Assignee: nobody → doug.turner
Status: UNCONFIRMED → NEW
Ever confirmed: true
Doug says this may be intentional.

# ps
app_89    16719 1212  283124 65664 ffffffff afd0eb68 S org.mozilla.fennec
app_89    16738 16719 77604  25448 ffffffff afd0eb68 S /data/data/org.mozilla.fennec/plugin-container

# cat cgroups.procs
1302
1455
1475
7856
12634
13085
13436
13464
16596
16649
16652
16661
16671
16678
16686
16696
16719
Assignee: doug.turner → nobody
Status: NEW → UNCONFIRMED
Ever confirmed: false
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

6 years ago
I'm guessing that by "intentional" you just mean that some manual hacking was necessary in order to force plugin-container into the foreground cgroup in the first place, which is understandable. But leaving it there, even when the app is backgrounded, can have severe performance consequences. Even with only a single tab open to a well-behaved web page, it results in a small, but noticeable degradation of the device's performance for as long as the process remains running... if you happen to have loaded a page which contains poorly-written javascript, it can slow your phone's performance to a crawl.
Blocks: 446418
Version: Firefox 7 → Trunk
Created attachment 547628 [details] [diff] [review]
patch
Assignee: nobody → blassey.bugs
Attachment #547628 - Flags: review?(doug.turner)
(Assignee)

Updated

6 years ago
tracking-fennec: --- → ?

Comment 4

6 years ago
Comment on attachment 547628 [details] [diff] [review]
patch

Since we are outside the kernel, i really don't want to be poking cpuctl if we don't have to.  We have no guarantee that these control groups wont change in the future or if they are supported on every device that Android runs on. 


Is there a safer way of changing cgroups?  Is nicing might a safer, more portable, solution?


Also, moving to clone() from exec() will allow our child process to inherit any cgroup we are in.
Attachment #547628 - Flags: review?(doug.turner) → review-
Created attachment 547934 [details] [diff] [review]
patch v2

(In reply to comment #4)
> Comment on attachment 547628 [details] [diff] [review] [review]
> patch
> 
> Since we are outside the kernel, i really don't want to be poking cpuctl if
> we don't have to.  We have no guarantee that these control groups wont
> change in the future or if they are supported on every device that Android
> runs on. 
true, this patch future proofs us by getting the cgroup from the chrome process and setting the content process to it

> 
> 
> Is there a safer way of changing cgroups?  Is nicing might a safer, more
> portable, solution?
As far as I know, this is the way you're supposed to set cgroups
> 
> 
> Also, moving to clone() from exec() will allow our child process to inherit
> any cgroup we are in.
clone() will simply copy the cgroup at the time we create the content process (which is the root cgroup) and wouldn't change this situation as far as I can tell.
Attachment #547628 - Attachment is obsolete: true
Attachment #547934 - Flags: review?(doug.turner)

Comment 6

6 years ago
Comment on attachment 547934 [details] [diff] [review]
patch v2

What data can we collect to show that this patch makes a difference?  Alon and I were discussing measuring CPU usage when Fennec is in the background.  Assume that you have a content page that does consume lots of CPU.  Now, if we put fennec in the background, with this patch, would we be able to detect a significant improvement in CPU usage?
(In reply to comment #6)
> Comment on attachment 547934 [details] [diff] [review] [review]
> patch v2
> 
> What data can we collect to show that this patch makes a difference?  Alon
> and I were discussing measuring CPU usage when Fennec is in the background. 
> Assume that you have a content page that does consume lots of CPU.  Now, if
> we put fennec in the background, with this patch, would we be able to detect
> a significant improvement in CPU usage?

I suppose its possible to collect some data about that with telemetry, maybe Taras can tell us more specifically. The idea here is just to put the content process in the same cgroup as the chrome process so that it has the same policy constraints as the chrome process.

I don't know that we consume a lot of cpu cycles while in the background, however I have personally gotten in the habit of killing firefox when I'm not using it anymore in order to protect my battery life (real or imagined). I see this as just being a good citizen on the OS and using the tools that Android is giving us to make sure we don't consume more battery than we intend to in the background.

Comment 8

6 years ago
I don't see what telemetry could measure here. It makes sense to me to have plugin-container inherit chrome's cgroup

Comment 9

6 years ago
Besides, we don't have telemetry for mobile yet :(
(Assignee)

Updated

6 years ago
Whiteboard: [needs-review (dougt)]
(Reporter)

Comment 10

6 years ago
>with this patch, would we be able to detect a significant improvement in CPU >usage?

Sorry to butt in here, but I just thought I'd point out that it should be pretty trivial to measure the difference, just with 'top' or any of the various system monitor apps available for android... 

The background cgroup has a hard cap at 5% cpu use (not per-process, 5% combined for all background processes)... in real world use on my device (without this patch), the content process will typically seem to hover around 1-10%, with occasional spikes into 20+%. That's just with typical, non-pathological pages loaded - with a javascript heavy page loaded, it can completely peg the cpu. My point is just that whatever testing methodology you use, the difference should be big enough to be easily measurable.
I'm not seeing any difference in the cpu time reported by top with and without this patch
Blocks: 608440
Hi Kevin,

Are you able to verify blassey's data?  I am happy to send you two builds (with and without this patch).

Doug
(Reporter)

Comment 13

6 years ago
Yes, please send me the builds and I'll test them and get back to you. thanks!
builds sent to kevin for analysis.
(Reporter)

Comment 15

6 years ago
I'll copy my reply to Doug into here for anyone else who might be interested:

> So I'm not sure if you were intending this to be a blind test by
> naming the builds 'a' and 'b' - if so, then 'b' is the one with the
> patch... of course, I could have easily cheated the test by just
> looking at the cgroups, but I swear I didn't ;).
> 
> So it turns out cgroups don't work quite the way I thought they did -
> I was under the impression that giving a group a 5% share gave it a
> hard cap at 5% cpu time, but apparently it's more of a soft cap - it
> can use more cpu than that as long as there's idle cycles - so it only
> ends up getting capped when there are other processes in contention
> for cpu time. That makes it a bit harder than I expected to test, but
> doesn't make the patch useless.... here's how I tested:
> 
> -start 'top' running via adb shell
> -open fennec, and start a kraken benchmark running in it
> -hit the home button, then launch and run an android system benchmark
> app (I used qualcomm vellamo)
> 
> Without the patch, the two benchmarks get a roughly 50/50 split of cpu
> time.... with the patch, fennec stays at <5%, with the foreground
> benchmark getting 90+%. So it's definitely working.
> 
> In real world use, it doesn't make quite as huge a difference as I had
> hoped it might, but it's a step in the right direction, and it is,
> after all, how apps are "supposed" to work on Android, so I don't see
> any reason not to merge it.
Comment on attachment 547934 [details] [diff] [review]
patch v2

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

close.  lets see another patch.

::: embedding/android/GeckoAppShell.java
@@ +1166,5 @@
>      }
>  
> +    public static void putChildInBackground() {
> +        try {
> +        BufferedReader br = new BufferedReader(new FileReader(new File(new File(new File("/proc"), new Integer( android.os.Process.myPid()).toString()), "cgroup")));

Do we really need to create 3 new File objects here?  Does something like:

new File("/proc" + android.os.Process.myPid() + "/cgroup")

work?


Also, you should close the BufferedReader

@@ +1171,5 @@
> +        String[] cpuLine = br.readLine().split("/");
> +        final String backgroundGroup = cpuLine.length == 2 ? cpuLine[1] : "";
> +        GeckoProcessesVisitor visitor = new GeckoProcessesVisitor() {
> +            public boolean callback(int pid) {
> +                if (pid != android.os.Process.myPid()) {

you could change the EnumerateGeckoProcesses() method to take a "ignoreCurrentProcess" flag.  This will avoid extra calls to android.os.Process.myPid().

@@ +1172,5 @@
> +        final String backgroundGroup = cpuLine.length == 2 ? cpuLine[1] : "";
> +        GeckoProcessesVisitor visitor = new GeckoProcessesVisitor() {
> +            public boolean callback(int pid) {
> +                if (pid != android.os.Process.myPid()) {
> +                    try {

at some point, we should change the implementation of EnumerateGeckoProcesses.  We already know which processes are geckos (since we exec them) and having to read ps output kinda sucks.

@@ +1173,5 @@
> +        GeckoProcessesVisitor visitor = new GeckoProcessesVisitor() {
> +            public boolean callback(int pid) {
> +                if (pid != android.os.Process.myPid()) {
> +                    try {
> +                        new FileOutputStream(new File(new File(new File("/dev/cpuctl"), 

You need to close the stream.

@@ +1174,5 @@
> +            public boolean callback(int pid) {
> +                if (pid != android.os.Process.myPid()) {
> +                    try {
> +                        new FileOutputStream(new File(new File(new File("/dev/cpuctl"), 
> +                                                               backgroundGroup), "tasks")).

new File("/dev/cpuctl/" + backgroundGroup + "/tasks")

@@ +1176,5 @@
> +                    try {
> +                        new FileOutputStream(new File(new File(new File("/dev/cpuctl"), 
> +                                                               backgroundGroup), "tasks")).
> +                            write(new Integer(pid).toString().getBytes());
> +                    }catch(Exception e) {

space after }

@@ +1203,5 @@
> +                }
> +                return true;
> +            }
> +        };
> +            

kill the extra space
Attachment #547934 - Flags: review?(doug.turner) → review-

Comment 17

6 years ago
(In reply to comment #16)

> at some point, we should change the implementation of
> EnumerateGeckoProcesses.  We already know which processes are geckos (since
> we exec them) and having to read ps output kinda sucks.

Btw, the correct linux way to do is is to read all of /proc/*/stat and figure out your descendants by looking at their parent pids :)
taras, sounds like you are signing up to fix? ;)

Updated

6 years ago
Whiteboard: [needs-review (dougt)]

Comment 19

6 years ago
(In reply to comment #18)
> taras, sounds like you are signing up to fix? ;)

no way, i'm just commenting on the whimsical linux api
(In reply to comment #16)
> Comment on attachment 547934 [details] [diff] [review] [review]
> patch v2
> 
> Review of attachment 547934 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> close.  lets see another patch.
> 
> ::: embedding/android/GeckoAppShell.java
> @@ +1166,5 @@
> >      }
> >  
> > +    public static void putChildInBackground() {
> > +        try {
> > +        BufferedReader br = new BufferedReader(new FileReader(new File(new File(new File("/proc"), new Integer( android.os.Process.myPid()).toString()), "cgroup")));
> 
> Do we really need to create 3 new File objects here?  Does something like:
> 
> new File("/proc" + android.os.Process.myPid() + "/cgroup")
> 
> work?
sure, should work. But I prefer not to construct the path through string manipulation if there's another api for it.

> 
> @@ +1171,5 @@
> > +        String[] cpuLine = br.readLine().split("/");
> > +        final String backgroundGroup = cpuLine.length == 2 ? cpuLine[1] : "";
> > +        GeckoProcessesVisitor visitor = new GeckoProcessesVisitor() {
> > +            public boolean callback(int pid) {
> > +                if (pid != android.os.Process.myPid()) {
> 
> you could change the EnumerateGeckoProcesses() method to take a
> "ignoreCurrentProcess" flag.  This will avoid extra calls to
> android.os.Process.myPid().
sure, different bug though (we do this in other calls). Please file it.
> 
> @@ +1172,5 @@
> > +        final String backgroundGroup = cpuLine.length == 2 ? cpuLine[1] : "";
> > +        GeckoProcessesVisitor visitor = new GeckoProcessesVisitor() {
> > +            public boolean callback(int pid) {
> > +                if (pid != android.os.Process.myPid()) {
> > +                    try {
> 
> at some point, we should change the implementation of
> EnumerateGeckoProcesses.  We already know which processes are geckos (since
> we exec them) and having to read ps output kinda sucks.
Perhaps the same different bug as above.

> @@ +1174,5 @@
> > +            public boolean callback(int pid) {
> > +                if (pid != android.os.Process.myPid()) {
> > +                    try {
> > +                        new FileOutputStream(new File(new File(new File("/dev/cpuctl"), 
> > +                                                               backgroundGroup), "tasks")).
> 
> new File("/dev/cpuctl/" + backgroundGroup + "/tasks")
see above
> sure, should work. But I prefer not to construct the path
> through string manipulation if there's another api for it.

By that reasoning you should change new File("/proc") to new File(new File("/"), "proc").

Come on, it is *alot* easier to read if you use string manipulation to construct your path here.
filed bug 674808 to clean up the enumerator.
Created attachment 549028 [details] [diff] [review]
patch
Attachment #547934 - Attachment is obsolete: true
Attachment #549028 - Flags: review?(doug.turner)
Comment on attachment 549028 [details] [diff] [review]
patch

Review of attachment 549028 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #549028 - Flags: review?(doug.turner) → review+
(Assignee)

Updated

6 years ago
tracking-fennec: ? → 8+
http://hg.mozilla.org/integration/mozilla-inbound/rev/f43cd1c730f8
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/f43cd1c730f8
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8
(Assignee)

Updated

6 years ago
status-firefox8: --- → fixed
You need to log in before you can comment on or make changes to this bug.