From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 78D5625761 for ; Mon, 18 Jun 2018 14:57:32 -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 zth_lvNhpqKx for ; Mon, 18 Jun 2018 14:57:32 -0400 (EDT) Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (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 2F2B52574F for ; Mon, 18 Jun 2018 14:57:32 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] sql: remove redundant goto from VDBE prologue References: <20180612182607.53450-1-korablev@tarantool.org> <24a1a33e-02cc-ccf4-7ff9-a155f5e18781@tarantool.org> <28322F56-E52D-42C7-AE52-188D6F50AF63@tarantool.org> <5b137003-b3ea-918d-4528-a1c30712abd5@tarantool.org> <27FB91D4-8386-4AE9-8A1E-B68FE60EAB6B@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Mon, 18 Jun 2018 21:57:29 +0300 MIME-Version: 1.0 In-Reply-To: <27FB91D4-8386-4AE9-8A1E-B68FE60EAB6B@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: "n.pettik" , tarantool-patches@freelists.org Thanks for the fixes! On 18/06/2018 20:45, n.pettik wrote: > >> On 18 Jun 2018, at 14:06, Vladislav Shpilevoy 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.