[tarantool-patches] Re: [PATCH] sql: remove redundant goto from VDBE prologue
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Jun 18 21:57:29 MSK 2018
Thanks for the fixes!
On 18/06/2018 20:45, n.pettik wrote:
>
>> On 18 Jun 2018, at 14:06, Vladislav Shpilevoy <v.shpilevoy at 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.
More information about the Tarantool-patches
mailing list