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 008992636E for ; Wed, 20 Feb 2019 08:43:02 -0500 (EST) 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 M9-s5t_81inQ for ; Wed, 20 Feb 2019 08:43:01 -0500 (EST) Received: from smtp15.mail.ru (smtp15.mail.ru [94.100.176.133]) (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 AC8AF24278 for ; Wed, 20 Feb 2019 08:43:01 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v1 3/4] sql: got rid of redundant MASKBIT32 definition References: <4330bedcb868e7f44185f1179bb5e9c2e46a78c5.1549629707.git.kshcherbatov@tarantool.org> <82b74351-95c9-9ad3-7371-4c796eac1a58@tarantool.org> From: Kirill Shcherbatov Message-ID: Date: Wed, 20 Feb 2019 16:42:59 +0300 MIME-Version: 1.0 In-Reply-To: <82b74351-95c9-9ad3-7371-4c796eac1a58@tarantool.org> Content-Type: text/plain; charset=utf-8 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: 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, Vladislav Shpilevoy > Thanks for the patch! See 4 comments below. > > 1. We do not use the past in commit titles. Please, use 'get', > not 'got'. In the next commit too. Ok, done. > 2. In the previous commit you said, that 32bit mask is already > replaced with 64bit one where possible. But why was not it > possible to replace this one? The answer is in previous commit description: " The 32b bitmasks are still used to specify constant OP_Function arguments because this change would require changing the P1 type of the VDBE p1 argument, which is not desirable. Moreover, the 64 function's arguments is an explicit overkill. " >> for (i = 0; i < nFarg; i++) { >> - if (i < 32 >> - && sqlite3ExprIsConstant(pFarg->a[i]. >> - pExpr)) { >> - testcase(i == 31); >> - constMask |= MASKBIT32(i); >> + if (i < 32 && > > 3. Why 32? The column mask is smart, it has no size restrictions. All function arguments that are not able to fit in mask to be marked as constant, assumed to by dynamic. There is nice commit before sqlVdbeDeleteAuxData routine definition. i < 32 must be kept to avoid calculation sqlite3ExprIsConstant where unnecessary. In fact such 32bit column mask is not smart, but it is not matter. Smart mask also works fine in this scenario. > >> + sqlite3ExprIsConstant(pFarg->a[i].pExpr)) { >> + column_mask_set_fieldno((uint64_t *) >> + &const_mask, i); > > 4. column_mask_set_fieldo does *mask = ... . It means, that it rewrites 8 bytes > from the specified address. But you passed a value of only 4 bytes, so the next > 4 bytes of foreign memory are polluted. You are right. It is not problem anymore. ============================================= MASK BIT 32 macro is now redundant, so we got rid of it. Also refactored related code to use core bitmasks helpers Part of #3571 --- src/box/sql/expr.c | 24 +++++++++++++----------- src/box/sql/sqlInt.h | 1 - src/box/sql/vdbeaux.c | 8 +++----- 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 35145cef6..a72f986c6 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -3914,7 +3914,11 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) int nFarg; /* Number of function arguments */ FuncDef *pDef; /* The function definition object */ const char *zId; /* The function name */ - u32 constMask = 0; /* Mask of function arguments that are constant */ + /* + * Mask of function arguments that are + * constant. + */ + uint32_t const_mask = 0; int i; /* Loop counter */ sql *db = pParse->db; /* The database connection */ struct coll *coll = NULL; @@ -3988,11 +3992,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) } for (i = 0; i < nFarg; i++) { - if (i < 32 - && sqlExprIsConstant(pFarg->a[i]. - pExpr)) { - testcase(i == 31); - constMask |= MASKBIT32(i); + if (i < 32 && + sqlExprIsConstant(pFarg->a[i].pExpr)) { + column_mask32_set_fieldno(&const_mask, + i); } if ((pDef->funcFlags & SQL_FUNC_NEEDCOLL) != 0 && coll == NULL) { @@ -4004,7 +4007,7 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) } } if (pFarg) { - if (constMask) { + if (const_mask != 0) { r1 = pParse->nMem + 1; pParse->nMem += nFarg; } else { @@ -4052,12 +4055,11 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) sqlVdbeAddOp4(v, OP_CollSeq, 0, 0, 0, (char *)coll, P4_COLLSEQ); } - sqlVdbeAddOp4(v, OP_Function0, constMask, r1, - target, (char *)pDef, P4_FUNCDEF); + sqlVdbeAddOp4(v, OP_Function0, const_mask, r1, target, + (char *)pDef, P4_FUNCDEF); sqlVdbeChangeP5(v, (u8) nFarg); - if (nFarg && constMask == 0) { + if (nFarg && const_mask == 0) sqlReleaseTempRange(pParse, r1, nFarg); - } return target; } case TK_EXISTS: diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 5be695e51..a9b7dfa4e 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -2255,7 +2255,6 @@ typedef u64 Bitmask; * A bit in a Bitmask */ #define MASKBIT(n) (((Bitmask)1)<<(n)) -#define MASKBIT32(n) (((unsigned int)1)<<(n)) #define ALLBITS ((Bitmask)-1) /* diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index b831b52ad..fedbc0a15 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -2698,11 +2698,9 @@ (sql * db, AuxData ** pp, int iOp, int mask) { while (*pp) { AuxData *pAux = *pp; - if ((iOp < 0) - || (pAux->iOp == iOp - && (pAux->iArg > 31 || !(mask & MASKBIT32(pAux->iArg)))) - ) { - testcase(pAux->iArg == 31); + if (iOp < 0 || (pAux->iOp == iOp && + (pAux->iArg >= 32 || + !column_mask32_fieldno_is_set(mask, pAux->iArg)))) { if (pAux->xDelete) { pAux->xDelete(pAux->pAux); } -- 2.20.1