Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8
On Sat, May 11, 2013 at 02:17:10AM -0700, David Aguilar wrote: > Good catch. I had a config.mak without any -O flags in CFLAGS. > Here are the timings with -O3. We're back to parity. > > $ time git rev-list --all --objects --verify-objects >/dev/null > > # CommonCrypto 28.95s user 4.62s system 99% cpu 33.630 total > # master 29.81s user 4.70s system 99% cpu 34.760 total > # BLK_SHA1 29.80s user 4.62s system 99% cpu 34.505 total > > If BLK_SHA1 were the default on all platforms then I wouldn't have > bothered with the SHA-1 patch. With this patch it makes it like Linux > in that Git can choose between the built-in functions and the external > library. With those numbers above, it seems like CommonCrypto is still a reasonable choice. I don't know anything about availability or other OS X specific issues, but assuming there are no issues there, your patch makes sense to me. Thanks for double-checking the timings. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8
On Sat, May 11, 2013 at 1:45 AM, Jeff King wrote: > On Sat, May 11, 2013 at 01:38:32AM -0700, David Aguilar wrote: > >> > Adding "--verify-objects" would sha1 the blobs, too, which might be more >> > reasonable (or running "git fsck"). Something like "git add" on a large >> > blob would also be a good test. >> >> Thanks. Here are the numbers with --verify-objects: >> >> $ time git rev-list --all --objects --verify-objects >/dev/null >> >> # CommonCrypto 32.24s user 4.65s system 99% cpu 37.098 total >> # master 33.00s user 4.68s system 99% cpu 37.852 total >> # BLK_SHA1 54.17s user 4.67s system 99% cpu 58.928 total >> >> Doing BLK_SHA1 seems like less of a good idea now, so I think my >> latest re-roll might be the way to go... > > Wow, that's really terrible. What level of optimization do you compile > with? With the other implementations, you are calling into > (presumably) optimized library code, but with BLK_SHA1 you are getting > whatever you just compiled. > > Here are three timings that show how big a difference that can make: > > openssl, -O00m21.152s > BLK_SHA1, -O00m31.920s > BLK_SHA1, -O30m19.652s > > So it is a win over openssl with optimizations on, but a pretty big loss > otherwise. Good catch. I had a config.mak without any -O flags in CFLAGS. Here are the timings with -O3. We're back to parity. $ time git rev-list --all --objects --verify-objects >/dev/null # CommonCrypto 28.95s user 4.62s system 99% cpu 33.630 total # master 29.81s user 4.70s system 99% cpu 34.760 total # BLK_SHA1 29.80s user 4.62s system 99% cpu 34.505 total If BLK_SHA1 were the default on all platforms then I wouldn't have bothered with the SHA-1 patch. With this patch it makes it like Linux in that Git can choose between the built-in functions and the external library. That's why I moved this patch to 3/3.. it could go either way. -- David -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8
On Sat, May 11, 2013 at 01:38:32AM -0700, David Aguilar wrote: > > Adding "--verify-objects" would sha1 the blobs, too, which might be more > > reasonable (or running "git fsck"). Something like "git add" on a large > > blob would also be a good test. > > Thanks. Here are the numbers with --verify-objects: > > $ time git rev-list --all --objects --verify-objects >/dev/null > > # CommonCrypto 32.24s user 4.65s system 99% cpu 37.098 total > # master 33.00s user 4.68s system 99% cpu 37.852 total > # BLK_SHA1 54.17s user 4.67s system 99% cpu 58.928 total > > Doing BLK_SHA1 seems like less of a good idea now, so I think my > latest re-roll might be the way to go... Wow, that's really terrible. What level of optimization do you compile with? With the other implementations, you are calling into (presumably) optimized library code, but with BLK_SHA1 you are getting whatever you just compiled. Here are three timings that show how big a difference that can make: openssl, -O00m21.152s BLK_SHA1, -O00m31.920s BLK_SHA1, -O30m19.652s So it is a win over openssl with optimizations on, but a pretty big loss otherwise. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8
On Sat, May 11, 2013 at 1:22 AM, Jeff King wrote: > On Sat, May 11, 2013 at 12:11:05AM -0700, David Aguilar wrote: > >> > Does this perform better or worse than just setting >> > BLK_SHA1=YesPlease? I'd naively think it could go either way: on one >> > hand adding another library dependency can slow down startup, and on >> > the other hand the implementation may or may not be optimized better >> > than the generic block-sha1/ implementation. >> >> Pretty much identical. >> >> Here are the timings (I should probably read t/perf/README and get >> better numbers): >> >> Best of ten >> $ time git rev-list --all --objects >/dev/null >> [...] > > I'm not sure that's a great test of sha1 performance. It will hash the > commit and tree objects it loads during the traversal, but that time is > almost certainly dwarfed by zlib inflation and by lookup_object. > > Adding "--verify-objects" would sha1 the blobs, too, which might be more > reasonable (or running "git fsck"). Something like "git add" on a large > blob would also be a good test. Thanks. Here are the numbers with --verify-objects: $ time git rev-list --all --objects --verify-objects >/dev/null # CommonCrypto 32.24s user 4.65s system 99% cpu 37.098 total # master 33.00s user 4.68s system 99% cpu 37.852 total # BLK_SHA1 54.17s user 4.67s system 99% cpu 58.928 total Doing BLK_SHA1 seems like less of a good idea now, so I think my latest re-roll might be the way to go... -- David -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8
On Sat, May 11, 2013 at 12:11:05AM -0700, David Aguilar wrote: > > Does this perform better or worse than just setting > > BLK_SHA1=YesPlease? I'd naively think it could go either way: on one > > hand adding another library dependency can slow down startup, and on > > the other hand the implementation may or may not be optimized better > > than the generic block-sha1/ implementation. > > Pretty much identical. > > Here are the timings (I should probably read t/perf/README and get > better numbers): > > Best of ten > $ time git rev-list --all --objects >/dev/null > [...] I'm not sure that's a great test of sha1 performance. It will hash the commit and tree objects it loads during the traversal, but that time is almost certainly dwarfed by zlib inflation and by lookup_object. Adding "--verify-objects" would sha1 the blobs, too, which might be more reasonable (or running "git fsck"). Something like "git add" on a large blob would also be a good test. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8
On Fri, May 10, 2013 at 11:23 PM, Jonathan Nieder wrote: > Hi, > > David Aguilar wrote: > >> Mac OS X Mountain Lion prints warnings when building git: >> >> warning: 'SHA1_Init' is deprecated >> (declared at /usr/include/openssl/sha.h:121) >> >> Silence the warnings by using the Common Digest SHA-1 >> functions for SHA1_Init(), SHA1_Update(), and SHA1_Final(). > > Thanks. > > Does this perform better or worse than just setting > BLK_SHA1=YesPlease? I'd naively think it could go either way: on one > hand adding another library dependency can slow down startup, and on > the other hand the implementation may or may not be optimized better > than the generic block-sha1/ implementation. Pretty much identical. Here are the timings (I should probably read t/perf/README and get better numbers): Best of ten $ time git rev-list --all --objects >/dev/null # master git rev-list --all --objects > /dev/null 5.16s user 0.11s system 99% cpu 5.277 total # BLK_SHA1 git rev-list --all --objects > /dev/null 5.17s user 0.11s system 99% cpu 5.299 total # CommonDigest git rev-list --all --objects > /dev/null 5.18s user 0.11s system 99% cpu 5.301 total The library startup cost is identical to master since at the end of the day it's still libcrypto. FWIW the way it's done in this patch still allows defining BLK_SHA1, which is parity with how the Makefile behaves on Linux. Re-roll? -- David -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8
Hi, David Aguilar wrote: > Mac OS X Mountain Lion prints warnings when building git: > > warning: 'SHA1_Init' is deprecated > (declared at /usr/include/openssl/sha.h:121) > > Silence the warnings by using the Common Digest SHA-1 > functions for SHA1_Init(), SHA1_Update(), and SHA1_Final(). Thanks. Does this perform better or worse than just setting BLK_SHA1=YesPlease? I'd naively think it could go either way: on one hand adding another library dependency can slow down startup, and on the other hand the implementation may or may not be optimized better than the generic block-sha1/ implementation. Curious, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8
Mac OS X Mountain Lion prints warnings when building git:
warning: 'SHA1_Init' is deprecated
(declared at /usr/include/openssl/sha.h:121)
Silence the warnings by using the Common Digest SHA-1
functions for SHA1_Init(), SHA1_Update(), and SHA1_Final().
Add a COMMON_DIGEST_SHA1 knob to the Makefile to allow
choosing this implementation and define it by default on Darwin.
Signed-off-by: David Aguilar
---
The previous version of this patch (somehow?) eliminated the SHA1
warnings, but it was not redefining SHA1_HEADER to CommonDigest.h.
This version goes all the way.
Some warnings still exist around the HMAC functions (and others)
used by e.g. imap.c, which should be dealt with separately.
Makefile | 7 +++
cache.h | 7 +++
2 files changed, 14 insertions(+)
diff --git a/Makefile b/Makefile
index 0f931a2..d8a45b4 100644
--- a/Makefile
+++ b/Makefile
@@ -1054,6 +1054,7 @@ ifeq ($(uname_S),Darwin)
BASIC_LDFLAGS += -L/opt/local/lib
endif
endif
+ COMMON_DIGEST_SHA1 = YesPlease
PTHREAD_LIBS =
endif
@@ -1388,10 +1389,16 @@ ifdef PPC_SHA1
LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
LIB_H += ppc/sha1.h
else
+ifdef COMMON_DIGEST_SHA1
+ BASIC_CFLAGS += -DCOMMON_DIGEST_FOR_SHA1=1
+ SHA1_HEADER =
+ EXTLIBS += $(LIB_4_CRYPTO)
+else
SHA1_HEADER =
EXTLIBS += $(LIB_4_CRYPTO)
endif
endif
+endif
ifdef NO_PERL_MAKEMAKER
export NO_PERL_MAKEMAKER
endif
diff --git a/cache.h b/cache.h
index 94ca1ac..e2b24c6 100644
--- a/cache.h
+++ b/cache.h
@@ -10,11 +10,18 @@
#include SHA1_HEADER
#ifndef git_SHA_CTX
+#ifdef COMMON_DIGEST_FOR_SHA1
+#define git_SHA_CTXCC_SHA1_CTX
+#define git_SHA1_Init CC_SHA1_Init
+#define git_SHA1_UpdateCC_SHA1_Update
+#define git_SHA1_Final CC_SHA1_Final
+#else
#define git_SHA_CTXSHA_CTX
#define git_SHA1_Init SHA1_Init
#define git_SHA1_UpdateSHA1_Update
#define git_SHA1_Final SHA1_Final
#endif
+#endif
#include
typedef struct git_zstream {
--
1.8.3.rc1.45.g88f80b8.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
