Review Request 108992: Simple optimizations in SignalPlotter

[Aaron J. Seigo]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108992/#review27877
-----------------------------------------------------------


ints and other POD types do not need to be pulled from the loops. it just uglifies with no benefit.

moreover, there is a bug that exists in the code which has gone unfixed (noted below)


plasma/widgets/signalplotter.cpp
<http://git.reviewboard.kde.org/r/108992/#comment20901>

    no point in taking PODs out of the loop



plasma/widgets/signalplotter.cpp
<http://git.reviewboard.kde.org/r/108992/#comment20892>

    QList<double> &data



plasma/widgets/signalplotter.cpp
<http://git.reviewboard.kde.org/r/108992/#comment20893>

    er .. this does nothing.
    
    the data member in the foreach loop is a *copy* of the QList<double> from d->plotData. assigning to it just assigns to that copy which only has scope within the foreach.
    
    this *ought* to be:
    
        QMutableListIterator<QList<double> > it(d->plotData);
        while (it.hasNext()) {
            it.next();
            newPlot.clear();
            newPlot.append(data.at(newIndex));
            it.setValue(newPlot);
        }
    
    



plasma/widgets/signalplotter.cpp
<http://git.reviewboard.kde.org/r/108992/#comment20894>

    this is unlikely to have any impact; PODs are cheap and compilers are smart.



plasma/widgets/signalplotter.cpp
<http://git.reviewboard.kde.org/r/108992/#comment20895>

    const QList<double> &?



plasma/widgets/signalplotter.cpp
<http://git.reviewboard.kde.org/r/108992/#comment20900>

    since we're changing things here, how about the rather more readable:
    
    QListIterator<double> it(newestData);
    it.toBack();
    while (it.hasPrevious()) {
       it.previous();



plasma/widgets/signalplotter.cpp
<http://git.reviewboard.kde.org/r/108992/#comment20896>

    this is exceedingly ugly and the PODs should not be incurring much overhead. keeping QList instantiation out of the loops makes sense, but not PODs.



plasma/widgets/signalplotter.cpp
<http://git.reviewboard.kde.org/r/108992/#comment20899>

    this can probably go (see next comment)



plasma/widgets/signalplotter.cpp
<http://git.reviewboard.kde.org/r/108992/#comment20898>

    why not just avoid the assignment (and the hard to read global instantiation) with:
    
    val = QString("%1 %2").arg(KGlobal::locale()->formatNumber(value, d->precision), d->unit);
    
    i mean, if we're going to ugly it up, let's ugly it up as efficiently as we can ;)



plasma/widgets/signalplotter.cpp
<http://git.reviewboard.kde.org/r/108992/#comment20897>

    unlikely to make any difference other than make the code harder to read.


- Aaron J. Seigo


On Feb. 17, 2013, 12:57 p.m., Raul Fernandes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108992/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2013, 12:57 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Description
> -------
> 
> - create variables and classes outside the loops
> - reserve space in QList if we know already how many items will be added (avoid unnecessary reallocations)
> - use const_iterator when possible
> - remove a useless call (p->setPen(Qt::NoPen) - it will be set latter before be used)
> - avoid multiplications (x3, x2, x1 and x0)
> 
> 
> Diffs
> -----
> 
>   plasma/widgets/signalplotter.cpp 8e9e294 
> 
> Diff: http://git.reviewboard.kde.org/r/108992/diff/
> 
> 
> Testing
> -------
> 
> I have tested with KDE 4.10 with no problems.
> I have seen a improvement of about 5% in drawPlots() function, the most expensive function in painting.
> 
> 
> Thanks,
> 
> Raul Fernandes
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20130222/b422347f/attachment-0001.html>