[cairo] OVER / SOURCE optimization for cairo_paint
Carl Worth
cworth at cworth.org
Sat Feb 16 10:43:48 PST 2008
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
More information about the cairo
mailing list