Use a separate X display on the compositor thread for basic composition buffering paths

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: acomminos, Assigned: acomminos)

Tracking

50 Branch
mozilla50
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 wontfix, firefox51 fixed)

Details

Attachments

(1 attachment)

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.
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/2a69345f4f4e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(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.
(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.)
Depends on: 1289233
Depends on: 1304637
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.
You need to log in before you can comment on or make changes to this bug.