From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 5489A469710 for ; Wed, 20 May 2020 00:24:36 +0300 (MSK) References: <172feb69ec2caea2aa63a24de8b76a4dba13bebb.1589583614.git.v.shpilevoy@tarantool.org> <52af1bb7-b125-7490-3882-ce74698789d2@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Tue, 19 May 2020 23:24:33 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 2/2] uuid: fix unaligned memory access List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aleksandr Lyapunov , tarantool-patches@dev.tarantool.org, tsafin@tarantool.org, gorcunov@gmail.com 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);