D7107: Move QMenu allocation from heap to stack

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D7107: Move QMenu allocation from heap to stack

Francisco Navarro Morales
View Revision
dkurz created this revision.
dkurz added a project: KDE PIM.
Restricted Application added a subscriber: KDE PIM.

REPOSITORY
R206 KMail


AFFECTED FILES
src/searchdialog/searchwindow.cpp

To: dkurz, KDE PIM: KMail
Cc: KDE PIM, dvasin, winterz, vkrause, mlaurent, knauss, dvratil
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D7107: Move QMenu allocation from heap to stack

Francisco Navarro Morales
View Revision
mlaurent added a comment.

Why ? What is the improvement ?


REPOSITORY
R206 KMail


To: dkurz, KDE PIM: KMail
Cc: mlaurent, KDE PIM, dvasin, winterz, vkrause, knauss, dvratil
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D7107: Move QMenu allocation from heap to stack

Francisco Navarro Morales
In reply to this post by Francisco Navarro Morales
View Revision
dkurz added a comment.

We save one heap allocation. Stack allocations are always cheaper. Also, we reduce the risk of a memory leak, when for some reason the "delete menu" isn't reached.


REPOSITORY
R206 KMail


To: dkurz, KDE PIM: KMail
Cc: mlaurent, KDE PIM, dvasin, winterz, vkrause, knauss, dvratil
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D7107: Move QMenu allocation from heap to stack

Francisco Navarro Morales
In reply to this post by Francisco Navarro Morales
View Revision
mlaurent added a comment.

As I see there is a delete menu without other branch so for me it's not really useful.
Regards


REPOSITORY
R206 KMail


To: dkurz, KDE PIM: KMail
Cc: mlaurent, KDE PIM, dvasin, winterz, vkrause, knauss, dvratil
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D7107: Move QMenu allocation from heap to stack

Francisco Navarro Morales
In reply to this post by Francisco Navarro Morales
View Revision
dkurz added a comment.

But we still save an inefficient heap allocation. That's already useful in itself. And there's really no point in creating this QMenu on the heap here.


REPOSITORY
R206 KMail


To: dkurz, KDE PIM: KMail
Cc: mlaurent, KDE PIM, dvasin, winterz, vkrause, knauss, dvratil
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D7107: Move QMenu allocation from heap to stack

Francisco Navarro Morales
In reply to this post by Francisco Navarro Morales
View Revision
dvratil added a comment.

From my point of view, I think it's a harmless change that makes the code better readable, safer and faster (even though the performance gain here has a negligible impact). I don't see a reason to not merge it.


REPOSITORY
R206 KMail


To: dkurz, KDE PIM: KMail
Cc: dvratil, mlaurent, KDE PIM, dvasin, winterz, vkrause, knauss
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D7107: Move QMenu allocation from heap to stack

Francisco Navarro Morales
In reply to this post by Francisco Navarro Morales
View Revision
dkurz added a comment.

So now we have readability, memory safety and performance on the plus side. To further back my performance claim, [1] investigates the difference between stack and heap allocation. Short answer: depends on the platform, but stack allocation can easily be faster by two orders of magnitude for a simple single-threaded program. KMail is multithreaded, so we can expect bigger differences: Heap allocation causes threads to synchronize on most platforms, because the heap is a shared resource (see e.g. [2]). The kind of allocation also hints at the expected lifetime of an object (in C++; in Qt, this is somewhat questionable).

[1] https://stackoverflow.com/questions/161053/which-is-faster-stack-allocation-or-heap-allocation
[2] https://stackoverflow.com/questions/1665419/do-threads-have-a-distinct-heap


REPOSITORY
R206 KMail


To: dkurz, KDE PIM: KMail
Cc: dvratil, mlaurent, KDE PIM, dvasin, winterz, vkrause, knauss
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D7107: Move QMenu allocation from heap to stack

Francisco Navarro Morales
In reply to this post by Francisco Navarro Morales
View Revision
smartins added a comment.

stack is always a good default

Note however that the performance claims don't apply here because menu.exec() is a "time bottleneck", if you call this function 100000 times you can't say it ran noticeably faster, since all the time was spent in exec().

anyway, +1, it's good to have code without new and delete


REPOSITORY
R206 KMail


To: dkurz, KDE PIM: KMail
Cc: smartins, dvratil, mlaurent, KDE PIM, dvasin, winterz, vkrause, knauss
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D7107: Move QMenu allocation from heap to stack

Francisco Navarro Morales
In reply to this post by Francisco Navarro Morales
View Revision
dvratil accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
R206 KMail


To: dkurz, KDE PIM: KMail, dvratil
Cc: smartins, dvratil, mlaurent, KDE PIM, dvasin, winterz, vkrause, knauss
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D7107: Move QMenu allocation from heap to stack

Francisco Navarro Morales
In reply to this post by Francisco Navarro Morales
View Revision
dkurz closed this revision.

REPOSITORY
R206 KMail


To: dkurz, KDE PIM: KMail, dvratil
Cc: smartins, dvratil, mlaurent, KDE PIM, dvasin, winterz, vkrause, knauss
Loading...