{"thread":{"id":"64708","subject":"[PATCH 0/1] compat: modernize and simplify byte swapping functions","startedAt":"2026-01-02T00:27:47Z","lastAt":"2026-01-14T21:14:10Z","messageCount":7,"participants":["Rostislav Krasny","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"532895","messageId":"20260102002735.31390-1-rostiprodev@gmail.com","threadId":"64708","inReplyTo":null,"subject":"[PATCH 0/1] compat: modernize and simplify byte swapping functions","fromName":"Rostislav Krasny","fromEmail":"rostiprodev@gmail.com","sentAt":"2026-01-02T00:27:34Z","receivedAt":"2026-01-02T00:27:47Z","isPatch":true,"sender":{"key":"rostiprodev@gmail.com","avatar":null},"body":"When I read sha256/block/sha256.c I noticed it uses both the htonl macro and\nthe get_be32() static inline function. I was surprised how different the\nimplementations of those two kindred things are. When GCC or Clang is used the\nhtonl macro is translated into the __builtin_bswap32() call, which is assembled\ninto one single CPU instruction, in the case of x86. And the original\nimplementation of the get_be32() function used eight bitwise operations. Even\nif the compiler can optimize that code it's still less readable and more error\nprone.\n\nThe main reason it was implemented so complicated is UB when conversion of a\npointer to one object type into a pointer of a different object type is used.\nOn the other hand, memcpy is protected from such UB and this allows us to make\nthat code simpler and even more optimal, in some cases.\n\nAdditionally I made a few more small improvements related to the same\nfunctionality.\n\nI've measured performance of the original and the new code on my Intel\nXeon W-2135 based computer in Fedora 43 Linux with:\n\n* glibc 2.42-5.fc43\n* gcc   15.2.1-5.fc43\n* clang 21.1.7-1.fc43\n\nI used the following code for these measurements:\n\n#include <inttypes.h>\n#include <stdio.h>\n#include <stdint.h>\n#include <string.h>\n#include <time.h>\n\n#include \"bswap.h\"\n\n#define ITERATIONS 1000000\n#define BUF_SIZE 8192\n\nint main() {\n    uint8_t buffer[BUF_SIZE];\n    uint64_t sum = 0;\n\n    for (int i = 0; i < BUF_SIZE; i++) {\n        buffer[i] = (uint8_t)i;\n    }\n\n    clock_t start = clock();\n\n    for (int i = 0; i < ITERATIONS; i++) {\n        // use a volatile pointer to force the compiler to read memory\n        volatile uint8_t *p = buffer; \n        for (int j = 0; j < BUF_SIZE - 8; j++) {\n            sum += get_be64((const void*)(p + j));\n        }\n    }\n    \n    clock_t end = clock();\n    double time_taken = (double)(end - start) / CLOCKS_PER_SEC;\n\n    printf(\"Time taken: %f seconds\\n\", time_taken);\n    printf(\"Checksum: %\" PRIu64 \"\\n\", sum);\n\n    return 0;\n}\n\nAnd these are the results:\n\nGCC 15.2.1\nversion |  -Os     |  -O0     |  -O1     |  -O2     |  -O3\n================================================================\n        | 3.721806 |72.342204 |11.956021 | 3.119833 | 0.919873  \noriginal| 3.726111 |72.326920 |11.963618 | 3.128222 | 0.921128  \n        | 3.719791 |72.328175 |11.949108 | 3.130956 | 0.920296         \n================================================================\n        | 3.719899 |17.177719 | 3.005065 | 3.120747 | 0.920609  \nnew     | 3.714785 |17.168950 | 3.004978 | 3.119227 | 0.918851  \n        | 3.716782 |17.145386 | 3.009364 | 3.119573 | 0.920030  \n================================================================\n\nClang 21.1.7\nversion |  -Os     |  -O0     |  -O1     |  -O2     |  -O3\n================================================================\n        | 3.690718 |62.916338 | 3.017460 | 3.768443 | 3.778840  \noriginal| 3.686283 |62.965916 | 3.014674 | 3.777897 | 3.774776  \n        | 3.687775 |62.850648 | 3.003496 | 3.766108 | 3.765313         \n================================================================\n        | 3.681818 |16.753385 | 3.008131 | 2.075271 | 2.076090  \nnew     | 3.687184 |16.737982 | 3.004365 | 2.071597 | 2.074507  \n        | 3.683960 |16.765067 | 2.999775 | 2.075354 | 2.075759  \n================================================================\n\nRostislav Krasny (1):\n  compat: modernize and simplify byte swapping functions\n\n compat/bswap.h | 74 ++++++++++++++++++++++++++++++--------------------\n 1 file changed, 44 insertions(+), 30 deletions(-)\n\n-- \n2.52.0\n\n"},{"id":"532896","messageId":"20260102002735.31390-2-rostiprodev@gmail.com","threadId":"64708","inReplyTo":"20260102002735.31390-1-rostiprodev@gmail.com","subject":"[PATCH 1/1] compat: modernize and simplify byte swapping functions","fromName":"Rostislav Krasny","fromEmail":"rostiprodev@gmail.com","sentAt":"2026-01-02T00:27:35Z","receivedAt":"2026-01-02T00:27:49Z","isPatch":true,"sender":{"key":"rostiprodev@gmail.com","avatar":null},"body":"Replace manual bit operations with memcpy + network functions for better\nmaintainability. Add missing 16-bit network byte order conversion.\n\nKey improvements:\n- Simplify the get_be*() and put_be*() functions\n- Add bswap16() macro with automatic compiler optimization:\n  * GCC/Clang: __builtin_bswap16() intrinsic\n  * MSVC: _byteswap_ushort() intrinsic\n- Add default_bswap16() static inline function with manual fallback implementation\n- Rename default_swab32() to default_bswap32() for naming consistency\n- Add ntohs() and htons() macros for complete 16/32/64-bit network conversion\n- Add put_be16() function for API completeness alongside existing put_be*() and\n  get_be*() functions\n- Performance improvements (GCC 15.2.1, Clang 21.1.7):\n  * on x86-64 with -O0 4.2x faster (GCC), 3.7x faster (Clang)\n  * on x86-64 with -O1 4x faster (GCC), identical (Clang)\n  * on x86-64 with -O2 identical (GCC), 1.8x faster (Clang)\n\nSigned-off-by: Rostislav Krasny <rostiprodev@gmail.com>\n---\n compat/bswap.h | 74 ++++++++++++++++++++++++++++++--------------------\n 1 file changed, 44 insertions(+), 30 deletions(-)\n\ndiff --git a/compat/bswap.h b/compat/bswap.h\nindex 28635ebc69..f9954ef090 100644\n--- a/compat/bswap.h\n+++ b/compat/bswap.h\n@@ -9,10 +9,15 @@\n  */\n \n /*\n- * Default version that the compiler ought to optimize properly with\n+ * Default versions that the compiler ought to optimize properly with\n  * constant values.\n  */\n-static inline uint32_t default_swab32(uint32_t val)\n+static inline uint16_t default_bswap16(uint16_t val)\n+{\n+\treturn ((val & 0xff00) >> 8) | ((val & 0x00ff) << 8);\n+}\n+\n+static inline uint32_t default_bswap32(uint32_t val)\n {\n \treturn (((val & 0xff000000) >> 24) |\n \t\t((val & 0x00ff0000) >>  8) |\n@@ -40,6 +45,7 @@ static inline uint64_t default_bswap64(uint64_t val)\n # define __has_builtin(x) 0\n #endif\n \n+#undef bswap16\n #undef bswap32\n #undef bswap64\n \n@@ -47,6 +53,7 @@ static inline uint64_t default_bswap64(uint64_t val)\n \n #include <stdlib.h>\n \n+#define bswap16(x) _byteswap_ushort(x)\n #define bswap32(x) _byteswap_ulong(x)\n #define bswap64(x) _byteswap_uint64(x)\n \n@@ -54,8 +61,9 @@ static inline uint64_t default_bswap64(uint64_t val)\n #define GIT_BIG_ENDIAN 4321\n #define GIT_BYTE_ORDER GIT_LITTLE_ENDIAN\n \n-#elif __has_builtin(__builtin_bswap32) && __has_builtin(__builtin_bswap64)\n+#elif __has_builtin(__builtin_bswap16) && __has_builtin(__builtin_bswap32) && __has_builtin(__builtin_bswap64)\n \n+#define bswap16(x) __builtin_bswap16((x))\n #define bswap32(x) __builtin_bswap32((x))\n #define bswap64(x) __builtin_bswap64((x))\n \n@@ -98,24 +106,36 @@ static inline uint64_t default_bswap64(uint64_t val)\n \n #endif\n \n+#undef ntohs\n+#undef htons\n #undef ntohl\n #undef htonl\n #undef ntohll\n #undef htonll\n \n #if GIT_BYTE_ORDER == GIT_BIG_ENDIAN\n+# define ntohs(x) (x)\n+# define htons(x) (x)\n # define ntohl(x) (x)\n # define htonl(x) (x)\n # define ntohll(x) (x)\n # define htonll(x) (x)\n #else\n \n+# if defined(bswap16)\n+#  define ntohs(x) bswap16(x)\n+#  define htons(x) bswap16(x)\n+# else\n+#  define ntohs(x) default_bswap16(x)\n+#  define htons(x) default_bswap16(x)\n+# endif\n+\n # if defined(bswap32)\n #  define ntohl(x) bswap32(x)\n #  define htonl(x) bswap32(x)\n # else\n-#  define ntohl(x) default_swab32(x)\n-#  define htonl(x) default_swab32(x)\n+#  define ntohl(x) default_bswap32(x)\n+#  define htonl(x) default_bswap32(x)\n # endif\n \n # if defined(bswap64)\n@@ -129,47 +149,41 @@ static inline uint64_t default_bswap64(uint64_t val)\n \n static inline uint16_t get_be16(const void *ptr)\n {\n-\tconst unsigned char *p = ptr;\n-\treturn\t(uint16_t)p[0] << 8 |\n-\t\t(uint16_t)p[1] << 0;\n+\tuint16_t n;\n+\tmemcpy(&n, ptr, sizeof n);\n+\treturn ntohs(n);\n }\n \n static inline uint32_t get_be32(const void *ptr)\n {\n-\tconst unsigned char *p = ptr;\n-\treturn\t(uint32_t)p[0] << 24 |\n-\t\t(uint32_t)p[1] << 16 |\n-\t\t(uint32_t)p[2] <<  8 |\n-\t\t(uint32_t)p[3] <<  0;\n+\tuint32_t n;\n+\tmemcpy(&n, ptr, sizeof n);\n+\treturn ntohl(n);\n }\n \n static inline uint64_t get_be64(const void *ptr)\n {\n-\tconst unsigned char *p = ptr;\n-\treturn\t(uint64_t)get_be32(&p[0]) << 32 |\n-\t\t(uint64_t)get_be32(&p[4]) <<  0;\n+\tuint64_t n;\n+\tmemcpy(&n, ptr, sizeof n);\n+\treturn ntohll(n);\n+}\n+\n+static inline void put_be16(void *ptr, uint16_t value)\n+{\n+\tuint16_t n = htons(value);\n+\tmemcpy(ptr, &n, sizeof n);\n }\n \n static inline void put_be32(void *ptr, uint32_t value)\n {\n-\tunsigned char *p = ptr;\n-\tp[0] = (value >> 24) & 0xff;\n-\tp[1] = (value >> 16) & 0xff;\n-\tp[2] = (value >>  8) & 0xff;\n-\tp[3] = (value >>  0) & 0xff;\n+\tuint32_t n = htonl(value);\n+\tmemcpy(ptr, &n, sizeof n);\n }\n \n static inline void put_be64(void *ptr, uint64_t value)\n {\n-\tunsigned char *p = ptr;\n-\tp[0] = (value >> 56) & 0xff;\n-\tp[1] = (value >> 48) & 0xff;\n-\tp[2] = (value >> 40) & 0xff;\n-\tp[3] = (value >> 32) & 0xff;\n-\tp[4] = (value >> 24) & 0xff;\n-\tp[5] = (value >> 16) & 0xff;\n-\tp[6] = (value >>  8) & 0xff;\n-\tp[7] = (value >>  0) & 0xff;\n+\tuint64_t n = htonll(value);\n+\tmemcpy(ptr, &n, sizeof n);\n }\n \n #endif /* COMPAT_BSWAP_H */\n-- \n2.52.0\n\n"},{"id":"532899","messageId":"20260102061626.GA2581074@coredump.intra.peff.net","threadId":"64708","inReplyTo":"20260102002735.31390-2-rostiprodev@gmail.com","subject":"Re: [PATCH 1/1] compat: modernize and simplify byte swapping functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-02T06:16:27Z","receivedAt":"2026-01-02T06:16:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 02, 2026 at 02:27:35AM +0200, Rostislav Krasny wrote:\n\n> Replace manual bit operations with memcpy + network functions for better\n> maintainability. Add missing 16-bit network byte order conversion.\n\nThis is burying the lede a bit, as they say. I don't know that the\nmaintainability is much changed, especially as these are not functions\nthat anybody looks at or touches very often.\n\nBut this part might be compelling:\n\n> - Performance improvements (GCC 15.2.1, Clang 21.1.7):\n>   * on x86-64 with -O0 4.2x faster (GCC), 3.7x faster (Clang)\n>   * on x86-64 with -O1 4x faster (GCC), identical (Clang)\n>   * on x86-64 with -O2 identical (GCC), 1.8x faster (Clang)\n\nThe -O0 numbers are IMHO not very interesting (and are entirely\nexpected; you are comparing optimized library memcpy versus unoptimized\nassignments). But clang making -O2 faster is quite interesting.\n\nIf we are going to do this, I think it would be for the improved\nperformance. And it would be nice for the commit message to go into\ndetails about what was measured and how. I'll respond elsewhere in the\nthread with some more thoughts.\n\n-Peff\n"},{"id":"532902","messageId":"20260102072943.GB2581074@coredump.intra.peff.net","threadId":"64708","inReplyTo":"20260102002735.31390-1-rostiprodev@gmail.com","subject":"Re: [PATCH 0/1] compat: modernize and simplify byte swapping functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-02T07:29:43Z","receivedAt":"2026-01-02T07:29:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 02, 2026 at 02:27:34AM +0200, Rostislav Krasny wrote:\n\n> The main reason it was implemented so complicated is UB when conversion of a\n> pointer to one object type into a pointer of a different object type is used.\n> On the other hand, memcpy is protected from such UB and this allows us to make\n> that code simpler and even more optimal, in some cases.\n\nWe actually used to do casts and unaligned loads on platforms that\nallowed it (like x86). But what I measured in c578e29ba0 (bswap.h: drop\nunaligned loads, 2020-09-24) indicated that it did not really help, and\nthe -O2-optimized long-hand code performed the same. So I'm a little\nsurprised that going back in that direction is beneficial. But my\nnumbers were all with gcc, and your improvement was seen with clang, so\nlet's see if we can tease out the reasons.\n\n> #define ITERATIONS 1000000\n> #define BUF_SIZE 8192\n> \n> int main() {\n>     uint8_t buffer[BUF_SIZE];\n>     uint64_t sum = 0;\n> \n>     for (int i = 0; i < BUF_SIZE; i++) {\n>         buffer[i] = (uint8_t)i;\n>     }\n> \n>     clock_t start = clock();\n> \n>     for (int i = 0; i < ITERATIONS; i++) {\n>         // use a volatile pointer to force the compiler to read memory\n>         volatile uint8_t *p = buffer; \n>         for (int j = 0; j < BUF_SIZE - 8; j++) {\n>             sum += get_be64((const void*)(p + j));\n>         }\n>     }\n\nOK, so this is a measure of pure be64 speeds. That's over-emphasizing\nwhat we'd see in a real workload, but it should at least help us focus\non whether we can see any improvement.\n\nYou mentioned sha256 earlier, but it only has a single get_be32() call.\nI couldn't measure any change with \"test-tool sha256\" before/after your\npatch. For block-sha1, we'd see get_be32() calls, too, but these days\nwe'd almost always use the (much slower) sha1dc anyway.\n\nIt looks like you iterate byte by byte, so we do get some unaligned\ncalls there. Good.\n\nI pulled this into Git itself to make it easier to test particular\nbuilds, and to measure it with hyperfine (which introduces some extra\nnoise due to program startup and exit, but also gives us nicer\nstatistics):\n\ndiff --git a/common-main.c b/common-main.c\nindex 6b7ab077b0..9f19dfe68c 100644\n--- a/common-main.c\n+++ b/common-main.c\n@@ -1,10 +1,31 @@\n #include \"git-compat-util.h\"\n #include \"common-init.h\"\n \n+#define ITERATIONS 1000000\n+#define BUF_SIZE 8192\n+\n int main(int argc, const char **argv)\n {\n \tint result;\n \n+\tuint8_t buffer[BUF_SIZE];\n+\tuint64_t sum = 0;\n+\n+\tfor (int i = 0; i < BUF_SIZE; i++) {\n+\t\tbuffer[i] = (uint8_t)i;\n+\t}\n+\n+\tfor (int i = 0; i < ITERATIONS; i++) {\n+\t\t// use a volatile pointer to force the compiler to read memory\n+\t\tvolatile uint8_t *p = buffer;\n+\t\tfor (int j = 0; j < BUF_SIZE - 8; j++) {\n+\t\t\tsum += get_be64((const void*)(p + j));\n+\t\t}\n+\t}\n+\tprintf(\"%\"PRIuMAX, (uintmax_t)sum);\n+\t/* skip git stuff */\n+\texit(0);\n+\n \tinit_git(argv);\n \tresult = cmd_main(argc, argv);\n \n\n> And these are the results:\n> \n> GCC 15.2.1\n> version |  -Os     |  -O0     |  -O1     |  -O2     |  -O3\n> ================================================================\n>         | 3.721806 |72.342204 |11.956021 | 3.119833 | 0.919873  \n> original| 3.726111 |72.326920 |11.963618 | 3.128222 | 0.921128  \n>         | 3.719791 |72.328175 |11.949108 | 3.130956 | 0.920296         \n> ================================================================\n>         | 3.719899 |17.177719 | 3.005065 | 3.120747 | 0.920609  \n> new     | 3.714785 |17.168950 | 3.004978 | 3.119227 | 0.918851  \n>         | 3.716782 |17.145386 | 3.009364 | 3.119573 | 0.920030  \n> ================================================================\n\nI think we can disregard the -O0 results. They're both horribly slow,\nand for obvious reasons.\n\nThe -O2 results match what I saw back when I measured sha1 performance\n(though I also wouldn't be surprised if get_be32() is lost in the noise\nthere). That was also with gcc. Testing again with the patch above, I\nget similar results to you (actually a slight slowdown, but within the\nstatistical noise).\n\nThe -O3 results are interesting. Not because they change, but because\nthey outperform -O2 so handily in this case. I get similar results here.\n\nThe diff of the generated asm between o2 and o3 looks like this:\n\n--- asm.o2\t2026-01-02 01:51:35.312098163 -0500\n+++ asm.o3\t2026-01-02 01:51:45.744134814 -0500\n@@ -19,6 +19,7 @@\n \tmovdqa\t.LC0(%rip), %xmm2\n \tmovd\t%edi, %xmm9\n \tmovl\t$8, %edi\n+\tmovq\t%rsp, %rcx\n \tmovq\t%rsp, %rax\n \tmovd\t%edi, %xmm8\n \tmovl\t$12, %edi\n@@ -59,24 +60,23 @@\n \tpand\t%xmm5, %xmm0\n \tpackuswb\t%xmm0, %xmm1\n \tmovaps\t%xmm1, -16(%rax)\n-\tcmpq\t%rdi, %rax\n+\tcmpq\t%rax, %rdi\n \tjne\t.L2\n-\tmovl\t$1000000, %edi\n+\tleaq\t8184(%rcx), %rdi\n \txorl\t%esi, %esi\n-\tleaq\t8184(%rsp), %rcx\n .L3:\n-\tmovq\t%rsp, %rax\n-\t.p2align 5\n+\tmovq\t(%rcx), %rdx\n+\tmovl\t$1000000, %eax\n+\tbswap\t%rdx\n+\t.p2align 4\n \t.p2align 4\n \t.p2align 3\n .L4:\n-\tmovq\t(%rax), %rdx\n-\taddq\t$1, %rax\n-\tbswap\t%rdx\n-\taddq\t%rdx, %rsi\n-\tcmpq\t%rcx, %rax\n+\tleaq\t(%rsi,%rdx,2), %rsi\n+\tsubl\t$2, %eax\n \tjne\t.L4\n-\tsubl\t$1, %edi\n+\taddq\t$1, %rcx\n+\tcmpq\t%rcx, %rdi\n \tjne\t.L3\n \tleaq\t.LC6(%rip), %rdi\n \txorl\t%eax, %eax\n\nIt looks like the loops were inverted and the bswap was hoisted out of\nthe inner loop. ;) So that is really just an artifact of how this test\ncode was written (since we are summing, we do not care about iteration\norder).\n\nOh well, a 3x speedup for -O3 was probably too good to be true anyway.\n\nIt feels like there is an easy optimization on top of that loop\ninversion: rather than iterating and adding, we could just multiply by\nITERATIONS. And indeed, if we re-order the loops ourselves:\n\ndiff --git a/common-main.c b/common-main.c\nindex 9f19dfe68c..5960755dcc 100644\n--- a/common-main.c\n+++ b/common-main.c\n@@ -15,11 +15,12 @@ int main(int argc, const char **argv)\n \t\tbuffer[i] = (uint8_t)i;\n \t}\n \n-\tfor (int i = 0; i < ITERATIONS; i++) {\n+\tfor (int j = 0; j < BUF_SIZE - 8; j++) {\n \t\t// use a volatile pointer to force the compiler to read memory\n \t\tvolatile uint8_t *p = buffer;\n-\t\tfor (int j = 0; j < BUF_SIZE - 8; j++) {\n-\t\t\tsum += get_be64((const void*)(p + j));\n+\t\tuint64_t swapped = get_be64((const void *)(p + j));\n+\t\tfor (int i = 0; i < ITERATIONS; i++) {\n+\t\t\tsum += swapped;\n \t\t}\n \t}\n \tprintf(\"%\"PRIuMAX, (uintmax_t)sum);\n\nthen even at -O2 the whole thing runs in ~2ms, and the generated asm\nlooks like this:\n\n        bswap   %rax\n        imulq   $1000000, %rax, %rax\n        addq    %rax, %rsi\n\nIt's a little funny that -O3 can do the loop inversion but misses the\nmultiplication. :)\n\n> Clang 21.1.7\n> version |  -Os     |  -O0     |  -O1     |  -O2     |  -O3\n> ================================================================\n>         | 3.690718 |62.916338 | 3.017460 | 3.768443 | 3.778840  \n> original| 3.686283 |62.965916 | 3.014674 | 3.777897 | 3.774776  \n>         | 3.687775 |62.850648 | 3.003496 | 3.766108 | 3.765313         \n> ================================================================\n>         | 3.681818 |16.753385 | 3.008131 | 2.075271 | 2.076090  \n> new     | 3.687184 |16.737982 | 3.004365 | 2.071597 | 2.074507  \n>         | 3.683960 |16.765067 | 2.999775 | 2.075354 | 2.075759  \n> ================================================================\n\nOK, so here we are getting into the interesting bits, I think. -O0 is\nagain not really that interesting. And -O3 here behaves like -O2, so\npresumably it doesn't figure out the loop inversion.\n\nBut -O2 shows two interesting things. One, the existing code is much\nslower with clang than gcc. And two, it gets much faster with your\npatch. I was able to replicate both results.\n\nFor the first, diffing against gcc's -O2 asm might be interesting. Using\n\"diff\" is not productive because there are too many other differences. A\ncursory inspection by hand shows the inner loops pretty similar:\n\n  gcc:\n        movq    (%rax), %rdx\n        addq    $1, %rax\n        bswap   %rdx\n        addq    %rdx, %rsi\n        cmpq    %rcx, %rax\n        jne     .L4\n\n  clang:\n        movq    -7(%rsp,%rcx), %rdx\n        bswapq  %rdx\n        addq    %rdx, %rsi\n        incq    %rcx\n        cmpq    $8191, %rcx                     # imm = 0x1FFF\n        jne     .LBB0_4\n\nI think the bswapq vs bswap difference is a red herring, since rdx is a\n64-bit register and both should do a 64-bit swap. The loop counters are\nhandled a bit differently, but fundamentally we are just bswapping,\nadding, and looping. I wonder why they perform so differently. But I\nsuspect it has more to do with the fake test looping and not get_be64()\nitself.\n\nSo let's see what changes after your patch:\n\n--- asm.old\t2026-01-02 02:07:25.455631337 -0500\n+++ asm.new\t2026-01-02 02:17:40.702545167 -0500\n@@ -87,15 +87,30 @@\n \t.p2align\t4\n .LBB0_3:                                # =>This Loop Header: Depth=1\n                                         #     Child Loop BB0_4 Depth 2\n-\tmovl\t$7, %ecx\n+\tmovl\t$5, %ecx\n \t.p2align\t4\n .LBB0_4:                                #   Parent Loop BB0_3 Depth=1\n                                         # =>  This Inner Loop Header: Depth=2\n-\tmovq\t-7(%rsp,%rcx), %rdx\n+\tmovq\t-5(%rsp,%rcx), %rdx\n+\tmovq\t-4(%rsp,%rcx), %rdi\n \tbswapq\t%rdx\n+\tbswapq\t%rdi\n+\taddq\t%rsi, %rdx\n+\tmovq\t-3(%rsp,%rcx), %r8\n+\tbswapq\t%r8\n+\taddq\t%rdi, %r8\n+\tmovq\t-2(%rsp,%rcx), %rsi\n+\tbswapq\t%rsi\n+\taddq\t%rdx, %r8\n+\tmovq\t-1(%rsp,%rcx), %rdx\n+\tbswapq\t%rdx\n+\taddq\t%rsi, %rdx\n+\tmovq\t(%rsp,%rcx), %rsi\n+\tbswapq\t%rsi\n \taddq\t%rdx, %rsi\n-\tincq\t%rcx\n-\tcmpq\t$8191, %rcx                     # imm = 0x1FFF\n+\taddq\t%r8, %rsi\n+\taddq\t$6, %rcx\n+\tcmpq\t$8189, %rcx                     # imm = 0x1FFD\n \tjne\t.LBB0_4\n # %bb.5:                                #   in Loop: Header=BB0_3 Depth=1\n \tincl\t%eax\n\nHuh. So we do a bunch more bswap instructions, but move in larger chunks\nthrough the loop. I didn't work out the details, but it looks like it's\ntaking advantage of the fact that we're bswapping overlapping bits of\nmemory (which is something a real program would hardly ever do).\n\nIf we tweak our program to just swap the whole buffer, 8 bytes at a\ntime:\n\ndiff --git a/common-main.c b/common-main.c\nindex 9f19dfe68c..55a7066c85 100644\n--- a/common-main.c\n+++ b/common-main.c\n@@ -18,7 +18,7 @@ int main(int argc, const char **argv)\n \tfor (int i = 0; i < ITERATIONS; i++) {\n \t\t// use a volatile pointer to force the compiler to read memory\n \t\tvolatile uint8_t *p = buffer;\n-\t\tfor (int j = 0; j < BUF_SIZE - 8; j++) {\n+\t\tfor (int j = 0; j < BUF_SIZE; j += 8) {\n \t\t\tsum += get_be64((const void*)(p + j));\n \t\t}\n \t}\n\nthen the difference before/after your patch goes away:\n\n  Benchmark 1: ./git.old.clang.o2\n    Time (mean ± σ):     482.9 ms ±   7.2 ms    [User: 482.5 ms, System: 0.4 ms]\n    Range (min … max):   471.7 ms … 495.3 ms    10 runs\n  \n  Benchmark 2: ./git.new.clang.o2\n    Time (mean ± σ):     472.4 ms ±   4.5 ms    [User: 471.6 ms, System: 0.8 ms]\n    Range (min … max):   465.3 ms … 477.8 ms    10 runs\n  \n  Summary\n    ./git.new.clang.o2 ran\n      1.02 ± 0.02 times faster than ./git.old.clang.o2\n\nNote that everything is 8x faster than before because we are doing 8x\nfewer bswaps. But the time doesn't change before/after the patch.\n\nSo what does it all mean?\n\nI _think_ most of the speed differences we've seen are artifacts of the\ntest program, and not a big difference in how get_be64() is being\nimplemented. We end up with bswap instructions either way. I could be\nwrong, though; there were a lot of versions to juggle, and I'm pretty\nbad at reading assembly language.\n\nAll that said, I'm not particularly opposed to your patch. The memcpy\nversion may be easier to read. I'm just a little skeptical that it\nprovides performance improvements.\n\n-Peff\n"},{"id":"532923","messageId":"CAKU3Xk5=dmdQhTgHB8WrPbbOOo3cyJtCgFgo7juW06F9YaceRQ@mail.gmail.com","threadId":"64708","inReplyTo":"20260102061626.GA2581074@coredump.intra.peff.net","subject":"Re: [PATCH 1/1] compat: modernize and simplify byte swapping functions","fromName":"Rostislav Krasny","fromEmail":"rostiprodev@gmail.com","sentAt":"2026-01-02T17:37:44Z","receivedAt":"2026-01-02T17:37:56Z","isPatch":true,"sender":{"key":"rostiprodev@gmail.com","avatar":null},"body":"On Fri, 2 Jan 2026 at 08:16, Jeff King <peff@peff.net> wrote:\n>\n> On Fri, Jan 02, 2026 at 02:27:35AM +0200, Rostislav Krasny wrote:\n>\n> > Replace manual bit operations with memcpy + network functions for better\n> > maintainability. Add missing 16-bit network byte order conversion.\n>\n> This is burying the lede a bit, as they say. I don't know that the\n> maintainability is much changed, especially as these are not functions\n> that anybody looks at or touches very often.\n\nI just meant that the code has become simpler and more understandable.\nEnglish is not my native language, but if you would like me to\nrephrase the commit message, I'll be happy to do so. Just suggest a\nbetter wording and I will send v2.\n\n> But this part might be compelling:\n>\n> > - Performance improvements (GCC 15.2.1, Clang 21.1.7):\n> >   * on x86-64 with -O0 4.2x faster (GCC), 3.7x faster (Clang)\n> >   * on x86-64 with -O1 4x faster (GCC), identical (Clang)\n> >   * on x86-64 with -O2 identical (GCC), 1.8x faster (Clang)\n>\n> The -O0 numbers are IMHO not very interesting (and are entirely\n> expected; you are comparing optimized library memcpy versus unoptimized\n> assignments). But clang making -O2 faster is quite interesting.\n>\n> If we are going to do this, I think it would be for the improved\n> performance. And it would be nice for the commit message to go into\n> details about what was measured and how. I'll respond elsewhere in the\n> thread with some more thoughts.\n\nThe main motivation of this pull request is improved simplicity,\nreadability and consistency of the new code. It looked strange to me\nthat for converting a value the optimized ntoh* and hton* macros from\nthe same bswap.h could be used, while for pointers a more complicated\ncode in the get_be* and put_be* functions is used. This PR also brings\na few more small improvements like fix of typos in names and\nadditional functions for API completeness.\n\nMy performance tests (that you discuss in your second email) show that\nat least there is no regression in performance. They also show that\nwith n < 2 in -On it is much easier for the compiler to optimize the\nnew code. I can agree that -O0 and -O1 are less relevant in release\nbuilds of Git, but the fact that the new code makes the compiler's\nwork easier with any -On only strengthens the assertion that the new\ncode has no performance loss.\n\nAnd yes, the code that I used to measure the performance of get_be64()\nis probably not ideal but at least it proves that there is no\nregression in performance.\n\n> -Peff\n"},{"id":"533546","messageId":"CAKU3Xk5kCEDU7JZBhb6a46dZ=gEkP4neCNLMHXVB4RDnYZHG0w@mail.gmail.com","threadId":"64708","inReplyTo":"CAKU3Xk5=dmdQhTgHB8WrPbbOOo3cyJtCgFgo7juW06F9YaceRQ@mail.gmail.com","subject":"Re: [PATCH 1/1] compat: modernize and simplify byte swapping functions","fromName":"Rostislav Krasny","fromEmail":"rostiprodev@gmail.com","sentAt":"2026-01-11T22:05:22Z","receivedAt":"2026-01-11T22:05:34Z","isPatch":true,"sender":{"key":"rostiprodev@gmail.com","avatar":null},"body":"Hello again,\n\nDid you decide something about this pull request? Should I improve it\nand send v2?\n"},{"id":"533892","messageId":"20260114211409.GA1010080@coredump.intra.peff.net","threadId":"64708","inReplyTo":"CAKU3Xk5kCEDU7JZBhb6a46dZ=gEkP4neCNLMHXVB4RDnYZHG0w@mail.gmail.com","subject":"Re: [PATCH 1/1] compat: modernize and simplify byte swapping functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-14T21:14:09Z","receivedAt":"2026-01-14T21:14:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 12, 2026 at 12:05:22AM +0200, Rostislav Krasny wrote:\n\n> Did you decide something about this pull request? Should I improve it\n> and send v2?\n\nI don't have a strong opinion on it. To some degree, it feels a bit like\ncode churn, because I do not recall anybody complaining particularly\nabout the current implementation (and as we saw, it ends up as bswap\neither way). But maybe others feel differently.\n\n-Peff\n"}]}