[Tarantool-patches] [PATCH v3] txn: convert flags to explicit bitfield

[Tarantool-patches] [PATCH v3] txn: convert flags to explicit bitfield

[Cyrill Gorcunov via Tarantool-patches]

Instead of shifting flags inside txn_x_flag() helpers
lets define precompiled value. This allows us to
operate with several flags at onces and shrink code
a bit.

Closes #5128

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
issue https://github.com/tarantool/tarantool/issues/5128
branch gorcunov/gh-5128-txn-flags-3

 src/box/txn.c                  |  3 +--
 src/box/txn.h                  | 26 +++++++++++++-------------
 src/box/txn_limbo.c            |  9 +++------
 test/unit/snap_quorum_delay.cc |  3 +--
 4 files changed, 18 insertions(+), 23 deletions(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index a5edbfc60..150280905 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -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_set_flag(txn, TXN_WAIT_SYNC | TXN_WAIT_ACK);
 		} else if (!txn_limbo_is_empty(&txn_limbo)) {
 			/*
 			 * There some sync entries on the
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) */
diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index 9c4c3cdf1..1bf292670 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);
+		txn_clear_flag(e->txn, 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);
+		txn_clear_flag(e->txn, 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);
+		txn_clear_flag(e->txn, TXN_WAIT_SYNC | TXN_WAIT_ACK);
 		if (e->txn->signature >= 0) {
 			/* Rollback the transaction. */
 			e->txn->signature = TXN_SIGNATURE_SYNC_ROLLBACK;
diff --git a/test/unit/snap_quorum_delay.cc b/test/unit/snap_quorum_delay.cc
index b9d4cc6c4..e618b1cc4 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_set_flag(txn, 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 v3] txn: convert flags to explicit bitfield

[Serge Petrenko via Tarantool-patches]

04.02.2021 14:19, Cyrill Gorcunov пишет:
> Instead of shifting flags inside txn_x_flag() helpers
> lets define precompiled value. This allows us to
> operate with several flags at onces and shrink code
> a bit.
>
> Closes #5128
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---

Hi! Thanks for the patch! Please, find one comment below.

> issue https://github.com/tarantool/tarantool/issues/5128
> branch gorcunov/gh-5128-txn-flags-3
>
>   src/box/txn.c                  |  3 +--
>   src/box/txn.h                  | 26 +++++++++++++-------------
>   src/box/txn_limbo.c            |  9 +++------
>   test/unit/snap_quorum_delay.cc |  3 +--
>   4 files changed, 18 insertions(+), 23 deletions(-)
>
> diff --git a/src/box/txn.c b/src/box/txn.c
> index a5edbfc60..150280905 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -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_set_flag(txn, TXN_WAIT_SYNC | TXN_WAIT_ACK);
>   		} else if (!txn_limbo_is_empty(&txn_limbo)) {
>   			/*
>   			 * There some sync entries on the
> 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,

Since setting `TXN_WAIT_ACK` always implies setting `TXN_WAIT_SYNC`, and
when TXN_WAIT_ACK is cleared, TXN_WAIT_SYNC is also cleared, you may
define TXN_WAIT_ACK = 0x30 for simplicity.
Then setting TXN_WAIT_ACK will set TXN_WAIT_SYNC implicitly. And same for
clearing TXN_WAIT_ACK.

Other than that LGTM.

>   	/**
>   	 * 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) */
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index 9c4c3cdf1..1bf292670 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);
> +		txn_clear_flag(e->txn, 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);
> +		txn_clear_flag(e->txn, 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);
> +		txn_clear_flag(e->txn, TXN_WAIT_SYNC | TXN_WAIT_ACK);
>   		if (e->txn->signature >= 0) {
>   			/* Rollback the transaction. */
>   			e->txn->signature = TXN_SIGNATURE_SYNC_ROLLBACK;
> diff --git a/test/unit/snap_quorum_delay.cc b/test/unit/snap_quorum_delay.cc
> index b9d4cc6c4..e618b1cc4 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_set_flag(txn, 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()

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v3] txn: convert flags to explicit bitfield

[Cyrill Gorcunov via Tarantool-patches]

On Thu, Feb 04, 2021 at 05:01:07PM +0300, Serge Petrenko wrote:
...
> >   	/**
> >   	 * 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,
> 
> Since setting `TXN_WAIT_ACK` always implies setting `TXN_WAIT_SYNC`, and
> when TXN_WAIT_ACK is cleared, TXN_WAIT_SYNC is also cleared, you may
> define TXN_WAIT_ACK = 0x30 for simplicity.
> Then setting TXN_WAIT_ACK will set TXN_WAIT_SYNC implicitly. And same for
> clearing TXN_WAIT_ACK.

Yes, but this won't get exact match for txn_has_flag, which tests
for a single bit now. Surely I can setup

	TXN_WAIT_ACK = 0x20 | TXN_WAIT_SYNC

if you and Vlad agrees.

Re: [Tarantool-patches] [PATCH v3] txn: convert flags to explicit bitfield

[Serge Petrenko via Tarantool-patches]

04.02.2021 17:31, Cyrill Gorcunov пишет:
> On Thu, Feb 04, 2021 at 05:01:07PM +0300, Serge Petrenko wrote:
> ...
>>>    	/**
>>>    	 * 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,
>> Since setting `TXN_WAIT_ACK` always implies setting `TXN_WAIT_SYNC`, and
>> when TXN_WAIT_ACK is cleared, TXN_WAIT_SYNC is also cleared, you may
>> define TXN_WAIT_ACK = 0x30 for simplicity.
>> Then setting TXN_WAIT_ACK will set TXN_WAIT_SYNC implicitly. And same for
>> clearing TXN_WAIT_ACK.
> Yes, but this won't get exact match for txn_has_flag, which tests
> for a single bit now. Surely I can setup

Ok, I see. Why not make txn_has_flag check for entire flag then?

>
> 	TXN_WAIT_ACK = 0x20 | TXN_WAIT_SYNC
>
> if you and Vlad agrees.
Let's wait for what Vlad has to say

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v3] txn: convert flags to explicit bitfield

[Cyrill Gorcunov via Tarantool-patches]

On Fri, Feb 05, 2021 at 10:53:32AM +0300, Serge Petrenko wrote:
> > Yes, but this won't get exact match for txn_has_flag, which tests
> > for a single bit now. Surely I can setup
> 
> Ok, I see. Why not make txn_has_flag check for entire flag then?
> 
> > 
> > 	TXN_WAIT_ACK = 0x20 | TXN_WAIT_SYNC
> > 
> > if you and Vlad agrees.
> Let's wait for what Vlad has to say

You know, I tried various ways. None of them satisfied me :/
I propose to close this issue and leave everything as is,
at least currently code as a way more readable I think.