Re: [Tarantool-patches] [PATCH 04/11] txn: txn_commit_async -- drop redundant variable

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Sat, Nov 21, 2020 at 02:29:39PM +0100, Vladislav Shpilevoy wrote:
> 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.

Code readability hits reason (2), ie you make code shorter providing an
alias. Which is prefectly fine. But when someone reads this code first
thing pops up is "why the variable is reassigned?" Maybe we need a local
copy because someone can change it externally while we're operating with
this variable? Looks like nope. Ok, then it must be making code shorter?
No it doesn't either. What is the reason to make an alias, use it once
immediately and that's all :/

> 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 must be kidding... git history is not a holy cow and never been.
We work with "the code" itself not a git history. This qsync code is
a new one, you don't even have to backport it to 1.x series if there
are some cleanups on top.

> - 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;

I gave you 2 reasons where vars aliasing is acceptable and needed.
If you really think that "is_sync" is a better name you should consider
just name a bitfield in txn itself instead of declaring new aliases
left and right. This gonna be shorter and won't force a code reader
to scratch the head for the usage context.

> 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.

You know, we've a different view on the code architecture, this is
understandable. Lets drop this series I can live even with such
code. I continue commenting the message below just in a hope I
could explain you why I did this cleanup but surely won't do
such things in future.

> > 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,

For debug build it makes impossible to track flag read/write
modifications via watching the memory variable on hw level.
And I don't like to disappoint you but _every_ new variable
hurts perf. This is how computers work. Sometime compiler
optimize the usage on release build, sometime not.

The whole patch was about to not procreate new entities where
not needed and keep the code as simple as possible. Even if
it would hurt perf the maintainability reason is a way more
important.

> 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?

The txn_limbo_change_owner is special, it moved outside exactly because
changing owner is separate and important state of limbo existance, no?

We need the queue to be empty and I think we will need more tests there
in future because it is very sensitive. Thus even if we have to spend
some time for additional call it should be acceptable in a sake for
further modifications.

If you remember I've been asking you about limbo state in f2f meeting,
and I think it deserves explicit handling even with some additional
cycles.

> 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.

Please don't mixture plain new variables and aliases.

> > 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 :)

Please read the changelog: we use it once, immediately, no need
for alias. This removes one line of code.

> 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.

I see your position and won't insist.

> > 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.

You're welcome. This was not only one place. The patch I've been
proposing to drop all aliases for flag access. You said somthing
like "it is a cache for flag to not reread it". And I gave up
because we've been in time pressue.

> > 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

Again, Vlad, it is perfectly fine to have an alias for long
names, here this is made just for nothing. But you already
said "I'm author and I'm to deside", ok, accepted.

> 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.

U+1F926