Last Comment Bug 672356 - plugin-container not set to background cgroup
: plugin-container not set to background cgroup
Status: RESOLVED FIXED
[inbound]
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: Firefox 8
Assigned To: Brad Lassey [:blassey] (use needinfo?)
:
Mentors:
Depends on:
Blocks: 446418 608440
  Show dependency treegraph
 
Reported: 2011-07-18 14:21 PDT by kevin.t.bowen
Modified: 2011-08-11 12:31 PDT (History)
6 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (2.72 KB, patch)
2011-07-22 01:08 PDT, Brad Lassey [:blassey] (use needinfo?)
doug.turner: review-
Details | Diff | Splinter Review
patch v2 (3.17 KB, patch)
2011-07-23 08:37 PDT, Brad Lassey [:blassey] (use needinfo?)
doug.turner: review-
Details | Diff | Splinter Review
patch (3.33 KB, patch)
2011-07-27 23:26 PDT, Brad Lassey [:blassey] (use needinfo?)
doug.turner: review+
Details | Diff | Splinter Review

Description kevin.t.bowen 2011-07-18 14:21:37 PDT
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.
Comment 1 Kevin Brosnan [:kbrosnan] 2011-07-18 16:46:26 PDT
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
Comment 2 kevin.t.bowen 2011-07-18 17:23:30 PDT
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.
Comment 3 Brad Lassey [:blassey] (use needinfo?) 2011-07-22 01:08:13 PDT
Created attachment 547628 [details] [diff] [review]
patch
Comment 4 Doug Turner (:dougt) 2011-07-22 22:57:01 PDT
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.
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2011-07-23 08:37:20 PDT
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.
Comment 6 Doug Turner (:dougt) 2011-07-25 10:11:36 PDT
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?
Comment 7 Brad Lassey [:blassey] (use needinfo?) 2011-07-25 11:25:53 PDT
(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 (dormant account) 2011-07-25 11:29:54 PDT
I don't see what telemetry could measure here. It makes sense to me to have plugin-container inherit chrome's cgroup
Comment 9 (dormant account) 2011-07-25 11:30:18 PDT
Besides, we don't have telemetry for mobile yet :(
Comment 10 kevin.t.bowen 2011-07-25 14:18:58 PDT
>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.
Comment 11 Brad Lassey [:blassey] (use needinfo?) 2011-07-25 20:27:49 PDT
I'm not seeing any difference in the cpu time reported by top with and without this patch
Comment 12 Doug Turner (:dougt) 2011-07-27 10:54:05 PDT
Hi Kevin,

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

Doug
Comment 13 kevin.t.bowen 2011-07-27 11:04:31 PDT
Yes, please send me the builds and I'll test them and get back to you. thanks!
Comment 14 Doug Turner (:dougt) 2011-07-27 13:41:36 PDT
builds sent to kevin for analysis.
Comment 15 kevin.t.bowen 2011-07-27 15:24:36 PDT
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 16 Doug Turner (:dougt) 2011-07-27 16:16:45 PDT
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
Comment 17 (dormant account) 2011-07-27 16:25:34 PDT
(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 :)
Comment 18 Doug Turner (:dougt) 2011-07-27 16:35:57 PDT
taras, sounds like you are signing up to fix? ;)
Comment 19 (dormant account) 2011-07-27 18:03:08 PDT
(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
Comment 20 Brad Lassey [:blassey] (use needinfo?) 2011-07-27 20:05:47 PDT
(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
Comment 21 Doug Turner (:dougt) 2011-07-27 20:26:21 PDT
> 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.
Comment 22 Doug Turner (:dougt) 2011-07-27 20:31:18 PDT
filed bug 674808 to clean up the enumerator.
Comment 23 Brad Lassey [:blassey] (use needinfo?) 2011-07-27 23:26:00 PDT
Created attachment 549028 [details] [diff] [review]
patch
Comment 24 Doug Turner (:dougt) 2011-07-27 23:29:05 PDT
Comment on attachment 549028 [details] [diff] [review]
patch

Review of attachment 549028 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 25 Brad Lassey [:blassey] (use needinfo?) 2011-08-01 19:55:06 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/f43cd1c730f8
Comment 26 Marco Bonardo [::mak] 2011-08-02 03:44:55 PDT
http://hg.mozilla.org/mozilla-central/rev/f43cd1c730f8

Note You need to log in before you can comment on or make changes to this bug.