threads / patch / 22755

patchDisable OpenSSL SHA1 implementation by default

Subject: [PATCH] Disable OpenSSL SHA1 implementation by default

## tl;dr

7 messages between Feb 22, 2010 and Feb 26, 2010. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Jonathan Nieder· Feb 22, 2010, 11:08 UTC · lore

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(-)
Show changes to 6 files +64 −24

Makefile, compat/mingw.h, config.mak.in, configure.ac, git-compat-util.h, imap-send.c

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
Jeff King· Feb 22, 2010, 11:23 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Disable OpenSSL SHA1 implementation by default

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 ?
Show 5 quoted lines
> -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
Jonathan Nieder· Feb 22, 2010, 11:55 UTC · re: Jeff King · lore

Re: [PATCH] Disable OpenSSL SHA1 implementation by default

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
Mark Lodato· Feb 26, 2010, 04:11 UTC · re: Jeff King · lore

Re: [PATCH] Disable OpenSSL SHA1 implementation by default

On Mon, Feb 22, 2010 at 6:23 AM, Jeff King <peff@peff.net> wrote:
Show 8 quoted lines
> 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

Jeff King· Feb 26, 2010, 09:36 UTC · re: Mark Lodato · lore

Re: [PATCH] Disable OpenSSL SHA1 implementation by default

On Thu, Feb 25, 2010 at 11:11:59PM -0500, Mark Lodato wrote:
Show 18 quoted lines
> 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
Mark Lodato· Feb 26, 2010, 21:33 UTC · re: Jeff King · lore

Re: [PATCH] Disable OpenSSL SHA1 implementation by default

On Fri, Feb 26, 2010 at 4:36 AM, Jeff King <peff@peff.net> wrote:
Show 9 quoted lines
> 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

Jeff King· Feb 26, 2010, 22:40 UTC · re: Mark Lodato · lore

Re: [PATCH] Disable OpenSSL SHA1 implementation by default

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

← back to recent threads