Here is a patch which adds APValue::swap and uses it.
I'm not sure I like it, anyone else have an opinion for putting it in or not?
- Daniel
This was actually something I was intending to investigate soon. A few thoughts:
I’d prefer to see operator= changed as follows, all the code which says ‘foo = APValue(…);’ left alone:
APValue &operator=(APValue RHS) {
swap(RHS);
return *this;
}
Copy elision will do the rest. I’d then prefer to see copy_from killed, with the implementation moved to APValue’s copy constructor (which should then be out-of-line).
Do you have any performance numbers for this?
I note that the patch also introduces:
void copy_from(const APValue &RHS);
and changes all:
- Result = Frame->Arguments[PVD->getFunctionScopeIndex()];
to:
+ Result.copy_from(Frame->Arguments[PVD->getFunctionScopeIndex()]);
My observation is that in C++ the syntax:
x = y;
is universally understood. Whereas the syntax:
x.copy_from(y);
is obviously readable, and yet it does immediately raise questions for the reader of the code: What does copy_from do that copy assignment doesn't? Additionally if APValue is ever going to be used in generic code, that generic code is probably going to use the syntax:
x = y;
I guess my message is: if there is common syntax for doing something in C++, prefer that syntax over inventing new syntax.
For swap, the member swap you've added looks good. Most types also add a namespace scope function:
inline void swap(APValue& x, APValue& y) {x.swap(y);}
to aid in generic code which will do:
using std::swap;
swap(x, y);
Disclaimer: These comments are made without the benefit of knowledge of APValue.
Howard
The attached patch is the direction I was thinking of. I believe the changes outside APValue are all redundant with this approach; this is a pure optimization of the existing interface. The ExtractSubobject change isn’t necessary, but it avoids some potentially-expensive copying. Some care is required to avoid it creating a cyclic value.
I’ve not got any performance numbers for this change yet.
apvalue-move-semantics.diff (5.58 KB)
Thanks Richard,
I'm not really a C++ guy, maybe I wasn't expecting copy elision to do
what I wanted. I'll try and look into your patch today and see why my
numbers look like for it.
- Daniel
Here is a patch which adds APValue::swap and uses it.
I'm not sure I like it, anyone else have an opinion for putting it in or not?
I note that the patch also introduces:
void copy_from(const APValue &RHS);
and changes all:
- Result = Frame->Arguments[PVD->getFunctionScopeIndex()];
to:
+ Result.copy_from(Frame->Arguments[PVD->getFunctionScopeIndex()]);
My observation is that in C++ the syntax:
x = y;
is universally understood. Whereas the syntax:
x.copy_from(y);
I wanted to make the copy assignment operator private to encourage
callers to see if they could use swap instead.
My motivation here seems confused though, per subsequent discussion.
- Daniel
I think you're just ahead of your time. Sounds like you're trying to encourage C++11 move assignment. 
(which I think would be a very good idea, but requires commitment to using C++11)
Howard
I think that it would be completely reasonable (maybe even, "insanely great"?) to put move constructors and assignment operators into llvm & clang if they help performance and are *optional* and ifdef'd on compiler support.
That would just mean that people with c++'11 compilers can get better build times, but that we remain compatible with older compilers.
-Chris
Usually we have to/have worked around the cases (you know, the usual
"don't return big things by value") - the most common things that
would benefit would be stuff that this already happens for (eg:
std::string). Anyone know of good candidates that we wouldn't
reasonably fix for the 98 case by removing the copies?
- David
I wanted to make the copy assignment operator private to encourage
callers to see if they could use swap instead.My motivation here seems confused though, per subsequent discussion.
I think you're just ahead of your time. Sounds like you're trying to encourage C++11 move assignment.
I think that it would be completely reasonable (maybe even, "insanely great"?) to put move constructors and assignment operators into llvm & clang if they help performance and are *optional* and ifdef'd on compiler support.
That would just mean that people with c++'11 compilers can get better build times, but that we remain compatible with older compilers.
Usually we have to/have worked around the cases (you know, the usual
"don't return big things by value") - the most common things that
would benefit would be stuff that this already happens for (eg:
std::string). Anyone know of good candidates that we wouldn't
reasonably fix for the 98 case by removing the copies?
We have some cases of SmallVectors and DenseMaps of objects with non-trivial copy ctors. It would spare us a few deep copies if the container would move them when it reallocates.
Not sure if it's worth introducing a ton of #ifdef'd code. Sounds like a maintenance nightmare to me.
- Ben
I wanted to make the copy assignment operator private to encourage
callers to see if they could use swap instead.My motivation here seems confused though, per subsequent discussion.
I think you're just ahead of your time. Sounds like you're trying to encourage C++11 move assignment.
I think that it would be completely reasonable (maybe even, "insanely great"?) to put move constructors and assignment operators into llvm & clang if they help performance and are *optional* and ifdef'd on compiler support.
That would just mean that people with c++'11 compilers can get better build times, but that we remain compatible with older compilers.
Usually we have to/have worked around the cases (you know, the usual
"don't return big things by value") - the most common things that
would benefit would be stuff that this already happens for (eg:
std::string). Anyone know of good candidates that we wouldn't
reasonably fix for the 98 case by removing the copies?We have some cases of SmallVectors and DenseMaps of objects with non-trivial copy ctors. It would spare us a few deep copies if the container would move them when it reallocates.
Not sure if it's worth introducing a ton of #ifdef'd code. Sounds like a maintenance nightmare to me.
I agree. It's also a testing nightmare, I'd hate to have another way
for "it worked for me".
- Daniel
The #ifdef'ing can be a pain.
I do not want to influence this decision one way or another. But perhaps it would be helpful to relate my experience/practice in libc++ on how I've handled this issue:
For adding move semantics to a class, I just #ifdef around the added members:
class SmallVector
{
public:
// The C++03 constructors & assignment
// ...
#ifdef RVALUE_REFS
SmallVector(SmallVector&&);
SmallVector& operator=(SmallVector&&);
#endif
// ...
};
In /using/ move semantics, I go ahead and define std::move in both C++03 and C++11 modes, and so code always looks like:
x = std::move(y);
For llvm you could do something like:
#ifndef RVALUE_REFS
namespace std
{
template <class T>
inline
const T&
move(const T& t)
{
return t;
}
}
#endif
So in C++03:
x = std::move(y);
is equivalent to:
x = y;
but in C++11 it becomes a move assignment, with no syntax change at the use site. And that is key to keeping this from becoming a total nightmare. #ifdef's are limited to class implementation, and do not leak out to class clients. Class clients can immediately move to C++11 syntax, whether or not they actually get the C++11 optimization.
For factory functions, there is absolutely nothing to do:
SmallVector
make_SmallVector(int data)
{
SmallVector v(data);
// ....
return v;
}
The above syntax works for both C++03 and C++11. Typically in both, RVO kicks in. In the relatively rare cases where RVO does not kick in, C++03 will copy the SmallVector out, and C++11 will move it out. And on the client side, it is also identical:
SmallVector v = make_SmallVector(data);
or
v = make_SmallVector(data);
This may not be the right time to migrate llvm to C++11. But whenever that time comes, the above is a reasonable approach for making that migration.
Howard
Thanks Richard,
I'm not really a C++ guy, maybe I wasn't expecting copy elision to do
what I wanted. I'll try and look into your patch today and see why my
numbers look like for it.
Just tried your patch, on my three standard tests I see 1-2% slowdown
from it. Maybe the code size bloat from inlining swap()? I didn't
investigate any more...
- Daniel
Thanks Richard,
I'm not really a C++ guy, maybe I wasn't expecting copy elision to do
what I wanted. I'll try and look into your patch today and see why my
numbers look like for it.Just tried your patch, on my three standard tests I see 1-2% slowdown
from it. Maybe the code size bloat from inlining swap()? I didn't
investigate any more...
Bah, I used the wrong baseline compiler. The slowdown I see is .4-.7%
which is more believable.
- Daniel
Thanks Richard,
I'm not really a C++ guy, maybe I wasn't expecting copy elision to do
what I wanted. I'll try and look into your patch today and see why my
numbers look like for it.Just tried your patch, on my three standard tests I see 1-2% slowdown
from it. Maybe the code size bloat from inlining swap()? I didn't
investigate any more...Bah, I used the wrong baseline compiler. The slowdown I see is .4-.7%
which is more believable.
Couldn't help myself, apparently.
Moving APValue::swap() out of line changed it to being a .75% win on
403.gcc/combine.c and OGF/NSBezierPath-OAExtensions.m, and no change
on JSC.
Seems good to me (with swap() out of line), but I'll leave it up to you.
- Daniel