Nux

Merge lp:~3v1n0/nux/callgrind-improvements into lp:nux

Proposed by Marco Trevisan (Treviño) on 2013-04-10
Status: Merged
Approved by: Brandon Schaefer on 2013-04-12
Approved revision: 778
Merged at revision: 773
Proposed branch: lp:~3v1n0/nux/callgrind-improvements
Merge into: lp:nux
Prerequisite: lp:~3v1n0/nux/rect-c++11
Diff against target: 67 lines (+12/-7)
4 files modified
Nux/WindowThread.cpp (+4/-5)
Nux/WindowThread.h (+1/-1)
configure.ac (+1/-1)
debian/changelog (+6/-0)
To merge this branch: bzr merge lp:~3v1n0/nux/callgrind-improvements
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve on 2013-04-13
Andrea Azzarone (community) Approve on 2013-04-11
Michi Henning (community) 2013-04-10 Approve on 2013-04-11
Review via email: mp+158233@code.launchpad.net

Commit message

WindowThread: return a const& in GetDrawList

This will reduces a lot the copies that unity has to do in very-often called functions

Description of the change

Returning a const& we have practically no cost when running this in unity, see this pre/after comparison:

http://ubuntuone.com/3ukWhqzPdcBreLxTNayzBv

To post a comment you must log in.
lp:~3v1n0/nux/callgrind-improvements updated on 2013-04-10
778. By Marco Trevisan (Treviño) on 2013-04-10

configure.ac: increase ABI version

Michi Henning (michihenning) wrote :

Geometry const& geo = view->GetAbsoluteGeometry();
parent = view->GetToplevel();

As far as I can see, if this actually works, it works only by accident. This binds a C++ reference to the return value of a function. The return value is a temporary that will disappear the instant the function completes.

Further down in the function, we have:

m_dirty_areas.push_back(geo);

This looks like it pushes a dangling reference.

review: Needs Fixing
Andrea Azzarone (azzar1) wrote :

> Geometry const& geo = view->GetAbsoluteGeometry();
> parent = view->GetToplevel();
>
> As far as I can see, if this actually works, it works only by accident. This
> binds a C++ reference to the return value of a function. The return value is a
> temporary that will disappear the instant the function completes.
>

Are you sure? The standards says:

«...
C obj1 ;
const C & cr = C (16)+ C (23);
C obj2 ;

the expression C(16)+C(23) creates three temporaries. A first temporary T1 to hold the result of the expression C(16), a second temporary T2 to hold the result of the expression C(23), and a third temporary T3 to hold the result of the addition of these two expressions. The temporary T3 is then bound to the reference cr. It is unspecified whether T1 or T2 is created first. On an implementation where T1 is created before T2, it is guaranteed that T2 is destroyed before T1.
The temporaries T1 and T2 are bound to the reference parameters of operator+; these temporaries are destroyed at the
end of the full expression containing the call to operator+. **** The temporary T3 bound to the reference cr is destroyed at the end of cr’s lifetime, that is, at the end of the program... *** »

> Further down in the function, we have:
>
> m_dirty_areas.push_back(geo);
>
> This looks like it pushes a dangling reference.

Andrea Azzarone (azzar1) wrote :

*standard

Michi Henning (michihenning) wrote :

Oops, me bad, you are right. The const makes it legal.

review: Approve
Andrea Azzarone (azzar1) :
review: Approve
Marco Trevisan (Treviño) (3v1n0) wrote :

Il giorno 11/apr/2013 03:19, "Michi Henning" <email address hidden>
ha scritto:
>
> Review: Approve
>
> Oops, me bad, you are right. The const makes it legal.

Yep, in any case the move ctor that rect now has would make this implicit.

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'Nux/WindowThread.cpp'
2--- Nux/WindowThread.cpp 2013-02-01 09:14:07 +0000
3+++ Nux/WindowThread.cpp 2013-04-10 22:45:26 +0000
4@@ -1352,11 +1352,10 @@
5 void WindowThread::AddToDrawList(View *view)
6 {
7 Area *parent;
8- Geometry geo, pgeo;
9-
10- geo = view->GetAbsoluteGeometry();
11+
12+ Geometry const& geo = view->GetAbsoluteGeometry();
13 parent = view->GetToplevel();
14-
15+
16 if (parent && (view != parent))
17 {
18 // pgeo = parent->GetGeometry();
19@@ -1385,7 +1384,7 @@
20 m_dirty_areas.clear();
21 }
22
23- std::vector<Geometry> WindowThread::GetDrawList()
24+ std::vector<Geometry> const& WindowThread::GetDrawList() const
25 {
26 return m_dirty_areas;
27 }
28
29=== modified file 'Nux/WindowThread.h'
30--- Nux/WindowThread.h 2013-02-01 09:14:07 +0000
31+++ Nux/WindowThread.h 2013-04-10 22:45:26 +0000
32@@ -323,7 +323,7 @@
33
34 void ClearDrawList();
35
36- std::vector<Geometry> GetDrawList();
37+ std::vector<Geometry> const& GetDrawList() const;
38
39 #ifdef NUX_GESTURES_SUPPORT
40 /*!
41
42=== modified file 'configure.ac'
43--- configure.ac 2013-04-10 22:45:26 +0000
44+++ configure.ac 2013-04-10 22:45:26 +0000
45@@ -23,7 +23,7 @@
46 # e.g.: december 5th, 2011 is: 20111205
47 # To make more than one API change in a day, add a number to the date. Like 20111205.xx
48
49-m4_define([nux_abi_version], [20130408.1])
50+m4_define([nux_abi_version], [20130411.0])
51
52 m4_define([nux_version],
53 [nux_major_version.nux_minor_version.nux_micro_version])
54
55=== modified file 'debian/changelog'
56--- debian/changelog 2013-03-25 04:02:27 +0000
57+++ debian/changelog 2013-04-10 22:45:26 +0000
58@@ -1,3 +1,9 @@
59+nux (4.0.1-0ubuntu1) UNRELEASED; urgency=low
60+
61+ * Buming version to 4.0.1 as per recent ABI changes
62+
63+ -- Marco Trevisan (Treviño) <marco@ubuntu.com> Mon, 08 Apr 2013 17:21:37 +0200
64+
65 nux (4.0.0daily13.03.25-0ubuntu1) raring; urgency=low
66
67 [ Brandon Schaefer ]

Subscribers

People subscribed via source and target branches