Re: [Tarantool-patches] [PATCH 2/2] uuid: fix unaligned memory access

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the comments!

On 19/05/2020 09:28, Aleksandr Lyapunov wrote:
> 
> On 5/19/20 12:17 AM, Vladislav Shpilevoy wrote:
>>
>> Yeah, well. This is the same like saying that we degrade performance
>> when we do OOM checks. Unaligned memory access is UB. This is a bug.
> It's not the same. You could see I propose a solution that both violates no
> rules and have the best performance. It's very far from just a complain.

The best performance, but not in tarantool as I see. Since your bench
has nothing to do with tarantool code.

>>> I would suggest to use memcmp in this case.
>>> It's portable and allows a compiler to generate the best possible code.
>>> I've measured it (gcc) and memcmp version is twice faster than your solution.
>>> Even for _is_nil method it's better to use memcmp with statically allocated zero buffer.
>> Could you please show the benchmark? I did my own, and I can't see any
>> significant difference. The present difference is so small, that it
>> looks like jitter. Both in is_nil and is_eq.
>>
>> I did a very simple bench in Lua, without any GCed objects.
> I fear that Lua is not suitable for performance tests.> Here you are: https://pastebin.com/VXkS1v6M
> Also please take a look at disasm: https://godbolt.org/z/s6Cti4

Lua is fine if you don't use GC, for most of the cases. To see if there
is any observable difference. In the code we check I couldn't find a
difference. *Observable*.

However the disassemble persuaded me. I checked tarantool's executable in
Release mode. Here is what I've got for 4 x uint32 in objdump:

10021b210:      55      pushq   %rbp
10021b211:      48 89 e5        movq    %rsp, %rbp
10021b214:      8b 07   movl    (%rdi), %eax
10021b216:      3b 06   cmpl    (%rsi), %eax
10021b218:      75 1b   jne     27 <_tt_uuid_is_equal+0x25>
10021b21a:      8b 47 04        movl    4(%rdi), %eax
10021b21d:      3b 46 04        cmpl    4(%rsi), %eax
10021b220:      75 17   jne     23 <_tt_uuid_is_equal+0x29>
10021b222:      8b 47 08        movl    8(%rdi), %eax
10021b225:      3b 46 08        cmpl    8(%rsi), %eax
10021b228:      75 13   jne     19 <_tt_uuid_is_equal+0x2d>
10021b22a:      8b 47 0c        movl    12(%rdi), %eax
10021b22d:      3b 46 0c        cmpl    12(%rsi), %eax
10021b230:      0f 94 c0        sete    %al
10021b233:      5d      popq    %rbp
10021b234:      c3      retq

Here is what I got for memcmp:

10021b220:      55      pushq   %rbp
10021b221:      48 89 e5        movq    %rsp, %rbp
10021b224:      f3 0f 6f 07     movdqu  (%rdi), %xmm0
10021b228:      f3 0f 6f 0e     movdqu  (%rsi), %xmm1
10021b22c:      66 0f 74 c8     pcmpeqb %xmm0, %xmm1
10021b230:      66 0f d7 c1     pmovmskb        %xmm1, %eax
10021b234:      3d ff ff 00 00  cmpl    $65535, %eax
10021b239:      0f 94 c0        sete    %al
10021b23c:      5d      popq    %rbp
10021b23d:      c3      retq

So yeah, looks like memcmp() is better indeed. Also I looked at
is_nil() - it looked like huge shit. I am really surprised the
compiler couldn't optimize it to the same what we see with memcmp().
Thanks for helping, I am glad you took a look at this. I used
memcmp() for both places.

I tried using a const 0 buffer in tt_uuid_is_nil() inside of it,
but appeared there is no difference with comparing with uuid_nil
directly. In the assembly. In clang. So I just took

    tt_uuid_is_equal(uu, &uuid_nil)

====================
diff --git a/src/lib/uuid/tt_uuid.h b/src/lib/uuid/tt_uuid.h
index d62991c65..70c3b98b1 100644
--- a/src/lib/uuid/tt_uuid.h
+++ b/src/lib/uuid/tt_uuid.h
@@ -149,19 +149,6 @@ tt_uuid_bswap(struct tt_uuid *uu)
 	uu->time_hi_and_version = bswap_u16(uu->time_hi_and_version);
 }
 
-/**
- * \brief Test that uuid is nil
- * \param uu UUID
- * \retval true if all members of \a uu 0
- * \retval false otherwise
- */
-inline bool
-tt_uuid_is_nil(const struct tt_uuid *uu)
-{
-	const uint32_t *p = (const uint32_t *) uu;
-	return p[0] == 0 && p[1] == 0 && p[2] == 0 && p[3] == 0;
-}
-
 /**
  * \brief Test that \a lhs equal \a rhs
  * \param lhs UUID
@@ -172,14 +159,23 @@ tt_uuid_is_nil(const struct tt_uuid *uu)
 inline bool
 tt_uuid_is_equal(const struct tt_uuid *lhs, const struct tt_uuid *rhs)
 {
-	const uint32_t *lp = (const uint32_t *) lhs;
-	const uint32_t *rp = (const uint32_t *) rhs;
-	return lp[0] == rp[0] && lp[1] == rp[1] && lp[2] == rp[2] &&
-	       lp[3] == rp[3];
+	return memcmp(lhs, rhs, sizeof(*lhs)) == 0;
 }
 
 extern const struct tt_uuid uuid_nil;
 
+/**
+ * \brief Test that uuid is nil.
+ * \param uu UUID.
+ * \retval true If all members of \a uu 0.
+ * \retval false Otherwise.
+ */
+inline bool
+tt_uuid_is_nil(const struct tt_uuid *uu)
+{
+	return tt_uuid_is_equal(uu, &uuid_nil);
+}
+
 char *
 tt_uuid_str(const struct tt_uuid *uu);