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

[Daniel Dunbar]

On Fri, Mar 9, 2012 at 12:23 PM, Daniel Dunbar <daniel at zuster.org> wrote:
> 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.

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

>
>  - 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?
>>>>
>>>>