[Tarantool-patches] [PATCH 0/5] qsync: code cleanup

[Tarantool-patches] [PATCH 0/5] qsync: code cleanup

[Cyrill Gorcunov]

While I'm working on qsync bug trying to figure out how all our
machinery spinning I've made a few cleanups which I think worth
to be posted early.

branch gorcunov/gh-4842-qsync-cleanup

Cyrill Gorcunov (5):
  qsync: eliminate redundant writes
  qsync: add a comment about sync txn in journal allocation
  qsync: txn_commit_async -- drop redundant variable
  qsync: txn_commit -- use txn flag instead of caching variable
  qsync: sanitize txn_limbo_on_rollback

 src/box/txn.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)


base-commit: ff3123a603244dd5f2410973a345e7aa076eecf2
-- 
2.26.2

[Tarantool-patches] [PATCH 1/5] qsync: eliminate redundant writes

[Cyrill Gorcunov]

Instead of updating is_sync variable on every
cycle write it once.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index a2df23833..49b2b2649 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -567,8 +567,12 @@ txn_journal_entry_new(struct txn *txn)
 		if (stmt->row == NULL)
 			continue;
 
-		is_sync = is_sync || (stmt->space != NULL &&
-				      stmt->space->def->opts.is_sync);
+		if (!is_sync) {
+			if (stmt->space != NULL &&
+			    stmt->space->def->opts.is_sync) {
+				is_sync = true;
+			}
+		}
 
 		if (stmt->row->replica_id == 0)
 			*local_row++ = stmt->row;
-- 
2.26.2

[Tarantool-patches] [PATCH 2/5] qsync: add a comment about sync txn in journal allocation

[Cyrill Gorcunov]

Otherwise it is not clear why we should setup a flag here.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/box/txn.c b/src/box/txn.c
index 49b2b2649..4251b2092 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -591,6 +591,13 @@ txn_journal_entry_new(struct txn *txn)
 			txn_set_flag(txn, TXN_WAIT_SYNC);
 			txn_set_flag(txn, TXN_WAIT_ACK);
 		} else if (!txn_limbo_is_empty(&txn_limbo)) {
+			/*
+			 * There some sync entries on the
+			 * fly thus wait for their completion
+			 * even if this particular transaction
+			 * doesn't touch sync space (each sync txn
+			 * should be considered as a barrier).
+			 */
 			txn_set_flag(txn, TXN_WAIT_SYNC);
 		}
 	}
-- 
2.26.2

[Tarantool-patches] [PATCH 3/5] qsync: txn_commit_async -- drop redundant variable

[Cyrill Gorcunov]

Flag test is enough no need for additional variable.
And move limbo_entry declaration into the scope where
it belongs.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index 4251b2092..613eb7aef 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -732,9 +732,8 @@ txn_commit_async(struct txn *txn)
 		return -1;
 	}
 
-	bool is_sync = txn_has_flag(txn, TXN_WAIT_SYNC);
-	struct txn_limbo_entry *limbo_entry;
-	if (is_sync) {
+	if (txn_has_flag(txn, TXN_WAIT_SYNC)) {
+		struct txn_limbo_entry *limbo_entry;
 		/*
 		 * We'll need this trigger for sync transactions later,
 		 * but allocation failure is inappropriate after the entry
-- 
2.26.2

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

[Cyrill Gorcunov]

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)) {
 		/*
 		 * Remote rows, if any, come before local rows, so
 		 * check for originating instance id here.
@@ -834,7 +833,7 @@ txn_commit(struct txn *txn)
 
 	fiber_set_txn(fiber(), NULL);
 	if (journal_write(req) != 0) {
-		if (is_sync)
+		if (txn_has_flag(txn, TXN_WAIT_SYNC))
 			txn_limbo_abort(&txn_limbo, limbo_entry);
 		fiber_set_txn(fiber(), txn);
 		txn_rollback(txn);
@@ -844,7 +843,8 @@ txn_commit(struct txn *txn)
 		diag_log();
 		return -1;
 	}
-	if (is_sync) {
+
+	if (txn_has_flag(txn, TXN_WAIT_SYNC)) {
 		if (txn_has_flag(txn, TXN_WAIT_ACK)) {
 			int64_t lsn = req->rows[req->n_rows - 1]->lsn;
 			txn_limbo_assign_local_lsn(&txn_limbo, limbo_entry,
@@ -857,6 +857,7 @@ txn_commit(struct txn *txn)
 			return -1;
 		}
 	}
+
 	if (!txn_has_flag(txn, TXN_IS_DONE)) {
 		txn->signature = req->res;
 		txn_complete(txn);
-- 
2.26.2

[Tarantool-patches] [PATCH 5/5] qsync: sanitize txn_limbo_on_rollback

[Cyrill Gorcunov]

 - no need to explicit cast from void
 - shrink changing condition
 - drop unneed reference to event

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index feb9a10c6..6cd2a346e 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -691,13 +691,12 @@ txn_commit_nop(struct txn *txn)
 static int
 txn_limbo_on_rollback(struct trigger *trig, void *event)
 {
-	(void) event;
-	struct txn *txn = (struct txn *) event;
+	struct txn *txn = event;
 	/* Check whether limbo has performed the cleanup. */
-	if (txn->signature != TXN_SIGNATURE_ROLLBACK)
-		return 0;
-	struct txn_limbo_entry *entry = (struct txn_limbo_entry *) trig->data;
-	txn_limbo_abort(&txn_limbo, entry);
+	if (txn->signature == TXN_SIGNATURE_ROLLBACK) {
+		struct txn_limbo_entry *e = trig->data;
+		txn_limbo_abort(&txn_limbo, e);
+	}
 	return 0;
 }
 
-- 
2.26.2

Re: [Tarantool-patches] [PATCH 1/5] qsync: eliminate redundant writes

[Vladislav Shpilevoy]

Hi!

On 10/07/2020 09:56, Cyrill Gorcunov wrote:
> Instead of updating is_sync variable on every
> cycle write it once.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/box/txn.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/txn.c b/src/box/txn.c
> index a2df23833..49b2b2649 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -567,8 +567,12 @@ txn_journal_entry_new(struct txn *txn)
>  		if (stmt->row == NULL)
>  			continue;
>  
> -		is_sync = is_sync || (stmt->space != NULL &&
> -				      stmt->space->def->opts.is_sync);
> +		if (!is_sync) {
> +			if (stmt->space != NULL &&
> +			    stmt->space->def->opts.is_sync) {
> +				is_sync = true;
> +			}
> +		}
>  
>  		if (stmt->row->replica_id == 0)
>  			*local_row++ = stmt->row;

The 'redundant' writes were done intentionally, to avoid unnecessary
branching. I see no point in this change.

Re: [Tarantool-patches] [PATCH 2/5] qsync: add a comment about sync txn in journal allocation

[Vladislav Shpilevoy]

On 10/07/2020 09:56, Cyrill Gorcunov wrote:
> Otherwise it is not clear why we should setup a flag here.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/box/txn.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/box/txn.c b/src/box/txn.c
> index 49b2b2649..4251b2092 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -591,6 +591,13 @@ txn_journal_entry_new(struct txn *txn)
>  			txn_set_flag(txn, TXN_WAIT_SYNC);
>  			txn_set_flag(txn, TXN_WAIT_ACK);
>  		} else if (!txn_limbo_is_empty(&txn_limbo)) {
> +			/*
> +			 * There some sync entries on the
> +			 * fly thus wait for their completion
> +			 * even if this particular transaction
> +			 * doesn't touch sync space (each sync txn
> +			 * should be considered as a barrier).
> +			 */
>  			txn_set_flag(txn, TXN_WAIT_SYNC);
>  		}
>  	}

The comment is correct, but it is useless, sorry. You just
narrate what TXN_FORCE_ASYNC means. I don't see a point in
narrative comments. The code is self-explanatory already.

Я понимаю, что ты так в коде разбираешься, меняя его по чуть-чуть,
но плиз, постарайся сократить кол-во таких изменений. Они вообще
ничего не привносят кроме того, что историю затирают, и
вызывают споры вроде этого.

Re: [Tarantool-patches] [PATCH 2/5] qsync: add a comment about sync txn in journal allocation

[Vladislav Shpilevoy]

Не, сори, все ок. Коммент норм. Я уже не соображаю. Но
замечание про свормы мелких изменений все еще справедливо.

Re: [Tarantool-patches] [PATCH 3/5] qsync: txn_commit_async -- drop redundant variable

[Vladislav Shpilevoy]

Опять же. Патч технически корректен, но так же и бесполезен.
Его полезность сравнима с исправлением опечатки в каком-нибудь
не особо нужном комменте. Сори, если токсично звучит.

Но не вижу в этом патче нужды.

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

[Vladislav Shpilevoy]

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

Здесь это сохранено в переменную не просто так. А
как раз за тем, чтобы не вычитывать флаг заново на каждый иф.
Не надо это менять плиз.

>  		/*
>  		 * Remote rows, if any, come before local rows, so
>  		 * check for originating instance id here.
> @@ -834,7 +833,7 @@ txn_commit(struct txn *txn)
>  
>  	fiber_set_txn(fiber(), NULL);
>  	if (journal_write(req) != 0) {
> -		if (is_sync)
> +		if (txn_has_flag(txn, TXN_WAIT_SYNC))
>  			txn_limbo_abort(&txn_limbo, limbo_entry);
>  		fiber_set_txn(fiber(), txn);
>  		txn_rollback(txn);
> @@ -844,7 +843,8 @@ txn_commit(struct txn *txn)
>  		diag_log();
>  		return -1;
>  	}
> -	if (is_sync) {
> +
> +	if (txn_has_flag(txn, TXN_WAIT_SYNC)) {
>  		if (txn_has_flag(txn, TXN_WAIT_ACK)) {
>  			int64_t lsn = req->rows[req->n_rows - 1]->lsn;
>  			txn_limbo_assign_local_lsn(&txn_limbo, limbo_entry,
> @@ -857,6 +857,7 @@ txn_commit(struct txn *txn)
>  			return -1;
>  		}
>  	}
> +
>  	if (!txn_has_flag(txn, TXN_IS_DONE)) {
>  		txn->signature = req->res;
>  		txn_complete(txn);
> 

Re: [Tarantool-patches] [PATCH 5/5] qsync: sanitize txn_limbo_on_rollback

[Vladislav Shpilevoy]

On 10/07/2020 09:56, Cyrill Gorcunov wrote:
>  - no need to explicit cast from void
>  - shrink changing condition
>  - drop unneed reference to event
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/box/txn.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/src/box/txn.c b/src/box/txn.c
> index feb9a10c6..6cd2a346e 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -691,13 +691,12 @@ txn_commit_nop(struct txn *txn)
>  static int
>  txn_limbo_on_rollback(struct trigger *trig, void *event)
>  {
> -	(void) event;
> -	struct txn *txn = (struct txn *) event;
> +	struct txn *txn = event;

У нас так принято в коде, делать касты явными, даже в С и
даже из воида. Хотя мы это уже обсуждали вроде, я почти
уверен.

>  	/* Check whether limbo has performed the cleanup. */
> -	if (txn->signature != TXN_SIGNATURE_ROLLBACK)
> -		return 0;
> -	struct txn_limbo_entry *entry = (struct txn_limbo_entry *) trig->data;
> -	txn_limbo_abort(&txn_limbo, entry);
> +	if (txn->signature == TXN_SIGNATURE_ROLLBACK) {
> +		struct txn_limbo_entry *e = trig->data;
> +		txn_limbo_abort(&txn_limbo, e);
> +	}

Зачем это изменение? Что оно улучшает? Я правда не понимаю. В
этом патчсете все коммиты выглядят как просто пощупывание кода
в процессе его чтения, и как вкусовщина. Давай плиз таких
изменений избегать.

Re: [Tarantool-patches] [PATCH 1/5] qsync: eliminate redundant writes

[Cyrill Gorcunov]

On Fri, Jul 10, 2020 at 10:31:22PM +0200, Vladislav Shpilevoy wrote:
> Hi!
> 
> On 10/07/2020 09:56, Cyrill Gorcunov wrote:
> > Instead of updating is_sync variable on every
> > cycle write it once.
> > 
> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> > ---
> >  src/box/txn.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/box/txn.c b/src/box/txn.c
> > index a2df23833..49b2b2649 100644
> > --- a/src/box/txn.c
> > +++ b/src/box/txn.c
> > @@ -567,8 +567,12 @@ txn_journal_entry_new(struct txn *txn)
> >  		if (stmt->row == NULL)
> >  			continue;
> >  
> > -		is_sync = is_sync || (stmt->space != NULL &&
> > -				      stmt->space->def->opts.is_sync);
> > +		if (!is_sync) {
> > +			if (stmt->space != NULL &&
> > +			    stmt->space->def->opts.is_sync) {
> > +				is_sync = true;
> > +			}
> > +		}
> >  
> >  		if (stmt->row->replica_id == 0)
> >  			*local_row++ = stmt->row;
> 
> The 'redundant' writes were done intentionally, to avoid unnecessary
> branching. I see no point in this change.

Vlad, I'm sorry to say that your assumption about unnecessary branching
is simply wrong. Look, the || operator means that there is a read and
compare on asm level, iow there is _already_ a branch present while it
might be not that obvious on C level.

0000000000002578 <txn_journal_entry_new>:
...
    26ab:       84 c0                   test   %al,%al
    26ad:       74 07                   je     26b6 <txn_journal_entry_new+0x13e>
    26af:       b8 01 00 00 00          mov    $0x1,%eax
    26b4:       eb 05                   jmp    26bb <txn_journal_entry_new+0x143>
    26b6:       b8 00 00 00 00          mov    $0x0,%eax
    26bb:       88 45 e7                mov    %al,-0x19(%rbp)
    26be:       80 65 e7 01             andb   $0x1,-0x19(%rbp)

see this test+je pair? IOW, in my version we eliminate redundant write operation.
And just for the future -- branching is cheaper than write operation on hot paths.

Re: [Tarantool-patches] [PATCH 2/5] qsync: add a comment about sync txn in journal allocation

[Cyrill Gorcunov]

On Fri, Jul 10, 2020 at 10:34:27PM +0200, Vladislav Shpilevoy wrote:
> Не, сори, все ок. Коммент норм. Я уже не соображаю. Но
> замечание про свормы мелких изменений все еще справедливо.

The main reason for small changes is for easier merging, iirc you
were fixing some code and I didn't want to tie-up with the changes,
making potential merge conflict easier to resolve.

Re: [Tarantool-patches] [PATCH 2/5] qsync: add a comment about sync txn in journal allocation

[Vladislav Shpilevoy]

On 10/07/2020 23:07, Cyrill Gorcunov wrote:
> On Fri, Jul 10, 2020 at 10:34:27PM +0200, Vladislav Shpilevoy wrote:
>> Не, сори, все ок. Коммент норм. Я уже не соображаю. Но
>> замечание про свормы мелких изменений все еще справедливо.
> 
> The main reason for small changes is for easier merging, iirc you
> were fixing some code and I didn't want to tie-up with the changes,
> making potential merge conflict easier to resolve.

You are missing the point. The problem is not in the commit size. It
is in it being unnecessary.

Re: [Tarantool-patches] [PATCH 3/5] qsync: txn_commit_async -- drop redundant variable

[Cyrill Gorcunov]

On Fri, Jul 10, 2020 at 10:35:50PM +0200, Vladislav Shpilevoy wrote:
> Опять же. Патч технически корректен, но так же и бесполезен.
> Его полезность сравнима с исправлением опечатки в каком-нибудь
> не особо нужном комменте. Сори, если токсично звучит.
> 
> Но не вижу в этом патче нужды.

We've to allocate a variable which we simply don't need. Look,
the former code reads the flag, puts it into variable then
immediately read it and that's all :/ I think we should not
spread @is_sync but read the flag as much as possible for
better grep'ability.

Of course there are places where read-once variable is preferred
but this is not our case.

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

[Cyrill Gorcunov]

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.

Re: [Tarantool-patches] [PATCH 3/5] qsync: txn_commit_async -- drop redundant variable

[Vladislav Shpilevoy]

On 10/07/2020 23:10, Cyrill Gorcunov wrote:
> On Fri, Jul 10, 2020 at 10:35:50PM +0200, Vladislav Shpilevoy wrote:
>> Опять же. Патч технически корректен, но так же и бесполезен.
>> Его полезность сравнима с исправлением опечатки в каком-нибудь
>> не особо нужном комменте. Сори, если токсично звучит.
>>
>> Но не вижу в этом патче нужды.
> 
> We've to allocate a variable which we simply don't need. Look,
> the former code reads the flag, puts it into variable then
> immediately read it and that's all :/ I think we should not
> spread @is_sync but read the flag as much as possible for
> better grep'ability.

Do you have a proof that this change improves anything? That
the variable 'allocation' on the stack actually happens, and
costs even 1 ns?

Re: [Tarantool-patches] [PATCH 3/5] qsync: txn_commit_async -- drop redundant variable

[Cyrill Gorcunov]

On Fri, Jul 10, 2020 at 11:28:26PM +0200, Vladislav Shpilevoy wrote:
> On 10/07/2020 23:10, Cyrill Gorcunov wrote:
> > On Fri, Jul 10, 2020 at 10:35:50PM +0200, Vladislav Shpilevoy wrote:
> >> Опять же. Патч технически корректен, но так же и бесполезен.
> >> Его полезность сравнима с исправлением опечатки в каком-нибудь
> >> не особо нужном комменте. Сори, если токсично звучит.
> >>
> >> Но не вижу в этом патче нужды.
> > 
> > We've to allocate a variable which we simply don't need. Look,
> > the former code reads the flag, puts it into variable then
> > immediately read it and that's all :/ I think we should not
> > spread @is_sync but read the flag as much as possible for
> > better grep'ability.
> 
> Do you have a proof that this change improves anything? That
> the variable 'allocation' on the stack actually happens, and
> costs even 1 ns?

You simply don't need it. I think the compiler will rip it off.
Again, if you suspect that the code gonna be changed then ignoring
the commit is perfectly fine. If not -- the useless variable is just
a bad taste at minimum.

Re: [Tarantool-patches] [PATCH 3/5] qsync: txn_commit_async -- drop redundant variable

[Vladislav Shpilevoy]

On 10/07/2020 23:36, Cyrill Gorcunov wrote:
> On Fri, Jul 10, 2020 at 11:28:26PM +0200, Vladislav Shpilevoy wrote:
>> On 10/07/2020 23:10, Cyrill Gorcunov wrote:
>>> On Fri, Jul 10, 2020 at 10:35:50PM +0200, Vladislav Shpilevoy wrote:
>>>> Опять же. Патч технически корректен, но так же и бесполезен.
>>>> Его полезность сравнима с исправлением опечатки в каком-нибудь
>>>> не особо нужном комменте. Сори, если токсично звучит.
>>>>
>>>> Но не вижу в этом патче нужды.
>>>
>>> We've to allocate a variable which we simply don't need. Look,
>>> the former code reads the flag, puts it into variable then
>>> immediately read it and that's all :/ I think we should not
>>> spread @is_sync but read the flag as much as possible for
>>> better grep'ability.
>>
>> Do you have a proof that this change improves anything? That
>> the variable 'allocation' on the stack actually happens, and
>> costs even 1 ns?
> 
> You simply don't need it. I think the compiler will rip it off.
> Again, if you suspect that the code gonna be changed then ignoring
> the commit is perfectly fine. If not -- the useless variable is just
> a bad taste at minimum.

So we came to a conclusion that the patch has nothing to do with
perf. It is just subjective feeling of taste. Sorry, but I am not
buying it. If everyone in the team would rewrite every place in
the source code they consider 'bad taste', we would end up
rewriting each other's code 100% of work time.

Btw, talking of bad taste of mine - you yourself declare all
variables in the beginning like in C89. Isn't it bad taste? - I
don't know. Or is it a bad taste to do the same one-line change
in two neighbor functions in 2 separate commits like removal of

    (void) txn_limbo;

? This easily could be one commit, easier to review, and less commits
to skip in git log, when look for something. I don't know whether it
is a bad taste, because I don't have definition of bad taste. And you
either.

Even if I feel I would do something a bit different, it is certainly
not a reason for me now to go and "fix" all such places. Just because
I consider them not beautiful enough. There is a sanity border after
which refactoring becomes useless or even harmful when it comes to git
history and waste of time on that.

Re: [Tarantool-patches] [PATCH 3/5] qsync: txn_commit_async -- drop redundant variable

[Cyrill Gorcunov]

On Sat, Jul 11, 2020 at 04:10:34PM +0200, Vladislav Shpilevoy wrote:
> On 10/07/2020 23:36, Cyrill Gorcunov wrote:
> > On Fri, Jul 10, 2020 at 11:28:26PM +0200, Vladislav Shpilevoy wrote:
> >> On 10/07/2020 23:10, Cyrill Gorcunov wrote:
> >>> On Fri, Jul 10, 2020 at 10:35:50PM +0200, Vladislav Shpilevoy wrote:
> >>>> Опять же. Патч технически корректен, но так же и бесполезен.
> >>>> Его полезность сравнима с исправлением опечатки в каком-нибудь
> >>>> не особо нужном комменте. Сори, если токсично звучит.
> >>>>
> >>>> Но не вижу в этом патче нужды.
> >>>
> >>> We've to allocate a variable which we simply don't need. Look,
> >>> the former code reads the flag, puts it into variable then
> >>> immediately read it and that's all :/ I think we should not
> >>> spread @is_sync but read the flag as much as possible for
> >>> better grep'ability.
> >>
> >> Do you have a proof that this change improves anything? That
> >> the variable 'allocation' on the stack actually happens, and
> >> costs even 1 ns?
> > 
> > You simply don't need it. I think the compiler will rip it off.
> > Again, if you suspect that the code gonna be changed then ignoring
> > the commit is perfectly fine. If not -- the useless variable is just
> > a bad taste at minimum.
> 
> So we came to a conclusion that the patch has nothing to do with
> perf. It is just subjective feeling of taste. Sorry, but I am not

This _have_ impact on debug builds at least. Look, the problem is
that here is what we've done

	a = b
	if (a)
		...

iow we've allocated a variable, use it immediately and that's all. We
don't even have one more usage. Why do we acllocate it at all?
Why we need this variable? Why can't we read @b directly but use
a temporary variable instead? Maybe the @b is being modifying externally
so we need a local immutable copy?

> buying it. If everyone in the team would rewrite every place in
> the source code they consider 'bad taste', we would end up
> rewriting each other's code 100% of work time.
> 
> Btw, talking of bad taste of mine - you yourself declare all

I didn't said that you have a bad taste, I pointed that if we leave
it in this form forever it gonna be a bad taste simply because every
line in source code must have a meaning, but with this redundant
variable there is no any meaning simply because "it just happen this way".
This is perfectly fine for initial commits where we're consider various
design for features and rolling code to and from.

I don't get why you take it so personal. I didn't mean to offence.

> variables in the beginning like in C89. Isn't it bad taste? - I

This has a reason -- not always compiler warns about shadow variables.
But if you look into my commits you'll see that this is not mandatory
for me either, I use both.

> don't know. Or is it a bad taste to do the same one-line change
> in two neighbor functions in 2 separate commits like removal of
> 
>     (void) txn_limbo;
> 
> ? This easily could be one commit, easier to review, and less commits
> to skip in git log, when look for something. I don't know whether it

And this has a reason as well -- I wanted to make changes small because
you've been modifying source code and when you need to apply patches
you could easily resolve conflicts. Moreover you could merge the small
patches then.

> is a bad taste, because I don't have definition of bad taste. And you
> either.
> 
> Even if I feel I would do something a bit different, it is certainly
> not a reason for me now to go and "fix" all such places. Just because
> I consider them not beautiful enough. There is a sanity border after
> which refactoring becomes useless or even harmful when it comes to git
> history and waste of time on that.

Re: [Tarantool-patches] [PATCH 5/5] qsync: sanitize txn_limbo_on_rollback

[Cyrill Gorcunov]

On Fri, Jul 10, 2020 at 10:38:39PM +0200, Vladislav Shpilevoy wrote:
> > @@ -691,13 +691,12 @@ txn_commit_nop(struct txn *txn)
> >  static int
> >  txn_limbo_on_rollback(struct trigger *trig, void *event)
> >  {
> > -	(void) event;
> > -	struct txn *txn = (struct txn *) event;
> > +	struct txn *txn = event;
> 
> У нас так принято в коде, делать касты явными, даже в С и
> даже из воида. Хотя мы это уже обсуждали вроде, я почти
> уверен.

Не, не было такого требования. Если у нас так принято, то надо
прописать в SOB (и добавить такие вещи везде, только тогда еще
придется и все вызовы malloc/calloc проверить). "C" стандарт
гарантирует приведение void* к другому типу (в отличие от C++).
Наверное отсюда и пошло, что в коде мешанина: где-то есть
приведение, где-то нет. Поскольку я меняю код этой функции заодно
убрал и этот момент. Мне не принципиально, если тебе нравится
явное приведение типов, то без проблем.

> >  	/* Check whether limbo has performed the cleanup. */
> > -	if (txn->signature != TXN_SIGNATURE_ROLLBACK)
> > -		return 0;
> > -	struct txn_limbo_entry *entry = (struct txn_limbo_entry *) trig->data;
> > -	txn_limbo_abort(&txn_limbo, entry);
> > +	if (txn->signature == TXN_SIGNATURE_ROLLBACK) {
> > +		struct txn_limbo_entry *e = trig->data;
> > +		txn_limbo_abort(&txn_limbo, e);
> > +	}
> 
> Зачем это изменение? Что оно улучшает? Я правда не понимаю. В
> этом патчсете все коммиты выглядят как просто пощупывание кода
> в процессе его чтения, и как вкусовщина. Давай плиз таких
> изменений избегать.

Убирает лишний return. До этого их было два. Я думаю ты просто не
заметил, что удаляется первый return (и кода поменьше становится
как на уровне сорцов, так и на уровне бинарника).

Re: [Tarantool-patches] [PATCH 2/5] qsync: add a comment about sync txn in journal allocation

[Vladislav Shpilevoy]

This commit is pushed to master.