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

RESOLVED FIXED in Firefox 55

Status

()

Core
General
RESOLVED FIXED
27 days ago
15 days ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

50 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Updated

20 days ago
Depends on: 1346644
(Assignee)

Updated

20 days ago
Summary: Enable high priority vsync in parent process → Enable high priority vsync in parent process (April 2017 edition)
(Assignee)

Comment 1

20 days ago
Created attachment 8855909 [details] [diff] [review]
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
Attachment #8855909 - Flags: review?(ehsan)
(Assignee)

Comment 2

20 days ago
Comment on attachment 8855909 [details] [diff] [review]
parent_process_vsync_5.diff

...and this breaks some tests these days :/
Attachment #8855909 - Flags: review?(ehsan)
(Assignee)

Comment 3

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

Comment 4

20 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e25aa1baf6e9d196548cfa444697b3ab54cb0c6
(Assignee)

Comment 5

17 days ago
oh, silly me, the failure I was looking at is after all Bug 1340786
(Assignee)

Updated

17 days ago
Attachment #8855909 - Flags: review?(ehsan)
(Assignee)

Comment 6

17 days ago
-m "Bug 1352523, enable high priority vsync processing in e10s parent process, r=ehsan"
Attachment #8855909 - Flags: review?(ehsan) → review+

Comment 7

16 days ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5123cf34c22
enable high priority vsync processing in e10s parent process, r=ehsan
(Assignee)

Comment 8

16 days ago
Created attachment 8856966 [details] [diff] [review]
a test fix
Attachment #8856966 - Flags: review+

Comment 9

16 days ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e7ffcd569cb
trying to fix browser_parsable_css.js to not depend on previous tests, r=Gijs
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
Flags: needinfo?(bugs)
(Assignee)

Comment 11

16 days ago
uh. devtools tests are so much fun :/
(Assignee)

Comment 12

16 days ago
Created attachment 8857025 [details] [diff] [review]
vsync_devtools_test_fix.diff

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

trying to fix the devtools test. Guess fixing.
Flags: needinfo?(bugs)
(Assignee)

Comment 13

16 days ago
aha, there is other very much racy thing in the test :/
(Assignee)

Comment 14

16 days ago
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

16 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6e7ffcd569cb
Status: NEW → RESOLVED
Last Resolved: 16 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

16 days ago
Attachment #8857025 - Attachment is obsolete: true
(Assignee)

Comment 16

16 days ago
Created attachment 8857374 [details] [diff] [review]
a test fix
Attachment #8857374 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

16 days ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8857374 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 18

16 days ago
Pushed by opettay@mozilla.com:
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 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

15 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ecad6588fcc3
https://hg.mozilla.org/mozilla-central/rev/c73cef124837
Status: REOPENED → RESOLVED
Last Resolved: 16 days ago15 days ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.