[cfe-dev] [PATCH] Add APValue::swap

[Daniel Dunbar]

On Fri, Mar 9, 2012 at 12:22 PM, Daniel Dunbar <daniel at zuster.org> wrote:
> On Thu, Mar 8, 2012 at 10:37 AM, Daniel Dunbar <daniel at zuster.org> wrote:
>> 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

>
>  - Daniel
>
>>
>>  - Daniel
>>
>> On Wed, Mar 7, 2012 at 9:17 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>> 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.
>>>
>>> On Wed, Mar 7, 2012 at 5:15 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>>>
>>>> On Wed, Mar 7, 2012 at 3:05 PM, Daniel Dunbar <daniel at zuster.org> wrote:
>>>>>
>>>>> 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?
>>>>
>>>>
>>>> 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?
>>>
>>>