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

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Nicolas Pitre
... 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 <[hidden email]>
---

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

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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Jeff King
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
Jeff King <[hidden email]> 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_?

I would certainly understand it if you made BLK_SHA1 the _default_, though.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Nicolas Pitre
On Mon, 24 Aug 2009, Junio C Hamano wrote:

> Jeff King <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html