[tarantool-patches] [PATCH v2 1/1] sql: use 64b bitmasks instead of 32b where possible

[tarantool-patches] [PATCH v2 1/1] sql: use 64b bitmasks instead of 32b where possible

[Kirill Shcherbatov]

In some cases(like foreign keys) the SQL code used
32-bit bit mask, while 64-bit bit masks will perform better
column optimizations. There was refactored code to work with 64b
bitmasks where required.
The 32b bitmasks are still used to specify constant OP_Function
arguments because this change would require changing the P1 type
of the VDBE p1 argument, which is not desirable. Moreover, the
64 function's arguments is an explicit overkill.

The ticket was created in connection with the introduction of
foreign keys and their use of 32-bit bit masks. In the rest of
the scripts in SQL already use 64 bit masks, and the "smart" bit
of the mask is not applicable.

Closes #3571

Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3571-sql-column-mask-64
Issue: https://github.com/tarantool/tarantool/issues/3571
---
 src/box/alter.cc      | 10 ++--------
 src/box/column_mask.h | 14 +++++++++++++
 src/box/sql/delete.c  |  7 ++-----
 src/box/sql/resolve.c | 19 ++++++------------
 src/box/sql/sqlInt.h  | 46 ++++++++++++++++++++++++-------------------
 src/box/sql/trigger.c | 14 ++++++-------
 src/box/sql/update.c  | 24 +++++++++-------------
 7 files changed, 66 insertions(+), 68 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 080a72b9f..0a9ec308b 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -29,6 +29,7 @@
  * SUCH DAMAGE.
  */
 #include "alter.h"
+#include "column_mask.h"
 #include "schema.h"
 #include "user.h"
 #include "space.h"
@@ -3731,13 +3732,6 @@ fk_constraint_remove(struct rlist *child_fk_list, const char *fk_name)
 	return NULL;
 }
 
-/**
- * FIXME: as sql legacy temporary we use such mask throughout
- * SQL code. It should be replaced later with regular
- * mask from column_mask.h
- */
-#define FKEY_MASK(x) (((x)>31) ? 0xffffffff : ((uint64_t)1<<(x)))
-
 /**
  * Set bits of @mask which correspond to fields involved in
  * given foreign key constraint.
@@ -3751,7 +3745,7 @@ static inline void
 fk_constraint_set_mask(const struct fk_constraint *fk, uint64_t *mask, int type)
 {
 	for (uint32_t i = 0; i < fk->def->field_count; ++i)
-		*mask |= FKEY_MASK(fk->def->links[i].fields[type]);
+		column_mask_set_fieldno(mask, fk->def->links[i].fields[type]);
 }
 
 /**
diff --git a/src/box/column_mask.h b/src/box/column_mask.h
index d71911d46..14da841f1 100644
--- a/src/box/column_mask.h
+++ b/src/box/column_mask.h
@@ -109,4 +109,18 @@ key_update_can_be_skipped(uint64_t key_mask, uint64_t update_mask)
 	return (key_mask & update_mask) == 0;
 }
 
+/**
+ * Test a bit in the bitmask corresponding to a column fieldno.
+ * @param column_mask Mask to test.
+ * @param fieldno Fieldno number to test (index base must be 0).
+ * @retval true If bit corresponding to a column fieldno.
+ * @retval false if bit is not set or fieldno > 64.
+ */
+static inline bool
+column_mask_fieldno_is_set(uint64_t column_mask, uint32_t fieldno)
+{
+	uint64_t mask = (uint64_t) 1 << (fieldno < 63 ? fieldno : 63);
+	return (column_mask & mask) != 0;
+}
+
 #endif
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index f4d0334f4..aff1149ae 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -463,7 +463,7 @@ sql_generate_row_delete(struct Parse *parse, struct space *space,
 	   (fk_constraint_is_required(space, NULL) || trigger_list != NULL)) {
 		/* Mask of OLD.* columns in use */
 		/* TODO: Could use temporary registers here. */
-		uint32_t mask =
+		uint64_t mask =
 			sql_trigger_colmask(parse, trigger_list, 0, 0,
 					    TRIGGER_BEFORE | TRIGGER_AFTER,
 					    space, onconf);
@@ -478,10 +478,7 @@ sql_generate_row_delete(struct Parse *parse, struct space *space,
 		 */
 		sqlVdbeAddOp2(v, OP_Copy, reg_pk, first_old_reg);
 		for (int i = 0; i < (int)space->def->field_count; i++) {
-			testcase(mask != 0xffffffff && iCol == 31);
-			testcase(mask != 0xffffffff && iCol == 32);
-			if (mask == 0xffffffff
-			    || (i <= 31 && (mask & MASKBIT32(i)) != 0)) {
+			if (column_mask_fieldno_is_set(mask, i)) {
 				sqlExprCodeGetColumnOfTable(v, space->def,
 								cursor, i,
 								first_old_reg +
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 94bb0affc..5a55a5d47 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -340,20 +340,13 @@ lookupName(Parse * pParse,	/* The parsing context */
 					if (iCol < 0) {
 						pExpr->type =
 							FIELD_TYPE_INTEGER;
-					} else if (pExpr->iTable == 0) {
-						testcase(iCol == 31);
-						testcase(iCol == 32);
-						pParse->oldmask |=
-						    (iCol >=
-						     32 ? 0xffffffff
-						     : (((u32) 1) << iCol));
 					} else {
-						testcase(iCol == 31);
-						testcase(iCol == 32);
-						pParse->newmask |=
-						    (iCol >=
-						     32 ? 0xffffffff
-						     : (((u32) 1) << iCol));
+						uint64_t *mask =
+							pExpr->iTable == 0 ?
+							&pParse->oldmask :
+							&pParse->newmask;
+						column_mask_set_fieldno(mask,
+									iCol);
 					}
 					pExpr->iColumn = (i16) iCol;
 					pExpr->space_def = space_def;
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 8967ea3e0..a48cd40a8 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -67,6 +67,7 @@
 
 #include <stdbool.h>
 
+#include "box/column_mask.h"
 #include "box/field_def.h"
 #include "box/sql.h"
 #include "box/txn.h"
@@ -2578,22 +2579,24 @@ struct SelectDest {
 #endif
 
 /*
- * At least one instance of the following structure is created for each
- * trigger that may be fired while parsing an INSERT, UPDATE or DELETE
- * statement. All such objects are stored in the linked list headed at
- * Parse.pTriggerPrg and deleted once statement compilation has been
- * completed.
- *
- * A Vdbe sub-program that implements the body and WHEN clause of trigger
- * TriggerPrg.pTrigger, assuming a default ON CONFLICT clause of
- * TriggerPrg.orconf, is stored in the TriggerPrg.pProgram variable.
- * The Parse.pTriggerPrg list never contains two entries with the same
- * values for both pTrigger and orconf.
- *
- * The TriggerPrg.aColmask[0] variable is set to a mask of old.* columns
- * accessed (or set to 0 for triggers fired as a result of INSERT
- * statements). Similarly, the TriggerPrg.aColmask[1] variable is set to
- * a mask of new.* columns used by the program.
+ * At least one instance of the following structure is created for
+ * each trigger that may be fired while parsing an INSERT, UPDATE
+ * or DELETE statement. All such objects are stored in the linked
+ * list headed at Parse.pTriggerPrg and deleted once statement
+ * compilation has been completed.
+ *
+ * A Vdbe sub-program that implements the body and WHEN clause of
+ * trigger TriggerPrg.pTrigger, assuming a default ON CONFLICT
+ * clause of TriggerPrg.orconf, is stored in the
+ * TriggerPrg.pProgram variable. The Parse.pTriggerPrg list never
+ * contains two entries with the same values for both pTrigger
+ * and orconf.
+ *
+ * The TriggerPrg.column_mask[0] variable is set to a mask of
+ * old.* columns accessed (or set to 0 for triggers fired as a
+ * result of INSERT statements). Similarly, the
+ * TriggerPrg.column_mask[1] variable is set to a mask of new.*
+ * columns used by the program.
  */
 struct TriggerPrg {
 	/** Trigger this program was coded from. */
@@ -2601,7 +2604,8 @@ struct TriggerPrg {
 	TriggerPrg *pNext;	/* Next entry in Parse.pTriggerPrg list */
 	SubProgram *pProgram;	/* Program implementing pTrigger/orconf */
 	int orconf;		/* Default ON CONFLICT policy */
-	u32 aColmask[2];	/* Masks of old.*, new.* columns accessed */
+	/* Masks of old.*, new.* columns accessed. */
+	uint64_t column_mask[2];
 };
 
 enum ast_type {
@@ -2680,8 +2684,10 @@ struct Parse {
 	int nSelectIndent;	/* How far to indent SELECTTRACE() output */
 	Parse *pToplevel;	/* Parse structure for main program (or NULL) */
 	u32 nQueryLoop;		/* Est number of iterations of a query (10*log2(N)) */
-	u32 oldmask;		/* Mask of old.* columns referenced */
-	u32 newmask;		/* Mask of new.* columns referenced */
+	/* Mask of old.* columns referenced. */
+	uint64_t oldmask;
+	/* Mask of new.* columns referenced. */
+	uint64_t newmask;
 	u8 eTriggerOp;		/* TK_UPDATE, TK_INSERT or TK_DELETE */
 	u8 eOrconf;		/* Default ON CONFLICT policy for trigger steps */
 	/** Region to make SQL temp allocations. */
@@ -4070,7 +4076,7 @@ TriggerStep *sqlTriggerDeleteStep(sql *, Token *, Expr *);
  *
  * @retval mask value.
  */
-u32
+uint64_t
 sql_trigger_colmask(Parse *parser, struct sql_trigger *trigger,
 		    ExprList *changes_list, int new, int tr_tm,
 		    struct space *space, int orconf);
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 7eacd33d4..e46edb3ea 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -784,8 +784,8 @@ sql_row_trigger_program(struct Parse *parser, struct sql_trigger *trigger,
 	sqlVdbeLinkSubProgram(pTop->pVdbe, pProgram);
 	pPrg->trigger = trigger;
 	pPrg->orconf = orconf;
-	pPrg->aColmask[0] = 0xffffffff;
-	pPrg->aColmask[1] = 0xffffffff;
+	pPrg->column_mask[0] = COLUMN_MASK_FULL;
+	pPrg->column_mask[1] = COLUMN_MASK_FULL;
 
 	/*
 	 * Allocate and populate a new Parse context to use for
@@ -857,8 +857,8 @@ sql_row_trigger_program(struct Parse *parser, struct sql_trigger *trigger,
 		pProgram->nMem = pSubParse->nMem;
 		pProgram->nCsr = pSubParse->nTab;
 		pProgram->token = (void *)trigger;
-		pPrg->aColmask[0] = pSubParse->oldmask;
-		pPrg->aColmask[1] = pSubParse->newmask;
+		pPrg->column_mask[0] = pSubParse->oldmask;
+		pPrg->column_mask[1] = pSubParse->newmask;
 		sqlVdbeDelete(v);
 	}
 
@@ -975,13 +975,13 @@ vdbe_code_row_trigger(struct Parse *parser, struct sql_trigger *trigger,
 	}
 }
 
-u32
+uint64_t
 sql_trigger_colmask(Parse *parser, struct sql_trigger *trigger,
 		    ExprList *changes_list, int new, int tr_tm,
 		    struct space *space, int orconf)
 {
 	const int op = changes_list != NULL ? TK_UPDATE : TK_DELETE;
-	u32 mask = 0;
+	uint64_t mask = 0;
 
 	assert(new == 1 || new == 0);
 	for (struct sql_trigger *p = trigger; p != NULL; p = p->next) {
@@ -990,7 +990,7 @@ sql_trigger_colmask(Parse *parser, struct sql_trigger *trigger,
 			TriggerPrg *prg =
 				sql_row_trigger(parser, p, space, orconf);
 			if (prg != NULL)
-				mask |= prg->aColmask[new];
+				mask |= prg->column_mask[new];
 		}
 	}
 
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 05ceeb411..9318429b7 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -100,7 +100,6 @@ sqlUpdate(Parse * pParse,		/* The parser context */
 	/* List of triggers on pTab, if required. */
 	struct sql_trigger *trigger;
 	int tmask;		/* Mask of TRIGGER_BEFORE|TRIGGER_AFTER */
-	int newmask;		/* Mask of NEW.* columns accessed by BEFORE triggers */
 	int iEph = 0;		/* Ephemeral table holding all primary key values */
 	int nKey = 0;		/* Number of elements in regKey */
 	int aiCurOnePass[2];	/* The write cursors opened by WHERE_ONEPASS */
@@ -332,18 +331,15 @@ sqlUpdate(Parse * pParse,		/* The parser context */
 	 */
 	if (is_pk_modified || hasFK != 0 || trigger != NULL) {
 		assert(space != NULL);
-		u32 oldmask = hasFK ? space->fk_constraint_mask : 0;
+		uint64_t oldmask = hasFK ? space->fk_constraint_mask : 0;
 		oldmask |= sql_trigger_colmask(pParse, trigger, pChanges, 0,
 					       TRIGGER_BEFORE | TRIGGER_AFTER,
 					       space, on_error);
 		for (i = 0; i < (int)def->field_count; i++) {
-			if (oldmask == 0xffffffff
-			    || (i < 32 && (oldmask & MASKBIT32(i)) != 0) ||
-				sql_space_column_is_in_pk(space, i)) {
-				testcase(oldmask != 0xffffffff && i == 31);
-				sqlExprCodeGetColumnOfTable(v, def,
-								pk_cursor, i,
-								regOld + i);
+			if (column_mask_fieldno_is_set(oldmask, i) ||
+			    sql_space_column_is_in_pk(space, i)) {
+				sqlExprCodeGetColumnOfTable(v, def, pk_cursor,
+							    i, regOld + i);
 			} else {
 				sqlVdbeAddOp2(v, OP_Null, 0, regOld + i);
 			}
@@ -364,22 +360,20 @@ sqlUpdate(Parse * pParse,		/* The parser context */
 	 * may have modified them). So not loading those that are not going to
 	 * be used eliminates some redundant opcodes.
 	 */
-	newmask = sql_trigger_colmask(pParse, trigger, pChanges, 1,
-				      TRIGGER_BEFORE, space, on_error);
+	uint64_t newmask = sql_trigger_colmask(pParse, trigger, pChanges, 1,
+					       TRIGGER_BEFORE, space, on_error);
 	for (i = 0; i < (int)def->field_count; i++) {
 		j = aXRef[i];
 		if (j >= 0) {
 			sqlExprCode(pParse, pChanges->a[j].pExpr,
 					regNew + i);
-		} else if (0 == (tmask & TRIGGER_BEFORE) || i > 31
-			   || (newmask & MASKBIT32(i))) {
+		} else if ((tmask & TRIGGER_BEFORE) == 0 ||
+			   column_mask_fieldno_is_set(newmask, i) != 0) {
 			/* This branch loads the value of a column that will not be changed
 			 * into a register. This is done if there are no BEFORE triggers, or
 			 * if there are one or more BEFORE triggers that use this value via
 			 * a new.* reference in a trigger program.
 			 */
-			testcase(i == 31);
-			testcase(i == 32);
 			sqlExprCodeGetColumnToReg(pParse, def, i,
 						      pk_cursor, regNew + i);
 		} else {
-- 
2.21.0

[tarantool-patches] Re: [PATCH v2 1/1] sql: use 64b bitmasks instead of 32b where possible

[Vladislav Shpilevoy]

On 20/03/2019 15:36, Kirill Shcherbatov wrote:
> In some cases(like foreign keys) the SQL code used
> 32-bit bit mask, while 64-bit bit masks will perform better
> column optimizations. There was refactored code to work with 64b
> bitmasks where required.
> The 32b bitmasks are still used to specify constant OP_Function
> arguments because this change would require changing the P1 type
> of the VDBE p1 argument, which is not desirable. Moreover, the
> 64 function's arguments is an explicit overkill.
> 
> The ticket was created in connection with the introduction of
> foreign keys and their use of 32-bit bit masks. In the rest of
> the scripts in SQL already use 64 bit masks, and the "smart" bit
> of the mask is not applicable.
> 
> Closes #3571
> 
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3571-sql-column-mask-64
> Issue: https://github.com/tarantool/tarantool/issues/3571
> ---

Please, put branch and issue links below that mark '---'. It
is not a cavil or 'red tape'. It really affects things. For example,
when somebody applies a patch from email, git can take these links
as a part of commit message.

LGTM.

[tarantool-patches] Re: [PATCH v2 1/1] sql: use 64b bitmasks instead of 32b where possible

[Kirill Yukhin]

Hello,

On 20 Mar 15:36, Kirill Shcherbatov wrote:
> In some cases(like foreign keys) the SQL code used
> 32-bit bit mask, while 64-bit bit masks will perform better
> column optimizations. There was refactored code to work with 64b
> bitmasks where required.
> The 32b bitmasks are still used to specify constant OP_Function
> arguments because this change would require changing the P1 type
> of the VDBE p1 argument, which is not desirable. Moreover, the
> 64 function's arguments is an explicit overkill.
> 
> The ticket was created in connection with the introduction of
> foreign keys and their use of 32-bit bit masks. In the rest of
> the scripts in SQL already use 64 bit masks, and the "smart" bit
> of the mask is not applicable.
> 
> Closes #3571
> 
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3571-sql-column-mask-64
> Issue: https://github.com/tarantool/tarantool/issues/3571

I've checked your patch into master branch.

--
Regards, Kirill Yukhin