* [Tarantool-patches] [PATCH v3] txn: convert flags to explicit bitfield
@ 2021-02-04 11:19 Cyrill Gorcunov via Tarantool-patches
2021-02-04 14:01 ` Serge Petrenko via Tarantool-patches
0 siblings, 1 reply; 5+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-02-04 11:19 UTC (permalink / raw)
To: tml; +Cc: Vladislav Shpilevoy
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Tarantool-patches] [PATCH v3] txn: convert flags to explicit bitfield
2021-02-04 11:19 [Tarantool-patches] [PATCH v3] txn: convert flags to explicit bitfield Cyrill Gorcunov via Tarantool-patches
@ 2021-02-04 14:01 ` Serge Petrenko via Tarantool-patches
2021-02-04 14:31 ` Cyrill Gorcunov via Tarantool-patches
0 siblings, 1 reply; 5+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-02-04 14:01 UTC (permalink / raw)
To: Cyrill Gorcunov, tml; +Cc: Vladislav Shpilevoy
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Tarantool-patches] [PATCH v3] txn: convert flags to explicit bitfield
2021-02-04 14:01 ` Serge Petrenko via Tarantool-patches
@ 2021-02-04 14:31 ` Cyrill Gorcunov via Tarantool-patches
2021-02-05 7:53 ` Serge Petrenko via Tarantool-patches
0 siblings, 1 reply; 5+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-02-04 14:31 UTC (permalink / raw)
To: Serge Petrenko; +Cc: tml, Vladislav Shpilevoy
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.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Tarantool-patches] [PATCH v3] txn: convert flags to explicit bitfield
2021-02-04 14:31 ` Cyrill Gorcunov via Tarantool-patches
@ 2021-02-05 7:53 ` Serge Petrenko via Tarantool-patches
2021-02-05 8:22 ` Cyrill Gorcunov via Tarantool-patches
0 siblings, 1 reply; 5+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-02-05 7:53 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: tml, Vladislav Shpilevoy
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Tarantool-patches] [PATCH v3] txn: convert flags to explicit bitfield
2021-02-05 7:53 ` Serge Petrenko via Tarantool-patches
@ 2021-02-05 8:22 ` Cyrill Gorcunov via Tarantool-patches
0 siblings, 0 replies; 5+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-02-05 8:22 UTC (permalink / raw)
To: Serge Petrenko; +Cc: tml, Vladislav Shpilevoy
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-02-05 8:22 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-04 11:19 [Tarantool-patches] [PATCH v3] txn: convert flags to explicit bitfield Cyrill Gorcunov via Tarantool-patches
2021-02-04 14:01 ` Serge Petrenko via Tarantool-patches
2021-02-04 14:31 ` Cyrill Gorcunov via Tarantool-patches
2021-02-05 7:53 ` Serge Petrenko via Tarantool-patches
2021-02-05 8:22 ` Cyrill Gorcunov via Tarantool-patches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox