Parent process may get vsync in background thread and then dispatches an event to main thread. It could use high priority runnables for that.

This is quite risky. I would be a bit surprised if this doesn't reveal some racy browser chrome tests. remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4014f0df97b961434e66346437ca3f95fcc7e247 remote: remote: It looks like this try push has talos jobs. Compare performance against a baseline revision: remote: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=4014f0df97b961434e66346437ca3f95fcc7e247

Not too bad breakage. Mostly devtools test issues. Luckily bug 1193394 will fix many of those.

Hmm, I think there are also some other failures.

ejpbruel, any chance you could help here with devtool tests. These seem to be after all different broken tests than those ones I've been fixing for Promise changes. At least devtools/client/commandline/test/browser_cmd_highlight_01.js seem to fail pretty much everywhere on debug builds.

(In reply to Olli Pettay [:smaug] from comment #4) > ejpbruel, any chance you could help here with devtool tests. These seem to > be after all different broken tests than those ones I've been fixing for > Promise changes. > > At least devtools/client/commandline/test/browser_cmd_highlight_01.js seem > to fail pretty much everywhere on debug builds. I can take a look at this, sure. Is the patch in this bug still up to date, or do I need an up to date version? I'm quite deep in some other stuff right now, so could you please ping me on irc tomorrow and remind me that I should work on this? :-)

The patch should be still up-to-date.

(In reply to Olli Pettay [:smaug] from comment #6) > The patch should be still up-to-date. I tried the patch in this bug, but it does not apply for me on mozilla-central, from which I just pulled today.

The previous patch does apply too with some --fuzz, but here is up-to-date

The test in question is actually a compound test. As a first step, I started bisecting it, to see if there was a specific subtest that failed. When I only run the second to last subtest, I run into a browser crash. Here is the relevant test output: https://pastebin.mozilla.org/8928763

remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=615111dae59f2d64729a82acd6daf05b7ef2d321 remote: remote: It looks like this try push has talos jobs. Compare performance against a baseline revision: remote: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=615111dae59f2d64729a82acd6daf05b7ef2d321 to get talos results

remote: View the pushlog for these changes here: remote: https://hg.mozilla.org/try/pushloghtml?changeset=8fa88672fc6729289a0bc12a41be9df610908549 remote: remote: Follow the progress of your build on Treeherder: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fa88672fc6729289a0bc12a41be9df610908549 remote: remote: It looks like this try push has talos jobs. Compare performance against a baseline revision: remote: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=8fa88672fc6729289a0bc12a41be9df610908549 remote: recorded changegroup in replication log in 0.197s On top of m-i 9034d9e08ee3

Unfortunately, I couldn't reproduce this issue on an opt build, and I don't really have time to setup a debug build (at least not this week). One thing I did notice is that the test runs very slowly on an opt build, makes the browser freeze for a moment before finishing (though the test still passes). Though this isn't related. From the logs, it seems like the test times out just before the last sub-test (0), so the sub-test just before is probably causing the timeout. Though I don't know enough about how gcli works to tell why. Ping Alex who might be able to help debug the failure. [0]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/commandline/test/browser_cmd_highlight_04.js#79

Yes this test looks slow with and without the patch. (>20s locally) Did you looked at the overall performance impact of this patch? With this patch you may just trigger bug 1317662 more and make it almost permanent. Could be worth putting requestLongerTimeout(2); in the failing mochitests... especially this one. I'm also off, I'll be back on 3rd.

This try run seems to confirm it is just about browser_cmd_highlight_04.js being slower to run... A call to requestLongerTimeout made it green. But let me push to try without the requestLongerTimeout to be sure it still fails or if there is anything special with my try push (there is only taskcluster runs...)

So browser_cmd_highlight_04.js is just slower to run with your patch. You can land with this change: https://hg.mozilla.org/try/rev/9b611984f9e3561628c6cb6ef7fb5f12ea050480 But I would suggest to take a quick look at a profile of this test with and without your patch to help tracking a potential perf regression...

devtools have silly loops doing tons of stuff in animation frame, way over the frame budget. That has caused issues before too.

Comment on attachment 8835994 [details] [diff] [review] v3 (v2 + test fix) Review of attachment 8835994 [details] [diff] [review]: ----------------------------------------------------------------- Stealing. ::: layout/base/nsRefreshDriver.cpp @@ +492,5 @@ > { > MOZ_ASSERT(NS_IsMainThread()); > } > > + class ParentProcessVsyncNotifier : public Runnable, Nit: final

Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/910ceca7e863 vsync handling in parent process' main thread should use high priority runnables, r=ehsan