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

[Vladislav Shpilevoy]

Hello. Thanks for the patch! See 4 comments below.

On 12/06/2018 21:26, Nikita Pettik wrote:
> 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
> ---
> Branch: https://github.com/tarantool/tarantool/commits/np/gh-3231-remove-goto-from-vdbe-prologue
> Issue: https://github.com/tarantool/tarantool/issues/3231
> 
>   src/box/sql/build.c | 21 +++++++++++++++------
>   1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 62d687b17..66bef404c 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -94,9 +94,6 @@ sqlite3FinishCoding(Parse * pParse)

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.

>   			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.

> -
>   			/* 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.

>   	}
>   
>   	/* 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.