Merge lp:~vanvugt/compiz/fix-1005569.2 into lp:compiz/0.9.8

Proposed by Daniel van Vugt
Status: Merged
Merged at revision: 3237
Proposed branch: lp:~vanvugt/compiz/fix-1005569.2
Merge into: lp:compiz/0.9.8
Diff against target: 30 lines (+9/-0)
2 files modified
include/core/option.h (+1/-0)
src/option.cpp (+8/-0)
To merge this branch: bzr merge lp:~vanvugt/compiz/fix-1005569.2
Reviewer Review Type Date Requested Status
Sam Spilsbury Approve
Review via email: mp+107941@code.launchpad.net

Description of the change

Calling CompOption::setName should not implicitly construct a new string
object every time, when the name is not changing. That wastes lots of CPU.
(LP: #1005569)

To post a comment you must log in.
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

If you don't like this, then look at the alternative:
https://code.launchpad.net/~vanvugt/compiz/fix-1005569.1/+merge/107942

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

23 + if (priv->name != name)
24 + priv->name = name;

I'm not entirely convinced as to why this is safe - it seems like this would either be comparing pointers to compare if strings are different, or doing a strcmp () (which would be slow per-frame)

review: Needs Information
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

It invokes:
bool operator!= ( const string& lhs, const char* rhs );
[http://www.cplusplus.com/reference/string/operators/]

A string comparison can be done typically in under 100 clock ticks, so it's fast. And most importantly it avoids heap operations (relatively expensive) which are incurred by re-assigning priv->name unnecessarily.

This solution also avoids constructing/destructing a temporary CompString parameter. However I imagine that would be done on the stack so maybe we could eliminate the new function and just add "if (priv->name != name)" to the existing setName... ?

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Actually, no. Even constructing a temporary string object on the stack will cause some heap to be allocated to store the dynamic character array. Therefore, this new function is still required to ensure setName can work without touching the heap. And avoiding the heap is the most important thing here.

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

I suppose this makes sense then, however, client code should not be resetting the name all the time (since there is still the cost of the strcmp)

review: Approve
Revision history for this message
Andrea Azzarone (azzar1) wrote :

What about using
void setName (CompString const& name, ...)

Just curious :)

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

void setName (CompString const& name, ...)
would re-introduce the bug we're trying to fix! :)

Because it requires a CompString temporary to be constructed.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/core/option.h'
2--- include/core/option.h 2012-02-16 08:30:33 +0000
3+++ include/core/option.h 2012-05-30 09:30:35 +0000
4@@ -238,6 +238,7 @@
5 ~CompOption ();
6
7 void setName (CompString name, Type type);
8+ void setName (const char *name, Type type);
9
10 void reset ();
11
12
13=== modified file 'src/option.cpp'
14--- src/option.cpp 2012-04-19 11:59:20 +0000
15+++ src/option.cpp 2012-05-30 09:30:35 +0000
16@@ -440,6 +440,14 @@
17 priv->type = type;
18 }
19
20+void
21+CompOption::setName (const char *name, CompOption::Type type)
22+{
23+ if (priv->name != name)
24+ priv->name = name;
25+ priv->type = type;
26+}
27+
28 CompString
29 CompOption::name ()
30 {

Subscribers

People subscribed via source and target branches