From c578e29ba0791041ad7fabf1166dd6f7e7f26d1f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Sep 2020 15:21:11 -0400 Subject: [PATCH 1/2] bswap.h: drop unaligned loads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Our put_be32() routine and its variants (get_be32(), put_be64(), etc) has two implementations: on some platforms we cast memory in place and use nothl()/htonl(), which can cause unaligned memory access. And on others, we pick out the individual bytes using bitshifts. This introduces extra complexity, and sometimes causes compilers to generate warnings about type-punning. And it's not clear there's any performance advantage. This split goes back to 660231aa97 (block-sha1: support for architectures with memory alignment restrictions, 2009-08-12). The unaligned versions were part of the original block-sha1 code in d7c208a92e (Add new optimized C 'block-sha1' routines, 2009-08-05), which says it is: Based on the mozilla SHA1 routine, but doing the input data accesses a word at a time and with 'htonl()' instead of loading bytes and shifting. Back then, Linus provided timings versus the mozilla code which showed a 27% improvement: https://lore.kernel.org/git/alpine.LFD.2.01.0908051545000.3390@localhost.localdomain/ However, the unaligned loads were either not the useful part of that speedup, or perhaps compilers and processors have changed since then. Here are times for computing the sha1 of 4GB of random data, with and without -DNO_UNALIGNED_LOADS (and BLK_SHA1=1, of course). This is with gcc 10, -O2, and the processor is a Core i9-9880H. [stock] Benchmark #1: t/helper/test-tool sha1 Signed-off-by: Junio C Hamano --- Makefile | 1 - compat/bswap.h | 24 ------------------------ 2 files changed, 25 deletions(-) diff --git a/Makefile b/Makefile index 372139f1f2..a8bedd4243 100644 --- a/Makefile +++ b/Makefile @@ -1214,7 +1214,6 @@ SANITIZERS := $(foreach flag,$(subst $(comma),$(space),$(SANITIZE)),$(flag)) BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE) BASIC_CFLAGS += -fno-omit-frame-pointer ifneq ($(filter undefined,$(SANITIZERS)),) -BASIC_CFLAGS += -DNO_UNALIGNED_LOADS BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS endif ifneq ($(filter leak,$(SANITIZERS)),) diff --git a/compat/bswap.h b/compat/bswap.h index e4e25735ce..c0bb744adc 100644 --- a/compat/bswap.h +++ b/compat/bswap.h @@ -145,28 +145,6 @@ static inline uint64_t git_bswap64(uint64_t x) #endif -/* - * Performance might be improved if the CPU architecture is OK with - * unaligned 32-bit loads and a fast ntohl() is available. - * Otherwise fall back to byte loads and shifts which is portable, - * and is faster on architectures with memory alignment issues. - */ - -#if !defined(NO_UNALIGNED_LOADS) && ( \ - defined(__i386__) || defined(__x86_64__) || \ - defined(_M_IX86) || defined(_M_X64) || \ - defined(__ppc__) || defined(__ppc64__) || \ - defined(__powerpc__) || defined(__powerpc64__) || \ - defined(__s390__) || defined(__s390x__)) - -#define get_be16(p) ntohs(*(unsigned short *)(p)) -#define get_be32(p) ntohl(*(unsigned int *)(p)) -#define get_be64(p) ntohll(*(uint64_t *)(p)) -#define put_be32(p, v) do { *(unsigned int *)(p) = htonl(v); } while (0) -#define put_be64(p, v) do { *(uint64_t *)(p) = htonll(v); } while (0) - -#else - static inline uint16_t get_be16(const void *ptr) { const unsigned char *p = ptr; @@ -212,6 +190,4 @@ static inline void put_be64(void *ptr, uint64_t value) p[7] = value >> 0; } -#endif - #endif /* COMPAT_BSWAP_H */ From 176380fd113a97b09c99a5f7273725527823fa1b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Sep 2020 15:22:29 -0400 Subject: [PATCH 2/2] Revert "fast-export: use local array to store anonymized oid" This reverts commit f39ad38410da554af54966bf74fa0402355852ac. That commit was trying to silence a type-punning warning on older versions of gcc. However, its analysis was all wrong. I didn't notice that we _were_ in fact type-punning because there are two versions of put_be32(): one that uses casts and unaligned loads, and another that uses bitshifts. I looked at the latter, but on my platform we were defaulting to the former. However, as of the previous commit, we'll always use the bitshift version. So we can drop this hackery to avoid the warning, making the code slightly cleaner. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fast-export.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 9f37895d4c..c6ecf404d7 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -405,12 +405,12 @@ static char *generate_fake_oid(void *data) { static uint32_t counter = 1; /* avoid null oid */ const unsigned hashsz = the_hash_algo->rawsz; - unsigned char out[GIT_MAX_RAWSZ]; + struct object_id oid; char *hex = xmallocz(GIT_MAX_HEXSZ); - hashclr(out); - put_be32(out + hashsz - 4, counter++); - return hash_to_hex_algop_r(hex, out, the_hash_algo); + oidclr(&oid); + put_be32(oid.hash + hashsz - 4, counter++); + return oid_to_hex_r(hex, &oid); } static const char *anonymize_oid(const char *oid_hex)