Re: [PATCH 0/1] compat: modernize and simplify byte swapping functions
On Fri, Jan 02, 2026 at 02:27:34AM +0200, Rostislav Krasny wrote:
> The main reason it was implemented so complicated is UB when conversion of a
> pointer to one object type into a pointer of a different object type is used.
> On the other hand, memcpy is protected from such UB and this allows us to make
> that code simpler and even more optimal, in some cases.
We actually used to do casts and unaligned loads on platforms that allowed it (like x86). But what I measured in c578e29ba0 (bswap.h: drop unaligned loads, 2020-09-24) indicated that it did not really help, and the -O2-optimized long-hand code performed the same. So I'm a little surprised that going back in that direction is beneficial. But my numbers were all with gcc, and your improvement was seen with clang, so let's see if we can tease out the reasons.
Show 20 quoted lines
> #define ITERATIONS 1000000
> #define BUF_SIZE 8192
>
> int main() {
> uint8_t buffer[BUF_SIZE];
> uint64_t sum = 0;
>
> for (int i = 0; i < BUF_SIZE; i++) {
> buffer[i] = (uint8_t)i;
> }
>
> clock_t start = clock();
>
> for (int i = 0; i < ITERATIONS; i++) {
> // use a volatile pointer to force the compiler to read memory
> volatile uint8_t *p = buffer;
> for (int j = 0; j < BUF_SIZE - 8; j++) {
> sum += get_be64((const void*)(p + j));
> }
> }OK, so this is a measure of pure be64 speeds. That's over-emphasizing what we'd see in a real workload, but it should at least help us focus on whether we can see any improvement.
You mentioned sha256 earlier, but it only has a single get_be32() call. I couldn't measure any change with "test-tool sha256" before/after your patch. For block-sha1, we'd see get_be32() calls, too, but these days we'd almost always use the (much slower) sha1dc anyway.
It looks like you iterate byte by byte, so we do get some unaligned calls there. Good.
I pulled this into Git itself to make it easier to test particular builds, and to measure it with hyperfine (which introduces some extra noise due to program startup and exit, but also gives us nicer statistics):
Show changes to common-main.c +56 −20
diff --git a/common-main.c b/common-main.c
index 6b7ab077b0..9f19dfe68c 100644
--- a/common-main.c
+++ b/common-main.c
@@ -1,10 +1,31 @@
#include "git-compat-util.h"
#include "common-init.h"
+#define ITERATIONS 1000000
+#define BUF_SIZE 8192
+
int main(int argc, const char **argv)
{
int result;
+ uint8_t buffer[BUF_SIZE];
+ uint64_t sum = 0;
+
+ for (int i = 0; i < BUF_SIZE; i++) {
+ buffer[i] = (uint8_t)i;
+ }
+
+ for (int i = 0; i < ITERATIONS; i++) {
+ // use a volatile pointer to force the compiler to read memory
+ volatile uint8_t *p = buffer;
+ for (int j = 0; j < BUF_SIZE - 8; j++) {
+ sum += get_be64((const void*)(p + j));
+ }
+ }
+ printf("%"PRIuMAX, (uintmax_t)sum);
+ /* skip git stuff */
+ exit(0);
+
init_git(argv);
result = cmd_main(argc, argv);
> And these are the results:
>
> GCC 15.2.1
> version | -Os | -O0 | -O1 | -O2 | -O3
> ================================================================
> | 3.721806 |72.342204 |11.956021 | 3.119833 | 0.919873
> original| 3.726111 |72.326920 |11.963618 | 3.128222 | 0.921128
> | 3.719791 |72.328175 |11.949108 | 3.130956 | 0.920296
> ================================================================
> | 3.719899 |17.177719 | 3.005065 | 3.120747 | 0.920609
> new | 3.714785 |17.168950 | 3.004978 | 3.119227 | 0.918851
> | 3.716782 |17.145386 | 3.009364 | 3.119573 | 0.920030
> ================================================================
I think we can disregard the -O0 results. They're both horribly slow,
and for obvious reasons.
The -O2 results match what I saw back when I measured sha1 performance
(though I also wouldn't be surprised if get_be32() is lost in the noise
there). That was also with gcc. Testing again with the patch above, I
get similar results to you (actually a slight slowdown, but within the
statistical noise).
The -O3 results are interesting. Not because they change, but because
they outperform -O2 so handily in this case. I get similar results here.
The diff of the generated asm between o2 and o3 looks like this:
--- asm.o2 2026-01-02 01:51:35.312098163 -0500
+++ asm.o3 2026-01-02 01:51:45.744134814 -0500
@@ -19,6 +19,7 @@
movdqa .LC0(%rip), %xmm2
movd %edi, %xmm9
movl $8, %edi
+ movq %rsp, %rcx
movq %rsp, %rax
movd %edi, %xmm8
movl $12, %edi
@@ -59,24 +60,23 @@
pand %xmm5, %xmm0
packuswb %xmm0, %xmm1
movaps %xmm1, -16(%rax)
- cmpq %rdi, %rax
+ cmpq %rax, %rdi
jne .L2
- movl $1000000, %edi
+ leaq 8184(%rcx), %rdi
xorl %esi, %esi
- leaq 8184(%rsp), %rcx
.L3:
- movq %rsp, %rax
- .p2align 5
+ movq (%rcx), %rdx
+ movl $1000000, %eax
+ bswap %rdx
+ .p2align 4
.p2align 4
.p2align 3
.L4:
- movq (%rax), %rdx
- addq $1, %rax
- bswap %rdx
- addq %rdx, %rsi
- cmpq %rcx, %rax
+ leaq (%rsi,%rdx,2), %rsi
+ subl $2, %eax
jne .L4
- subl $1, %edi
+ addq $1, %rcx
+ cmpq %rcx, %rdi
jne .L3
leaq .LC6(%rip), %rdi
xorl %eax, %eax
It looks like the loops were inverted and the bswap was hoisted out of
the inner loop. ;) So that is really just an artifact of how this test
code was written (since we are summing, we do not care about iteration
order).
Oh well, a 3x speedup for -O3 was probably too good to be true anyway.
It feels like there is an easy optimization on top of that loop
inversion: rather than iterating and adding, we could just multiply by
ITERATIONS. And indeed, if we re-order the loops ourselves:
diff --git a/common-main.c b/common-main.c
index 9f19dfe68c..5960755dcc 100644
--- a/common-main.c
+++ b/common-main.c
@@ -15,11 +15,12 @@ int main(int argc, const char **argv)
buffer[i] = (uint8_t)i;
}
- for (int i = 0; i < ITERATIONS; i++) {
+ for (int j = 0; j < BUF_SIZE - 8; j++) {
// use a volatile pointer to force the compiler to read memory
volatile uint8_t *p = buffer;
- for (int j = 0; j < BUF_SIZE - 8; j++) {
- sum += get_be64((const void*)(p + j));
+ uint64_t swapped = get_be64((const void *)(p + j));
+ for (int i = 0; i < ITERATIONS; i++) {
+ sum += swapped;
}
}
printf("%"PRIuMAX, (uintmax_t)sum);
then even at -O2 the whole thing runs in ~2ms, and the generated asm
looks like this:
bswap %rax
imulq $1000000, %rax, %rax
addq %rax, %rsi
It's a little funny that -O3 can do the loop inversion but misses the
multiplication. :)
> Clang 21.1.7
> version | -Os | -O0 | -O1 | -O2 | -O3
> ================================================================
> | 3.690718 |62.916338 | 3.017460 | 3.768443 | 3.778840
> original| 3.686283 |62.965916 | 3.014674 | 3.777897 | 3.774776
> | 3.687775 |62.850648 | 3.003496 | 3.766108 | 3.765313
> ================================================================
> | 3.681818 |16.753385 | 3.008131 | 2.075271 | 2.076090
> new | 3.687184 |16.737982 | 3.004365 | 2.071597 | 2.074507
> | 3.683960 |16.765067 | 2.999775 | 2.075354 | 2.075759
> ================================================================
OK, so here we are getting into the interesting bits, I think. -O0 is
again not really that interesting. And -O3 here behaves like -O2, so
presumably it doesn't figure out the loop inversion.
But -O2 shows two interesting things. One, the existing code is much
slower with clang than gcc. And two, it gets much faster with your
patch. I was able to replicate both results.
For the first, diffing against gcc's -O2 asm might be interesting. Using
"diff" is not productive because there are too many other differences. A
cursory inspection by hand shows the inner loops pretty similar:
gcc:
movq (%rax), %rdx
addq $1, %rax
bswap %rdx
addq %rdx, %rsi
cmpq %rcx, %rax
jne .L4
clang:
movq -7(%rsp,%rcx), %rdx
bswapq %rdx
addq %rdx, %rsi
incq %rcx
cmpq $8191, %rcx # imm = 0x1FFF
jne .LBB0_4
I think the bswapq vs bswap difference is a red herring, since rdx is a
64-bit register and both should do a 64-bit swap. The loop counters are
handled a bit differently, but fundamentally we are just bswapping,
adding, and looping. I wonder why they perform so differently. But I
suspect it has more to do with the fake test looping and not get_be64()
itself.
So let's see what changes after your patch:
--- asm.old 2026-01-02 02:07:25.455631337 -0500
+++ asm.new 2026-01-02 02:17:40.702545167 -0500
@@ -87,15 +87,30 @@
.p2align 4
.LBB0_3: # =>This Loop Header: Depth=1
# Child Loop BB0_4 Depth 2
- movl $7, %ecx
+ movl $5, %ecx
.p2align 4
.LBB0_4: # Parent Loop BB0_3 Depth=1
# => This Inner Loop Header: Depth=2
- movq -7(%rsp,%rcx), %rdx
+ movq -5(%rsp,%rcx), %rdx
+ movq -4(%rsp,%rcx), %rdi
bswapq %rdx
+ bswapq %rdi
+ addq %rsi, %rdx
+ movq -3(%rsp,%rcx), %r8
+ bswapq %r8
+ addq %rdi, %r8
+ movq -2(%rsp,%rcx), %rsi
+ bswapq %rsi
+ addq %rdx, %r8
+ movq -1(%rsp,%rcx), %rdx
+ bswapq %rdx
+ addq %rsi, %rdx
+ movq (%rsp,%rcx), %rsi
+ bswapq %rsi
addq %rdx, %rsi
- incq %rcx
- cmpq $8191, %rcx # imm = 0x1FFF
+ addq %r8, %rsi
+ addq $6, %rcx
+ cmpq $8189, %rcx # imm = 0x1FFD
jne .LBB0_4
# %bb.5: # in Loop: Header=BB0_3 Depth=1
incl %eax
Huh. So we do a bunch more bswap instructions, but move in larger chunks
through the loop. I didn't work out the details, but it looks like it's
taking advantage of the fact that we're bswapping overlapping bits of
memory (which is something a real program would hardly ever do).
If we tweak our program to just swap the whole buffer, 8 bytes at a
time:
diff --git a/common-main.c b/common-main.c
index 9f19dfe68c..55a7066c85 100644
--- a/common-main.c
+++ b/common-main.c
@@ -18,7 +18,7 @@ int main(int argc, const char **argv)
for (int i = 0; i < ITERATIONS; i++) {
// use a volatile pointer to force the compiler to read memory
volatile uint8_t *p = buffer;
- for (int j = 0; j < BUF_SIZE - 8; j++) {
+ for (int j = 0; j < BUF_SIZE; j += 8) {
sum += get_be64((const void*)(p + j));
}
}
then the difference before/after your patch goes away:
Benchmark 1: ./git.old.clang.o2
Time (mean ± σ): 482.9 ms ± 7.2 ms [User: 482.5 ms, System: 0.4 ms]
Range (min … max): 471.7 ms … 495.3 ms 10 runs
Benchmark 2: ./git.new.clang.o2
Time (mean ± σ): 472.4 ms ± 4.5 ms [User: 471.6 ms, System: 0.8 ms]
Range (min … max): 465.3 ms … 477.8 ms 10 runs
Summary
./git.new.clang.o2 ran
1.02 ± 0.02 times faster than ./git.old.clang.o2
Note that everything is 8x faster than before because we are doing 8x
fewer bswaps. But the time doesn't change before/after the patch.
So what does it all mean?
I _think_ most of the speed differences we've seen are artifacts of the
test program, and not a big difference in how get_be64() is being
implemented. We end up with bswap instructions either way. I could be
wrong, though; there were a lot of versions to juggle, and I'm pretty
bad at reading assembly language.
All that said, I'm not particularly opposed to your patch. The memcpy
version may be easier to read. I'm just a little skeptical that it
provides performance improvements.
-Peff
