Re: [PATCH v2] unpack-trees: avoid duplicate ODB lookups during checkout

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Apr 11, 2017, 20:42 UTC
Message-ID: <ad2b5a8c-f9d0-82cb-25b8-76c1922af7f5@jeffhostetler.com>
In-Reply-To: <001f55c6-a694-7dde-b14b-9d1dcc9c9a09@web.de>

On 4/10/2017 7:09 PM, René Scharfe wrote:

Show 17 quoted lines

> Am 10.04.2017 um 23:26 schrieb Jeff Hostetler:
>> On 4/8/2017 10:06 AM, René Scharfe wrote:
>>> Am 07.04.2017 um 17:53 schrieb git@jeffhostetler.com:
>>>> +            /* implicitly borrow buf[i-1] inside tree_desc[i] */
>>>> +            memcpy(&t[i], &t[i-1], sizeof(struct tree_desc));
>>>
>>> An assignment would be simpler:
>>>
>>>             t[i] = t[i - 1];
>>
>> True, but this might be a coin toss.  Maybe we should
>> see what the generated assembly looks like for each ??
>
> Clang, GCC and ICC inline that memcpy call; the assembly output is the
> same for both variants: https://godbolt.org/g/1q0YwK.  I guess you worry
> about compilers that are just bad at struct assignments (i.e. worse than
> calling memcpy)?  Do you have examples (just curious)?

Nice website!  Really!

Yes, my concern was that structure copies would do it field by field rather than just a block copy. No, I don't have any examples -- maybe just some very old brain cells.... :-)

And I just checked VS2015 and the structure copy is a few instructions shorter, but roughly the same.

>
> Assignments are easier on the eye of human readers in any case, and
> there is no way to silently get the size wrong.

agreed. thanks.

Show 10 quoted lines

>
>> I tried to hit the common cases.  This loop runs a lot
>> and I didn't want to put an O(n^2) thing in there to
>> look for any matching peer.  Most of the time we are
>> in a simple 2 or 3 way effort.  I didn't want to pay
>> for the looping/branching overhead for the obscure [4..8]
>> efforts.
>
> Makes sense, and it's a nice heuristic.  Perhaps it would be a good idea
> to document these choices in a comment?

Good point. Thanks.