760228 - Prepare for multi-threaded Xlib

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: patch

Several folks are using omtc for desktop dev on X11, and the current setup for XInitThreads() is annoying to work with.

If we're ever to enable omtc for x11, we also need to quantify the hit from multithreaded xlib.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

First batch of talos results

http://perf.snarkfest.net/compare-talos/index.html?oldRevs=f28d1ec8bd33,4c68e77d89d8,7c3cd4824f94,b5af439f1717,f6d082275253&newRev=7afe144780de&tests=a11y,tdhtml,tdhtml_nochrome,a11y_paint,tdhtml_paint,tdhtml_nochrome_paint,tp4,tp4_memset,tp4_pbytes,tp4_rss,tp4_shutdown,tp4_xres,tp4m,tp4m_content_rss,tp4m_main_rss,tp4m_main_rss_nochrome,tp4m_nochrome,tp4m_shutdown,tp4m_shutdown_nochrome,tp5r,tp5r_memset,tp5r_pbytes,tp5r_rss,tp5r_shutdown,tp5r_xres,tp5r_paint,tp5r_memset_paint,tp5r_pbytes_paint,tp5r_%cpu_paint,tp5r_modlistbytes_paint,tp5r_rss_paint,tp5r_shutdown_paint,tp5r_xres_paint,dromaeo_basics,dromaeo_css,dromaeo_dom,dromaeo_jslib,dromaeo_sunspider,dromaeo_v8,tsspider,tsspider_nochrome,tsspider_paint,tsspider_nochrome_paint,v8,tgfx,tgfx_nochrome,tgfx_paint,tgfx_nochrome_paint,tscroll,tsvg,tsvg_opacity,tzoom,ts,ts_paint,ts_cold,ts_cold_generated_max,ts_cold_generated_max_shutdown,ts_cold_generated_med,ts_cold_generated_med_shutdown,ts_cold_generated_min,ts_cold_generated_min_shutdown,ts_cold_shutdown,ts_places_generated_max,ts_places_generated_max_shutdown,ts_places_generated_med,ts_places_generated_med_shutdown,ts_places_generated_min,ts_places_generated_min_shutdown,ts_shutdown,twinopen,tpaint&submit=true

nothing suspicious beyond variance, but some gfx-related numbers are in the wrong direction.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/try/pushloghtml?changeset=a331e614e191

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Second batch, different base cset

http://perf.snarkfest.net/compare-talos/index.html?oldRevs=0aa7fc75cad5,f4981b5e1f7a,d6ae9ef0eb50,e6da6ece3818,bccfd4fa5160&newRev=a331e614e191&tests=a11y,tdhtml,tdhtml_nochrome,a11y_paint,tdhtml_paint,tdhtml_nochrome_paint,tp4,tp4_memset,tp4_pbytes,tp4_rss,tp4_shutdown,tp4_xres,tp4m,tp4m_content_rss,tp4m_main_rss,tp4m_main_rss_nochrome,tp4m_nochrome,tp4m_shutdown,tp4m_shutdown_nochrome,tp5r,tp5r_memset,tp5r_pbytes,tp5r_rss,tp5r_shutdown,tp5r_xres,tp5r_paint,tp5r_memset_paint,tp5r_pbytes_paint,tp5r_%cpu_paint,tp5r_modlistbytes_paint,tp5r_rss_paint,tp5r_shutdown_paint,tp5r_xres_paint,dromaeo_basics,dromaeo_css,dromaeo_dom,dromaeo_jslib,dromaeo_sunspider,dromaeo_v8,tsspider,tsspider_nochrome,tsspider_paint,tsspider_nochrome_paint,v8,tgfx,tgfx_nochrome,tgfx_paint,tgfx_nochrome_paint,tscroll,tsvg,tsvg_opacity,tzoom,ts,ts_paint,ts_cold,ts_cold_generated_max,ts_cold_generated_max_shutdown,ts_cold_generated_med,ts_cold_generated_med_shutdown,ts_cold_generated_min,ts_cold_generated_min_shutdown,ts_cold_shutdown,ts_places_generated_max,ts_places_generated_max_shutdown,ts_places_generated_med,ts_places_generated_med_shutdown,ts_places_generated_min,ts_places_generated_min_shutdown,ts_shutdown,twinopen,tpaint&submit=true

Looks like we're within noise, as far as talos can measure.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Always use XInitThreads()

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Always use XInitThreads(), v2

When working on another patch, I noticed some code that would have been obsoleted by the v1 patch.  Removed here in v2.  Nothing else changed.

The differences in this patch wouldn't have affected the talos numbers.

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 630486 [details] [diff] [review]
Always use XInitThreads(), v2

If we do this, then we should revert the visibility of the pref (changed in
bug 722012).  So the r- here is easy to decide based on that.

However, I doubt this benefit of a less annoying workflow for developers is
worth the extra risks.

Perhaps the main thing I don't understand here is how an environment variable
can be more annoying to work with than a pref.  Sure, the fact that
environment variables replicate into every process is unfortunate, but that
doesn't really make it annoying to use.

We know the effect on performance is negative, but we don't know whether it is
a significant cost.  Regarding the talos results:

  I'm aware that it is much harder to statistically demonstrate the lack of a
  functionally significant difference than to detect a significant difference,
  but if anything the data seems to suggest there is a difference.

  Although each result is within the margin of error, having a set of results
  that all move in the same direction may well demonstrate that there is a
  statistical difference.

Beyond that risk, there is the risk of using Xlib in a way that is not well
tested.

  One bug that I know exists is that our X11 error handler will lock up in
  XSynchronize when XInitThreads is used with default configurations of libX11
  versions 1.4.0 and 1.4.1 as well as --with-xcb configurations of version
  1.3.4.

  http://lists.x.org/archives/xorg-devel/2011-March/020409.html
  http://cgit.freedesktop.org/xorg/lib/libX11/commit/src/XlibInt.c?id=83e1ba59c48c79f8b0a7e7aa0b9c9cfd84fa403d

I think we need to find a workaround for the old(ish) libX11 bug (perhaps just
disabling something for those versions) before XInitThreads is enabled.

Beyond that, the risks seem justifiable if there is the benefit of being able
to switch on OMTC.  If there is no benefit to users then the decision is less
clear.

If OMTC is the definite future, then there is some testing value in switching
on XInitThreads.  This more than developer annoyance seems a better reason to
make the call, but we'd disable the call in release (and beta?) builds.  I
feel a bit weary of using our testers as guinea pigs with no immediate
benefit, but perhaps that's justifiable.

I wonder how well OMTC is working right now?
If it is close, then perhaps it is time to start switching things on.
I suspect it is not that close, one reason being that currently OMTC needs
OpenGL layers which AFAIK are not close on X11.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Thanks for the very thorough review and summary, Karl.  We need to work out these issues for bug 722012.

omtc works pretty well.  It's shipping in android-fennec.  It doesn't require GL layers per se, although the patch to remove that requirement was just backed out.

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #6)
> I wonder how well OMTC is working right now?

I meant on X11, with it possibly having some unique challenges.

> one reason being that currently OMTC needs OpenGL layers

And here too I meant with X11, but I'm pleased if the GL requirement is easily avoided.

X11 may have some unique issues to solve for OMTC, such as making ScopedXErrorHandler work across threads, but perhaps that is not necessary on the compositing thread.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

omtc works well for me on X11, although I've only tested with GL compositing force-enabled.

Comment 10

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #6)
> Perhaps the main thing I don't understand here is how an environment variable
> can be more annoying to work with than a pref.

One disadvantage of environment variables may be that they don't show up in about support.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Not working on this currently.)

Comment 12

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #6)
>   One bug that I know exists is that our X11 error handler will lock up in
>   XSynchronize when XInitThreads is used with default configurations of
> libX11
>   versions 1.4.0 and 1.4.1 as well as --with-xcb configurations of version
>   1.3.4.

Debian squeeze (currently oldstable) has libx11-6 based on 1.3.3 and 
wheezy (stable) based on 1.5.0.

Ubuntu 12.04 LTS precise has libx11-6 based on 1.4.99.1, so the major long term support distributions have avoided this issue, and so I don't think we need to worry about this now.

Comment 13

[Chris Peterson [:cpeterson]]

Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".

Comment 14

[AnAkkk]

I think this was solved by https://bugzilla.mozilla.org/show_bug.cgi?id=994541

Comment 15

[Nicolas Silva [:nical]]

(In reply to AnAkkk from comment #14)
> I think this was solved by
> https://bugzilla.mozilla.org/show_bug.cgi?id=994541

Indeed.