[Tarantool-patches] [PATCH] txn: convert flags to explicit bitfield
Cyrill Gorcunov
gorcunov at gmail.com
Thu Feb 18 11:32:57 MSK 2021
On Thu, Feb 18, 2021 at 12:45:56AM +0300, Cyrill Gorcunov wrote:
> On Wed, Feb 17, 2021 at 10:15:30PM +0100, Vladislav Shpilevoy wrote:
> > Hi! Thanks for the patch!
> >
> > > @@ -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)
> >
> > 1. Did you do a self-review before sending this? Can you
> > tell yourself what is wrong here?
>
> Yes I did, and I don't get what's wrong with this line? Previously
> we have `if (txn_has_flag(txn, TXN_HAS_TRIGGERS))` where txn_has_flag
> get expanded to
>
> return (txn->flags & TXN_HAS_TRIGGERS) != 0;
>
> so where is an error?
>
> > 2. What was wrong with the idea of having the helpers
> > but operating on bitfields? And having has_flags() instead
> > of has_flag() to check presense of all specified flags +
> > clear_flags() to remove all specified flags.
> >
> > I don't understand. I think we discussed it already 2 or 3
> > times and all seemed to agree?
>
> The initial idea was to operate with flags directly, you pointed
> that lines of code as if (!(txn->flags & mask)) violates our
> code style. Then we thought of renaming the helpers to txn_x_flags
> to operate with multiple flags, which lead us to a vague moment --
> it is unclear how exactly txn_has_flag should operate: exact
> matching or any matching?
>
> Finlly I simply moved back to direct access to the fields
> with explicit `!= 0` or `== 0` form.
Anyway, here is an another approach, sitting in
gorcunov/gh-5128-txn-flags-5 branch
---
>From 0e4484213d53f58e34db16e3cb96c63805bdd5e7 Mon Sep 17 00:00:00 2001
From: Cyrill Gorcunov <gorcunov at gmail.com>
Date: Thu, 4 Feb 2021 14:19:12 +0300
Subject: [PATCH] txn: convert flags to explicit bitfield
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 at gmail.com>
---
src/box/applier.cc | 4 ++--
src/box/memtx_engine.c | 2 +-
src/box/txn.c | 15 +++++++--------
src/box/txn.h | 27 ++++++++++++++-------------
src/box/txn_limbo.c | 9 +++------
test/unit/snap_quorum_delay.cc | 3 +--
6 files changed, 28 insertions(+), 32 deletions(-)
diff --git a/src/box/applier.cc b/src/box/applier.cc
index 553db76fc..8d7ce5d99 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_set_flags(txn, 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_set_flags(txn, 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 1174234d2..a4cd671f5 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_set_flags(txn, 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..71c89ce5f 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_set_flags(txn, 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_set_flags(txn, 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_set_flags(txn, 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_set_flags(txn, TXN_WAIT_SYNC);
}
}
@@ -994,10 +993,10 @@ 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_set_flags(txn, TXN_CAN_YIELD);
trigger_clear(&txn->fiber_on_yield);
} else if (!set && could) {
- txn_clear_flag(txn, TXN_CAN_YIELD);
+ txn_clear_flags(txn, TXN_CAN_YIELD);
trigger_create(&txn->fiber_on_yield, txn_on_yield, NULL, NULL);
trigger_add(&fiber()->on_yield, &txn->fiber_on_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_set_flags(txn, TXN_IS_ABORTED_BY_YIELD);
return 0;
}
diff --git a/src/box/txn.h b/src/box/txn.h
index fca9bc1d0..29fe6d5ce 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 {
@@ -396,19 +396,20 @@ struct txn {
static inline bool
txn_has_flag(struct txn *txn, enum txn_flag flag)
{
- return (txn->flags & (1 << flag)) != 0;
+ assert((flag & (flag - 1)) == 0);
+ return (txn->flags & flag) != 0;
}
static inline void
-txn_set_flag(struct txn *txn, enum txn_flag flag)
+txn_set_flags(struct txn *txn, unsigned int flags)
{
- txn->flags |= 1 << flag;
+ txn->flags |= flags;
}
static inline void
-txn_clear_flag(struct txn *txn, enum txn_flag flag)
+txn_clear_flags(struct txn *txn, unsigned int flags)
{
- txn->flags &= ~(1 << flag);
+ txn->flags &= ~flags;
}
/* Pointer to the current transaction (if any) */
@@ -491,7 +492,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_set_flags(txn, TXN_HAS_TRIGGERS);
}
}
diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index 9c4c3cdf1..cf0ad9350 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_flags(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_flags(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_flags(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..803bbbea8 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_flags(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
More information about the Tarantool-patches
mailing list