Re: [ANNOUNCE] Git v2.19.0-rc0
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Aug 22, 2018, 17:04 UTC
- Message-ID
- <387295e3-3112-ed09-d988-3d8156e3c7ce@gmail.com>
- In-Reply-To
- <CACsJy8By49nz6-q78GuMYP8ssrjhWYyhYrJcL8eWpi4-mZzzxA@mail.gmail.com>
On 8/22/2018 12:58 PM, Duy Nguyen wrote:
Show 32 quoted lines
> On Wed, Aug 22, 2018 at 6:49 PM Derrick Stolee <stolee@gmail.com> wrote: >> On 8/22/2018 12:26 PM, Jeff King wrote: >>> On Wed, Aug 22, 2018 at 06:14:24PM +0200, Duy Nguyen wrote: >>> >>>> On Wed, Aug 22, 2018 at 6:08 PM Duy Nguyen <pclouds@gmail.com> wrote: >>>>> On Wed, Aug 22, 2018 at 6:03 PM Jeff King <peff@peff.net> wrote: >>>>>> On Wed, Aug 22, 2018 at 07:14:42AM -0400, Derrick Stolee wrote: >>>>>> >>>>>>> The other thing I was going to recommend (and I'll try to test this out >>>>>>> myself later) is to see if 'the_hash_algo->rawsz' is being treated as a >>>>>>> volatile variable, since it is being referenced through a pointer. Perhaps >>>>>>> storing the value locally and then casing on it would help? >>>>>> I tried various sprinkling of "const" around the declarations to make it >>>>>> clear that the values wouldn't change once we saw them. But I couldn't >>>>>> detect any difference. At most I think that would let us hoist the "if" >>>>>> out of the loop, but gcc still seems unwilling to expand the memcmp when >>>>>> there are other branches. >>>>>> >>>>>> I think if that's the thing we want to have happen, we really do need to >>>>>> just write it out on that branch rather than saying "memcmp". >>>>> This reminds me of an old discussion about memcpy() vs doing explicit >>>>> compare loop with lots of performance measurements.. >>>> Ah found it. Not sure if it is still relevant in light of multiple hash support >>>> >>>> https://public-inbox.org/git/20110427225114.GA16765@elte.hu/ >>> Yes, that was what I meant. We actually did switch to that hand-rolled >>> loop, but later we went back to memcmp in 0b006014c8 (hashcmp: use >>> memcmp instead of open-coded loop, 2017-08-09). >> Looking at that commit, I'm surprised the old logic was just a for loop, instead of a word-based approach, such as the following: > Might work on x86 but it breaks on cpu architectures with stricter > alignment. I don't think we have a guarantee that object_id is always > 8 byte aligned.
You (and Peff) are probably correct here, which is unfortunate. I'm not familiar with alignment constraints, but assume that such a word-based approach is best.
Thanks, -Stolee