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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the fixes!

On 18/06/2018 20:45, n.pettik wrote:
> 
>> On 18 Jun 2018, at 14:06, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>
>>
>>>>
>>>> 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.
>>
>> It is done in the same function few lines above. How about
>> to do not do this jump + not jump? I have slightly refactored the
>> code to do not this jump. Please, see the separate commit on
>> the branch.
>>
>> (I did not check the tests).
> 
> Thx for refactoring, but it wouldn’t work this way:
> We must firstly make lable to jump (sqlite3VdbeJumpHere(v, 0);),
> then test on emitting OP_TTransaction and const exprs (or both). If we need to
> add such opcodes, then we should jump back. Otherwise, dismiss initial jump.

We do not need to change P2 until it is really needed. That is what I
mean.

Please, consider this diff:

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a7b9d20b3..3a361cfad 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -74,7 +74,7 @@ sql_finish_coding(struct Parse *parse_context)
  	 */
  	assert(!parse_context->isMultiWrite ||
  	       sqlite3VdbeAssertMayAbort(v, parse_context->mayAbort));
-	sqlite3VdbeJumpHere(v, 0);
+	int last_instruction = v->nOp;
  	if (parse_context->initiateTTrans)
  		sqlite3VdbeAddOp0(v, OP_TTransaction);
  	if (parse_context->pConstExpr != NULL) {
@@ -101,10 +101,10 @@ sql_finish_coding(struct Parse *parse_context)
  	 *    ...
  	 * vdbe_end: OP_Goto 0 1 ...
  	 */
-	if (parse_context->initiateTTrans || parse_context->pConstExpr != NULL)
+	if (parse_context->initiateTTrans || parse_context->pConstExpr != NULL) {
+		sqlite3VdbeChangeP2(v, 0, last_instruction);
  		sqlite3VdbeGoto(v, 1);
-	else
-		sqlite3VdbeChangeP2(v, 0, 1);
+	}
  	/* Get the VDBE program ready for execution. */
  	if (parse_context->nErr == 0 && !db->mallocFailed) {
  		assert(parse_context->iCacheLevel == 0);

The tests pass ok.