618725 - re-nice'ing the plugin process above the chrome and content process

Status: RESOLVED WONTFIX

(Core :: IPC, defect)

Description

[Oleg Romashin (:romaxa)]

Attachment: Renice plugin process

Followup 594121

I found that once started flash plugin while loading page, slow-down main content process loading, because content process has +10, and plugin process 0 nice value

suggesting to renice plugins down to +15

Comment 1

[Doug Turner (:dougt)]

Comment on attachment 497181 [details] [diff] [review]
Renice plugin process

bsmedberg probably should take a look.  The code looks very similar to what we have done for the content process.  I know there was some concern about doing this nice'ing for the plugin process.

Comment 2

[Oleg Romashin (:romaxa)]

Probably it will be safer to do it only on android and Maemo platforms (instead LINUX)
But it might be useful for linux platform too, because I know some complains that plugin process eating lot of CPU on the system...

Comment 3

[Benjamin Smedberg]

Comment on attachment 497181 [details] [diff] [review]
Renice plugin process

We've tested, and for optimum performance the plugin process must be at the same niceness as its parent. For desktop linux this should be 0, and when using a content process it should be 10.

Comment 4

[Oleg Romashin (:romaxa)]

Attachment: Set the same priority as parent

Comment 5

[Benjamin Smedberg]

Don't child processes automatically inherit the priority of their parent? I don't see why this patch is necessary.

Comment 6

[Colin Walters]

(In reply to comment #5)
> Don't child processes automatically inherit the priority of their parent? I
> don't see why this patch is necessary.

Yes.  My guess is that the plugins are either being executed via the parent, or some intermediary is resetting the process priority.

Comment 7

[Benjamin Smedberg]

Comment on attachment 497234 [details] [diff] [review]
Set the same priority as parent

r- until there's a better explanation why this is actually needed

Comment 8

[Oleg Romashin (:romaxa)]

> r- until there's a better explanation why this is actually needed

that is needed for mobile where without this patch we have flash plugin process running with the same priority as UI process...

> Don't child processes automatically inherit the priority of their parent?
no, at least what I've see is that plugin process was returning 0 in getpriority call

Comment 9

[Benjamin Smedberg]

Processes inherit priority by default. I need you to figure out why that isn't working in this case.

Comment 10

[Oleg Romashin (:romaxa)]

(In reply to comment #9)
> Processes inherit priority by default. I need you to figure out why that isn't
> working in this case.

Processes inherit priority by default only for the SCHED_FIFO and SCHED_RR scheduling policies. see
http://linux.die.net/man/3/fork

in our case schedule == SCHED_OTHER, and new process does not inherit parent process priority...

will check also on desktop linux build

Comment 11

[Oleg Romashin (:romaxa)]

Tested on desktop fennec, and see exactly same problem, sched = OTHER, priority not inherited...

IIRC cjones were saying that possibly soon Chrome process will fork plugins-child process and not content... in that case I would suggest return to previous version of patch where we just set for plugin process relative nice value + 10... and that should help also for Desktop firefox

Comment 12

[Colin Walters]

(In reply to comment #10)
> (In reply to comment #9)
> > Processes inherit priority by default. I need you to figure out why that isn't
> > working in this case.
> 
> Processes inherit priority by default only for the SCHED_FIFO and SCHED_RR
> scheduling policies. see
> http://linux.die.net/man/3/fork
> 
> in our case schedule == SCHED_OTHER, and new process does not inherit parent
> process priority...

That's just wrong, according to "man 2 setpriority".  See also "man sched_setscheduler".  Not sure if there's a maemo libc/kernel patch or something involved here.

Comment 13

[Oleg Romashin (:romaxa)]

> That's just wrong, according to "man 2 setpriority".  See also "man
> sched_setscheduler".  Not sure if there's a maemo libc/kernel patch or
> something involved here.

the same on Ubuntu and Debian desktop...

Comment 14

[Oleg Romashin (:romaxa)]

I've checked priority in plugin-container main() , and current priority (plugin child) = 0, parent process priority (content) = 10...
also right before fork process has priority 10.

this is on maemo and on linux desktop

What should I do here?

Comment 15

[Benjamin Smedberg]

jimb, do you have any guru experience about process priority inheritance?

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

My man page and this test program agree with comment 12

#include <stdio.h>
#include <stdlib.h>
#include <sys/time.h>
#include <sys/resource.h>
#include <unistd.h>

void dumpprio(const char* msg) {
    printf("[process %d; priority=%d] %s\n", getpid(), getpriority(PRIO_PROCESS, 0), msg);
}

int main(int argc, char** argv) {
    if (argc > 1) {
        dumpprio("after exec");
        exit(0);
    }
    dumpprio("before setprio()");
    setpriority(PRIO_PROCESS, 0, getpriority(PRIO_PROCESS, 0) + 10);
    dumpprio("after setprio()");
    if (!fork()) {
        dumpprio("after fork()");
        execl(argv[0], argv[0], "donuts", NULL);
    }
    return 0;
}

$ ./testprio
[process 29362; priority=0] before setprio()
[process 29362; priority=10] after setprio()
[process 29363; priority=10] after fork()
[process 29363; priority=10] after exec

Comment 17

[Benjamin Smedberg]

Yeah, `man setpriority` for me says:

NOTES
       A  child created by fork(2) inherits its parent’s nice value.  The nice
       value is preserved across execve(2).

Comment 18

[Oleg Romashin (:romaxa)]

This is output from device:
[process 2245; priority=0] before setprio()
[process 2245; priority=10] after setprio()
[process 2246; priority=0] after fork()
[process 2246; priority=0] after exec

on desktop Debian and in scratchbox output the same as in comment#16

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment 18 shows that the device (I assume Nokia device) does not follow desktop (and 'man' specified) behavior.

So I guess the preprocessor checks in the patch should be limited MOZ_PLATFORM_MAEMO, and perhaps ANDROID if we determine Android acts the same way.

Comment 20

[Oleg Romashin (:romaxa)]

I reported internal maemo bug, will check answer first.

Comment 21

[Jim Blandy :jimb]

(In reply to comment #15)
> jimb, do you have any guru experience about process priority inheritance?

None whatsoever. Apologies. :(

Comment 22

[Jim Blandy :jimb]

(In reply to comment #21)
> (In reply to comment #15)
> > jimb, do you have any guru experience about process priority inheritance?
> 
> None whatsoever. Apologies. :(

Can you use strace to catch some process changing its priority when we're not expecting it to?