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 DAA3225128 for ; Mon, 18 Jun 2018 13:45:30 -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 8_wgZ66NUcpt for ; Mon, 18 Jun 2018 13:45:30 -0400 (EDT) Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (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 16860250E1 for ; Mon, 18 Jun 2018 13:45:30 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH] sql: remove redundant goto from VDBE prologue From: "n.pettik" In-Reply-To: <5b137003-b3ea-918d-4528-a1c30712abd5@tarantool.org> Date: Mon, 18 Jun 2018 20:45:27 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <27FB91D4-8386-4AE9-8A1E-B68FE60EAB6B@tarantool.org> 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> 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: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy > On 18 Jun 2018, at 14:06, Vladislav Shpilevoy = wrote: >=20 >=20 >>>=20 >>> 3. As far as I see, P2 in OP_Init is 1 already when we are here. It = is >>> not? See allocVdbe function. P2 =3D=3D 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. >=20 > 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. >=20 > (I did not check the tests). Thx for refactoring, but it wouldn=E2=80=99t 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. +++ b/src/box/sql/build.c @@ -74,11 +74,11 @@ sql_finish_coding(struct Parse *parse_context) */ assert(!parse_context->isMultiWrite || sqlite3VdbeAssertMayAbort(v, parse_context->mayAbort)); + sqlite3VdbeJumpHere(v, 0); + if (parse_context->initiateTTrans) + sqlite3VdbeAddOp0(v, OP_TTransaction); if (parse_context->pConstExpr !=3D NULL) { assert(sqlite3VdbeGetOp(v, 0)->opcode =3D=3D OP_Init); - sqlite3VdbeJumpHere(v, 0); - if (parse_context->initiateTTrans) - sqlite3VdbeAddOp0(v, OP_TTransaction); /* * Code constant expressions that where * factored out of inner loops. @@ -89,21 +89,22 @@ sql_finish_coding(struct Parse *parse_context) sqlite3ExprCode(parse_context, = exprs->a[i].pExpr, exprs->a[i].u. iConstExprReg); } - /* - * 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 ... - */ - sqlite3VdbeGoto(v, 1); - } else if (parse_context->initiateTTrans) { - 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 (parse_context->initiateTTrans || parse_context->pConstExpr = !=3D NULL) + sqlite3VdbeGoto(v, 1); + else + sqlite3VdbeChangeP2(v, 0, 1); >=20 >>>=20 >>>> } >>>> /* Get the VDBE program ready for execution >>>=20 >>> 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. >=20 > I think, that we should start test plans. At least, here it is = possible to call > EXPLAIN, test first result line only and ignore others. Then this test = will not > fail on any plan change. Ok, I added a couple of simple tests: +++ b/test/sql-tap/explain.test.lua @@ -0,0 +1,43 @@ +#!/usr/bin/env tarantool +test =3D require("sqltester") +test:plan(3) + +-- gh-3231: make sure that there is no redundant OP_Goto at the +-- start of VDBE program. In other words OP_Init jumps exactly to +-- the next opcode (i.e. opcode with address 1). +-- +test:do_execsql_test( + "explain-1.0", + [[ + CREATE TABLE t1(id INTEGER PRIMARY KEY, a INT); + INSERT INTO t1 VALUES(1, 2), (3, 4), (5, 6); + SELECT * FROM t1; + ]], { + -- + 1, 2, 3, 4, 5, 6 + -- + }) + +test:do_test( + "explain-1.1", + function() + opcodes =3D test:execsql("EXPLAIN SELECT * FROM t1;") + return opcodes[1] + end, + -- + 0, 'Init', 0, 1, 0, '', '00', 'Start at 1' + -- + ) + +test:do_test( + "explain-1.2", + function() + opcodes =3D test:execsql("EXPLAIN SELECT a + 1 FROM t1 WHERE id = =3D 4 OR id =3D 5;") + return opcodes[1] + end, + -- + 0, 'Init', 0, 1, 0, '', '00', 'Start at 1' + -- + ) + +test:finish_test()