Re: [PATCH 0/9] introducing oideq()

[Derrick Stolee <stolee@gmail.com>]

On 8/26/2018 4:56 PM, brian m. carlson wrote:
> On Sat, Aug 25, 2018 at 04:00:31AM -0400, Jeff King wrote:
>> This is a follow-up to the discussion in:
>>
>>    https://public-inbox.org/git/20180822030344.GA14684@sigill.intra.peff.net/
>>
>> The general idea is that the majority of callers don't care about actual
>> plus/minus ordering from oidcmp() and hashcmp(); they just want to know
>> if two oids are equal. The stricter equality check can be optimized
>> better by the compiler.
>>
>> Due to the simplicity of the current code and our inlining, the compiler
>> can usually figure this out for now. So I wouldn't expect this patch to
>> actually improve performance right away. But as that discussion shows,
>> we are likely to take a performance hit as we move to more runtime
>> determination of the_hash_algo parameters. Having these callers use the
>> more strict form will potentially help us recover that.
>>
>> So in that sense we _could_ simply punt on this series until then (and
>> it's certainly post-v2.19 material). But I think it's worth doing now,
>> simply from a readability/annotation standpoint. IMHO the resulting code
>> is more clear (though I've long since taught myself to read !foocmp() as
>> equality).
> I would quite like to see this series picked up for v2.20.  If we want
> to minimize performance regressions with the SHA-256 work, I think it's
> important.

Seconded.

> Applying the following patch on top of this series causes gcc to inline
> both branches, which is pretty much the best we can expect.  I haven't
> benchmarked it, though, so I can't say what the actual performance
> consequence is.

We should definitely measure the cost here, but when we have the option 
to move to newhash, that cost should be acceptable.

Thanks,

-Stolee