1332226 - Consider process more non-high priority runnables

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

I do think it is a bug in the platform if it is processing too long taking tasks when refresh driver ticks. But fixing those may take some time, so perhaps we need to relax the setup a bit and let normal runnables to run a bit more after refresh driver tick.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9aa3f01e0e09dbeca7f236f4afbde0a71d233e9

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8828279 [details] [diff] [review]
WIP

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

I understand (mostly) why this is needed, but this patch feels like a hack.  Not necessarily objecting to it going in, but it feels hackish.  We ought to be able to do better.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Feel free to suggest something else, but it is clear that we need something here.

Note, we're doing very similar thing in (parent process) appshell.

The patch (+ high prio vsync) works ok with https://bugzilla.mozilla.org/show_bug.cgi?id=1331706#c3

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip2

Tiny bit simpler and less ::Now() calls
https://treeherder.mozilla.org/#/jobs?repo=try&revision=707371bb62b339dd025cd231252176dcbf5007f0

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

billm, froydnj, I'm all ears to suggestions what else could be done here.
We obviously need to process more normal runnables, but how to decide how much more.

2ms is static limit, working usually quite well. But we could also do some dynamic limit based on the refresh driver tick timing. Something like giving always at least 1/4 of the refresh driver handling time to normal runnables too, or 1/8. 
Or something else.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I have couple of ideas of making child process to ask vsync tick from parent less aggressively and that way let child process more normal events.
I'll try and report here.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: p1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c4079dd3dcefabc000ba058a243cbbff8547b9b

If we're slow at processing ticks, skip some of them.

We already do have in place the skipped paints for slow compositing, but that doesn't cover the slow tick processing time.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8834082 [details] [diff] [review]
p1

Not sure who should review this.

This re-enables high priority vsync again (it was disabled in Bug 1331706) in child process. The idea is that we skip some refresh driver ticks if processing them would end up causing child process to basically process only them.

GC/CC has a bit special handling so that we'd skip ticks less often.

With this setup we might want to remove the explicit DoRefresh call in
nsRefreshDriver::FinishedWaitingForTransaction(), but that is somewhat orthogonal to this issue.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

and since parent process uses different setup for vsync handling, it already has flooding protection, so the patch in bug 1315570 should be still fine.

Comment 11

[Andreas Farre [:farre]]

So I think this looks ok, for what it's worth.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4df3aa93c77
skip some refreshdriver ticks if processing ticks takes too much time, and re-enable high priority vsync, r=afarre

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a4df3aa93c77