[PATCH] Disable OpenSSL SHA1 implementation by default

[PATCH] Disable OpenSSL SHA1 implementation by default

[Jonathan Nieder]

The OpenSSL SHA-1 routine is about as fast as block-sha1, but linking
to libcrypto slows down the startup of git commands by an appreciable
amount.  Use the BLK_SHA1 implementation by default instead.

Even without its SHA-1 functions, OpenSSL is useful for teaching
imap-send to use TLS.  Now people building git can decide separately
whether to use each of these two facilities by setting or unsetting
the OPENSSL_SHA1 and OPENSSL_TLS options.

Let the configure script’s --with-openssl option and SSL library
checks toggle OPENSSL_TLS without touching OPENSSL_SHA1.  I am
guessing most people will not want to enable OPENSSL_SHA1.  If that
turns out to be false, we can add a new option to the configure
script.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Typed “make NO_OPENSSL=1” for the umpteenth time today, but this time
I thought I should something about it.

Good idea?  Bad idea?

 Makefile          |   24 +++++++++++++++++-------
 compat/mingw.h    |    2 +-
 config.mak.in     |    2 +-
 configure.ac      |   42 ++++++++++++++++++++++++++++++++++++------
 git-compat-util.h |    2 +-
 imap-send.c       |   16 ++++++++--------
 6 files changed, 64 insertions(+), 24 deletions(-)

diff --git a/Makefile b/Makefile
index afedb54..a82d29c 100644
--- a/Makefile
+++ b/Makefile
@@ -15,8 +15,11 @@ all::
 # Define FREAD_READS_DIRECTORIES if your are on a system which succeeds
 # 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 OPENSSL_SHA1 if you would like to use the SHA-1 routine from
+# OpenSSL.  Otherwise, BLK_SHA1 will be used.
+#
+# Define OPENSSL_TLS if you would like the imap-send utility to be
+# able to use SSL.
 #
 # 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://
@@ -1103,7 +1106,18 @@ EXTLIBS += -lz
 ifndef NO_POSIX_ONLY_PROGRAMS
 	PROGRAMS += git-daemon$X
 endif
-ifndef NO_OPENSSL
+ifdef OPENSSL_TLS
+	BASIC_CFLAGS += -DOPENSSL_TLS
+	USE_OPENSSL = Yes
+endif
+ifdef OPENSSL_SHA1
+	BASIC_CFLAGS += -DOPENSSL_SHA1
+	USE_OPENSSL = Yes
+else
+	BLK_SHA1 = 1
+endif
+ifdef USE_OPENSSL
+	BASIC_CFLAGS += -DUSE_OPENSSL
 	OPENSSL_LIBSSL = -lssl
 	ifdef OPENSSLDIR
 		BASIC_CFLAGS += -I$(OPENSSLDIR)/include
@@ -1114,10 +1128,6 @@ ifndef NO_OPENSSL
 	ifdef NEEDS_CRYPTO_WITH_SSL
 		OPENSSL_LINK += -lcrypto
 	endif
-else
-	BASIC_CFLAGS += -DNO_OPENSSL
-	BLK_SHA1 = 1
-	OPENSSL_LIBSSL =
 endif
 ifdef NEEDS_SSL_WITH_CRYPTO
 	LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto -lssl
diff --git a/compat/mingw.h b/compat/mingw.h
index e254fb4..70c3392 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -125,7 +125,7 @@ static inline int waitpid(pid_t pid, int *status, unsigned options)
 	return -1;
 }
 
-#ifndef NO_OPENSSL
+#ifdef USE_OPENSSL
 #include <openssl/ssl.h>
 static inline int mingw_SSL_set_fd(SSL *ssl, int fd)
 {
diff --git a/config.mak.in b/config.mak.in
index 6008ac9..a0cb30e 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -27,7 +27,7 @@ export srcdir VPATH
 
 ASCIIDOC8=@ASCIIDOC8@
 NEEDS_SSL_WITH_CRYPTO=@NEEDS_SSL_WITH_CRYPTO@
-NO_OPENSSL=@NO_OPENSSL@
+OPENSSL_TLS=@OPENSSL_TLS@
 NO_CURL=@NO_CURL@
 NO_EXPAT=@NO_EXPAT@
 NO_LIBGEN_H=@NO_LIBGEN_H@
diff --git a/configure.ac b/configure.ac
index 914ae57..0efcdb9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -80,6 +80,26 @@ else \
 fi \
 ])# GIT_PARSE_WITH
 #
+# GIT_PARSE_WITH_USE(PACKAGE, VARNAME)
+# ------------------------------------
+# For use in AC_ARG_WITH action-if-found, for packages default ON.
+# * Unset VARNAME for --without-PACKAGE
+# * Always set VARNAME=YesPlease for --with-PACKAGE
+# * Set PACKAGEDIR=PATH for --with-PACKAGE=PATH
+AC_DEFUN([GIT_PARSE_WITH_USE],
+[PACKAGE=m4_toupper($1); \
+if test "$withval" = "no"; then \
+	m4_toupper($2)=; \
+elif test "$withval" = "yes"; then \
+	m4_toupper($2)=YesPlease; \
+else \
+	m4_toupper($2)=YesPlease; \
+	m4_toupper($1)DIR=$withval; \
+	AC_MSG_NOTICE([Setting m4_toupper($1)DIR to $withval]); \
+	GIT_CONF_APPEND_LINE(${PACKAGE}DIR=$withval); \
+fi \
+])# GIT_PARSE_WITH_USE
+#
 # GIT_PARSE_WITH_SET_MAKE_VAR(WITHNAME, VAR, HELP_TEXT)
 # ---------------------
 # Set VAR to the value specied by --with-WITHNAME.
@@ -190,15 +210,18 @@ AC_MSG_NOTICE([CHECKS for site configuration])
 # Define PPC_SHA1 environment variable when running make to make use of
 # a bundled SHA1 routine optimized for PowerPC.
 #
-# Define NO_OPENSSL environment variable if you do not have OpenSSL.
-# This also implies BLK_SHA1.
+# Define OPENSSL_SHA1 if you would like to use the SHA-1 routine from
+# OpenSSL.  Otherwise, BLK_SHA1 will be used.
+#
+# Define OPENSSL_TLS if you would like the imap-send utility to be
+# able to use SSL.
 #
 # Define OPENSSLDIR=/foo/bar if your openssl header and library files are in
 # /foo/bar/include and /foo/bar/lib directories.
 AC_ARG_WITH(openssl,
 AS_HELP_STRING([--with-openssl],[use OpenSSL library (default is YES)])
 AS_HELP_STRING([],              [ARG can be prefix for openssl library and headers]),\
-GIT_PARSE_WITH(openssl))
+GIT_PARSE_WITH_USE(openssl, [OPENSSL_TLS]))
 #
 # Define NO_CURL if you do not have curl installed.  git-http-pull and
 # git-http-push are not built, and you cannot use http:// and https://
@@ -383,7 +406,7 @@ AC_SUBST(ASCIIDOC8)
 ## Checks for libraries.
 AC_MSG_NOTICE([CHECKS for libraries])
 #
-# Define NO_OPENSSL environment variable if you do not have OpenSSL.
+# Define OPENSSL_TLS to empty if you do not have OpenSSL.
 # Define NEEDS_SSL_WITH_CRYPTO if you need -lcrypto with -lssl (Darwin).
 
 GIT_STASH_FLAGS($OPENSSLDIR)
@@ -392,12 +415,19 @@ AC_CHECK_LIB([crypto], [SHA1_Init],
 [NEEDS_SSL_WITH_CRYPTO=],
 [AC_CHECK_LIB([ssl], [SHA1_Init],
  [NEEDS_SSL_WITH_CRYPTO=YesPlease],
- [NEEDS_SSL_WITH_CRYPTO= NO_OPENSSL=YesPlease])])
+ [NEEDS_SSL_WITH_CRYPTO=])])
+
+if test -z "${OPENSSL_TLS+set}"
+then
+AC_CHECK_LIB([ssl], [SSL_CTX_new],
+ [OPENSSL_TLS=YesPlease],
+ [OPENSSL_TLS=])
+fi
 
 GIT_UNSTASH_FLAGS($OPENSSLDIR)
 
 AC_SUBST(NEEDS_SSL_WITH_CRYPTO)
-AC_SUBST(NO_OPENSSL)
+AC_SUBST(OPENSSL_TLS)
 
 #
 # Define NO_CURL if you do not have libcurl installed.  git-http-pull and
diff --git a/git-compat-util.h b/git-compat-util.h
index a3c4537..c095b61 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -135,7 +135,7 @@ extern char *gitbasename(char *);
 #include <iconv.h>
 #endif
 
-#ifndef NO_OPENSSL
+#ifdef USE_OPENSSL
 #include <openssl/ssl.h>
 #include <openssl/err.h>
 #endif
diff --git a/imap-send.c b/imap-send.c
index 5631930..9318c7f 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -25,7 +25,7 @@
 #include "cache.h"
 #include "exec_cmd.h"
 #include "run-command.h"
-#ifdef NO_OPENSSL
+#ifndef OPENSSL_TLS
 typedef void *SSL;
 #endif
 
@@ -238,7 +238,7 @@ static const char *Flags[] = {
 	"Deleted",
 };
 
-#ifndef NO_OPENSSL
+#ifdef OPENSSL_TLS
 static void ssl_socket_perror(const char *func)
 {
 	fprintf(stderr, "%s: %s\n", func, ERR_error_string(ERR_get_error(), NULL));
@@ -247,7 +247,7 @@ static void ssl_socket_perror(const char *func)
 
 static void socket_perror(const char *func, struct imap_socket *sock, int ret)
 {
-#ifndef NO_OPENSSL
+#ifdef OPENSSL_TLS
 	if (sock->ssl) {
 		int sslerr = SSL_get_error(sock->ssl, ret);
 		switch (sslerr) {
@@ -272,7 +272,7 @@ static void socket_perror(const char *func, struct imap_socket *sock, int ret)
 
 static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int verify)
 {
-#ifdef NO_OPENSSL
+#ifndef OPENSSL_TLS
 	fprintf(stderr, "SSL requested but SSL support not compiled in\n");
 	return -1;
 #else
@@ -333,7 +333,7 @@ static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int ve
 static int socket_read(struct imap_socket *sock, char *buf, int len)
 {
 	ssize_t n;
-#ifndef NO_OPENSSL
+#ifdef OPENSSL_TLS
 	if (sock->ssl)
 		n = SSL_read(sock->ssl, buf, len);
 	else
@@ -351,7 +351,7 @@ static int socket_read(struct imap_socket *sock, char *buf, int len)
 static int socket_write(struct imap_socket *sock, const char *buf, int len)
 {
 	int n;
-#ifndef NO_OPENSSL
+#ifdef OPENSSL_TLS
 	if (sock->ssl)
 		n = SSL_write(sock->ssl, buf, len);
 	else
@@ -368,7 +368,7 @@ static int socket_write(struct imap_socket *sock, const char *buf, int len)
 
 static void socket_shutdown(struct imap_socket *sock)
 {
-#ifndef NO_OPENSSL
+#ifdef OPENSSL_TLS
 	if (sock->ssl) {
 		SSL_shutdown(sock->ssl);
 		SSL_free(sock->ssl);
@@ -1087,7 +1087,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
 		goto bail;
 
 	if (!preauth) {
-#ifndef NO_OPENSSL
+#ifdef OPENSSL_TLS
 		if (!srvc->use_ssl && CAP(STARTTLS)) {
 			if (imap_exec(ctx, 0, "STARTTLS") != RESP_OK)
 				goto bail;
-- 
1.7.0

Re: [PATCH] Disable OpenSSL SHA1 implementation by default

[Jeff King]

On Mon, Feb 22, 2010 at 05:08:14AM -0600, Jonathan Nieder wrote:

> The OpenSSL SHA-1 routine is about as fast as block-sha1, but linking
> to libcrypto slows down the startup of git commands by an appreciable
> amount.  Use the BLK_SHA1 implementation by default instead.

What is your definition of "about as fast"? I benchmarked up to a 20%
slow-down a while back:

  http://article.gmane.org/gmane.comp.version-control.git/126995

Now my complaint then was specifically about removing openssl sha1
support entirely, and I have no problem with setting OPENSSL_SHA1 in my
build options, but it does make sense to me that the default should
be whatever is fastest for most people. And that means benchmarking
blk_sha1 versus the libcrypto linking slow-down on several machines to
get actual numbers.

Unfortunately, I think we may end up with an apples-to-oranges
comparison, as sha1-heavy tasks are affected one way, and scripted
many-git-invocation tasks the other way.

> Typed “make NO_OPENSSL=1” for the umpteenth time today, but this time
> I thought I should something about it.

echo 'NO_OPENSSL=1' >config.mak ?

> -ifndef NO_OPENSSL
> +ifdef OPENSSL_TLS
> +	BASIC_CFLAGS += -DOPENSSL_TLS
> +	USE_OPENSSL = Yes
> +endif

Doesn't this flip the default for using TLS on imap-send?

-Peff

Re: [PATCH] Disable OpenSSL SHA1 implementation by default

[Jonathan Nieder]

Jeff King wrote:

> What is your definition of "about as fast"? I benchmarked up to a 20%
> slow-down a while back:
> 
>   http://article.gmane.org/gmane.comp.version-control.git/126995

Thanks for the pointer --- I had missed that.  I’ll run some numbers
and I’d be glad to see other people’s too (for whatever workloads are
most interesting).  But anyway, I think we should table this patch,
and I’ll write another one that just splits NO_OPENSSL in two without
changing any defaults.

Jonathan

Re: [PATCH] Disable OpenSSL SHA1 implementation by default

[Mark Lodato]

On Mon, Feb 22, 2010 at 6:23 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Feb 22, 2010 at 05:08:14AM -0600, Jonathan Nieder wrote:
>
>> The OpenSSL SHA-1 routine is about as fast as block-sha1, but linking
>> to libcrypto slows down the startup of git commands by an appreciable
>> amount.  Use the BLK_SHA1 implementation by default instead.
>
> What is your definition of "about as fast"? I benchmarked up to a 20%
> slow-down a while back:

For what it's worth, here are my numbers from running git-fsck on my
x86-64 machine.  It appears that BLK-SHA1 is slightly faster.  If you
know a better benchmark to run, let me know.  Perhaps it would be good
to write some sort of suite to test this, and let people post their
results to some website.

make git-fsck
./git-fsck  66.75s user 0.28s system 99% cpu 1:07.17 total
./git-fsck  66.70s user 1.28s system 99% cpu 1:08.06 total
./git-fsck  66.77s user 0.63s system 99% cpu 1:07.42 total

make BLK_SHA1=1 git-fsck
./git-fsck  65.60s user 0.65s system 99% cpu 1:06.26 total
./git-fsck  65.39s user 0.65s system 99% cpu 1:06.06 total
./git-fsck  65.24s user 1.36s system 100% cpu 1:06.60 total

Core 2 Duo E6300 1.86 GHz (2MB L2 cache), 2GB memory
Ubuntu 9.10 x86-64, gcc 4.4.1, git v1.7.0-90-g251a495

Re: [PATCH] Disable OpenSSL SHA1 implementation by default

[Jeff King]

On Thu, Feb 25, 2010 at 11:11:59PM -0500, Mark Lodato wrote:

> For what it's worth, here are my numbers from running git-fsck on my
> x86-64 machine.  It appears that BLK-SHA1 is slightly faster.  If you
> know a better benchmark to run, let me know.  Perhaps it would be good
> to write some sort of suite to test this, and let people post their
> results to some website.
> 
> make git-fsck
> ./git-fsck  66.75s user 0.28s system 99% cpu 1:07.17 total
> ./git-fsck  66.70s user 1.28s system 99% cpu 1:08.06 total
> ./git-fsck  66.77s user 0.63s system 99% cpu 1:07.42 total
> 
> make BLK_SHA1=1 git-fsck
> ./git-fsck  65.60s user 0.65s system 99% cpu 1:06.26 total
> ./git-fsck  65.39s user 0.65s system 99% cpu 1:06.06 total
> ./git-fsck  65.24s user 1.36s system 100% cpu 1:06.60 total
> 
> Core 2 Duo E6300 1.86 GHz (2MB L2 cache), 2GB memory
> Ubuntu 9.10 x86-64, gcc 4.4.1, git v1.7.0-90-g251a495

Thanks for the numbers. I get the opposite results:

  openssl, ./git-fsck (best of 3):
  62.29user 2.08system 1:04.43elapsed 99%CPU

  blk-sha1, ./git-fsck (best of 3):
  65.57user 1.33system 1:07.19elapsed 99%CPU

So about a 5% slow-down to use blk-sha1. But that is only half the
story. Dropping the openssl link dependency gives:

  stock, make -j4 test (best of 3):
  57.43user 74.60system 1:09.48elapsed 190%CPU

  NO_OPENSSL=TooSlow, make -j4 test (best of 3):
  50.64user 68.74system 1:03.48elapsed 188%CPU

Which is almost a 12% speedup by dropping openssl.

Now, a few qualifiers on what this means.

git-fsck is not a particularly good test of what users experience. They
just don't do it very often. We use it here because it's sha1-heavy. The
other heavy sha1 operation would be "git add" on large files. But it
means we are singling out pure sha1 performance, which is a relatively
small part of what users do.

The "make -j4 test" is also skewed a bit. Notice how we kept the dual
CPUs almost pegged by running a bunch of tests in parallel. In terms of
actual latency to the user, any small git speedup in the couple of git
commands somebody is running may be lost in the noise (e.g., just
invoking perl once destroys any git startup cost). The place that gets
the most benefit is going to be shell scripts which invoke git many
times.

And on top of that, there is the question of where users perceive
slowness. Even a 1000% speedup in the startup code of git may not be
noticeable to me if the different is .002 seconds to .02 seconds. But if
my gigantic "git add" takes 4 seconds instead of 4.5, I might notice it.

So I don't think there is any objective winner. But, I am willing to
assume that:

  1. As many people actually see blk-sha1 _faster_, let's assume my
     numbers for its slowdown are among the worst case (i.e., we do not
     need to do a massive campaign to collect more speed results).

  2. Most people don't care about large "git add" or "fsck". The sha-1
     performance for them falls into the "if it's not .02 instead of .002,
     I don't care" realm.

which argues for dropping openssl as the default.

In other words, the reverse of what I said in my previous message (well,
not the reverse; what I really wanted was for somebody to generate
actual numbers so we could base the decision on data).

-Peff

Re: [PATCH] Disable OpenSSL SHA1 implementation by default

[Mark Lodato]

On Fri, Feb 26, 2010 at 4:36 AM, Jeff King <peff@peff.net> wrote:
> Dropping the openssl link dependency gives:
>
>  stock, make -j4 test (best of 3):
>  57.43user 74.60system 1:09.48elapsed 190%CPU
>
>  NO_OPENSSL=TooSlow, make -j4 test (best of 3):
>  50.64user 68.74system 1:03.48elapsed 188%CPU
>
> Which is almost a 12% speedup by dropping openssl.

I got about an 8% speedup on this test.

master
make -j4 test  216.72s user 296.83s system 171% cpu 4:58.80 total
make -j4 test  217.21s user 293.97s system 171% cpu 4:58.53 total
make -j4 test  216.08s user 293.76s system 165% cpu 5:08.39 total

master + this patch
make -j4 test  199.21s user 258.88s system 159% cpu 4:47.60 total
make -j4 test  198.61s user 264.40s system 168% cpu 4:35.42 total
make -j4 test  197.12s user 268.74s system 169% cpu 4:35.33 total

Re: [PATCH] Disable OpenSSL SHA1 implementation by default

[Jeff King]

On Fri, Feb 26, 2010 at 04:33:23PM -0500, Mark Lodato wrote:

> > Which is almost a 12% speedup by dropping openssl.
> 
> I got about an 8% speedup on this test.

Thanks for more data. It looks like the patch is probably a win for me
(depending on how you slice the workload), and definitely a win for you,
then. So probably it is a sane thing to change the default.

I suspect the reason I got more of a speedup for "make test" is that I
don't do the svn or cvs tests, both of which are really slow and won't
be improved by the patch. If you have either installed, then that is
diluting your speedup.

-Peff