Review Request 108992: Simple optimizations in SignalPlotter

[Raul Fernandes]

> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, line 230
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line230>
> >
> >     no point in taking PODs out of the loop

Actually, the benefit exists in PODs too (the compiler has to adjust the stack each iteration) but is minor. I agree.
I don't agree that it uglifies the code or make it less readable.
To me, it doesn't make any difference to readability and has the (little) gain in performance. My opinion.


> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, lines 781-782
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line781>
> >
> >     this is unlikely to have any impact; PODs are cheap and compilers are smart.

I agree that the impact is minor, but if we educate the coders to always declare variables outside the loops the code will always be better and faster.


> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, line 784
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line784>
> >
> >     const QList<double> &?

Good point. I didn't see that.


> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, lines 1091-1092
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line1091>
> >
> >     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 ;)

Putting KGlobal::locale... in a new line will make more readable.


> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, line 1075
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line1075>
> >
> >     this can probably go (see next comment)

Good point.


> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, line 1100
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line1100>
> >
> >     unlikely to make any difference other than make the code harder to read.

Again, the benefit is minimal but educate the coders to always declare variables outside loops.
Really, I don't see any ugliness. Maybe because I used to code this way.


> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, lines 231-244
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line231>
> >
> >     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);
> >         }
> >     
> >

Sorry but you are wrong in one point.
Creating the classe inside the loop leads the compiler to create the object, make the copy and destroy the object in each iteration.
With my patch, it only makes the copy.
I have saw the constructors and destructors in profiler. After the patch, they disappear.
I think there are something wrong with the code too.
The data variable will only be a copy and the statement:

data = newPlot;

will not change the actual list.

I think the code should be (using your suggestion):

    QList<double> newPlot;
    QList<double> data;
    int newIndex;
    QMutableListIterator<QList<double> > it(d->plotData);
    while (it.hasNext()) {
        data = it.next();
        if (newOrder.count() != data.count()) {
            kDebug() << "Serious problem in move sample.  plotdata[i] has "
                     << data.count() << " and neworder has " << newOrder.count();
        } else {
            newPlot.clear();
            newPlot.reserve(newOrder.count());
            for (int i = 0; i < newOrder.count(); i++) {
                newIndex = newOrder[i];
                newPlot.append(data.at(newIndex));
            }
            it.setValue(newPlot);
        }
    }

Maybe creating data as const reference (const QList<double> data = it.next()) will avoid the copy and the constructor and destructor.
I will test it.


> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, lines 860-863
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line860>
> >
> >     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.

The gain is minimal but I think it is valid to educate the coders to always declare variables outside the loops.
It is a good practice. Do you agree?


> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, line 785
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line785>
> >
> >     since we're changing things here, how about the rather more readable:
> >     
> >     QListIterator<double> it(newestData);
> >     it.toBack();
> >     while (it.hasPrevious()) {
> >        it.previous();

It can't be done as is.
The variable i will be used latter (d->plotColors[i].darkColor), so we have to use it.


> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, line 231
> > <http://git.reviewboard.kde.org/r/108992/diff/1/?file=114209#file114209line231>
> >
> >     QList<double> &data

The idea is good but the problem is the data will be changed latter in the code.


- Raul


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


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/96543bba/attachment-0001.html>