From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f68.google.com (mail-lf1-f68.google.com [209.85.167.68]) (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 407A1469710 for ; Sat, 21 Nov 2020 15:30:41 +0300 (MSK) Received: by mail-lf1-f68.google.com with SMTP id 74so17312005lfo.5 for ; Sat, 21 Nov 2020 04:30:41 -0800 (PST) Date: Sat, 21 Nov 2020 15:30:38 +0300 From: Cyrill Gorcunov Message-ID: <20201121123038.GB935718@grain> References: <20201112195121.191366-1-gorcunov@gmail.com> <20201112195121.191366-5-gorcunov@gmail.com> <08169c31-2e14-dd0e-4e8d-8ac96fde1d2a@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <08169c31-2e14-dd0e-4e8d-8ac96fde1d2a@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 04/11] txn: txn_commit_async -- drop redundant variable List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tml On Sat, Nov 21, 2020 at 12:55:16AM +0100, Vladislav Shpilevoy wrote: > Thanks for the patch! > > NACK. Look, there must be a serious reason why you use temp variable instead of direct access to the variable itself. They could be 1) The variable might be changed externally after you fetched it into another place (which means you might to need a compiler barrier as well but it depends, sometime even compiler help is not enough and you need hw barrier). 2) The variable is immutable but its name is too long and you use temp variable as an alias (which compiler most likely to optimize). If code usage in this case is too big then this become unacceptable and better redesign the code. 3) It simply happened this way. The reason 1) and 2) are pretty usable all over the world. In this particular code we've (3). So I'm asking you "why do you need a temp variable here?" When we've been reviewing the whole qsync series I already told that we better should use direct access to the flags all the time until there is a reson 1) or 2) to cache the value. But we've been under the time pressure then and I simply closed my eyes. Since I'm working on this code right now I hate with a passion such aggressive caching for NO reason, this even makes flags grepping more hard - instead of validating flags access you need to walk two ways: find flag read operation then find the use of a new variable... and I wonder do you *really* thing this is normal practice?