On Wed, Sep 25, 2024 at 09:58:47AM -0700, Elijah Newren wrote: > > Co-authored-by: Jeff King <peff@xxxxxxxx> > > Signed-off-by: Jeff King <peff@xxxxxxxx> > > I don't even get a Helped-by for finding the cause of the pile-up of > temporary object files and the suggested tweak to this patch? ;-) Oops, that is definitely an oversight. I'll add your Helped-by to the next version (though it really should probably be a My-bacon-was-saved-by trailer ;-)). > > 9: 4018261366f ! 8: 4b83dd05e9f csum-file.c: use fast SHA-1 implementation when available > > @@ Metadata > > Author: Taylor Blau <me@xxxxxxxxxxxx> > > > > ## Commit message ## > > - csum-file.c: use fast SHA-1 implementation when available > > + csum-file.c: use unsafe SHA-1 implementation when available > > > > - Update hashwrite() and friends to use the fast_-variants of hashing > > - functions, calling for e.g., "the_hash_algo->fast_update_fn()" instead > > + Update hashwrite() and friends to use the unsafe_-variants of hashing > > + functions, calling for e.g., "the_hash_algo->unsafe_update_fn()" instead > > of "the_hash_algo->update_fn()". > > > > These callers only use the_hash_algo to produce a checksum, which we > > depend on for data integrity, but not for cryptographic purposes, so > > - these callers are safe to use the fast (and potentially non-collision > > + these callers are safe to use the unsafe (and potentially non-collision > > detecting) SHA-1 implementation. > > Is the "and potentially non-collision detecting" parenthetical comment > still needed now that it's referred to as unsafe? Even if we keep > most of it, maybe we should drop the "and"? Yeah, I think that's a good idea. Without specifying any of the new build knobs, the "unsafe" function pointers are identical to the safe ones, which is why I wrote "potentially" here. But I think that's a semantic argument that is confusing at best and misleading at worst, so I like your suggestion to make the parenthetical just read "(non-collision detecting)". > > @@ Commit message > > , and the resulting "clone" takes 19.219 seconds of wall clock time, > > 18.94 seconds of user time and 0.28 seconds of system time. > > > > - Compiling with OPENSSL_SHA1_FAST=1, we spend ~60% fewer instructions in > > - hashwrite(): > > + Compiling with OPENSSL_SHA1_UNSAFE=1, we spend ~60% fewer instructions > > + in hashwrite(): > > > > $ callgrind_annotate --inclusive=yes | grep hashwrite | head -n1 > > 59,164,001,176 (58.79%) /home/ttaylorr/src/git/csum-file.c:hashwrite [/home/ttaylorr/src/git/git-pack-objects] > > > > - , and generate the resulting "clone" much faster, in only 11.597 seconds > > + , and generate the resulting "clone" much unsafeer, in only 11.597 seconds > > This fast->unsafe translation isn't so good; this one should be undone. Oops. Great catch, thanks. Thanks, Taylor
