Closed
Bug 1287463
Opened 8 years ago
Closed 8 years ago
Use a separate X display on the compositor thread for basic composition buffering paths
Categories
(Core :: Widget: Gtk, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: acomminos, Assigned: acomminos)
References
Details
Attachments
(1 file)
We need this for XCB in nsShmImage to be useful; if a round trip is made to the X server using the same display on the main thread, we won't gracefully check the error.
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/64956/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64956/
Attachment #8772017 -
Flags: review?(lsalzman)
Assignee | ||
Comment 2•8 years ago
|
||
This switches to using our compositor display for all non-XRender draw paths (using a separate display with XRender would likely require additional X synchronization). Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=732b79f1e57f
Comment 3•8 years ago
|
||
Comment on attachment 8772017 [details] Bug 1287463 - Use a separate X display for non-XRender basic composition paths. https://reviewboard.mozilla.org/r/64956/#review61974 ::: widget/nsShmImage.cpp:43 (Diff revision 1) > { > mConnection = XGetXCBConnection(aDisplay); > mozilla::PodZero(&mLastRequest); > + if (aDisplay == mozilla::DefaultXDisplay()) { > + // If another thread spins the X event loop during a checked call, > + // an error that should've been checked by XCB may by handled by the Xlib Typo: "may by handled" -> "may be handled"
Attachment #8772017 -
Flags: review?(lsalzman) → review+
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8772017 [details] Bug 1287463 - Use a separate X display for non-XRender basic composition paths. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64956/diff/1-2/
Pushed by acomminos@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2a69345f4f4e Use a separate X display for non-XRender basic composition paths. r=lsalzman
Comment 6•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2a69345f4f4e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 7•8 years ago
|
||
(In reply to Andrew Comminos [:acomminos] from comment #0) > We need this for XCB in nsShmImage to be useful; if a round trip is made to > the X server using the same display on the main thread, we won't gracefully > check the error. Are you sure? I would expect xcb to filter out the error into the cookie while handling the connection before Xlib on the main thread gets a chance to see it.
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Karl Tomlinson (:karlt) from comment #7) > (In reply to Andrew Comminos [:acomminos] from comment #0) > > We need this for XCB in nsShmImage to be useful; if a round trip is made to > > the X server using the same display on the main thread, we won't gracefully > > check the error. > > Are you sure? I would expect xcb to filter out the error into the cookie > while handling the connection before Xlib on the main thread gets a chance > to see it. You're absolutely correct; I was mislead by some X_ShmAttach errors being caught in XCB-based builds on crash-stats.mozilla.org. The following test confirms it (at least on my machine); > #include <X11/Xlib-xcb.h> > #include <assert.h> > > static int xlib_error = 0; > int x_error_trap(Display* display, XErrorEvent* e) { > xlib_error = 1; > } > > void main() { > XSetErrorHandler(x_error_trap); > > Display* display = XOpenDisplay(NULL); > xcb_connection_t* conn = XGetXCBConnection(display); > assert(xlib_error == 0); > > // Make illegal request via XCB > xcb_get_geometry_cookie_t cookie = xcb_get_geometry(conn, -1); > > // Test to ensure that XCB caught our error despite Xlib processing events > XSync(display, False); > xcb_generic_error_t* error; > xcb_get_geometry_reply(conn, cookie, &error); > assert(error != NULL); > assert(xlib_error == 0); > > // Ensure that our request should have generated an error in the first place > XGetGeometry(display, -1, 0, 0, 0, 0, 0, 0, 0); > XSync(display, False); > assert(xlib_error == 1); > } We might want to switch back to a single X display then for bug 1287666. We do get interesting performance improvements from the use of a separate X display, at the expense of having SHM images swap slower; https://treeherder.mozilla.org/perf.html#/alerts?id=1922 (fwiw, these tests were unaffected by the XCB switch- their e10s variants were, however.)
Comment 9•8 years ago
|
||
I backed this out on beta (Firefox 50) for causing bug 1304637. It may get backed out on 51 as well if we don't get around to fixing the regression in time.
status-firefox51:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•