Closed Bug 879529 Opened 8 years ago Closed 8 years ago

Non-main thread priorities in child processes are completely wrong

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set
normal

Tracking

(blocking-b2g:leo+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

RESOLVED FIXED
1.1 QE3 (26jun)
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

(Keywords: perf, Whiteboard: c= , [YouTubeCertBlocker+])

Attachments

(1 file)

> +    int newtaskpriority =
> +      std::max(origtaskpriority + aNice - origProcPriority, origProcPriority);

The second argument of the max should be the new process priority, not the original process priority.

As it is, this prevents us from increasing the priority of threads.  For example, if we're changing a process from bg to fg, the origProcPriority might be 18.  This means that the new thread priority can't be greater than 18!
blocking-b2g: --- → leo?
See bug 874165 comment 27 for an example of this happening in the wild and having user-visible effects.
Attached patch Patch, v1Splinter Review
Ouch.
Attachment #758256 - Flags: review?(gsvelto)
We need to fix b2g-procrank so that it can show this data, so we're not totally in the dark anymore.
Blocks: 877024
Whiteboard: [YouTubeCertBlocker]
Whiteboard: [YouTubeCertBlocker] → [YouTubeCertBlocker+]
Blocks: 874165
(In reply to Justin Lebar [:jlebar] from comment #2)
> Ouch.

My fault, I was the reviewer, I should have spotted this :)

> We need to fix b2g-procrank so that it can show this data, so we're not
> totally in the dark anymore.

Can you open a bug for this and CC me?
Attachment #758256 - Flags: review?(gsvelto) → review+
> Can you open a bug for this and CC me?

Basically, it's bug 864597.

> My fault, I was the reviewer, I should have spotted this :)

As I recall, you wrote the original code, then I reviewed it, then you reviewed it, then I pushed it.  So we both share some blame here.  :)
(In reply to Justin Lebar [:jlebar] from comment #5)
> Basically, it's bug 864597.

Thanks, I've cc'd that bug.

> As I recall, you wrote the original code, then I reviewed it, then you
> reviewed it, then I pushed it.  So we both share some blame here.  :)

Fair enough :) I haven't looked at your priority unit-tests but I will soon enough because it would be good to cover this stuff (and possibly things like bug 841563).

BTW I had a chat with dhylands about I/O priorities which is something we might want to add to the ProcessPriorityManager (to prevent BG applications from freezing the phone with I/O and stuff). That might give me more chances to introduce obscure bugs ;)
(In reply to Gabriele Svelto [:gsvelto] from comment #7)
> (and possibly things like bug 841563).

Wrong bug, I meant bug 873284.
(In reply to Justin Lebar [:jlebar] from comment #3)
> We need to fix b2g-procrank so that it can show this data, so we're not
> totally in the dark anymore.

Hi Justin,

Maybe you can try b2g-ps from gonk-misc "master" branch. I think "b2g-ps -t -P -p" may give you some of the data you want.
>$ adb shell b2g-ps -t -P -p
>APPLICATION      USER     PID   PPID  VSIZE  RSS   PRIO  NICE  RTPRI SCHED  PCY  WCHAN    PC         NAME
>b2g              root      144   112   175172 67316 20    0     0     0     fg  ffffffff 40086430 S /system/b2g/b2g
>b2g              root      249   144   175172 67316 20    0     0     0     fg  c00f89ec 400857ec S b2g
>b2g              root      258   144   175172 67316 20    0     0     0     fg  c0176438 ffff0520 S b2g
>b2g              root      274   144   175172 67316 20    0     0     0     fg  c0104d84 40084678 S b2g
>b2g              root      322   144   175172 67316 20    0     0     0     fg  c03bdd38 400848b0 S b2g
>b2g              root      492   144   175172 67316 20    0     0     0     fg  c00f89ec 400857ec S b2g
>Usage            app_376   376   144   65692  26820 38    18    0     0     fg  ffffffff 400c7430 S /system/b2g/plugin-container
>Homescreen       app_377   377   144   69380  27676 38    18    0     0     fg  ffffffff 40045430 S /system/b2g/plugin-container
>Gallery          app_406   406   144   74036  29144 21    1     0     0     fg  ffffffff 400f5430 S /system/b2g/plugin-container
>E-Mail           app_540   540   144   77668  27684 38    18    0     0     fg  ffffffff 40018430 S /system/b2g/plugin-container
>(Preallocated a  root      566   144   60020  20460 38    18    0     0     fg  ffffffff 400c9430 S /system/b2g/plugin-container

This doesn't seem to list threads for anything other than the root b2g process.
This is probably because the script only shows entries which for
/system/b2g/b2g, or whose PPID is 144.

You're welcome to write a patch to fix this; doing so by only using the shell
commands available in the Android toolbox is too hard for me.  :)
(In reply to Justin Lebar [:jlebar] from comment #10)
> 
> This doesn't seem to list threads for anything other than the root b2g
> process.
> This is probably because the script only shows entries which for
> /system/b2g/b2g, or whose PPID is 144.
> 
> You're welcome to write a patch to fix this; doing so by only using the shell
> commands available in the Android toolbox is too hard for me.  :)

Hmm, that's weird. I remember Dave fixed "-t" option in master branch (only) of gonk-misc. It should be below commit:
https://github.com/mozilla-b2g/gonk-misc/commit/c94522947c4362fd3bb55123d0dd1f9887c26c2e

I have no device in hand now, so I will check it again tomorrow.
https://hg.mozilla.org/mozilla-central/rev/de1f999797ab
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
This patch seems to provide a ~7% improvement on sunspider, and 3.5% on kraken test when ran in b2g browser:
  http://www.arewefastyet.com/#machine=14
(In reply to Alan Huang [:ahuang] from comment #11)
> (In reply to Justin Lebar [:jlebar] from comment #10)
> > 
> > This doesn't seem to list threads for anything other than the root b2g
> > process.
> > This is probably because the script only shows entries which for
> > /system/b2g/b2g, or whose PPID is 144.
> > 
> > You're welcome to write a patch to fix this; doing so by only using the shell
> > commands available in the Android toolbox is too hard for me.  :)

I use master branch gonk-misc. I got below output:

$ adb shell b2g-ps -t -P -p
APPLICATION      USER     PID   PPID  VSIZE  RSS   PRIO  NICE  RTPRI SCHED  PCY  WCHAN    PC         NAME
b2g              root      110   1     185504 60600 20    0     0     0     fg  ffffffff 400d5330 S /system/b2g/b2g
Binder Thread #  root      180   110   185504 60600 20    0     0     0     fg  c03dcb4c 400d37b0 S Binder Thread #
Binder Thread #  root      182   110   185504 60600 20    0     0     0     fg  c03dcb4c 400d37b0 S Binder Thread #
b2g              root      234   110   185504 60600 20    0     0     0     fg  c00ddcdc 400d46ec S b2g
Gecko_IOThread   root      238   110   185504 60600 20    0     0     0     fg  c0166e80 400d5330 S Gecko_IOThread
Socket Thread    root      243   110   185504 60600 20    0     0     0     fg  c0142bc4 400d4558 S Socket Thread
JS GC Helper     root      244   110   185504 60600 20    0     0     0     fg  c00ddcdc 400d46ec S JS GC Helper
JS Sour~ Thread  root      245   110   185504 60600 20    0     0     0     fg  c00ddcdc 400d46ec S JS Sour~ Thread
JS Watchdog      root      246   110   185504 60600 20    0     0     0     fg  c00ddcdc 400d46ec S JS Watchdog
Hang Monitor     root      247   110   185504 60600 21    1     0     0     fg  c00ddcdc 400d46ec S Hang Monitor
b2g              root      248   110   185504 60600 20    0     0     0     fg  c00ddcdc 400d46ec S b2g
DOM Worker       root      249   110   185504 60600 21    1     0     0     fg  c00ddcdc 400d46ec S DOM Worker
Timer            root      250   110   185504 60600 21    1     0     0     fg  c00ddcdc 400d46ec S Timer
DOM Worker       root      251   110   185504 60600 21    1     0     0     fg  c00ddcdc 400d46ec S DOM Worker
DOM Worker       root      252   110   185504 60600 21    1     0     0     fg  c00ddcdc 400d46ec S DOM Worker
DOM Worker       root      253   110   185504 60600 21    1     0     0     fg  c00ddcdc 400d46ec S DOM Worker
b2g              root      254   110   185504 60600 20    0     0     0     fg  c00e88c8 400d3578 S b2g
InputReader      root      255   110   185504 60600 12    -8    0     0     fg  c0166e80 400d5330 S InputReader
GonkSensors      root      256   110   185504 60600 20    0     0     0     fg  c0142bc4 400d4558 S GonkSensors
Compositor       root      257   110   185504 60600 20    0     0     0     fg  c00ddcdc 400d46ec S Compositor
ImageBridgeChil  root      259   110   185504 60600 20    0     0     0     fg  c00ddcdc 400d46ec S ImageBridgeChil
Storage I/O      root      297   110   185504 60600 20    0     0     0     fg  c00ddcdc 400d46ec S Storage I/O
HTML5 Parser     root      317   110   185504 60600 20    0     0     0     fg  c00ddcdc 400d46ec S HTML5 Parser
Indexed~rans #1  root      318   110   185504 60600 20    0     0     0     fg  c00ddcdc 400d46ec S Indexed~rans #1
Indexed~rans #2  root      319   110   185504 60600 20    0     0     0     fg  c00ddcdc 400d46ec S Indexed~rans #2
ImageDecoder #1  root      320   110   185504 60600 20    0     0     0     fg  c00ddcdc 400d46ec S ImageDecoder #1
b2g              root      321   110   185504 60600 20    0     0     0     fg  c036ed18 400d37b0 S b2g
Cache I/O        root      322   110   185504 60600 20    0     0     0     fg  c00ddcdc 400d46ec S Cache I/O
mozStorage #1    root      331   110   185504 60600 20    0     0     0     fg  c00ddcdc 400d46ec S mozStorage #1
Indexed~rans #3  root      332   110   185504 60600 20    0     0     0     fg  c00ddcdc 400d46ec S Indexed~rans #3
Indexed~rans #4  root      333   110   185504 60600 20    0     0     0     fg  c00ddcdc 400d46ec S Indexed~rans #4
Cert Verify      root      352   110   185504 60600 20    0     0     0     fg  c00ddcdc 400d46ec S Cert Verify
localStorage DB  root      374   110   185504 60600 21    1     0     0     fg  c00ddcdc 400d46ec S localStorage DB
GL updater       root      386   110   185504 60600 10    -10   0     0     fg  c00ddcdc 400d46ec S GL updater
DeviceS~che I/O  root      387   110   185504 60600 20    0     0     0     fg  c00ddcdc 400d46ec S DeviceS~che I/O
Communications   app_336   336   110   107256 34304 21    1     0     0     fg  ffffffff 4001d330 S /system/b2g/plugin-container
Binder Thread #  app_336   349   336   107256 34304 21    1     0     0     fg  c03dcb4c 4001b7b0 S Binder Thread #
Chrome_ChildThr  app_336   350   336   107256 34304 21    1     0     0     fg  c0166e80 4001d330 S Chrome_ChildThr
Binder Thread #  app_336   351   336   107256 34304 21    1     0     0     fg  c03dcb4c 4001b7b0 S Binder Thread #
JS GC Helper     app_336   371   336   107256 34304 21    1     0     0     fg  c00ddcdc 4001c6ec S JS GC Helper
JS Sour~ Thread  app_336   372   336   107256 34304 21    1     0     0     fg  c00ddcdc 4001c6ec S JS Sour~ Thread
JS Watchdog      app_336   373   336   107256 34304 21    1     0     0     fg  c00ddcdc 4001c6ec S JS Watchdog
Socket Thread    app_336   375   336   107256 34304 21    1     0     0     fg  c0142bc4 4001c558 S Socket Thread
(Preallocated a  app_336   388   336   107256 34304 21    1     0     0     fg  c00ddcdc 4001c6ec S (Preallocated a
ImageBridgeChil  app_336   389   336   107256 34304 21    1     0     0     fg  c00ddcdc 4001c6ec S ImageBridgeChil
Timer            app_336   392   336   107256 34304 21    1     0     0     fg  c00ddcdc 4001c6ec S Timer
ImageDecoder #1  app_336   395   336   107256 34304 21    1     0     0     fg  c00ddcdc 4001c6ec S ImageDecoder #1
HTML5 Parser     app_336   396   336   107256 34304 21    1     0     0     fg  c00ddcdc 4001c6ec S HTML5 Parser
Homescreen       app_391   391   110   121024 34948 38    18    0     0     fg  ffffffff 4005b330 S /system/b2g/plugin-container
Binder Thread #  app_391   399   391   121024 34948 38    18    0     0     fg  c03dcb4c 400597b0 S Binder Thread #
Binder Thread #  app_391   400   391   121024 34948 38    18    0     0     fg  c03dcb4c 400597b0 S Binder Thread #
Chrome_ChildThr  app_391   401   391   121024 34948 38    18    0     0     fg  c0166e80 4005b330 S Chrome_ChildThr
JS GC Helper     app_391   414   391   121024 34948 38    18    0     0     fg  c00ddcdc 4005a6ec S JS GC Helper
JS Sour~ Thread  app_391   415   391   121024 34948 38    18    0     0     fg  c00ddcdc 4005a6ec S JS Sour~ Thread
JS Watchdog      app_391   416   391   121024 34948 38    18    0     0     fg  c00ddcdc 4005a6ec S JS Watchdog
Socket Thread    app_391   435   391   121024 34948 38    18    0     0     fg  c0142bc4 4005a558 S Socket Thread
(Preallocated a  app_391   436   391   121024 34948 38    18    0     0     fg  c00ddcdc 4005a6ec S (Preallocated a
ImageBridgeChil  app_391   437   391   121024 34948 38    18    0     0     fg  c00ddcdc 4005a6ec S ImageBridgeChil
Timer            app_391   438   391   121024 34948 38    18    0     0     fg  c00ddcdc 4005a6ec S Timer
ImageDecoder #1  app_391   441   391   121024 34948 38    18    0     0     fg  c00ddcdc 4005a6ec S ImageDecoder #1
HTML5 Parser     app_391   442   391   121024 34948 38    18    0     0     fg  c00ddcdc 4005a6ec S HTML5 Parser
StreamTrans #18  app_391   466   391   121024 34948 38    18    0     0     fg  c00ddcdc 4005a6ec S StreamTrans #18
(Preallocated a  root      450   110   68820  20604 38    18    0     0     fg  ffffffff 400f9330 S /system/b2g/plugin-container
Binder Thread #  root      453   450   68820  20604 38    18    0     0     fg  c03dcb4c 400f77b0 S Binder Thread #
Chrome_ChildThr  root      454   450   68820  20604 38    18    0     0     fg  c0166e80 400f9330 S Chrome_ChildThr
Binder Thread #  root      455   450   68820  20604 38    18    0     0     fg  c03dcb4c 400f77b0 S Binder Thread #
JS GC Helper     root      458   450   68820  20604 38    18    0     0     fg  c00ddcdc 400f86ec S JS GC Helper
JS Sour~ Thread  root      459   450   68820  20604 38    18    0     0     fg  c00ddcdc 400f86ec S JS Sour~ Thread
JS Watchdog      root      460   450   68820  20604 38    18    0     0     fg  c00ddcdc 400f86ec S JS Watchdog
Socket Thread    root      462   450   68820  20604 38    18    0     0     fg  c0142bc4 400f8558 S Socket Thread
(Preallocated a  root      467   450   68820  20604 38    18    0     0     fg  c00ddcdc 400f86ec S (Preallocated a
ImageBridgeChil  root      468   450   68820  20604 38    18    0     0     fg  c00ddcdc 400f86ec S ImageBridgeChil
Timer            root      474   450   68820  20604 38    18    0     0     fg  c00ddcdc 400f86ec S Timer
StreamTrans #1   root      475   450   68820  20604 38    18    0     0     fg  c00ddcdc 400f86ec S StreamTrans #1
ImageDecoder #1  root      477   450   68820  20604 38    18    0     0     fg  c00ddcdc 400f86ec S ImageDecoder #1

Maybe we could do more to improve the APPLICATION column.
blocking-b2g: leo? → leo+
Keywords: perf
Whiteboard: [YouTubeCertBlocker+] → c= , [YouTubeCertBlocker+]
> I use master branch gonk-misc. I got below output:

This may have been because I had busybox installed.

Anyway, see bug 864597 for an alternative tool that I like a lot more.
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.