[Tarantool-patches] [PATCH 0/6] txn: drop txn_X_flag helpers

[Tarantool-patches] [PATCH 0/6] txn: drop txn_X_flag helpers

[Cyrill Gorcunov via Tarantool-patches]

When operating with bitfields it is a way better
to use opencoded operations instead of some helpers
which allow to modify one bit at once.

In the series we drop usage of txn_X_flag(), this
shrinks code a bit and make possible to test a set
of bits at once.

issue https://github.com/tarantool/tarantool/issues/5128
branch gorcunov/gh-5128-txn-flags

Cyrill Gorcunov (6):
  txn: convert flags to explicit bitfield
  txn: stop using txn_set_flag
  test/unit: snap_quorum_delay -- stop using txn_set_flag
  txn: stop using txn_clear_flag
  txn: stop using txn_has_flag
  txn: drop unused txn_x_flag helpers

 src/box/applier.cc             |  4 +--
 src/box/memtx_engine.c         |  2 +-
 src/box/txn.c                  | 54 ++++++++++++++++------------------
 src/box/txn.h                  | 36 ++++++-----------------
 src/box/txn_limbo.c            | 35 ++++++++++------------
 test/unit/snap_quorum_delay.cc |  3 +-
 6 files changed, 55 insertions(+), 79 deletions(-)


base-commit: a6fd00ab73705c3afb03b2828526c7b1dfceda08
-- 
2.29.2

[Tarantool-patches] [PATCH 1/6] txn: convert flags to explicit bitfield

[Cyrill Gorcunov via Tarantool-patches]

Instead of shifting flags inside txn_x_flag() helpers
lets define precompiled value. Moreover we're about
to drop this helpers completely: better operate with
bitfield directly which allows to test/set/clear a set
of bits at once instead of one by one.

Part-of #5128

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

diff --git a/src/box/txn.h b/src/box/txn.h
index fca9bc1d0..b42249b17 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -56,25 +56,25 @@ struct Vdbe;
 
 enum txn_flag {
 	/** Transaction has been processed. */
-	TXN_IS_DONE,
+	TXN_IS_DONE = 0x1,
 	/**
 	 * Transaction has been aborted by fiber yield so
 	 * should be rolled back at commit.
 	 */
-	TXN_IS_ABORTED_BY_YIELD,
+	TXN_IS_ABORTED_BY_YIELD = 0x2,
 	/**
 	 * fiber_yield() is allowed inside the transaction.
 	 * See txn_can_yield() for more details.
 	 */
-	TXN_CAN_YIELD,
+	TXN_CAN_YIELD = 0x4,
 	/** on_commit and/or on_rollback list is not empty. */
-	TXN_HAS_TRIGGERS,
+	TXN_HAS_TRIGGERS = 0x8,
 	/**
 	 * Synchronous transaction touched sync spaces, or an
 	 * asynchronous transaction blocked by a sync one until it
 	 * is confirmed.
 	 */
-	TXN_WAIT_SYNC,
+	TXN_WAIT_SYNC = 0x10,
 	/**
 	 * Synchronous transaction 'waiting for ACKs' state before
 	 * commit. In this state it waits until it is replicated
@@ -82,14 +82,14 @@ enum txn_flag {
 	 * commit and returns success to a user.
 	 * TXN_WAIT_SYNC is always set, if TXN_WAIT_ACK is set.
 	 */
-	TXN_WAIT_ACK,
+	TXN_WAIT_ACK = 0x20,
 	/**
 	 * A transaction may be forced to be asynchronous, not
 	 * wait for any ACKs, and not depend on prepending sync
 	 * transactions. This happens in a few special cases. For
 	 * example, when applier receives snapshot from master.
 	 */
-	TXN_FORCE_ASYNC,
+	TXN_FORCE_ASYNC = 0x40,
 };
 
 enum {
@@ -394,21 +394,21 @@ struct txn {
 };
 
 static inline bool
-txn_has_flag(struct txn *txn, enum txn_flag flag)
+txn_has_flag(struct txn *txn, unsigned int flag)
 {
-	return (txn->flags & (1 << flag)) != 0;
+	return (txn->flags & flag) != 0;
 }
 
 static inline void
-txn_set_flag(struct txn *txn, enum txn_flag flag)
+txn_set_flag(struct txn *txn, unsigned int flag)
 {
-	txn->flags |= 1 << flag;
+	txn->flags |= flag;
 }
 
 static inline void
-txn_clear_flag(struct txn *txn, enum txn_flag flag)
+txn_clear_flag(struct txn *txn, unsigned int flag)
 {
-	txn->flags &= ~(1 << flag);
+	txn->flags &= ~flag;
 }
 
 /* Pointer to the current transaction (if any) */
-- 
2.29.2

[Tarantool-patches] [PATCH 2/6] txn: stop using txn_set_flag

[Cyrill Gorcunov via Tarantool-patches]

Part-of #5128

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/applier.cc     |  4 ++--
 src/box/memtx_engine.c |  2 +-
 src/box/txn.c          | 13 ++++++-------
 src/box/txn.h          |  2 +-
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/src/box/applier.cc b/src/box/applier.cc
index 553db76fc..c7c85d329 100644
--- a/src/box/applier.cc
+++ b/src/box/applier.cc
@@ -237,7 +237,7 @@ apply_snapshot_row(struct xrow_header *row)
 	 * Do not wait for confirmation when fetching a snapshot.
 	 * Master only sends confirmed rows during join.
 	 */
-	txn_set_flag(txn, TXN_FORCE_ASYNC);
+	txn->flags |= TXN_FORCE_ASYNC;
 	if (txn_begin_stmt(txn, space) != 0)
 		goto rollback;
 	/* no access checks here - applier always works with admin privs */
@@ -308,7 +308,7 @@ apply_final_join_row(struct xrow_header *row)
 	 * Do not wait for confirmation while processing final
 	 * join rows. See apply_snapshot_row().
 	 */
-	txn_set_flag(txn, TXN_FORCE_ASYNC);
+	txn->flags |= TXN_FORCE_ASYNC;
 	if (apply_row(row) != 0) {
 		txn_rollback(txn);
 		fiber_gc();
diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index f79f14b4f..f0df3a07d 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -265,7 +265,7 @@ memtx_engine_recover_snapshot_row(struct memtx_engine *memtx,
 	 * Snapshot rows are confirmed by definition. They don't need to go to
 	 * the synchronous transactions limbo.
 	 */
-	txn_set_flag(txn, TXN_FORCE_ASYNC);
+	txn->flags |= TXN_FORCE_ASYNC;
 	rc = txn_commit(txn);
 	/*
 	 * Don't let gc pool grow too much. Yet to
diff --git a/src/box/txn.c b/src/box/txn.c
index a5edbfc60..32eca02c5 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -282,7 +282,7 @@ txn_begin(void)
 	 * It's a responsibility of an engine to disable yields
 	 * if they are not supported.
 	 */
-	txn_set_flag(txn, TXN_CAN_YIELD);
+	txn->flags |= TXN_CAN_YIELD;
 	return txn;
 }
 
@@ -516,7 +516,7 @@ txn_free_or_wakeup(struct txn *txn)
 	if (txn->fiber == NULL)
 		txn_free(txn);
 	else {
-		txn_set_flag(txn, TXN_IS_DONE);
+		txn->flags |= TXN_IS_DONE;
 		if (txn->fiber != fiber())
 			/* Wake a waiting fiber up. */
 			fiber_wakeup(txn->fiber);
@@ -642,8 +642,7 @@ txn_journal_entry_new(struct txn *txn)
 	 */
 	if (!txn_has_flag(txn, TXN_FORCE_ASYNC)) {
 		if (is_sync) {
-			txn_set_flag(txn, TXN_WAIT_SYNC);
-			txn_set_flag(txn, TXN_WAIT_ACK);
+			txn->flags |= TXN_WAIT_SYNC | TXN_WAIT_ACK;
 		} else if (!txn_limbo_is_empty(&txn_limbo)) {
 			/*
 			 * There some sync entries on the
@@ -652,7 +651,7 @@ txn_journal_entry_new(struct txn *txn)
 			 * doesn't touch sync space (each sync txn
 			 * should be considered as a barrier).
 			 */
-			txn_set_flag(txn, TXN_WAIT_SYNC);
+			txn->flags |= TXN_WAIT_SYNC;
 		}
 	}
 
@@ -994,7 +993,7 @@ txn_can_yield(struct txn *txn, bool set)
 	assert(txn == in_txn());
 	bool could = txn_has_flag(txn, TXN_CAN_YIELD);
 	if (set && !could) {
-		txn_set_flag(txn, TXN_CAN_YIELD);
+		txn->flags |= TXN_CAN_YIELD;
 		trigger_clear(&txn->fiber_on_yield);
 	} else if (!set && could) {
 		txn_clear_flag(txn, TXN_CAN_YIELD);
@@ -1229,6 +1228,6 @@ txn_on_yield(struct trigger *trigger, void *event)
 	assert(txn != NULL);
 	assert(!txn_has_flag(txn, TXN_CAN_YIELD));
 	txn_rollback_to_svp(txn, NULL);
-	txn_set_flag(txn, TXN_IS_ABORTED_BY_YIELD);
+	txn->flags |= TXN_IS_ABORTED_BY_YIELD;
 	return 0;
 }
diff --git a/src/box/txn.h b/src/box/txn.h
index b42249b17..201564000 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -491,7 +491,7 @@ txn_init_triggers(struct txn *txn)
 		rlist_create(&txn->on_commit);
 		rlist_create(&txn->on_rollback);
 		rlist_create(&txn->on_wal_write);
-		txn_set_flag(txn, TXN_HAS_TRIGGERS);
+		txn->flags |= TXN_HAS_TRIGGERS;
 	}
 }
 
-- 
2.29.2

[Tarantool-patches] [PATCH 3/6] test/unit: snap_quorum_delay -- stop using txn_set_flag

[Cyrill Gorcunov via Tarantool-patches]

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 test/unit/snap_quorum_delay.cc | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/test/unit/snap_quorum_delay.cc b/test/unit/snap_quorum_delay.cc
index b9d4cc6c4..ec78f837d 100644
--- a/test/unit/snap_quorum_delay.cc
+++ b/test/unit/snap_quorum_delay.cc
@@ -98,8 +98,7 @@ txn_process_func(va_list ap)
 	struct txn *txn = txn_begin();
 	txn->fiber = fiber();
 	/* Simulate a sync transaction. */
-	txn_set_flag(txn, TXN_WAIT_SYNC);
-	txn_set_flag(txn, TXN_WAIT_ACK);
+	txn->flags |= TXN_WAIT_SYNC | TXN_WAIT_ACK;
 	/*
 	 * The true way to push the transaction to limbo is to call
 	 * txn_commit() for sync transaction. But, if txn_commit()
-- 
2.29.2

Re: [Tarantool-patches] [PATCH 3/6] test/unit: snap_quorum_delay -- stop using txn_set_flag

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the patch!

Ok, I assume it is fine to drop these functions one by one, even
though it leaves the flags usage inconsistent between the commits.

But why the hell did you split even one function clearance into 2
commits? Why couldn't this commit be a part of the previous commit?

Any why couldn't you delete the unused txn_..._flag() helpers along
with their usage?

On 22.01.2021 14:26, Cyrill Gorcunov via Tarantool-patches wrote:
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  test/unit/snap_quorum_delay.cc | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/test/unit/snap_quorum_delay.cc b/test/unit/snap_quorum_delay.cc
> index b9d4cc6c4..ec78f837d 100644
> --- a/test/unit/snap_quorum_delay.cc
> +++ b/test/unit/snap_quorum_delay.cc
> @@ -98,8 +98,7 @@ txn_process_func(va_list ap)
>  	struct txn *txn = txn_begin();
>  	txn->fiber = fiber();
>  	/* Simulate a sync transaction. */
> -	txn_set_flag(txn, TXN_WAIT_SYNC);
> -	txn_set_flag(txn, TXN_WAIT_ACK);
> +	txn->flags |= TXN_WAIT_SYNC | TXN_WAIT_ACK;
>  	/*
>  	 * The true way to push the transaction to limbo is to call
>  	 * txn_commit() for sync transaction. But, if txn_commit()
> 

Re: [Tarantool-patches] [PATCH 3/6] test/unit: snap_quorum_delay -- stop using txn_set_flag

[Cyrill Gorcunov via Tarantool-patches]

On Sat, Jan 30, 2021 at 08:17:57PM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> Ok, I assume it is fine to drop these functions one by one, even
> though it leaves the flags usage inconsistent between the commits.

The flags usage remains consistent in terms of operations.

> 
> But why the hell did you split even one function clearance into 2
> commits? Why couldn't this commit be a part of the previous commit?

Because I don't know down to which branch the patches gonna be merged.
IOW, txn flags are appeared a way earlier than syncrho tests. Thus
one can simply skip this patch when backporting the commits.

> 
> Any why couldn't you delete the unused txn_..._flag() helpers along
> with their usage?

This is better done in one commit when everything is ready. I prefer
this way, though there is no strict rule and if you promote immediate
function zapping then I can update the series.

[Tarantool-patches] [PATCH 4/6] txn: stop using txn_clear_flag

[Cyrill Gorcunov via Tarantool-patches]

Part-of #5128

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn.c       | 2 +-
 src/box/txn_limbo.c | 9 +++------
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index 32eca02c5..6197cf012 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -996,7 +996,7 @@ txn_can_yield(struct txn *txn, bool set)
 		txn->flags |= TXN_CAN_YIELD;
 		trigger_clear(&txn->fiber_on_yield);
 	} else if (!set && could) {
-		txn_clear_flag(txn, TXN_CAN_YIELD);
+		txn->flags &= ~TXN_CAN_YIELD;
 		trigger_create(&txn->fiber_on_yield, txn_on_yield, NULL, NULL);
 		trigger_add(&fiber()->on_yield, &txn->fiber_on_yield);
 	}
diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index 9c4c3cdf1..d418530ad 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -266,8 +266,7 @@ txn_limbo_wait_complete(struct txn_limbo *limbo, struct txn_limbo_entry *entry)
 					 in_queue, tmp) {
 		e->txn->signature = TXN_SIGNATURE_QUORUM_TIMEOUT;
 		txn_limbo_abort(limbo, e);
-		txn_clear_flag(e->txn, TXN_WAIT_SYNC);
-		txn_clear_flag(e->txn, TXN_WAIT_ACK);
+		e->txn->flags &= ~(TXN_WAIT_SYNC | TXN_WAIT_ACK);
 		txn_complete_fail(e->txn);
 		if (e == entry)
 			break;
@@ -399,8 +398,7 @@ txn_limbo_read_confirm(struct txn_limbo *limbo, int64_t lsn)
 		}
 		e->is_commit = true;
 		txn_limbo_remove(limbo, e);
-		txn_clear_flag(e->txn, TXN_WAIT_SYNC);
-		txn_clear_flag(e->txn, TXN_WAIT_ACK);
+		e->txn->flags &= ~(TXN_WAIT_SYNC | TXN_WAIT_ACK);
 		/*
 		 * If already written to WAL by now, finish tx processing.
 		 * Otherwise just clear the sync flags. Tx procesing will finish
@@ -456,8 +454,7 @@ txn_limbo_read_rollback(struct txn_limbo *limbo, int64_t lsn)
 		return;
 	rlist_foreach_entry_safe_reverse(e, &limbo->queue, in_queue, tmp) {
 		txn_limbo_abort(limbo, e);
-		txn_clear_flag(e->txn, TXN_WAIT_SYNC);
-		txn_clear_flag(e->txn, TXN_WAIT_ACK);
+		e->txn->flags &= ~(TXN_WAIT_SYNC | TXN_WAIT_ACK);
 		if (e->txn->signature >= 0) {
 			/* Rollback the transaction. */
 			e->txn->signature = TXN_SIGNATURE_SYNC_ROLLBACK;
-- 
2.29.2

[Tarantool-patches] [PATCH 5/6] txn: stop using txn_has_flag

[Cyrill Gorcunov via Tarantool-patches]

Part-of #5128

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn.c       | 39 +++++++++++++++++++--------------------
 src/box/txn.h       |  2 +-
 src/box/txn_limbo.c | 26 +++++++++++++-------------
 3 files changed, 33 insertions(+), 34 deletions(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index 6197cf012..d1964b5f7 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -526,7 +526,7 @@ txn_free_or_wakeup(struct txn *txn)
 void
 txn_complete_fail(struct txn *txn)
 {
-	assert(!txn_has_flag(txn, TXN_IS_DONE));
+	assert(!(txn->flags & TXN_IS_DONE));
 	assert(txn->signature < 0);
 	txn->status = TXN_ABORTED;
 	struct txn_stmt *stmt;
@@ -535,7 +535,7 @@ txn_complete_fail(struct txn *txn)
 		txn_rollback_one_stmt(txn, stmt);
 	if (txn->engine != NULL)
 		engine_rollback(txn->engine, txn);
-	if (txn_has_flag(txn, TXN_HAS_TRIGGERS))
+	if (txn->flags & TXN_HAS_TRIGGERS)
 		txn_run_rollback_triggers(txn, &txn->on_rollback);
 	txn_free_or_wakeup(txn);
 }
@@ -543,13 +543,12 @@ txn_complete_fail(struct txn *txn)
 void
 txn_complete_success(struct txn *txn)
 {
-	assert(!txn_has_flag(txn, TXN_IS_DONE));
-	assert(!txn_has_flag(txn, TXN_WAIT_SYNC));
+	assert(!(txn->flags & (TXN_IS_DONE | TXN_WAIT_SYNC)));
 	assert(txn->signature >= 0);
 	txn->status = TXN_COMMITTED;
 	if (txn->engine != NULL)
 		engine_commit(txn->engine, txn);
-	if (txn_has_flag(txn, TXN_HAS_TRIGGERS))
+	if (txn->flags & TXN_HAS_TRIGGERS)
 		txn_run_commit_triggers(txn, &txn->on_commit);
 	txn_free_or_wakeup(txn);
 }
@@ -587,9 +586,9 @@ txn_on_journal_write(struct journal_entry *entry)
 				     "%.3f sec", n_rows,
 				     txn->signature - n_rows + 1, delta);
 	}
-	if (txn_has_flag(txn, TXN_HAS_TRIGGERS))
+	if (txn->flags & TXN_HAS_TRIGGERS)
 		txn_run_wal_write_triggers(txn);
-	if (!txn_has_flag(txn, TXN_WAIT_SYNC))
+	if (!(txn->flags & TXN_WAIT_SYNC))
 		txn_complete_success(txn);
 	else if (txn->fiber != NULL && txn->fiber != fiber())
 		fiber_wakeup(txn->fiber);
@@ -640,7 +639,7 @@ txn_journal_entry_new(struct txn *txn)
 	 * space can't be synchronous. So if there is at least one
 	 * synchronous space, the transaction is not local.
 	 */
-	if (!txn_has_flag(txn, TXN_FORCE_ASYNC)) {
+	if (!(txn->flags & TXN_FORCE_ASYNC)) {
 		if (is_sync) {
 			txn->flags |= TXN_WAIT_SYNC | TXN_WAIT_ACK;
 		} else if (!txn_limbo_is_empty(&txn_limbo)) {
@@ -692,8 +691,8 @@ txn_prepare(struct txn *txn)
 {
 	txn->psn = ++txn_last_psn;
 
-	if (txn_has_flag(txn, TXN_IS_ABORTED_BY_YIELD)) {
-		assert(!txn_has_flag(txn, TXN_CAN_YIELD));
+	if (txn->flags & TXN_IS_ABORTED_BY_YIELD) {
+		assert(!(txn->flags & TXN_CAN_YIELD));
 		diag_set(ClientError, ER_TRANSACTION_YIELD);
 		diag_log();
 		return -1;
@@ -747,7 +746,7 @@ txn_prepare(struct txn *txn)
 	assert(rlist_empty(&txn->conflicted_by_list));
 
 	trigger_clear(&txn->fiber_on_stop);
-	if (!txn_has_flag(txn, TXN_CAN_YIELD))
+	if (!(txn->flags & TXN_CAN_YIELD))
 		trigger_clear(&txn->fiber_on_yield);
 
 	txn->start_tm = ev_monotonic_now(loop());
@@ -814,7 +813,7 @@ txn_commit_async(struct txn *txn)
 	if (req == NULL)
 		goto rollback;
 
-	bool is_sync = txn_has_flag(txn, TXN_WAIT_SYNC);
+	bool is_sync = txn->flags & TXN_WAIT_SYNC;
 	struct txn_limbo_entry *limbo_entry;
 	if (is_sync) {
 		/*
@@ -837,7 +836,7 @@ txn_commit_async(struct txn *txn)
 		if (limbo_entry == NULL)
 			goto rollback;
 
-		if (txn_has_flag(txn, TXN_WAIT_ACK)) {
+		if (txn->flags & TXN_WAIT_ACK) {
 			int64_t lsn = req->rows[txn->n_applier_rows - 1]->lsn;
 			/*
 			 * Can't tell whether it is local or not -
@@ -894,7 +893,7 @@ txn_commit(struct txn *txn)
 	if (req == NULL)
 		goto rollback;
 
-	bool is_sync = txn_has_flag(txn, TXN_WAIT_SYNC);
+	bool is_sync = txn->flags & TXN_WAIT_SYNC;
 	if (is_sync) {
 		/*
 		 * Remote rows, if any, come before local rows, so
@@ -921,7 +920,7 @@ txn_commit(struct txn *txn)
 		goto rollback;
 	}
 	if (is_sync) {
-		if (txn_has_flag(txn, TXN_WAIT_ACK)) {
+		if (txn->flags & TXN_WAIT_ACK) {
 			int64_t lsn = req->rows[req->n_rows - 1]->lsn;
 			/*
 			 * Use local LSN assignment. Because
@@ -936,7 +935,7 @@ txn_commit(struct txn *txn)
 		if (txn_limbo_wait_complete(&txn_limbo, limbo_entry) < 0)
 			goto rollback;
 	}
-	assert(txn_has_flag(txn, TXN_IS_DONE));
+	assert(txn->flags & TXN_IS_DONE);
 	assert(txn->signature >= 0);
 
 	/* Synchronous transactions are freed by the calling fiber. */
@@ -945,7 +944,7 @@ txn_commit(struct txn *txn)
 
 rollback:
 	assert(txn->fiber != NULL);
-	if (!txn_has_flag(txn, TXN_IS_DONE)) {
+	if (!(txn->flags & TXN_IS_DONE)) {
 		fiber_set_txn(fiber(), txn);
 		txn_rollback(txn);
 	} else {
@@ -970,7 +969,7 @@ txn_rollback(struct txn *txn)
 	assert(txn == in_txn());
 	txn->status = TXN_ABORTED;
 	trigger_clear(&txn->fiber_on_stop);
-	if (!txn_has_flag(txn, TXN_CAN_YIELD))
+	if (!(txn->flags & TXN_CAN_YIELD))
 		trigger_clear(&txn->fiber_on_yield);
 	txn->signature = TXN_SIGNATURE_ROLLBACK;
 	txn_complete_fail(txn);
@@ -991,7 +990,7 @@ bool
 txn_can_yield(struct txn *txn, bool set)
 {
 	assert(txn == in_txn());
-	bool could = txn_has_flag(txn, TXN_CAN_YIELD);
+	bool could = txn->flags & TXN_CAN_YIELD;
 	if (set && !could) {
 		txn->flags |= TXN_CAN_YIELD;
 		trigger_clear(&txn->fiber_on_yield);
@@ -1226,7 +1225,7 @@ txn_on_yield(struct trigger *trigger, void *event)
 	(void) event;
 	struct txn *txn = in_txn();
 	assert(txn != NULL);
-	assert(!txn_has_flag(txn, TXN_CAN_YIELD));
+	assert(!(txn->flags & TXN_CAN_YIELD));
 	txn_rollback_to_svp(txn, NULL);
 	txn->flags |= TXN_IS_ABORTED_BY_YIELD;
 	return 0;
diff --git a/src/box/txn.h b/src/box/txn.h
index 201564000..36a413167 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -487,7 +487,7 @@ txn_commit_async(struct txn *txn);
 static inline void
 txn_init_triggers(struct txn *txn)
 {
-	if (!txn_has_flag(txn, TXN_HAS_TRIGGERS)) {
+	if (!(txn->flags & TXN_HAS_TRIGGERS)) {
 		rlist_create(&txn->on_commit);
 		rlist_create(&txn->on_rollback);
 		rlist_create(&txn->on_wal_write);
diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index d418530ad..86b3bc580 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -60,7 +60,7 @@ txn_limbo_last_synchro_entry(struct txn_limbo *limbo)
 {
 	struct txn_limbo_entry *entry;
 	rlist_foreach_entry_reverse(entry, &limbo->queue, in_queue) {
-		if (txn_has_flag(entry->txn, TXN_WAIT_ACK))
+		if (entry->txn->flags & TXN_WAIT_ACK)
 			return entry;
 	}
 	return NULL;
@@ -69,7 +69,7 @@ txn_limbo_last_synchro_entry(struct txn_limbo *limbo)
 struct txn_limbo_entry *
 txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
 {
-	assert(txn_has_flag(txn, TXN_WAIT_SYNC));
+	assert(txn->flags & TXN_WAIT_SYNC);
 	assert(limbo == &txn_limbo);
 	/*
 	 * Transactions should be added to the limbo before WAL write. Limbo
@@ -168,7 +168,7 @@ txn_limbo_assign_remote_lsn(struct txn_limbo *limbo,
 	assert(limbo->owner_id != instance_id);
 	assert(entry->lsn == -1);
 	assert(lsn > 0);
-	assert(txn_has_flag(entry->txn, TXN_WAIT_ACK));
+	assert(entry->txn->flags & TXN_WAIT_ACK);
 	(void) limbo;
 	entry->lsn = lsn;
 }
@@ -181,7 +181,7 @@ txn_limbo_assign_local_lsn(struct txn_limbo *limbo,
 	assert(limbo->owner_id == instance_id);
 	assert(entry->lsn == -1);
 	assert(lsn > 0);
-	assert(txn_has_flag(entry->txn, TXN_WAIT_ACK));
+	assert(entry->txn->flags & TXN_WAIT_ACK);
 
 	entry->lsn = lsn;
 	/*
@@ -215,14 +215,14 @@ txn_limbo_write_rollback(struct txn_limbo *limbo, int64_t lsn);
 int
 txn_limbo_wait_complete(struct txn_limbo *limbo, struct txn_limbo_entry *entry)
 {
-	assert(entry->lsn > 0 || !txn_has_flag(entry->txn, TXN_WAIT_ACK));
+	assert(entry->lsn > 0 || !(entry->txn->flags & TXN_WAIT_ACK));
 	bool cancellable = fiber_set_cancellable(false);
 
 	if (txn_limbo_entry_is_complete(entry))
 		goto complete;
 
-	assert(!txn_has_flag(entry->txn, TXN_IS_DONE));
-	assert(txn_has_flag(entry->txn, TXN_WAIT_SYNC));
+	assert(!(entry->txn->flags & TXN_IS_DONE));
+	assert(entry->txn->flags & TXN_WAIT_SYNC);
 	double start_time = fiber_clock();
 	while (true) {
 		double deadline = start_time + replication_synchro_timeout;
@@ -288,7 +288,7 @@ txn_limbo_wait_complete(struct txn_limbo *limbo, struct txn_limbo_entry *entry)
 	 * installed the commit/rollback flag.
 	 */
 	assert(rlist_empty(&entry->in_queue));
-	assert(txn_has_flag(entry->txn, TXN_IS_DONE));
+	assert(entry->txn->flags & TXN_IS_DONE);
 	fiber_set_cancellable(cancellable);
 	/*
 	 * The first tx to be rolled back already performed all
@@ -384,7 +384,7 @@ txn_limbo_read_confirm(struct txn_limbo *limbo, int64_t lsn)
 		 * it is last, it does not depend on a not finished sync
 		 * transaction anymore and can be confirmed right away.
 		 */
-		if (txn_has_flag(e->txn, TXN_WAIT_ACK)) {
+		if (e->txn->flags & TXN_WAIT_ACK) {
 			/* Sync transaction not covered by the confirmation. */
 			if (e->lsn > lsn)
 				break;
@@ -444,7 +444,7 @@ txn_limbo_read_rollback(struct txn_limbo *limbo, int64_t lsn)
 	struct txn_limbo_entry *e, *tmp;
 	struct txn_limbo_entry *last_rollback = NULL;
 	rlist_foreach_entry_reverse(e, &limbo->queue, in_queue) {
-		if (!txn_has_flag(e->txn, TXN_WAIT_ACK))
+		if (!(e->txn->flags & TXN_WAIT_ACK))
 			continue;
 		if (e->lsn < lsn)
 			break;
@@ -527,7 +527,7 @@ txn_limbo_ack(struct txn_limbo *limbo, uint32_t replica_id, int64_t lsn)
 		 * transactions are automatically committed right
 		 * after all the previous sync transactions are.
 		 */
-		if (!txn_has_flag(e->txn, TXN_WAIT_ACK)) {
+		if (!(e->txn->flags & TXN_WAIT_ACK)) {
 			assert(e->lsn == -1);
 			if (confirm_lsn == -1)
 				continue;
@@ -661,7 +661,7 @@ txn_limbo_force_empty(struct txn_limbo *limbo, int64_t confirm_lsn)
 	struct txn_limbo_entry *e, *last_quorum = NULL;
 	struct txn_limbo_entry *rollback = NULL;
 	rlist_foreach_entry(e, &limbo->queue, in_queue) {
-		if (txn_has_flag(e->txn, TXN_WAIT_ACK)) {
+		if (e->txn->flags & TXN_WAIT_ACK) {
 			if (e->lsn <= confirm_lsn) {
 				last_quorum = e;
 			} else {
@@ -690,7 +690,7 @@ txn_limbo_on_parameters_change(struct txn_limbo *limbo)
 	int64_t confirm_lsn = -1;
 	rlist_foreach_entry(e, &limbo->queue, in_queue) {
 		assert(e->ack_count <= VCLOCK_MAX);
-		if (!txn_has_flag(e->txn, TXN_WAIT_ACK)) {
+		if (!(e->txn->flags & TXN_WAIT_ACK)) {
 			assert(e->lsn == -1);
 			if (confirm_lsn == -1)
 				continue;
-- 
2.29.2

Re: [Tarantool-patches] [PATCH 5/6] txn: stop using txn_has_flag

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patch!

On 22.01.2021 14:26, Cyrill Gorcunov wrote:
> Part-of #5128
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/box/txn.c       | 39 +++++++++++++++++++--------------------
>  src/box/txn.h       |  2 +-
>  src/box/txn_limbo.c | 26 +++++++++++++-------------
>  3 files changed, 33 insertions(+), 34 deletions(-)
> 
> diff --git a/src/box/txn.c b/src/box/txn.c
> index 6197cf012..d1964b5f7 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -526,7 +526,7 @@ txn_free_or_wakeup(struct txn *txn)
>  void
>  txn_complete_fail(struct txn *txn)
>  {
> -	assert(!txn_has_flag(txn, TXN_IS_DONE));
> +	assert(!(txn->flags & TXN_IS_DONE));

Please, use explicit != 0. We don't apply '!' operator to
non-boolean values. The same in other places. This I can even
find in the code style guide:

https://github.com/tarantool/tarantool/wiki/Code-review-procedure#code-style

Re: [Tarantool-patches] [PATCH 5/6] txn: stop using txn_has_flag

[Cyrill Gorcunov via Tarantool-patches]

On Sat, Jan 30, 2021 at 08:17:59PM +0100, Vladislav Shpilevoy wrote:
> > --- a/src/box/txn.c
> > +++ b/src/box/txn.c
> > @@ -526,7 +526,7 @@ txn_free_or_wakeup(struct txn *txn)
> >  void
> >  txn_complete_fail(struct txn *txn)
> >  {
> > -	assert(!txn_has_flag(txn, TXN_IS_DONE));
> > +	assert(!(txn->flags & TXN_IS_DONE));
> 
> Please, use explicit != 0. We don't apply '!' operator to
> non-boolean values. The same in other places. This I can even
> find in the code style guide:
> 
> https://github.com/tarantool/tarantool/wiki/Code-review-procedure#code-style

I remember this. And used this style initially. But with this rule applied
code becomes a way more ugly. For example

-       if (!txn_has_flag(txn, TXN_CAN_YIELD))
+       if ((txn->flags & TXN_CAN_YIELD) == 0)

In first place a person notes the "logical not" operator immediately,
and this sounds more natural than excessive five symbols at the tail of
the 'if' statement.

Another example

-       assert(!txn_has_flag(txn, TXN_IS_DONE));
-       assert(!txn_has_flag(txn, TXN_WAIT_SYNC));
+       assert(!(txn->flags & (TXN_IS_DONE | TXN_WAIT_SYNC)));

Which should be changed to either

	assert((txn->flags & (TXN_IS_DONE | TXN_WAIT_SYNC)) == 0);

or back to pair

	assert((txn->flags & TXN_IS_DONE) == 0);
	assert((txn->flags & TXN_WAIT_SYNC) == 0);

which is a way more worse than it was with txn_has_flag() helper,
at least for me.

The initial rationale for this series was (as far as I remember) to
setup several flags at once, so I think you could consider implementing
txn_set_flags() helper which would do the trick instead. Thus lets drop
this series, it doesn't make anything better without using neg operator.

Re: [Tarantool-patches] [PATCH 5/6] txn: stop using txn_has_flag

[Vladislav Shpilevoy via Tarantool-patches]

On 31.01.2021 23:13, Cyrill Gorcunov wrote:
> On Sat, Jan 30, 2021 at 08:17:59PM +0100, Vladislav Shpilevoy wrote:
>>> --- a/src/box/txn.c
>>> +++ b/src/box/txn.c
>>> @@ -526,7 +526,7 @@ txn_free_or_wakeup(struct txn *txn)
>>>  void
>>>  txn_complete_fail(struct txn *txn)
>>>  {
>>> -	assert(!txn_has_flag(txn, TXN_IS_DONE));
>>> +	assert(!(txn->flags & TXN_IS_DONE));
>>
>> Please, use explicit != 0. We don't apply '!' operator to
>> non-boolean values. The same in other places. This I can even
>> find in the code style guide:
>>
>> https://github.com/tarantool/tarantool/wiki/Code-review-procedure#code-style
> 
> I remember this. And used this style initially. But with this rule applied
> code becomes a way more ugly. For example
> 
> -       if (!txn_has_flag(txn, TXN_CAN_YIELD))
> +       if ((txn->flags & TXN_CAN_YIELD) == 0)
> 
> In first place a person notes the "logical not" operator immediately,
> and this sounds more natural than excessive five symbols at the tail of
> the 'if' statement.
> 
> Another example
> 
> -       assert(!txn_has_flag(txn, TXN_IS_DONE));
> -       assert(!txn_has_flag(txn, TXN_WAIT_SYNC));
> +       assert(!(txn->flags & (TXN_IS_DONE | TXN_WAIT_SYNC)));
> 
> Which should be changed to either
> 
> 	assert((txn->flags & (TXN_IS_DONE | TXN_WAIT_SYNC)) == 0);
> 
> or back to pair
> 
> 	assert((txn->flags & TXN_IS_DONE) == 0);
> 	assert((txn->flags & TXN_WAIT_SYNC) == 0);
> 
> which is a way more worse than it was with txn_has_flag() helper,
> at least for me.
> 
> The initial rationale for this series was (as far as I remember) to
> setup several flags at once, so I think you could consider implementing
> txn_set_flags() helper which would do the trick instead. Thus lets drop
> this series, it doesn't make anything better without using neg operator.

Another purpose of the issue was to make TXN_WAIT_SYNC a part of
TXN_WAIT_ACK. Because ACK is never present without SYNC.

Talking of the setting many flags at once - you still can do this,
even with txn_set_flag(). Just rename txn_set_flag() to txn_set_flags(),
and make the flags proper bitfields like in the first commits of this
series. Then we could do

	txn_set_flags(TXN_WAIT_SYNC | TXN_WAIT_ACK)

Although I don't know what to do with checking flags. If we add
txn_has_flags(), then should it return true if all the requested
flags are present, or if any of them?

Re: [Tarantool-patches] [PATCH 5/6] txn: stop using txn_has_flag

[Cyrill Gorcunov via Tarantool-patches]

On Wed, Feb 03, 2021 at 08:47:39PM +0100, Vladislav Shpilevoy wrote:
> > 
> > The initial rationale for this series was (as far as I remember) to
> > setup several flags at once, so I think you could consider implementing
> > txn_set_flags() helper which would do the trick instead. Thus lets drop
> > this series, it doesn't make anything better without using neg operator.
> 
> Another purpose of the issue was to make TXN_WAIT_SYNC a part of
> TXN_WAIT_ACK. Because ACK is never present without SYNC.
> 
> Talking of the setting many flags at once - you still can do this,
> even with txn_set_flag(). Just rename txn_set_flag() to txn_set_flags(),
> and make the flags proper bitfields like in the first commits of this
> series. Then we could do
> 
> 	txn_set_flags(TXN_WAIT_SYNC | TXN_WAIT_ACK)
> 
> Although I don't know what to do with checking flags. If we add
> txn_has_flags(), then should it return true if all the requested
> flags are present, or if any of them?

I think we can stick with txn_has_flags to exact match and txn_has_flagsa
for any flag.

[Tarantool-patches] [PATCH 6/6] txn: drop unused txn_x_flag helpers

[Cyrill Gorcunov via Tarantool-patches]

Closes #5128

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn.h | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/src/box/txn.h b/src/box/txn.h
index 36a413167..82d18cf91 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -393,24 +393,6 @@ struct txn {
 	struct rlist read_set;
 };
 
-static inline bool
-txn_has_flag(struct txn *txn, unsigned int flag)
-{
-	return (txn->flags & flag) != 0;
-}
-
-static inline void
-txn_set_flag(struct txn *txn, unsigned int flag)
-{
-	txn->flags |= flag;
-}
-
-static inline void
-txn_clear_flag(struct txn *txn, unsigned int flag)
-{
-	txn->flags &= ~flag;
-}
-
 /* Pointer to the current transaction (if any) */
 static inline struct txn *
 in_txn(void)
-- 
2.29.2

Re: [Tarantool-patches] [PATCH 0/6] txn: drop txn_X_flag helpers

[Serge Petrenko via Tarantool-patches]

22.01.2021 16:26, Cyrill Gorcunov пишет:
> When operating with bitfields it is a way better
> to use opencoded operations instead of some helpers
> which allow to modify one bit at once.
>
> In the series we drop usage of txn_X_flag(), this
> shrinks code a bit and make possible to test a set
> of bits at once.
>
> issue https://github.com/tarantool/tarantool/issues/5128
> branch gorcunov/gh-5128-txn-flags
>
> Cyrill Gorcunov (6):
>    txn: convert flags to explicit bitfield
>    txn: stop using txn_set_flag
>    test/unit: snap_quorum_delay -- stop using txn_set_flag
>    txn: stop using txn_clear_flag
>    txn: stop using txn_has_flag
>    txn: drop unused txn_x_flag helpers
>
>   src/box/applier.cc             |  4 +--
>   src/box/memtx_engine.c         |  2 +-
>   src/box/txn.c                  | 54 ++++++++++++++++------------------
>   src/box/txn.h                  | 36 ++++++-----------------
>   src/box/txn_limbo.c            | 35 ++++++++++------------
>   test/unit/snap_quorum_delay.cc |  3 +-
>   6 files changed, 55 insertions(+), 79 deletions(-)
>
>
> base-commit: a6fd00ab73705c3afb03b2828526c7b1dfceda08

Hi! Thanks for the patchset!
LGTM.

-- 
Serge Petrenko