From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <tarantool-patches-bounce@freelists.org>
Received: from localhost (localhost [127.0.0.1])
	by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 43F6B24404
	for <tarantool-patches@freelists.org>; Sun, 17 Jun 2018 16:08:11 -0400 (EDT)
Received: from turing.freelists.org ([127.0.0.1])
	by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024)
	with ESMTP id 02zYw8rQaN6D for <tarantool-patches@freelists.org>;
	Sun, 17 Jun 2018 16:08:11 -0400 (EDT)
Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [94.100.177.97])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(No client certificate requested)
	by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id F1164241F0
	for <tarantool-patches@freelists.org>; Sun, 17 Jun 2018 16:08:10 -0400 (EDT)
Subject: [tarantool-patches] Re: [PATCH] sql: remove redundant goto from VDBE
 prologue
References: <20180612182607.53450-1-korablev@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <24a1a33e-02cc-ccf4-7ff9-a155f5e18781@tarantool.org>
Date: Sun, 17 Jun 2018 23:08:08 +0300
MIME-Version: 1.0
In-Reply-To: <20180612182607.53450-1-korablev@tarantool.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: tarantool-patches-bounce@freelists.org
Errors-to: tarantool-patches-bounce@freelists.org
Reply-To: tarantool-patches@freelists.org
List-help: <mailto:ecartis@freelists.org?Subject=help>
List-unsubscribe: <tarantool-patches-request@freelists.org?Subject=unsubscribe>
List-software: Ecartis version 1.0.0
List-Id: tarantool-patches <tarantool-patches.freelists.org>
List-subscribe: <tarantool-patches-request@freelists.org?Subject=subscribe>
List-owner: <mailto:>
List-post: <mailto:tarantool-patches@freelists.org>
List-archive: <http://www.freelists.org/archives/tarantool-patches>
To: tarantool-patches@freelists.org, Nikita Pettik <korablev@tarantool.org>

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.