[cairo] OVER / SOURCE optimization for cairo_paint

[Carl Worth]

On Sat, 16 Feb 2008 03:29:56 -0500, Antoine Azar wrote:
> I've got a working optimization that speeds up paint operations for
> opaque sources.

Excellent!

> I'm attaching my optimized _cairo_gstate_paint function.

Could you please attach a patch instead? That way we can more easily
review just what the changes are, and we can much more easily apply
the patch, and run your code.

> I'm also  attaching a summary of the speedups seen with perf (using only the
> cases affected by the optim). The best speedup is 11X, many are
> 3X-5X.

This sounds very encouraging. Thanks for your work here!

> At 05:31 PM 2/7/2008, Carl Worth wrote:
> >On Thu, 07 Feb 2008 16:51:03 -0500, Antoine Azar wrote:
> > > >Uhm... how exactly are you determining what sources are classified
> > > >as opaque here?
> > >
> > > There is a _cairo_pattern_is_opaque convenience function in
> > > cairo-pattern.c.
> >
> >Right, OK. I had forgotten this function existed. This looks just
> >fine.

Please don't quote text you're not directly applying to. It's just
clutter that makes more work for people reading your messages.

A few notes on the code itself:

>     //optimization related declarations
>     cairo_rectangle_int_t extents;
>     const cairo_pattern_union_t *pattern_union;
>     cairo_fixed_t x_fixed, y_fixed;

That's a comment that doesn't really add anything, so please remove
that. Also we don't use the C++-style one-line comments in
cairo[*]. So please just use the /*...*/ style instead.

It looks like we don't have a paragraph on commenting in the
CODING_STYLE document. Perhaps I should add that.

>     //Optimize a very common case of calling paint with an OVER operator for opaque surfaces.
>     //Replace it with a more efficient SOURCE operator, and constrain the operation to the source's extents.

The comments also go to too many columns here. Wrapping at something a
little less than 80 is much nicer.

>     if ( _cairo_pattern_is_opaque(&pattern.base) && (op == CAIRO_OPERATOR_OVER || op == CAIRO_OPERATOR_SOURCE))
>     {
>         pattern_union = (cairo_pattern_union_t *) &pattern;
>         switch (pattern_union->base.type)

Here, you're introducing a very long 'if' block for the
optimization. I think this would be much readable as a separate
function. Maybe something like _cairo_gstate_paint_opaque_with_source
or so. Fortunately, the current function is accepting only a single
parameter, so it's not painful to pass a lot of arguments down to a
new function, (nested functions would be really nice---and support for
them in gcc is rather tempting).

And there should be no concern about performance impact of a new
function here. Your new function will be static and called only from
here, so it should be inlined automatically.

I'll have more to say when this comes through as a patch.

Thanks,

-Carl

[*] And no, calling them C99-style one-line comments doesn't make them
any better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.cairographics.org/archives/cairo/attachments/20080216/8eff4c9c/attachment-0001.pgp