Re: [Tarantool-patches] [PATCH 4/5] qsync: txn_commit -- use txn flag instead of caching variable

[Cyrill Gorcunov <gorcunov@gmail.com>]

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 <gorcunov@gmail.com>
> > ---
> >  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.