threads / rfc / 20725

RFC patchmake the new block-sha1 the default

Subject: [PATCH/RFC] make the new block-sha1 the default

## tl;dr

4 messages between Aug 25, 2009 and Aug 25, 2009. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Nicolas Pitre· Aug 25, 2009, 03:04 UTC · lore
... and remove support for linking against the openssl SHA1 code.

The block-sha1 implementation is not significantly worse and sometimes even faster than the openssl SHA1 implementation. This allows for getting rid of the dependency and runtime linking to openssl which is a relatively important source of latency when executing git commands.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---

OK... So here it is. After all, wanting to get rid of openssl is what started it all in the first place.

Show changes to 4 files +3 −27

INSTALL, Makefile, cache.h, configure.ac

diff --git a/INSTALL b/INSTALL
index ae7f750..55eb962 100644
--- a/INSTALL
+++ b/INSTALL
@@ -52,13 +52,6 @@ Issues of note:
 
 	- "zlib", the compression library. Git won't build without it.
 
-	- "openssl".  Unless you specify otherwise, you'll get the SHA1
-	  library from here.
-
-	  If you don't have openssl, you can use one of the SHA1 libraries
-	  that come with git (git includes the one from Mozilla, and has
-	  its own PowerPC and ARM optimized ones too - see the Makefile).
-
 	- libcurl library; git-http-fetch and git-fetch use them.  You
 	  might also want the "curl" executable for debugging purposes.
 	  If you do not use http transfer, you are probably OK if you
diff --git a/Makefile b/Makefile
index 4190a5d..8f28b09 100644
--- a/Makefile
+++ b/Makefile
@@ -16,7 +16,6 @@ all::
 # when attempting to read from an fopen'ed directory.
 #
 # Define NO_OPENSSL environment variable if you do not have OpenSSL.
-# This also implies BLK_SHA1.
 #
 # Define NO_CURL if you do not have libcurl installed.  git-http-pull and
 # git-http-push are not built, and you cannot use http:// and https://
@@ -84,10 +83,6 @@ all::
 # specify your own (or DarwinPort's) include directories and
 # library directories by defining CFLAGS and LDFLAGS appropriately.
 #
-# Define BLK_SHA1 environment variable if you want the C version
-# of the SHA1 that assumes you can do unaligned 32-bit loads and
-# have a fast htonl() function.
-#
 # Define PPC_SHA1 environment variable when running make to make use of
 # a bundled SHA1 routine optimized for PowerPC.
 #
@@ -1013,7 +1008,6 @@ ifndef NO_OPENSSL
 	endif
 else
 	BASIC_CFLAGS += -DNO_OPENSSL
-	BLK_SHA1 = 1
 	OPENSSL_LIBSSL =
 endif
 ifdef NEEDS_SSL_WITH_CRYPTO
@@ -1162,18 +1156,14 @@ ifdef NO_DEFLATE_BOUND
 	BASIC_CFLAGS += -DNO_DEFLATE_BOUND
 endif
 
-ifdef BLK_SHA1
-	SHA1_HEADER = "block-sha1/sha1.h"
-	LIB_OBJS += block-sha1/sha1.o
-else
 ifdef PPC_SHA1
 	SHA1_HEADER = "ppc/sha1.h"
 	LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
 else
-	SHA1_HEADER = <openssl/sha.h>
-	EXTLIBS += $(LIB_4_CRYPTO)
-endif
+	SHA1_HEADER = "block-sha1/sha1.h"
+	LIB_OBJS += block-sha1/sha1.o
 endif
+
 ifdef NO_PERL_MAKEMAKER
 	export NO_PERL_MAKEMAKER
 endif
diff --git a/cache.h b/cache.h
index dd7f71e..39a4edd 100644
--- a/cache.h
+++ b/cache.h
@@ -6,12 +6,6 @@
 #include "hash.h"
 
 #include SHA1_HEADER
-#ifndef git_SHA_CTX
-#define git_SHA_CTX	SHA_CTX
-#define git_SHA1_Init	SHA1_Init
-#define git_SHA1_Update	SHA1_Update
-#define git_SHA1_Final	SHA1_Final
-#endif
 
 #include <zlib.h>
 #if defined(NO_DEFLATE_BOUND) || ZLIB_VERNUM < 0x1200
diff --git a/configure.ac b/configure.ac
index b09b8e4..634186c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -160,7 +160,6 @@ AC_MSG_NOTICE([CHECKS for site configuration])
 # a bundled SHA1 routine optimized for PowerPC.
 #
 # Define NO_OPENSSL environment variable if you do not have OpenSSL.
-# This also implies BLK_SHA1.
 #
 # Define OPENSSLDIR=/foo/bar if your openssl header and library files are in
 # /foo/bar/include and /foo/bar/lib directories.
Jeff King· Aug 25, 2009, 04:18 UTC · re: Nicolas Pitre · lore

Re: [PATCH/RFC] make the new block-sha1 the default

On Mon, Aug 24, 2009 at 11:04:37PM -0400, Nicolas Pitre wrote:
> ... and remove support for linking against the openssl SHA1 code.
> 
> The block-sha1 implementation is not significantly worse and sometimes 
> even faster than the openssl SHA1 implementation.  This allows for
Is there a reason not to leave the option of linking against openssl?

I'm still getting better numbers for OpenSSL over block-sha1 when doing "git fsck --full" in some repos. Particularly those with large files and few deltas, where the time is heavily influenced by sha-1 performance. I'm seeing up to 20% speed improvement using OpenSSL on those repos, and about 8% on linux-2.6 (the processor is a Conroe Core 2, git compiled with -O2).

But what really kills me is that I usually compile git with '-O0' because I am often investigating bugs and I like the debugger to act sanely. The performance hit is usually not noticeable, but in this case it is: my "git fsck --full" times jump from ~8.2s (OpenSSL) and ~10.3s (block-sha1, -O2) to ~18.2s (block-sha1, -O0).

Certainly you can argue that it is idiotic to benchmark anything at -O0. But right now, it is perfectly reasonable to compile git with -O0 and assume OpenSSL is compiled with sane optimizations. I'd rather not take that away without a good reason.

-Peff
Junio C Hamano· Aug 25, 2009, 06:50 UTC · re: Jeff King · lore

Re: [PATCH/RFC] make the new block-sha1 the default

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
> On Mon, Aug 24, 2009 at 11:04:37PM -0400, Nicolas Pitre wrote:
>
>> ... and remove support for linking against the openssl SHA1 code.
>> 
>> The block-sha1 implementation is not significantly worse and sometimes 
>> even faster than the openssl SHA1 implementation.  This allows for
>
> Is there a reason not to leave the option of linking against openssl?
I think it is a valid question.  Why remove the _option_?
I would certainly understand it if you made BLK_SHA1 the _default_, though.
Nicolas Pitre· Aug 25, 2009, 17:33 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] make the new block-sha1 the default

On Mon, 24 Aug 2009, Junio C Hamano wrote:
Show 12 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > On Mon, Aug 24, 2009 at 11:04:37PM -0400, Nicolas Pitre wrote:
> >
> >> ... and remove support for linking against the openssl SHA1 code.
> >> 
> >> The block-sha1 implementation is not significantly worse and sometimes 
> >> even faster than the openssl SHA1 implementation.  This allows for
> >
> > Is there a reason not to leave the option of linking against openssl?
> 
> I think it is a valid question.  Why remove the _option_?
Indeed, there is no value in limiting the choice.
> I would certainly understand it if you made BLK_SHA1 the _default_, though.

Since this is a RFC, and because this is not a clear choice, I'll simply let others play with it and see for themselves. Suffice to compile git with or without NO_OPENSSL defined. Some people (such as Jeff) are finding the openssl SHA1 faster (irrespective of the -O0 issue), whereas Linus simply hammered on the block-sha1 version until it was faster than openssl for him (this is faster for me as well, on X86 and ARM). Also those who initially found openssl to put a significant overhead on the dynamic linking should probably perform more measurements with and without NO_OPENSSL again. If more positive results are presented then changing the default might make sense.

Nicolas

← back to recent threads