From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 1F026469710 for ; Sat, 21 Nov 2020 16:29:41 +0300 (MSK) References: <20201112195121.191366-1-gorcunov@gmail.com> <20201112195121.191366-5-gorcunov@gmail.com> <08169c31-2e14-dd0e-4e8d-8ac96fde1d2a@tarantool.org> <20201121123038.GB935718@grain> From: Vladislav Shpilevoy Message-ID: <2210cf80-4129-3873-5a07-c85c313d723d@tarantool.org> Date: Sat, 21 Nov 2020 14:29:39 +0100 MIME-Version: 1.0 In-Reply-To: <20201121123038.GB935718@grain> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Cyrill Gorcunov Cc: tml Ok, lets continue arguing. On 21.11.2020 13:30, Cyrill Gorcunov wrote: > 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. Code readability. I consider having it in a variable often more readable. You don't consider. My word vs yours. Subjective. But there are real things - by doing such changes you - turn the git history into garbage by doing non-functional patches not changing a single bit of perf or objective readability; - you waste time on doing this, arguing about this, and time of other people on reviewing this. You have real features to implement and real bugs to fix. But instead you go around in circles changing already working code; Looks like I told it already. And after I said I won't explain it again, I did it anyway. I feel stupid about this :) But also I said I won't commit anything until we all agree on the entire patchset outcome, so here we go. > 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). Are you saying this variable existence hurts perf? Then firstly I ask you to prove that. Show me the bench, on which it matters, down to 0.0001% of perf improvement. The bench should use Tarantool and exactly this code. It should not be some kind of example.c you will create specifically for that with volatiles or stuff. Although if you can, I could take a look too, not related to Tarantool. Sounds interesting if it is possible. Also, since we are here, in the area of crazy blasting real-time perf, where a single variable introduction matters, lets also talk about your commit 7/11, where you introduced function txn_limbo_change_owner. Don't you think we will loose some parts of a nanosecond on doing a 'call' instruction for this helper? Also the function is a compiler barrier if the compiler somewhy can't inline it. Don't you think it may prevent the compiler from doing some reorderings for optimizations? Also we waste 'huge' time on passing arguments to the function, pushing them to the stack. Besides, in case the function is not inlined, it will be stored as a symbol in the executable file, with its name in it. If we build with debug info (RelWithDebInfo or Debug). It wastes some bytes to store the function name. I propose to inline all functions and remove all variables which are used one time. To gain the cosmic perf you are talking about. You can create a ticket, assign it to self, and start immediately. > 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?" I am asking you why do you need to change it :) Because you feel so? It is not a reason. Grepping? The variable usage is 2 lines below. Doing such 'optimizations' is fine while a patch is in progress, if the author does not mind. But changing it in the existing code I don't consider acceptable. > 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. And I told you it is subjective. Since there are no real reasons why I can't do that, it is up to me. As well it is up to you not to use variables in your patches until it starts hurting readability or perf. > But we've been under the time pressure then and I simply closed > my eyes. You closed your eyes on me using a variable to save a function result? Oh, thanks a lot. > 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... It is used literally 2 lines below. If this so hard for you to locate, I don't know how to help you. But I am not ready to sacrifice the things I listed above for that - git history and time. The second one is already lost here, unfortunately, but I will fight for the first one. > and I wonder do you > *really* thing this is normal practice? It is not a practice. It is not a code style rule. It is just a fucking variable. The code looks exactly the same before and after your change. It can be even that it was used 2 times earlier, but its other usage was deleted or moved, and the rest was kept intact so as not to do unnecessary changes.