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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.