[PATCH] txn: convert txn flags into bit mask

[PATCH] txn: convert txn flags into bit mask

[Vladimir Davydov]

---
https://github.com/tarantool/tarantool/commits/dv/txn-flags

 src/box/txn.c | 35 +++++++++++++++++------------------
 src/box/txn.h | 38 ++++++++++++++++++++++----------------
 2 files changed, 39 insertions(+), 34 deletions(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index c7d01b2e..93cc24c2 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -197,10 +197,7 @@ txn_begin()
 	txn->n_new_rows = 0;
 	txn->n_local_rows = 0;
 	txn->n_applier_rows = 0;
-	txn->has_triggers  = false;
-	txn->is_done = false;
-	txn->can_yield = true;
-	txn->is_aborted_by_yield = false;
+	txn->flags = TXN_CAN_YIELD;
 	txn->in_sub_stmt = 0;
 	txn->id = ++tsn;
 	txn->signature = -1;
@@ -396,13 +393,13 @@ txn_complete(struct txn *txn)
 		/* Undo the transaction. */
 		if (txn->engine)
 			engine_rollback(txn->engine, txn);
-		if (txn->has_triggers)
+		if (txn->flags & TXN_HAS_TRIGGERS)
 			txn_run_rollback_triggers(txn);
 	} else {
 		/* Commit the transaction. */
 		if (txn->engine != NULL)
 			engine_commit(txn->engine, txn);
-		if (txn->has_triggers)
+		if (txn->flags & TXN_HAS_TRIGGERS)
 			txn_run_commit_triggers(txn);
 
 		double stop_tm = ev_monotonic_now(loop());
@@ -422,7 +419,7 @@ txn_complete(struct txn *txn)
 	if (txn->fiber == NULL)
 		txn_free(txn);
 	else {
-		txn->is_done = true;
+		txn->flags |= TXN_IS_DONE;
 		if (txn->fiber != fiber())
 			/* Wake a waiting fiber up. */
 			fiber_wakeup(txn->fiber);
@@ -491,8 +488,8 @@ txn_write_to_wal(struct txn *txn)
 static int
 txn_prepare(struct txn *txn)
 {
-	if (txn->is_aborted_by_yield) {
-		assert(!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;
@@ -518,7 +515,7 @@ txn_prepare(struct txn *txn)
 			return -1;
 	}
 	trigger_clear(&txn->fiber_on_stop);
-	if (!txn->can_yield)
+	if (!(txn->flags & TXN_CAN_YIELD))
 		trigger_clear(&txn->fiber_on_yield);
 	return 0;
 }
@@ -558,7 +555,7 @@ txn_commit(struct txn *txn)
 	 * In case of non-yielding journal the transaction could already
 	 * be done and there is nothing to wait in such cases.
 	 */
-	if (!txn->is_done) {
+	if (!(txn->flags & TXN_IS_DONE)) {
 		bool cancellable = fiber_set_cancellable(false);
 		fiber_yield();
 		fiber_set_cancellable(cancellable);
@@ -586,7 +583,7 @@ txn_rollback(struct txn *txn)
 {
 	assert(txn == in_txn());
 	trigger_clear(&txn->fiber_on_stop);
-	if (!txn->can_yield)
+	if (!(txn->flags & TXN_CAN_YIELD))
 		trigger_clear(&txn->fiber_on_yield);
 	txn->signature = -1;
 	txn_complete(txn);
@@ -607,11 +604,13 @@ void
 txn_can_yield(struct txn *txn, bool set)
 {
 	assert(txn == in_txn());
-	assert(txn->can_yield != set);
-	txn->can_yield = set;
 	if (set) {
+		assert(!(txn->flags & TXN_CAN_YIELD));
+		txn->flags |= TXN_CAN_YIELD;
 		trigger_clear(&txn->fiber_on_yield);
 	} else {
+		assert(txn->flags & 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);
 	}
@@ -781,11 +780,11 @@ txn_on_yield(struct trigger *trigger, void *event)
 	(void) trigger;
 	(void) event;
 	struct txn *txn = in_txn();
-	assert(txn != NULL && !txn->can_yield);
+	assert(txn != NULL && !(txn->flags & TXN_CAN_YIELD));
 	txn_rollback_to_svp(txn, NULL);
-	if (txn->has_triggers) {
+	if (txn->flags & TXN_HAS_TRIGGERS) {
 		txn_run_rollback_triggers(txn);
-		txn->has_triggers = false;
+		txn->flags &= ~TXN_HAS_TRIGGERS;
 	}
-	txn->is_aborted_by_yield = true;
+	txn->flags |= TXN_IS_ABORTED_BY_YIELD;
 }
diff --git a/src/box/txn.h b/src/box/txn.h
index 2d5dea21..d5dceaed 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -51,6 +51,24 @@ struct tuple;
 struct xrow_header;
 struct Vdbe;
 
+/** Allowed values for txn::flags. */
+enum {
+	/** Transaction has been processed. */
+	TXN_IS_DONE		= 1 << 0,
+	/**
+	 * Transaction has been aborted by fiber yield so
+	 * should be rolled back at commit.
+	 */
+	TXN_IS_ABORTED_BY_YIELD	= 1 << 1,
+	/**
+	 * fiber_yield() is allowed inside the transaction.
+	 * See txn_can_yield() for more details.
+	 */
+	TXN_CAN_YIELD		= 1 << 2,
+	/** on_commit and/or on_rollback list is not empty. */
+	TXN_HAS_TRIGGERS	= 1 << 3,
+};
+
 enum {
 	/**
 	 * Maximum recursion depth for on_replace triggers.
@@ -160,20 +178,8 @@ struct txn {
 	 * already assigned LSN.
 	 */
 	int n_applier_rows;
-	/* True when transaction is processed. */
-	bool is_done;
-	/**
-	 * True if the transaction was aborted so should be
-	 * rolled back at commit.
-	 */
-	bool is_aborted_by_yield;
-	/**
-	 * True if yields are allowed inside a transaction,
-	 * see txn_can_yield().
-	 */
-	bool can_yield;
-	/** True if on_commit and on_rollback lists are non-empty. */
-	bool has_triggers;
+	/** Transaction flags, see TXN_*. */
+	unsigned flags;
 	/** The number of active nested statement-level transactions. */
 	int8_t in_sub_stmt;
 	/**
@@ -260,10 +266,10 @@ txn_write(struct txn *txn);
 static inline void
 txn_init_triggers(struct txn *txn)
 {
-	if (txn->has_triggers == false) {
+	if (!(txn->flags & TXN_HAS_TRIGGERS)) {
 		rlist_create(&txn->on_commit);
 		rlist_create(&txn->on_rollback);
-		txn->has_triggers = true;
+		txn->flags |= TXN_HAS_TRIGGERS;
 	}
 }
 
-- 
2.20.1

[tarantool-patches] Re: [PATCH] txn: convert txn flags into bit mask

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/29 13:56]:
> ---
> https://github.com/tarantool/tarantool/commits/dv/txn-flags

Curious, why not use bit fields?

-- 
Konstantin Osipov, Moscow, Russia

Re: [tarantool-patches] Re: [PATCH] txn: convert txn flags into bit mask

[Vladimir Davydov]

On Mon, Jul 29, 2019 at 03:14:11PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/29 13:56]:
> > ---
> > https://github.com/tarantool/tarantool/commits/dv/txn-flags
> 
> Curious, why not use bit fields?

Personally I don't like bit fields, because AFAIK the way they are
stored, accessed and updated depends on the compiler and, possibly,
the underlying architercture. There might be some unexpected side
effects, e.g. see

  https://lwn.net/Articles/478657/

If you prefer bit fields, no problem, I will change the patch as
you like.

Re: [tarantool-patches] Re: [PATCH] txn: convert txn flags into bit mask

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/29 15:33]:
> On Mon, Jul 29, 2019 at 03:14:11PM +0300, Konstantin Osipov wrote:
> > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/29 13:56]:
> > > ---
> > > https://github.com/tarantool/tarantool/commits/dv/txn-flags
> > 
> > Curious, why not use bit fields?

I will read the article, I do not have a strong opinion, it's just
checking for bit flags using an enum looks a bit clumsy, how about 
a bit of syntax sugar like
txn_has_flag(txn, FLAG), tnx_set_flag(txn, FLAG),
txn_clear_flag(txn, FLAG).

What do you think?


-- 
Konstantin Osipov, Moscow, Russia

Re: [tarantool-patches] Re: [PATCH] txn: convert txn flags into bit mask

[Vladimir Davydov]

On Mon, Jul 29, 2019 at 06:37:54PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/29 15:33]:
> > On Mon, Jul 29, 2019 at 03:14:11PM +0300, Konstantin Osipov wrote:
> > > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/29 13:56]:
> > > > ---
> > > > https://github.com/tarantool/tarantool/commits/dv/txn-flags
> > > 
> > > Curious, why not use bit fields?
> 
> I will read the article, I do not have a strong opinion, it's just
> checking for bit flags using an enum looks a bit clumsy, how about 
> a bit of syntax sugar like
> txn_has_flag(txn, FLAG), tnx_set_flag(txn, FLAG),
> txn_clear_flag(txn, FLAG).
> 
> What do you think?

Sounds reasonable. Here you go:

From 16610dedb84c78a96d720d7bb1b1b881f0fa9d11 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Date: Mon, 29 Jul 2019 13:49:04 +0300
Subject: [PATCH] txn: convert txn flags into bit mask


diff --git a/src/box/txn.c b/src/box/txn.c
index c7d01b2e..05ec6f14 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -197,10 +197,7 @@ txn_begin()
 	txn->n_new_rows = 0;
 	txn->n_local_rows = 0;
 	txn->n_applier_rows = 0;
-	txn->has_triggers  = false;
-	txn->is_done = false;
-	txn->can_yield = true;
-	txn->is_aborted_by_yield = false;
+	txn->flags = 0;
 	txn->in_sub_stmt = 0;
 	txn->id = ++tsn;
 	txn->signature = -1;
@@ -212,6 +209,12 @@ txn_begin()
 	/* fiber_on_yield is initialized by engine on demand */
 	trigger_create(&txn->fiber_on_stop, txn_on_stop, NULL, NULL);
 	trigger_add(&fiber()->on_stop, &txn->fiber_on_stop);
+	/*
+	 * By default all transactions may yield.
+	 * It's a responsibility of an engine to disable yields
+	 * if they are not supported.
+	 */
+	txn_set_flag(txn, TXN_CAN_YIELD);
 	return txn;
 }
 
@@ -396,13 +399,13 @@ txn_complete(struct txn *txn)
 		/* Undo the transaction. */
 		if (txn->engine)
 			engine_rollback(txn->engine, txn);
-		if (txn->has_triggers)
+		if (txn_has_flag(txn, TXN_HAS_TRIGGERS))
 			txn_run_rollback_triggers(txn);
 	} else {
 		/* Commit the transaction. */
 		if (txn->engine != NULL)
 			engine_commit(txn->engine, txn);
-		if (txn->has_triggers)
+		if (txn_has_flag(txn, TXN_HAS_TRIGGERS))
 			txn_run_commit_triggers(txn);
 
 		double stop_tm = ev_monotonic_now(loop());
@@ -422,7 +425,7 @@ txn_complete(struct txn *txn)
 	if (txn->fiber == NULL)
 		txn_free(txn);
 	else {
-		txn->is_done = true;
+		txn_set_flag(txn, TXN_IS_DONE);
 		if (txn->fiber != fiber())
 			/* Wake a waiting fiber up. */
 			fiber_wakeup(txn->fiber);
@@ -491,8 +494,8 @@ txn_write_to_wal(struct txn *txn)
 static int
 txn_prepare(struct txn *txn)
 {
-	if (txn->is_aborted_by_yield) {
-		assert(!txn->can_yield);
+	if (txn_has_flag(txn, TXN_IS_ABORTED_BY_YIELD)) {
+		assert(!txn_has_flag(txn, TXN_CAN_YIELD));
 		diag_set(ClientError, ER_TRANSACTION_YIELD);
 		diag_log();
 		return -1;
@@ -518,7 +521,7 @@ txn_prepare(struct txn *txn)
 			return -1;
 	}
 	trigger_clear(&txn->fiber_on_stop);
-	if (!txn->can_yield)
+	if (!txn_has_flag(txn, TXN_CAN_YIELD))
 		trigger_clear(&txn->fiber_on_yield);
 	return 0;
 }
@@ -558,7 +561,7 @@ txn_commit(struct txn *txn)
 	 * In case of non-yielding journal the transaction could already
 	 * be done and there is nothing to wait in such cases.
 	 */
-	if (!txn->is_done) {
+	if (!txn_has_flag(txn, TXN_IS_DONE)) {
 		bool cancellable = fiber_set_cancellable(false);
 		fiber_yield();
 		fiber_set_cancellable(cancellable);
@@ -586,7 +589,7 @@ txn_rollback(struct txn *txn)
 {
 	assert(txn == in_txn());
 	trigger_clear(&txn->fiber_on_stop);
-	if (!txn->can_yield)
+	if (!txn_has_flag(txn, TXN_CAN_YIELD))
 		trigger_clear(&txn->fiber_on_yield);
 	txn->signature = -1;
 	txn_complete(txn);
@@ -607,11 +610,13 @@ void
 txn_can_yield(struct txn *txn, bool set)
 {
 	assert(txn == in_txn());
-	assert(txn->can_yield != set);
-	txn->can_yield = set;
 	if (set) {
+		assert(!txn_has_flag(txn, TXN_CAN_YIELD));
+		txn_set_flag(txn, TXN_CAN_YIELD);
 		trigger_clear(&txn->fiber_on_yield);
 	} else {
+		assert(txn_has_flag(txn, TXN_CAN_YIELD));
+		txn_clear_flag(txn, TXN_CAN_YIELD);
 		trigger_create(&txn->fiber_on_yield, txn_on_yield, NULL, NULL);
 		trigger_add(&fiber()->on_yield, &txn->fiber_on_yield);
 	}
@@ -781,11 +786,12 @@ txn_on_yield(struct trigger *trigger, void *event)
 	(void) trigger;
 	(void) event;
 	struct txn *txn = in_txn();
-	assert(txn != NULL && !txn->can_yield);
+	assert(txn != NULL);
+	assert(!txn_has_flag(txn, TXN_CAN_YIELD));
 	txn_rollback_to_svp(txn, NULL);
-	if (txn->has_triggers) {
+	if (txn_has_flag(txn, TXN_HAS_TRIGGERS)) {
 		txn_run_rollback_triggers(txn);
-		txn->has_triggers = false;
+		txn_clear_flag(txn, TXN_HAS_TRIGGERS);
 	}
-	txn->is_aborted_by_yield = true;
+	txn_set_flag(txn, TXN_IS_ABORTED_BY_YIELD);
 }
diff --git a/src/box/txn.h b/src/box/txn.h
index 2d5dea21..0a5c5169 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -51,6 +51,23 @@ struct tuple;
 struct xrow_header;
 struct Vdbe;
 
+enum txn_flag {
+	/** Transaction has been processed. */
+	TXN_IS_DONE,
+	/**
+	 * Transaction has been aborted by fiber yield so
+	 * should be rolled back at commit.
+	 */
+	TXN_IS_ABORTED_BY_YIELD,
+	/**
+	 * fiber_yield() is allowed inside the transaction.
+	 * See txn_can_yield() for more details.
+	 */
+	TXN_CAN_YIELD,
+	/** on_commit and/or on_rollback list is not empty. */
+	TXN_HAS_TRIGGERS,
+};
+
 enum {
 	/**
 	 * Maximum recursion depth for on_replace triggers.
@@ -160,20 +177,8 @@ struct txn {
 	 * already assigned LSN.
 	 */
 	int n_applier_rows;
-	/* True when transaction is processed. */
-	bool is_done;
-	/**
-	 * True if the transaction was aborted so should be
-	 * rolled back at commit.
-	 */
-	bool is_aborted_by_yield;
-	/**
-	 * True if yields are allowed inside a transaction,
-	 * see txn_can_yield().
-	 */
-	bool can_yield;
-	/** True if on_commit and on_rollback lists are non-empty. */
-	bool has_triggers;
+	/** Bit mask of transaction flags, see txn_flag. */
+	unsigned flags;
 	/** The number of active nested statement-level transactions. */
 	int8_t in_sub_stmt;
 	/**
@@ -206,6 +211,24 @@ struct txn {
 	struct sql_txn *psql_txn;
 };
 
+static inline bool
+txn_has_flag(struct txn *txn, enum txn_flag flag)
+{
+	return (txn->flags & (1 << flag)) != 0;
+}
+
+static inline void
+txn_set_flag(struct txn *txn, enum txn_flag flag)
+{
+	txn->flags |= 1 << flag;
+}
+
+static inline void
+txn_clear_flag(struct txn *txn, enum txn_flag flag)
+{
+	txn->flags &= ~(1 << flag);
+}
+
 /* Pointer to the current transaction (if any) */
 static inline struct txn *
 in_txn()
@@ -260,10 +283,10 @@ txn_write(struct txn *txn);
 static inline void
 txn_init_triggers(struct txn *txn)
 {
-	if (txn->has_triggers == false) {
+	if (!txn_has_flag(txn, TXN_HAS_TRIGGERS)) {
 		rlist_create(&txn->on_commit);
 		rlist_create(&txn->on_rollback);
-		txn->has_triggers = true;
+		txn_set_flag(txn, TXN_HAS_TRIGGERS);
 	}
 }
 

Re: [tarantool-patches] Re: [PATCH] txn: convert txn flags into bit mask

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/30 11:00]:
> On Mon, Jul 29, 2019 at 06:37:54PM +0300, Konstantin Osipov wrote:
> > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/29 15:33]:
> > > On Mon, Jul 29, 2019 at 03:14:11PM +0300, Konstantin Osipov wrote:
> > > > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/29 13:56]:
> > > > > ---
> > > > > https://github.com/tarantool/tarantool/commits/dv/txn-flags
> > > > 
> > > > Curious, why not use bit fields?
> > 
> > I will read the article, I do not have a strong opinion, it's just
> > checking for bit flags using an enum looks a bit clumsy, how about 
> > a bit of syntax sugar like
> > txn_has_flag(txn, FLAG), tnx_set_flag(txn, FLAG),
> > txn_clear_flag(txn, FLAG).
> > 
> > What do you think?
> 
> Sounds reasonable. Here you go:

LGTM.


-- 
Konstantin Osipov, Moscow, Russia

Re: [tarantool-patches] Re: [PATCH] txn: convert txn flags into bit mask

[Vladimir Davydov]

On Tue, Jul 30, 2019 at 01:21:03PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/30 11:00]:
> > On Mon, Jul 29, 2019 at 06:37:54PM +0300, Konstantin Osipov wrote:
> > > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/29 15:33]:
> > > > On Mon, Jul 29, 2019 at 03:14:11PM +0300, Konstantin Osipov wrote:
> > > > > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/29 13:56]:
> > > > > > ---
> > > > > > https://github.com/tarantool/tarantool/commits/dv/txn-flags
> > > > > 
> > > > > Curious, why not use bit fields?
> > > 
> > > I will read the article, I do not have a strong opinion, it's just
> > > checking for bit flags using an enum looks a bit clumsy, how about 
> > > a bit of syntax sugar like
> > > txn_has_flag(txn, FLAG), tnx_set_flag(txn, FLAG),
> > > txn_clear_flag(txn, FLAG).
> > > 
> > > What do you think?
> > 
> > Sounds reasonable. Here you go:
> 
> LGTM.

Pushed to master.