1352523 - Enable high priority vsync in parent process (April 2017 edition)

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

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

Since Bug 1342849 is now fixed, we could try to enable high prio vsync again in parent process. 
I'll add a pref for it and make that pref by default false on Android at least, perhaps also
in cases when e10s is disabled.

Comment 1

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

Attachment: parent_process_vsync_5.diff

And trying again, this time with pref.
Note, since prefs are main thread only, we add pref cache the first time we run code in main thread. Shouldn't matter much that the first time we use normal priority runnable.

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

Comment 2

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

Comment on attachment 8855909 [details] [diff] [review]
parent_process_vsync_5.diff

...and this breaks some tests these days :/

Comment 3

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

Pushing to try again to see if the issue isn't about this bug
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b699de2abb9ae7c501022ecaa99dd91fd6b71d68

Comment 4

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

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

Comment 5

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

oh, silly me, the failure I was looking at is after all Bug 1340786

Comment 6

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

-m "Bug 1352523, enable high priority vsync processing in e10s parent process, r=ehsan"

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5123cf34c22
enable high priority vsync processing in e10s parent process, r=ehsan

Comment 8

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

Attachment: a test fix

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e7ffcd569cb
trying to fix browser_parsable_css.js to not depend on previous tests, r=Gijs

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out the original patch for failing browser_parsable_css.js for chrome://webide/skin/logs.css:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbd3dc0e58c7fadc768a7c871da0313b68be63be

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c5123cf34c22f08403ffb813515d5513647c7d80&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=90460053&repo=mozilla-inbound

[task 2017-04-11T12:22:25.378714Z] 12:22:25     INFO - Console message: [JavaScript Warning: "Property contained reference to invalid variable.  Error in parsing value for ‘background-color’.  Falling back to ‘initial’." {file: "resource://devtools/client/jsonview/css/general.css?always-parse-css-0.11074152484931188" line: 11 column: 377 source: " var(--theme-body-background)"}]
[task 2017-04-11T12:22:25.379118Z] 12:22:25     INFO - Buffered messages finished
[task 2017-04-11T12:22:25.379619Z] 12:22:25     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_parsable_css.js | Got error message for chrome://webide/skin/logs.css: Property contained reference to invalid variable.  Error in parsing value for ‘background-attachment’.  Falling back to ‘initial’. - 
[task 2017-04-11T12:22:25.379953Z] 12:22:25     INFO - Stack trace:
[task 2017-04-11T12:22:25.380273Z] 12:22:25     INFO -     chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:messageIsCSSError:176
[task 2017-04-11T12:22:25.380608Z] 12:22:25     INFO -     chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:checkAllTheCSS:336
[task 2017-04-11T12:22:25.380932Z] 12:22:25     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:752:9
[task 2017-04-11T12:22:25.381246Z] 12:22:25     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:672:7
[task 2017-04-11T12:22:25.381780Z] 12:22:25     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59

Comment 11

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

uh. devtools tests are so much fun :/

Comment 12

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

Attachment: vsync_devtools_test_fix.diff

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

trying to fix the devtools test. Guess fixing.

Comment 13

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

aha, there is other very much racy thing in the test :/

Comment 14

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

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


(I think the test is so bad that it should probably be removed. Adding more and more hacks)

Comment 15

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/6e7ffcd569cb

Comment 16

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

Attachment: a test fix

Comment 17

[Alexandre Poirot [:ochameau]]

Comment on attachment 8857374 [details] [diff] [review]
a test fix

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

First time I see this test. I don't see how it works for most devtools css files...

For sure background: var(--theme-body-background) is going to fail as the existing exception for color: var(--theme-body-color).
What concerns me is that most devtools css files depends on these variables.

These variables are declared here:
http://searchfox.org/mozilla-central/source/devtools/client/themes/variables.css
This file is dynamically injected into devtools document based on the theme preference.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecad6588fcc3
enable high priority vsync processing in e10s parent process, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/c73cef124837
add more hacks to browser_parsable_css.js to make it not fail randomly, r=poirot

Comment 19

[:Gijs (he/him)]

Comment on attachment 8857374 [details] [diff] [review]
a test fix

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

::: browser/base/content/test/static/browser_parsable_css.js
@@ +54,5 @@
>     errorMessage: /Property contained reference to invalid variable.*color/i,
>     isFromDevTools: true},
> +  {sourceName: /webide\/skin\/logs\.css$/i,
> +   intermittent: true,
> +   errorMessage: /Property contained reference to invalid variable.*background/i,

Could have just updated the errorMessage regex to the previous exception... oh well.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ecad6588fcc3
https://hg.mozilla.org/mozilla-central/rev/c73cef124837