From: Derrick Stolee Date: Mon, 27 Aug 2018 12:41:32 GMT Subject: Re: [PATCH 0/9] introducing oideq() Message-ID: In-Reply-To: <20180826205620.GC873448@genre.crustytoothpaste.net> 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