From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 2E65626F52 for ; Tue, 12 Feb 2019 14:35:15 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id mWBLNCOw4Ak5 for ; Tue, 12 Feb 2019 14:35:15 -0500 (EST) Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id C05AE26F35 for ; Tue, 12 Feb 2019 14:35:14 -0500 (EST) From: =?utf-8?B?0JPQtdC+0YDQs9C40Lkg0JrQuNGA0LjRh9C10L3QutC+?= Subject: [tarantool-patches] Re: [PATCH 1/2] Lightweight vclock_create and vclock_copy Date: Tue, 12 Feb 2019 22:37:17 +0300 Message-ID: <3149075.a6KHynF7k3@home.lan> In-Reply-To: <20190212192524.GB10042@chai> References: <20190212192524.GB10042@chai> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart1573923.YHLnD7loIh"; micalg="pgp-sha256"; protocol="application/pgp-signature" Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Konstantin Osipov Cc: tarantool-patches@freelists.org --nextPart1573923.YHLnD7loIh Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" On Tuesday, February 12, 2019 10:25:24 PM MSK Konstantin Osipov wrote: > * Georgy Kirichenko [19/02/12 17:09]: > > /** > > > > @@ -158,6 +159,8 @@ vclock_get(const struct vclock *vclock, uint32_t > > replica_id)> > > { > > > > if (replica_id >= VCLOCK_MAX) > > > > return 0; > > I checked the code and I there is no place which passes arbitrary > replica id. I believe this was added to avoid access to > uninitialized memory in case of corrupt network packet. Now that > vclock_get() becomes a hot path, let's avoid branching by having a > cheaper solution for this problem: > > /** Avoid segmentation fault in case of malformed packet. */ > replica_id &= VLCOCK_MAX - 1; > > > + if ((vclock->map & (1 << replica_id)) == 0) > > + return 0; > > > > return vclock->lsn[replica_id]; > > > > } > > > > @@ -165,6 +168,8 @@ static inline int64_t > > > > vclock_inc(struct vclock *vclock, uint32_t replica_id) > > { > > > > /* Easier add each time than check. */ > > > > + if ((vclock->map & (1 << replica_id)) == 0) > > + vclock->lsn[replica_id] = 0; > > > > vclock->map |= 1 << replica_id; > > Since you added a check, please move this assignment inside the > branch. > > > vclock->signature++; > > return ++vclock->lsn[replica_id]; > > > > @@ -173,7 +178,13 @@ vclock_inc(struct vclock *vclock, uint32_t > > replica_id) > > > > static inline void > > vclock_copy(struct vclock *dst, const struct vclock *src) > > { > > > > - *dst = *src; > > + if (src->map == 0) { > > + dst->map = src->map; > > + dst->signature = src->signature; > > + return; > > + } > > Why would you ever need this branch? Looks like you can safely > delete it. I added this because result of bit_clz_u32(src->map) is undefined in case when src->map == 0 > > > + unsigned int max_pos = VCLOCK_MAX - bit_clz_u32(src->map); > > + memcpy(dst, src, offsetof(struct vclock, lsn) + sizeof(*dst->lsn) * > > max_pos);> > > } > > Apart from these minor comments the branch is looking good, thank > you! --nextPart1573923.YHLnD7loIh Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEFB+nbqWGnp59Rk9ZFSyY70x8X3sFAlxjIG0ACgkQFSyY70x8 X3tZewf+NC1tVEcZ9NjDa4H60U1/YryW6a7zUVtS73gCJo7n4/sJp9qVFXxlf75/ tpfJEk5VUNvdQqHxmkZdj+Bm6rlcsh3dPCuMa/IFzlqRbbCNW94V+cgBbJWEva7j 7g3k6kJsqF7yDfbkl0RavC1kOYUIFmenFUTuls1Z0tO8Pv4X80Sx+0u6hcC7VfdR W9QBCOdMB4tSo3sEVkE1hkihe/YlBzsOT+07pr1FH8mNQ6D02iXaJFK0bsRPF/mE ySxqj+BH+7lyz6YriF+akhYpQJpT/v5q88QeLHytIoVfZ4iwnuOy6Ajh4T0g5bcA LJCPv0m/uaAYc+5HbUpBc226UhyKGQ== =cyR1 -----END PGP SIGNATURE----- --nextPart1573923.YHLnD7loIh--