[cairo-bugs] [Bug 9994] performance opportunities in cairo

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=9994





------- Comment #9 from cworth at cworth.org  2007-04-05 09:22 PST -------
I'd advise not applying the patch.

There could be different ways to justify a patch like this, neither of which
has been sufficiently demonstrated:

1. The patch could show a real performance improvement.

   If so, there would be almost nothing to argue against. We always want the
   code to run faster. (And I'd even accept a patch that only help when cairo
   is compiled with a braindead compiler that can't handle the most elementary
   forms of common sub-expression elimination---though I sincerely hope such
   compilers don't even exist.)

   But as discussed above, there has been no satisfactory demonstration of a
   performance benefit. (Any discussion of "overall" improvement to cairo-perf
   doesn't even make sense. A change like this to very specific code should
   show improvments to tests that exercise this code, and have no impact on
   other tests).

2. The patch could be an obvious clean-up, (for better aesthetics or easier
   maintainability).

   The patch is already adding more lines of code (165) than it removes (127)
   which is a strike against it in terms of aesthetics and maintainability.

   There are a few cases where the code does look marginally better, (like the
   first hunk where 11 occurrences of "pict->transform" get replaced with a
   simpler-to-read "transform"). And yes, a change like that will add more code
   than is removed, but still improve the code.

   But other portions of the code actually appear to add noise (to my eye at
   least).

   Now, I won't argue that the pixman code is actually aesthetically
pleasing---
   it's definitey not. Expressions with two sets of arrow operators, (such
   as pict->pDrawable->height), are definitely ugly, so I appreciate the effort
   to clean those up. But the suggested replacements are problematic ("pixels"
   for pict->pDrawable suggests there's mismatched naming here---though the
   problem could be that pDrawable should be named pixels), and (pix_height has
   an unfortunate abberviation, which is something we like to avoid in cairo).

   Finally, the most important thing to understand is that a lot of the poor
   aesthetics of pixman, (compared to most of the cairo code), comes from the
   legacy of this code coming from the X server fb implementation, (that's
where
   such an ugly name as pDrawable comes from, for example). And there's a
   effort happening right now to try to get the divergent pixman and fb
   implementations merged back together so the X server can share a common
   implementation of this code with cairo.

   So gratuitous changes, (even good style cleanups), should be avoided in
   pixman right now as they will make that merge effort more difficult.

So, that's a NACK from me on this patch for now.

But I definitely appreciate people looking closely at cairo---especially
efforts
to improve the performance. So please continue that, and if you can present
compelling evidence of a performance improvement, please let us know and we'll
be glad to accept it.

Thanks,

-Carl


-- 
Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.