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

RESOLVED WONTFIX

Status

()

Core
IPC
RESOLVED WONTFIX
7 years ago
4 months ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

Trunk
ARM
MeeGo
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
Created attachment 497181 [details] [diff] [review]
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
Attachment #497181 - Flags: review?(doug.turner)

Comment 1

7 years ago
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.
Attachment #497181 - Flags: review?(doug.turner) → review?(benjamin)
(Assignee)

Comment 2

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

7 years ago
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.
Attachment #497181 - Flags: review?(benjamin) → review-
(Assignee)

Comment 4

7 years ago
Created attachment 497234 [details] [diff] [review]
Set the same priority as parent
Assignee: nobody → romaxa
Attachment #497181 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #497234 - Flags: review?(benjamin)
(Assignee)

Updated

7 years ago
Blocks: 619056

Comment 5

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

Comment 6

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

7 years ago
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
Attachment #497234 - Flags: review?(benjamin) → review-
(Assignee)

Comment 8

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

7 years ago
Processes inherit priority by default. I need you to figure out why that isn't working in this case.
(Assignee)

Updated

7 years ago
Attachment #497234 - Flags: review- → review?(benjamin)
(Assignee)

Updated

7 years ago
Attachment #497234 - Flags: review?(benjamin) → review-
(Assignee)

Comment 10

7 years ago
(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
(Assignee)

Comment 11

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

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

Comment 13

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

Comment 14

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

7 years ago
jimb, do you have any guru experience about process priority inheritance?
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

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

Comment 18

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

Comment 20

7 years ago
I reported internal maemo bug, will check answer first.

Comment 21

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

None whatsoever. Apologies. :(

Comment 22

7 years ago
(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?

Updated

4 months ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.