Re: [PATCH v4 0/8] hash.h: support choosing a separate SHA-1 for non-cryptographic uses

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 25, 2024, 17:22 UTC
Message-ID: <ZvRGtnVVo0if3PH0@nand.local>
In-Reply-To: <CABPp-BEqWhSVSEArOu_45Bt8W0d8PWThSbWrYAEWKrcMsDGsHw@mail.gmail.com>

On Wed, Sep 25, 2024 at 09:58:47AM -0700, Elijah Newren wrote:

Show 5 quoted lines

> >          Co-authored-by: Jeff King <peff@peff.net>
> >          Signed-off-by: Jeff King <peff@peff.net>
>
> 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 ;-)).

Show 23 quoted lines

> >  9:  4018261366f !  8:  4b83dd05e9f csum-file.c: use fast SHA-1 implementation when available
> >     @@ Metadata
> >      Author: Taylor Blau <me@ttaylorr.com>
> >
> >       ## 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)".

Show 16 quoted lines

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