Upload GdkPixbufs into cairo surfaces so they (hopefully) live on X

Upload GdkPixbufs into cairo surfaces so they (hopefully) live on the X server and have better performance. In the presence of XRender, let cairo do things smarter.

This is a big win performance wise. BrowserWindowGtk::OnCustomFrameExpose, a heavy user of images sped up from an average runtime of 20.5ms to 0.7ms.

TEST=Run through valgrind, don't leak memory.
TEST=Run both before and after using xtrace. Notice fewer XCreatePixmap requests and more XRender-CreatePicture requests.
BUG=http://crbug.com/10499

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=25814

[Elliot Glaysher, 2009-09-08 21:42:32]

[agl for pre-review where I walk over to his desk and show him this]

[Elliot Glaysher, 2009-09-09 20:59:02]

Sending out for real review now.

[agl, 2009-09-09 21:07:01]

I don't really know this UI code, but LGTM.

[tony, 2009-09-09 21:26:21]

LGTM2.  cc-ing dean since he might be curious.

http://codereview.chromium.org/197046/diff/7001/5007
File chrome/browser/gtk/cairo_cached_surface.h (right):

http://codereview.chromium.org/197046/diff/7001/5007#newcode26
Line 26: bool valid() const;
Nit: Maybe include the implementation in the header for this since it's
lowercase?

http://codereview.chromium.org/197046/diff/7001/5007#newcode48
Line 48: GdkPixbuf* pixbuf_;
Maybe make this a ScopedGObject (in base/gfx/gtk_util.h)?

http://codereview.chromium.org/197046/diff/7001/5009
File chrome/browser/gtk/custom_button.h (right):

http://codereview.chromium.org/197046/diff/7001/5009#newcode40
Line 40: // Returns the dimensions() of the first pixbuf.
Nit: remove the () in dimensions.

[Evan Martin, 2009-09-09 21:28:08]

Since this is more code and complexity, can you measure the performance
difference?  Otherwise, why do it?

[Elliot Glaysher, 2009-09-09 21:45:53]

It's amazingly measurable. Average time to run
BrowserWindowGtk::CustomFrameExpose with the old code: 20.5ms. Average
time to run same function using CachedCairoContexts: 0.6ms.

Raw histograms to follow. Check my work, I might be wrong :)

Using GdkPixbufs:
Histogram: Gtk.OnCustomFrameExpose recorded 75 samples, average =3D
20.5, standard deviation =3D 16.1
  0 -------------------O
       (2 =3D 2.7%)
  1 --------------------------------------------------------O
       (6 =3D 8.0%) {2.7%}
  2 -----------------------------------------------O
       (5 =3D 6.7%) {10.7%}
  3 O
       (0 =3D 0.0%) {17.3%}
  4 ---------O
       (1 =3D 1.3%) {17.3%}
  5 ---------O
       (1 =3D 1.3%) {18.7%}
  6 O
       (0 =3D 0.0%) {20.0%}
  7 ---------O
       (1 =3D 1.3%) {20.0%}
  8 ...
 14 -----------------------------------------------------------------------=
-O
(23 =3D 30.7%) {21.3%}
 17 -----------------------------------------------------O
       (17 =3D 22.7%) {52.0%}
 20 --O
       (1 =3D 1.3%) {74.7%}
 24 O
       (0 =3D 0.0%) {76.0%}
 29 --O
       (1 =3D 1.3%) {76.0%}
 34 O
       (0 =3D 0.0%) {77.3%}
 40 -----------O
       (6 =3D 8.0%) {77.3%}
 48 ---------------------O
       (11 =3D 14.7%) {85.3%}
 57 ...


Using XRender backed cairo surfaces:

Histogram: Gtk.OnCustomFrameExpose recorded 66 samples, average =3D 0.7,
standard deviation =3D 3.9
  0 -----------------------------------------------------------------------=
-O
(63 =3D 95.5%)
  1 -O
       (1 =3D 1.5%) {95.5%}
  2 ...
 20 O
       (1 =3D 1.5%) {97.0%}
 24 O
       (1 =3D 1.5%) {98.5%}
 29 ...



On Wed, Sep 9, 2009 at 2:28 PM, <evan@chromium.org> wrote:
> Since this is more code and complexity, can you measure the performance
> difference? =A0Otherwise, why do it?
>
> http://codereview.chromium.org/197046
>

[Evan Martin, 2009-09-09 21:50:34]

LGTM

may wanna check with xrestop that your x server memory usage doesn't go through
the roof (or leak in general)

http://codereview.chromium.org/197046/diff/7001/5007
File chrome/browser/gtk/cairo_cached_surface.h (right):

http://codereview.chromium.org/197046/diff/7001/5007#newcode34
Line 34: // the X Server occurs at SetSource() time. Calling UsePixbuf() should
only
I think "server" should be lowercase, here and below.

http://codereview.chromium.org/197046/diff/7001/5009
File chrome/browser/gtk/custom_button.h (right):

http://codereview.chromium.org/197046/diff/7001/5009#newcode41
Line 41: int width();
const?
maybe should be uppercase because it's not a simple getter