[tarantool-patches] Re: [PATCH] sql: remove redundant goto from VDBE prologue

["n.pettik" <korablev@tarantool.org>]

> 1. This function looks small. Lets refactor it to Tarantool
> code style. If we don't, I am afraid this commit will be lost
> in 'git blame', when this function finally will be refactored
> completely.
> 

Ok. See diff at the end of letter.

>>  			sqlite3VdbeJumpHere(v, 0);
>>  			if (pParse->initiateTTrans)
>>  				sqlite3VdbeAddOp0(v, OP_TTransaction);
>> -			if (db->init.busy == 0)
>> -				sqlite3VdbeChangeP5(v, 1);
> 
> 2. Why is it removed? I do not see this code moved below.

Actually, this code looks strange. If OP_TTransaction is added,
then setting fifth param for it makes no sense (this opcode takes
no args). If this change is related to OP_Halt, then it is also useless,
since the last OP_Halt must be with signature 0 0 0 (comment from vdbe.c):

* There is an implied "Halt 0 0 0" instruction inserted at the very end of
* every program.  So a jump past the last instruction of the program
* is the same as executing Halt.

That is why I removed this code.

> 
>> -
>>  			/* Code constant expressions that where factored out of inner loops */
>>  			if (pParse->pConstExpr) {
>>  				ExprList *pEL = pParse->pConstExpr;
>> @@ -107,10 +104,22 @@ sqlite3FinishCoding(Parse * pParse)
>>  							iConstExprReg);
>>  				}
>>  			}
>> -
>> -			/* Finally, jump back to the beginning of the executable code. */
>> -			sqlite3VdbeGoto(v, 1);
>>  		}
>> +		/*
>> +		 * Finally, jump back to the beginning of
>> +		 * the executable code. In fact, it is required
>> +		 * only if some additional opcodes are generated.
>> +		 * Otherwise, it would be useless jump:
>> +		 *
>> +		 * 0:        OP_Init 0 vdbe_end ...
>> +		 * 1: ...
>> +		 *    ...
>> +		 * vdbe_end: OP_Goto 0 1 ...
>> +		 */
>> +		if (pParse->pConstExpr != NULL || pParse->initiateTTrans)
>> +			sqlite3VdbeGoto(v, 1);
>> +		else
>> +			sqlite3VdbeChangeP2(v, 0, 1);
> 
> 3. As far as I see, P2 in OP_Init is 1 already when we are here. It is
> not? See allocVdbe function. P2 == 1 by default, and here it can be changed to
> goto to ttrans.

In fact, it is changed by sqlite3VdbeJumpHere(v, 0); 
Thus, we have to again set its value to 1, in case of omitting jump.

> 
>>  	}
>>    	/* Get the VDBE program ready for execution
> 
> 4. Can we test the new VDBE plan using EXPLAIN? I am wondering why all
> plan changes are not tested using EXPLAIN.

Well, it it is quite complicated to test EXPLAIN command since any change to query
planner or code generator at all would result in rewriting such tests. Hence, once we
decided to avoid using tests with EXPLAIN until we get stable code generation.

=======================================================================

Subject: [PATCH] sql: remove redundant goto from VDBE prologue

Structure of VDBE prologue:

    0: OP_Init 0 N (address to start) 0 --|
|-> 1: ...                                |
|      ...                                |
|   N: OP_Transaction         <------------
|   N+1: (Constant expressions to be saved in registers)
|      ...
|-- M: OP_Goto 0 1 0

However, last opcode in VDBE program (i.e. OP_Goto) is generated always,
despite the existence of OP_Transaction or constant expressions.
Thus, VDBE program for queries like <SELECT * FROM table;> features
redundant jump. Such useless jump wastes exectuion time (although it is
executed once) and may affect jump prediction.

This patch adds conditional branch for generating jump opcode finishing
VDBE program: it is appended only if we need to start transaction or
code constrant expressions.

Closes #3231
---
 src/box/sql/build.c     | 105 +++++++++++++++++++++++-------------------------
 src/box/sql/parse.y     |   2 +-
 src/box/sql/sqliteInt.h |  17 +++++++-
 3 files changed, 68 insertions(+), 56 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 62d687b17..1c972c977 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -54,76 +54,73 @@
 #include "box/tuple_format.h"
 #include "box/coll_id_cache.h"
 
-/*
- * This routine is called after a single SQL statement has been
- * parsed and a VDBE program to execute that statement has been
- * prepared.  This routine puts the finishing touches on the
- * VDBE program and resets the pParse structure for the next
- * parse.
- *
- * Note that if an error occurred, it might be the case that
- * no VDBE code was generated.
- */
 void
-sqlite3FinishCoding(Parse * pParse)
+sql_finish_coding(struct Parse *parse_context)
 {
-	sqlite3 *db;
-	Vdbe *v;
-
-	assert(pParse->pToplevel == 0);
-	db = pParse->db;
-	if (pParse->nested)
+	if (parse_context->nested)
 		return;
-	if (db->mallocFailed || pParse->nErr) {
-		if (pParse->rc == SQLITE_OK)
-			pParse->rc = SQLITE_ERROR;
+	assert(parse_context->pToplevel == NULL);
+	struct sqlite3 *db = parse_context->db;
+	if (db->mallocFailed || parse_context->nErr != 0) {
+		if (parse_context->rc == SQLITE_OK)
+			parse_context->rc = SQLITE_ERROR;
 		return;
 	}
-
-	/* Begin by generating some termination code at the end of the
-	 * vdbe program
+	/*
+	 * Begin by generating some termination code at the end
+	 * of the vdbe program
 	 */
-	v = sqlite3GetVdbe(pParse);
-	assert(!pParse->isMultiWrite
-	       || sqlite3VdbeAssertMayAbort(v, pParse->mayAbort));
-	if (v) {
+	struct Vdbe *v = sqlite3GetVdbe(parse_context);
+	assert(!parse_context->isMultiWrite ||
+	       sqlite3VdbeAssertMayAbort(v, parse_context->mayAbort));
+	if (v != NULL) {
 		sqlite3VdbeAddOp0(v, OP_Halt);
-		if (db->mallocFailed == 0 || pParse->pConstExpr) {
-			int i;
+		if (db->mallocFailed == 0 ||
+		    parse_context->pConstExpr != NULL) {
 			assert(sqlite3VdbeGetOp(v, 0)->opcode == OP_Init);
 			sqlite3VdbeJumpHere(v, 0);
-			if (pParse->initiateTTrans)
+			if (parse_context->initiateTTrans)
 				sqlite3VdbeAddOp0(v, OP_TTransaction);
-			if (db->init.busy == 0)
-				sqlite3VdbeChangeP5(v, 1);
-
-			/* Code constant expressions that where factored out of inner loops */
-			if (pParse->pConstExpr) {
-				ExprList *pEL = pParse->pConstExpr;
-				pParse->okConstFactor = 0;
-				for (i = 0; i < pEL->nExpr; i++) {
-					sqlite3ExprCode(pParse, pEL->a[i].pExpr,
-							pEL->a[i].u.
+			/*
+			 * Code constant expressions that where
+			 * factored out of inner loops.
+			 */
+			if (parse_context->pConstExpr != NULL) {
+				struct ExprList *exprs =
+					parse_context->pConstExpr;
+				parse_context->okConstFactor = 0;
+				for (int i = 0; i < exprs->nExpr; ++i) {
+					sqlite3ExprCode(parse_context,
+							exprs->a[i].pExpr,
+							exprs->a[i].u.
 							iConstExprReg);
 				}
 			}
-
-			/* Finally, jump back to the beginning of the executable code. */
-			sqlite3VdbeGoto(v, 1);
 		}
-	}
-
-	/* Get the VDBE program ready for execution
-	 */
-	if (v && pParse->nErr == 0 && !db->mallocFailed) {
-		assert(pParse->iCacheLevel == 0);	/* Disables and re-enables match */
-		/* A minimum of one cursor is required if autoincrement is used
-		 *  See ticket [a696379c1f08866]
+		/*
+		 * Finally, jump back to the beginning of
+		 * the executable code. In fact, it is required
+		 * only if some additional opcodes are generated.
+		 * Otherwise, it would be useless jump:
+		 *
+		 * 0:        OP_Init 0 vdbe_end ...
+		 * 1: ...
+		 *    ...
+		 * vdbe_end: OP_Goto 0 1 ...
 		 */
-		sqlite3VdbeMakeReady(v, pParse);
-		pParse->rc = SQLITE_DONE;
+		if (parse_context->pConstExpr != NULL ||
+		    parse_context->initiateTTrans)
+			sqlite3VdbeGoto(v, 1);
+		else
+			sqlite3VdbeChangeP2(v, 0, 1);
+	}
+	/* Get the VDBE program ready for execution. */
+	if (v != NULL && parse_context->nErr == 0 && !db->mallocFailed) {
+		assert(parse_context->iCacheLevel == 0);
+		sqlite3VdbeMakeReady(v, parse_context);
+		parse_context->rc = SQLITE_DONE;
 	} else {
-		pParse->rc = SQLITE_ERROR;
+		parse_context->rc = SQLITE_ERROR;
 	}
 }
 
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 2e5f349c8..58f54e05f 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -110,7 +110,7 @@ static void disableLookaside(Parse *pParse){
 input ::= ecmd.
 ecmd ::= explain cmdx SEMI. {
 	if (!pParse->parse_only)
-		sqlite3FinishCoding(pParse);
+		sql_finish_coding(pParse);
 }
 ecmd ::= SEMI. {
   sqlite3ErrorMsg(pParse, "syntax error: empty request");
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 01351a183..4d2bcafbc 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3442,7 +3442,22 @@ void sqlite3NormalizeName(char *z);
 void sqlite3TokenInit(Token *, char *);
 int sqlite3KeywordCode(const unsigned char *, int);
 int sqlite3RunParser(Parse *, const char *, char **);
-void sqlite3FinishCoding(Parse *);
+
+/**
+ * This routine is called after a single SQL statement has been
+ * parsed and a VDBE program to execute that statement has been
+ * prepared.  This routine puts the finishing touches on the
+ * VDBE program and resets the pParse structure for the next
+ * parse.
+ *
+ * Note that if an error occurred, it might be the case that
+ * no VDBE code was generated.
+ *
+ * @param parse_context Current parsing context.
+ */
+void
+sql_finish_coding(struct Parse *parse_context);
+
 int sqlite3GetTempReg(Parse *);
 void sqlite3ReleaseTempReg(Parse *, int);
 int sqlite3GetTempRange(Parse *, int);
-- 
2.15.1