Review Request 108992: Simple optimizations in SignalPlotter

2013-02-17 Thread Raul Fernandes 

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

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

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 108992: Simple optimizations in SignalPlotter

2014-05-05 Thread Raul Fernandes 

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

(Updated May 5, 2014, 10:54 p.m.)


Status
--

This change has been discarded.


Review request for Plasma.


Repository: kdelibs


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: https://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

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 108992: Simple optimizations in SignalPlotter

2013-02-17 Thread Aleix Pol Gonzalez 

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


I don't see loosening the variables' scope as a codebase improvement. Mostly 
otherwise.

Also I'd like to know how you measured this 5% of improvement, which either way 
I'm unsure if it's worth it considering that this patch makes everything 
global, now.

- Aleix Pol Gonzalez


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
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 108992: Simple optimizations in SignalPlotter

2013-02-20 Thread Marco Martin 

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

Ship it!


as long their scope is still local to the function is fine.
patch looks good (not that the code of that plotter widget is a beauty queen 
anyways)

- Marco Martin


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
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 108992: Simple optimizations in SignalPlotter

2013-02-20 Thread Raul Fernandes 


> On Feb. 18, 2013, 1:39 a.m., Aleix Pol Gonzalez wrote:
> > I don't see loosening the variables' scope as a codebase improvement. 
> > Mostly otherwise.
> > 
> > Also I'd like to know how you measured this 5% of improvement, which either 
> > way I'm unsure if it's worth it considering that this patch makes 
> > everything global, now.

I think it is the worst response that I can have.
Never in my entire life I saw anyone that complains about creating classes 
outside loops looses the scope because it is one of the most basic forms of 
optimizing the code. Creating classes inside loops is a great waste of 
resources.
That why KDE4 is still bloat and slow.
Is is so true that KDE3 is still alive.
I think some developers should learn how to write better and fast code and 
avoid some commentaries like this or "This is only one more full update. Who 
cares??" (I saw this insanity in some place in KDE's code).
This patch is one of those that I should not put in review because it is so 
basic, but I do because I don't want to commit anything without approval.


- Raul


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


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
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 108992: Simple optimizations in SignalPlotter

2013-02-20 Thread Raul Fernandes 

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

Ship it!


Ship It!

- Raul Fernandes


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
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 108992: Simple optimizations in SignalPlotter

2013-02-21 Thread Sebastian Kügler 
Hi,

> On February 18th, 2013, 1:39 a.m. UTC, Aleix Pol Gonzalez wrote:
> 
> I don't see loosening the variables' scope as a codebase improvement. Mostly
> otherwise.
> 
> Also I'd like to know how you measured this 5% of improvement, which either
> way I'm unsure if it's worth it considering that this patch makes
> everything global, now. 

On Tuesday, February 19, 2013 13:47:37 Raul Fernandes wrote:
> I think it is the worst response that I can have.
> Never in my entire life I saw anyone that complains about creating classes
> outside loops looses the scope because it is one of the most basic forms of
> optimizing the code. Creating classes inside loops is a great waste of
> resources. That why KDE4 is still bloat and slow.
> Is is so true that KDE3 is still alive.
> I think some developers should learn how to write better and fast code and
> avoid some commentaries like this or "This is only one more full update.
> Who cares??" (I saw this insanity in some place in KDE's code). 

So the way we go about topical criticism in the Plasma team is to be calm and 
explain things, not to resort to personal or even project-wide attacks. If 
someone doesn't understand your patch, or has a different point of view, 
that's no reason to become overly defensive, offensive or even condescending.

In this case, Aleix has taken time out of his busy schedule to review your 
patch. He's met with unjustified hostility, however. Not good.

We try to keep the discourse in the Plasma team productive and friendly, 
please help with that.

> This patch
> is one of those that I should not put in review because it is so basic, but
> I do because I don't want to commit anything without approval.

Code reviews are an important means to maintain code quality. If a patch is 
"too basic to be reviewed", that says more about the person judging than about 
the code in question.

Do you have a git account? If not I can commit the patch for you.

More importantly: Please keep things friendly, we want Plasma to be an 
enjoyable place, not a place where people disrespect each other. No code 
improvement is worth that.

Cheers,
-- 
sebas

http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 108992: Simple optimizations in SignalPlotter

2013-02-21 Thread Aaron J. Seigo 


> On Feb. 18, 2013, 1:39 a.m., Aleix Pol Gonzalez wrote:
> > I don't see loosening the variables' scope as a codebase improvement. 
> > Mostly otherwise.
> > 
> > Also I'd like to know how you measured this 5% of improvement, which either 
> > way I'm unsure if it's worth it considering that this patch makes 
> > everything global, now.
> 
> Raul Fernandes wrote:
> I think it is the worst response that I can have.
> Never in my entire life I saw anyone that complains about creating 
> classes outside loops looses the scope because it is one of the most basic 
> forms of optimizing the code. Creating classes inside loops is a great waste 
> of resources.
> That why KDE4 is still bloat and slow.
> Is is so true that KDE3 is still alive.
> I think some developers should learn how to write better and fast code 
> and avoid some commentaries like this or "This is only one more full update. 
> Who cares??" (I saw this insanity in some place in KDE's code).
> This patch is one of those that I should not put in review because it is 
> so basic, but I do because I don't want to commit anything without approval.

Raul, thanks for the patch. We appreciate contributions such as these, 
particularly ones that work on improving what exists .. and this bit of code 
definitely could use it.

However, we do not interact with each other in the way you did in your comment 
here. We try to show each other respect and understanding. Not only do we find 
more enjoyment and team spirit this way, but it prevents people from treating 
us the same way. There are issues with your patch which I cover in my review, 
but I don't suggest you ought to learn how to write better code or otherwise 
insult you. What would that achieve, besides getting on your nerves? Nothing.

You did not answer Aleix's question about how you measured the improvement, 
which is a very valid question. The least one can do is respect an honest 
question with an honest answer, yes?

Our code of conduct describes our commitments to each other in these things: 
http://www.kde.org/code-of-conduct/

I hope you can receive this comment in the constructive manner which I intend 
it, and look forward to more of your patches in the future. :)


- Aaron J.


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


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
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 108992: Simple optimizations in SignalPlotter

2013-02-23 Thread 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


no point in taking PODs out of the loop



plasma/widgets/signalplotter.cpp


QList &data



plasma/widgets/signalplotter.cpp


er .. this does nothing.

the data member in the foreach loop is a *copy* of the QList from 
d->plotData. assigning to it just assigns to that copy which only has scope 
within the foreach.

this *ought* to be:

QMutableListIterator > it(d->plotData);
while (it.hasNext()) {
it.next();
newPlot.clear();
newPlot.append(data.at(newIndex));
it.setValue(newPlot);
}





plasma/widgets/signalplotter.cpp


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



plasma/widgets/signalplotter.cpp


const QList &?



plasma/widgets/signalplotter.cpp


since we're changing things here, how about the rather more readable:

QListIterator it(newestData);
it.toBack();
while (it.hasPrevious()) {
   it.previous();



plasma/widgets/signalplotter.cpp


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


this can probably go (see next comment)



plasma/widgets/signalplotter.cpp


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


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
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 108992: Simple optimizations in SignalPlotter

2013-02-23 Thread Raul Fernandes 


> On Feb. 18, 2013, 1:39 a.m., Aleix Pol Gonzalez wrote:
> > I don't see loosening the variables' scope as a codebase improvement. 
> > Mostly otherwise.
> > 
> > Also I'd like to know how you measured this 5% of improvement, which either 
> > way I'm unsure if it's worth it considering that this patch makes 
> > everything global, now.
> 
> Raul Fernandes wrote:
> I think it is the worst response that I can have.
> Never in my entire life I saw anyone that complains about creating 
> classes outside loops looses the scope because it is one of the most basic 
> forms of optimizing the code. Creating classes inside loops is a great waste 
> of resources.
> That why KDE4 is still bloat and slow.
> Is is so true that KDE3 is still alive.
> I think some developers should learn how to write better and fast code 
> and avoid some commentaries like this or "This is only one more full update. 
> Who cares??" (I saw this insanity in some place in KDE's code).
> This patch is one of those that I should not put in review because it is 
> so basic, but I do because I don't want to commit anything without approval.
> 
> Aaron J. Seigo wrote:
> Raul, thanks for the patch. We appreciate contributions such as these, 
> particularly ones that work on improving what exists .. and this bit of code 
> definitely could use it.
> 
> However, we do not interact with each other in the way you did in your 
> comment here. We try to show each other respect and understanding. Not only 
> do we find more enjoyment and team spirit this way, but it prevents people 
> from treating us the same way. There are issues with your patch which I cover 
> in my review, but I don't suggest you ought to learn how to write better code 
> or otherwise insult you. What would that achieve, besides getting on your 
> nerves? Nothing.
> 
> You did not answer Aleix's question about how you measured the 
> improvement, which is a very valid question. The least one can do is respect 
> an honest question with an honest answer, yes?
> 
> Our code of conduct describes our commitments to each other in these 
> things: http://www.kde.org/code-of-conduct/
> 
> I hope you can receive this comment in the constructive manner which I 
> intend it, and look forward to more of your patches in the future. :)

Sorry about that but it was a reaction to one thing that always happens in open 
software.
It is sometimes hard to a new contributor send patches to projects because the 
maintainers tend to reject new views from people outside. Sometimes in a rough 
way.
I have several patches optimizing the code in KDE and cannot send upstream 
because this way of thinking.
There are lot of developers in KDE that doesn't care about speed, maybe because 
they use superfast computers.
But I use a core i5 third generation in work place and the KDE is laggy to me. 
Even the 4.10.
This is why the KDE3 is still alive and the razor-qt exists. The reason they 
use is the speed.
Considering the speed of computers today, the KDE should executes smooothly in 
any hardware with all effects active.
Answering how I discovered the 5% of improvement I have profiled the code.
I didn't attach the output of profiler because it takes a lot of space and the 
patch seems too trivial to me.
This is the point. If such trivial patch have this reception, imagine other 
that makes huge changes in algoritms choosing one more complicated for the sake 
of speed.
In this case I have one. Grouping all drawPath calls to avoid the context 
switching in QPainter.


- Raul


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


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
> 
>

___
Plasma-devel mailing list
Plasma-d

Re: Review Request 108992: Simple optimizations in SignalPlotter

2013-02-23 Thread Raul Fernandes 


> On Feb. 18, 2013, 1:39 a.m., Aleix Pol Gonzalez wrote:
> > I don't see loosening the variables' scope as a codebase improvement. 
> > Mostly otherwise.
> > 
> > Also I'd like to know how you measured this 5% of improvement, which either 
> > way I'm unsure if it's worth it considering that this patch makes 
> > everything global, now.
> 
> Raul Fernandes wrote:
> I think it is the worst response that I can have.
> Never in my entire life I saw anyone that complains about creating 
> classes outside loops looses the scope because it is one of the most basic 
> forms of optimizing the code. Creating classes inside loops is a great waste 
> of resources.
> That why KDE4 is still bloat and slow.
> Is is so true that KDE3 is still alive.
> I think some developers should learn how to write better and fast code 
> and avoid some commentaries like this or "This is only one more full update. 
> Who cares??" (I saw this insanity in some place in KDE's code).
> This patch is one of those that I should not put in review because it is 
> so basic, but I do because I don't want to commit anything without approval.
> 
> Aaron J. Seigo wrote:
> Raul, thanks for the patch. We appreciate contributions such as these, 
> particularly ones that work on improving what exists .. and this bit of code 
> definitely could use it.
> 
> However, we do not interact with each other in the way you did in your 
> comment here. We try to show each other respect and understanding. Not only 
> do we find more enjoyment and team spirit this way, but it prevents people 
> from treating us the same way. There are issues with your patch which I cover 
> in my review, but I don't suggest you ought to learn how to write better code 
> or otherwise insult you. What would that achieve, besides getting on your 
> nerves? Nothing.
> 
> You did not answer Aleix's question about how you measured the 
> improvement, which is a very valid question. The least one can do is respect 
> an honest question with an honest answer, yes?
> 
> Our code of conduct describes our commitments to each other in these 
> things: http://www.kde.org/code-of-conduct/
> 
> I hope you can receive this comment in the constructive manner which I 
> intend it, and look forward to more of your patches in the future. :)
> 
> Raul Fernandes wrote:
> Sorry about that but it was a reaction to one thing that always happens 
> in open software.
> It is sometimes hard to a new contributor send patches to projects 
> because the maintainers tend to reject new views from people outside. 
> Sometimes in a rough way.
> I have several patches optimizing the code in KDE and cannot send 
> upstream because this way of thinking.
> There are lot of developers in KDE that doesn't care about speed, maybe 
> because they use superfast computers.
> But I use a core i5 third generation in work place and the KDE is laggy 
> to me. Even the 4.10.
> This is why the KDE3 is still alive and the razor-qt exists. The reason 
> they use is the speed.
> Considering the speed of computers today, the KDE should executes 
> smooothly in any hardware with all effects active.
> Answering how I discovered the 5% of improvement I have profiled the code.
> I didn't attach the output of profiler because it takes a lot of space 
> and the patch seems too trivial to me.
> This is the point. If such trivial patch have this reception, imagine 
> other that makes huge changes in algoritms choosing one more complicated for 
> the sake of speed.
> In this case I have one. Grouping all drawPath calls to avoid the context 
> switching in QPainter.

One thing good to say is the SignalPlotter continues to draw the contents to 
QPainter even it is not exposed, wasting CPU cycles that should be used by 
another thing.


- Raul


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


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
> --

Re: Review Request 108992: Simple optimizations in SignalPlotter

2013-02-23 Thread Raul Fernandes 


> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, line 230
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > const QList &?

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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > er .. this does nothing.
> > 
> > the data member in the foreach loop is a *copy* of the QList 
> > from d->plotData. assigning to it just assigns to that copy which only has 
> > scope within the foreach.
> > 
> > this *ought* to be:
> > 
> > QMutableListIterator > 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 newPlot;
QList data;
int newIndex;
QMutableListIterator > 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 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
> > 
> >
> > 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 variable

Re: Review Request 108992: Simple optimizations in SignalPlotter

2013-02-23 Thread Raul Fernandes 

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

(Updated Feb. 22, 2013, 7:17 p.m.)


Review request for Plasma.


Changes
---

Made some changes suggested by Aaron


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 (updated)
-

  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

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 108992: Simple optimizations in SignalPlotter

2013-02-26 Thread Aaron J. Seigo 


> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, line 230
> > 
> >
> > no point in taking PODs out of the loop
> 
> Raul Fernandes wrote:
> 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.
>

So we have a difference in opinion when it comes to readability, and that's 
what it is: an opinion. As the maintainer, my opinion wins in a tie :) My 
reason for this is that we end up with more variables outside the scope they 
are used which means to understand the loop one needs to read more outside the 
loop, and should other variables of similar name appear later on .

PODs have very, very little overhead and we value the readability over such 
micro-optimizations. to confirm this with actual numbers i wrote a small test 
program that interates 10 million times performing simple math on 6 ints that 
are declared in the loop or out of it. result? neither exceeded 1ms. so in this 
case, it is not worth it.

Conclusion: PODs will remain in the loops. Thanks :)


- Aaron J.


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


On Feb. 22, 2013, 7:17 p.m., Raul Fernandes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108992/
> ---
> 
> (Updated Feb. 22, 2013, 7:17 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
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 108992: Simple optimizations in SignalPlotter

2013-03-08 Thread Raul Fernandes 


> On Feb. 22, 2013, 1:32 a.m., Aaron J. Seigo wrote:
> > plasma/widgets/signalplotter.cpp, line 230
> > 
> >
> > no point in taking PODs out of the loop
> 
> Raul Fernandes wrote:
> 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.
>
> 
> Aaron J. Seigo wrote:
> So we have a difference in opinion when it comes to readability, and 
> that's what it is: an opinion. As the maintainer, my opinion wins in a tie :) 
> My reason for this is that we end up with more variables outside the scope 
> they are used which means to understand the loop one needs to read more 
> outside the loop, and should other variables of similar name appear later on 
> .
> 
> PODs have very, very little overhead and we value the readability over 
> such micro-optimizations. to confirm this with actual numbers i wrote a small 
> test program that interates 10 million times performing simple math on 6 ints 
> that are declared in the loop or out of it. result? neither exceeded 1ms. so 
> in this case, it is not worth it.
> 
> Conclusion: PODs will remain in the loops. Thanks :)

Ok, no problem. It is better to push these changes than push nothing. I will 
update the patch latter.


- Raul


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


On Feb. 22, 2013, 7:17 p.m., Raul Fernandes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108992/
> ---
> 
> (Updated Feb. 22, 2013, 7:17 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
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 108992: Simple optimizations in SignalPlotter

2013-04-05 Thread Albert Astals Cid 

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


Aaron can you re-review to see if the requests you made are fixed and this is 
ready to go?

- Albert Astals Cid


On Feb. 22, 2013, 7:17 p.m., Raul Fernandes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108992/
> ---
> 
> (Updated Feb. 22, 2013, 7:17 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
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 108992: Simple optimizations in SignalPlotter

2013-04-08 Thread Aaron J. Seigo 


> On April 5, 2013, 3:43 p.m., Albert Astals Cid wrote:
> > Aaron can you re-review to see if the requests you made are fixed and this 
> > is ready to go?

no, it still needs to be changed to address the points raised 


- Aaron J.


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


On Feb. 22, 2013, 7:17 p.m., Raul Fernandes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108992/
> ---
> 
> (Updated Feb. 22, 2013, 7:17 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
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request 108992: Simple optimizations in SignalPlotter

2014-01-11 Thread Albert Astals Cid 

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


Raul, can you please update the review to address Aaron's points or discard the 
review request?

- Albert Astals Cid


On Feb. 22, 2013, 7:17 p.m., Raul Fernandes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/108992/
> ---
> 
> (Updated Feb. 22, 2013, 7:17 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: kdelibs
> 
> 
> 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: https://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
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel