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@xxxxxxxxxxxxxxxxxxxxx/
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
