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 6103926D4A for ; Tue, 12 Feb 2019 14:25:27 -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 XDQe3V-9IxzL for ; Tue, 12 Feb 2019 14:25:27 -0500 (EST) Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (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 1A82426895 for ; Tue, 12 Feb 2019 14:25:27 -0500 (EST) Date: Tue, 12 Feb 2019 22:25:24 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH 1/2] Lightweight vclock_create and vclock_copy Message-ID: <20190212192524.GB10042@chai> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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: tarantool-patches@freelists.org Cc: Georgy Kirichenko * 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. > + 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! -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov