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

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

50 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
Depends on: 1346644
Summary: Enable high priority vsync in parent process → Enable high priority vsync in parent process (April 2017 edition)
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)
Comment on attachment 8855909 [details] [diff] [review]
parent_process_vsync_5.diff

...and this breaks some tests these days :/
Attachment #8855909 - Flags: review?(ehsan)
oh, silly me, the failure I was looking at is after all Bug 1340786
Attachment #8855909 - Flags: review?(ehsan)
-m "Bug 1352523, enable high priority vsync processing in e10s parent process, r=ehsan"

Updated

2 years ago
Attachment #8855909 - Flags: review?(ehsan) → review+

Comment 7

2 years 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

Updated

2 years ago
Attachment #8856966 - Flags: review+

Comment 9

2 years 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)
uh. devtools tests are so much fun :/
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)
aha, there is other very much racy thing in the test :/
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6e7ffcd569cb
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Attachment #8857025 - Attachment is obsolete: true
Created attachment 8857374 [details] [diff] [review]
a test fix
Attachment #8857374 - Flags: review?(gijskruitbosch+bugs)
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

2 years 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 19

2 years ago
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

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