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

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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);
 	}
 }