From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f195.google.com (mail-lj1-f195.google.com [209.85.208.195]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 6E3EA445320 for ; Sat, 11 Jul 2020 00:27:30 +0300 (MSK) Received: by mail-lj1-f195.google.com with SMTP id 9so7978536ljv.5 for ; Fri, 10 Jul 2020 14:27:30 -0700 (PDT) Date: Sat, 11 Jul 2020 00:27:25 +0300 From: Cyrill Gorcunov Message-ID: <20200710212725.GJ1999@grain> References: <20200710075605.217824-1-gorcunov@gmail.com> <20200710075605.217824-5-gorcunov@gmail.com> <804b33af-ddea-aefa-b69f-5615831967e7@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <804b33af-ddea-aefa-b69f-5615831967e7@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 4/5] qsync: txn_commit -- use txn flag instead of caching variable List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tml On Fri, Jul 10, 2020 at 10:36:32PM +0200, Vladislav Shpilevoy wrote: > On 10/07/2020 09:56, Cyrill Gorcunov wrote: > > All over the code (including txn_libmo) we use txn_has_flag > > helper, lets do the same here. There is no need for a separate > > variable. It simply confuses (note though that sometimes such > > trick is used to fetch current value of some variable which > > might be changed externally but this is not out case we rely > > on single thread here). > > > > I also added a few empty lines to separate logical code blocks > > for better readability. > > > > Signed-off-by: Cyrill Gorcunov > > --- > > src/box/txn.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/src/box/txn.c b/src/box/txn.c > > index 613eb7aef..feb9a10c6 100644 > > --- a/src/box/txn.c > > +++ b/src/box/txn.c > > @@ -811,8 +811,7 @@ txn_commit(struct txn *txn) > > return -1; > > } > > > > - bool is_sync = txn_has_flag(txn, TXN_WAIT_SYNC); > > - if (is_sync) { > > + if (txn_has_flag(txn, TXN_WAIT_SYNC)) { > > Здесь это сохранено в переменную не просто так. А > как раз за тем, чтобы не вычитывать флаг заново на каждый иф. > Не надо это менять плиз. 1) We're accessing txn entries very frequently in this code, which means it sits in cache. 2) The compiler will notice that the variable is read only (note that we didn't mark it as volatile or any other attribute, which sometimes needed indeed) and _most_ probably simply fetch it once but to be fair I didn't verify. Anyway I thik the grep'ability reason is very strong until cached variable gives you significant impact in performance. On the other hands if we're expecting this code to change in near future feel free to drop it I'm fine.