Review Request: Analog clock painting performance

[Aaron Seigo]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.vidsolbach.de/r/290/#review275
-----------------------------------------------------------


hm. that seems a little suspect to me, but if grabbing the cached rects is really that slow, then Theme::findInRectsCache is probably what needs to be optimized.

can you post the cachegrind output somewhere, along with the clock configuration you used?




/trunk/KDE/kdebase/workspace/plasma/applets/analog-clock/clock.cpp
<http://reviewboard.vidsolbach.de/r/290/#comment227>

    upon changing the theme, these will now be incorrect until a resize is made.



/trunk/KDE/kdebase/workspace/plasma/applets/analog-clock/clock.cpp
<http://reviewboard.vidsolbach.de/r/290/#comment226>

    two lines back you translate QSize(center, center) and now translate back -QSize(center, center)? the reason why the previous translation had all that crazy "translate based on the element size" is to make sure the elements always line up.
    
    now the assumption is that the element is == boundingRect/2
    
    i also wonder if this should be using boundingRect as opposed to contentsRect, but that's another topic =)



/trunk/KDE/kdebase/workspace/plasma/applets/analog-clock/clock.cpp
<http://reviewboard.vidsolbach.de/r/290/#comment224>

    are you sure about that?



/trunk/KDE/kdebase/workspace/plasma/applets/analog-clock/clock.cpp
<http://reviewboard.vidsolbach.de/r/290/#comment225>

    this paints the center screw in a completely different way, now doesn't it? that it still works the same is probably a fluke of the oxygen clock svg.


- Aaron


On 2008-11-30 07:32:56, Alain Boyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.vidsolbach.de/r/290/
> -----------------------------------------------------------
> 
> (Updated 2008-11-30 07:32:56)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> This patch attempts to speed up the analog clock painting performance. Essentially, it caches the QRects of the theme elements and streamlines some of the painting code.
> 
> The speedup is explained below, however I think we can do even better. For example, we could draw the hour and minute hands and shadows to a pixmap and then use that when only the second hand needs an update. I'm not sure if this is the way to go? Comments or suggestions?
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/plasma/applets/analog-clock/clock.h
>   /trunk/KDE/kdebase/workspace/plasma/applets/analog-clock/clock.cpp
> 
> Diff: http://reviewboard.vidsolbach.de/r/290/diff
> 
> 
> Testing
> -------
> 
> Valgrind tells me that this patch introduces a 1.46 speedup to the drawHand() method. This contributes to a 1.40 speedup of paintInterface(). Not bad.
> 
> Another way of looking at it, paintInterface() used to spend 63% of its time actually painting (m_theme->paint()). Now, paintInterface() spends 98% of its time painting. Hence, painting is now the bottleneck.
> 
> 
> Thanks,
> 
> Alain
> 
>