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 A749826C90 for ; Wed, 20 Feb 2019 08:42:59 -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 zsGeOBpk2ZDo for ; Wed, 20 Feb 2019 08:42:59 -0500 (EST) Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (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 3CB7524278 for ; Wed, 20 Feb 2019 08:42:59 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v1 2/4] sql: use 64b bitmasks instead of 32b where possible References: <26ee3cd45826f7de3290d14b37524d181ef320af.1549629707.git.kshcherbatov@tarantool.org> From: Kirill Shcherbatov Message-ID: Date: Wed, 20 Feb 2019 16:42:57 +0300 MIME-Version: 1.0 In-Reply-To: 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 > Now I see, why did you define COLUMN_MASK_SIZE. And still the problem > is in the way how you understand the last bit. > column_mask_fieldno_is_set() should check, if i >= COLUMN_MASK_SIZE, and in such > a case 'and' it with the last bit. In your code the mask is not 'smart' - > you consider fields >= 63 as set even if the last bit of the mask is 0. It is > worthy of note that literally every place of column_mask_fieldno_is_set() usage > also checks for i >= COLUMN_MASK_SIZE. You are right, we may omit such check for smart mask So, this is the central patch of this patchset extending SQLite functionality. All other are just boring refactoring. Maybe it worth stop just here. ==================================================== In some cases(like foreign keys) the SQL code is still used 32-bit bit mask, while 64-bit bit masks will perform better column optimizations. There was refactored code to work with 64b bitmasks where required. 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. Part of #3571 --- src/box/alter.cc | 10 ++-------- src/box/sql/delete.c | 7 ++----- src/box/sql/resolve.c | 19 ++++++------------ src/box/sql/sqlInt.h | 46 ++++++++++++++++++++++++------------------- src/box/sql/trigger.c | 14 ++++++------- src/box/sql/update.c | 24 +++++++++------------- 6 files changed, 52 insertions(+), 68 deletions(-) diff --git a/src/box/alter.cc b/src/box/alter.cc index bea15c8a2..f5f4c2112 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -29,6 +29,7 @@ * SUCH DAMAGE. */ #include "alter.h" +#include "column_mask.h" #include "schema.h" #include "user.h" #include "space.h" @@ -3729,13 +3730,6 @@ fkey_grab_by_name(struct rlist *list, const char *fkey_name) return NULL; } -/** - * FIXME: as sql legacy temporary we use such mask throught - * SQL code. It should be replaced later with regular - * mask from column_mask.h - */ -#define FKEY_MASK(x) (((x)>31) ? 0xffffffff : ((uint64_t)1<<(x))) - /** * Set bits of @mask which correspond to fields involved in * given foreign key constraint. @@ -3749,7 +3743,7 @@ static inline void fkey_set_mask(const struct fkey *fk, uint64_t *mask, int type) { for (uint32_t i = 0; i < fk->def->field_count; ++i) - *mask |= FKEY_MASK(fk->def->links[i].fields[type]); + column_mask_set_fieldno(mask, fk->def->links[i].fields[type]); } /** diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c index 95512ea9e..bd6228c0d 100644 --- a/src/box/sql/delete.c +++ b/src/box/sql/delete.c @@ -459,7 +459,7 @@ sql_generate_row_delete(struct Parse *parse, struct space *space, (fkey_is_required(space, NULL) || trigger_list != NULL)) { /* Mask of OLD.* columns in use */ /* TODO: Could use temporary registers here. */ - uint32_t mask = + uint64_t mask = sql_trigger_colmask(parse, trigger_list, 0, 0, TRIGGER_BEFORE | TRIGGER_AFTER, space, onconf); @@ -474,10 +474,7 @@ sql_generate_row_delete(struct Parse *parse, struct space *space, */ sqlVdbeAddOp2(v, OP_Copy, reg_pk, first_old_reg); for (int i = 0; i < (int)space->def->field_count; i++) { - testcase(mask != 0xffffffff && iCol == 31); - testcase(mask != 0xffffffff && iCol == 32); - if (mask == 0xffffffff - || (i <= 31 && (mask & MASKBIT32(i)) != 0)) { + if (column_mask_fieldno_is_set(mask, i)) { sqlExprCodeGetColumnOfTable(v, space->def, cursor, i, first_old_reg + diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c index bc208cc9d..a9a167477 100644 --- a/src/box/sql/resolve.c +++ b/src/box/sql/resolve.c @@ -340,20 +340,13 @@ lookupName(Parse * pParse, /* The parsing context */ if (iCol < 0) { pExpr->type = FIELD_TYPE_INTEGER; - } else if (pExpr->iTable == 0) { - testcase(iCol == 31); - testcase(iCol == 32); - pParse->oldmask |= - (iCol >= - 32 ? 0xffffffff - : (((u32) 1) << iCol)); } else { - testcase(iCol == 31); - testcase(iCol == 32); - pParse->newmask |= - (iCol >= - 32 ? 0xffffffff - : (((u32) 1) << iCol)); + uint64_t *mask = + pExpr->iTable == 0 ? + &pParse->oldmask : + &pParse->newmask; + column_mask_set_fieldno(mask, + iCol); } pExpr->iColumn = (i16) iCol; pExpr->space_def = space_def; diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 2830ab639..5be695e51 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -67,6 +67,7 @@ #include +#include "box/column_mask.h" #include "box/field_def.h" #include "box/sql.h" #include "box/txn.h" @@ -2580,22 +2581,24 @@ struct SelectDest { #endif /* - * At least one instance of the following structure is created for each - * trigger that may be fired while parsing an INSERT, UPDATE or DELETE - * statement. All such objects are stored in the linked list headed at - * Parse.pTriggerPrg and deleted once statement compilation has been - * completed. - * - * A Vdbe sub-program that implements the body and WHEN clause of trigger - * TriggerPrg.pTrigger, assuming a default ON CONFLICT clause of - * TriggerPrg.orconf, is stored in the TriggerPrg.pProgram variable. - * The Parse.pTriggerPrg list never contains two entries with the same - * values for both pTrigger and orconf. - * - * The TriggerPrg.aColmask[0] variable is set to a mask of old.* columns - * accessed (or set to 0 for triggers fired as a result of INSERT - * statements). Similarly, the TriggerPrg.aColmask[1] variable is set to - * a mask of new.* columns used by the program. + * At least one instance of the following structure is created for + * each trigger that may be fired while parsing an INSERT, UPDATE + * or DELETE statement. All such objects are stored in the linked + * list headed at Parse.pTriggerPrg and deleted once statement + * compilation has been completed. + * + * A Vdbe sub-program that implements the body and WHEN clause of + * trigger TriggerPrg.pTrigger, assuming a default ON CONFLICT + * clause of TriggerPrg.orconf, is stored in the + * TriggerPrg.pProgram variable. The Parse.pTriggerPrg list never + * contains two entries with the same values for both pTrigger + * and orconf. + * + * The TriggerPrg.column_mask[0] variable is set to a mask of + * old.* columns accessed (or set to 0 for triggers fired as a + * result of INSERT statements). Similarly, the + * TriggerPrg.column_mask[1] variable is set to a mask of new.* + * columns used by the program. */ struct TriggerPrg { /** Trigger this program was coded from. */ @@ -2603,7 +2606,8 @@ struct TriggerPrg { TriggerPrg *pNext; /* Next entry in Parse.pTriggerPrg list */ SubProgram *pProgram; /* Program implementing pTrigger/orconf */ int orconf; /* Default ON CONFLICT policy */ - u32 aColmask[2]; /* Masks of old.*, new.* columns accessed */ + /* Masks of old.*, new.* columns accessed. */ + uint64_t column_mask[2]; }; enum ast_type { @@ -2684,8 +2688,10 @@ struct Parse { int nSelectIndent; /* How far to indent SELECTTRACE() output */ Parse *pToplevel; /* Parse structure for main program (or NULL) */ u32 nQueryLoop; /* Est number of iterations of a query (10*log2(N)) */ - u32 oldmask; /* Mask of old.* columns referenced */ - u32 newmask; /* Mask of new.* columns referenced */ + /* Mask of old.* columns referenced. */ + uint64_t oldmask; + /* Mask of new.* columns referenced. */ + uint64_t newmask; u8 eTriggerOp; /* TK_UPDATE, TK_INSERT or TK_DELETE */ u8 eOrconf; /* Default ON CONFLICT policy for trigger steps */ /** Region to make SQL temp allocations. */ @@ -4083,7 +4089,7 @@ TriggerStep *sqlTriggerDeleteStep(sql *, Token *, Expr *); * * @retval mask value. */ -u32 +uint64_t sql_trigger_colmask(Parse *parser, struct sql_trigger *trigger, ExprList *changes_list, int new, int tr_tm, struct space *space, int orconf); diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c index 9901a5606..039a33198 100644 --- a/src/box/sql/trigger.c +++ b/src/box/sql/trigger.c @@ -786,8 +786,8 @@ sql_row_trigger_program(struct Parse *parser, struct sql_trigger *trigger, sqlVdbeLinkSubProgram(pTop->pVdbe, pProgram); pPrg->trigger = trigger; pPrg->orconf = orconf; - pPrg->aColmask[0] = 0xffffffff; - pPrg->aColmask[1] = 0xffffffff; + pPrg->column_mask[0] = COLUMN_MASK_FULL; + pPrg->column_mask[1] = COLUMN_MASK_FULL; /* * Allocate and populate a new Parse context to use for @@ -859,8 +859,8 @@ sql_row_trigger_program(struct Parse *parser, struct sql_trigger *trigger, pProgram->nMem = pSubParse->nMem; pProgram->nCsr = pSubParse->nTab; pProgram->token = (void *)trigger; - pPrg->aColmask[0] = pSubParse->oldmask; - pPrg->aColmask[1] = pSubParse->newmask; + pPrg->column_mask[0] = pSubParse->oldmask; + pPrg->column_mask[1] = pSubParse->newmask; sqlVdbeDelete(v); } @@ -977,13 +977,13 @@ vdbe_code_row_trigger(struct Parse *parser, struct sql_trigger *trigger, } } -u32 +uint64_t sql_trigger_colmask(Parse *parser, struct sql_trigger *trigger, ExprList *changes_list, int new, int tr_tm, struct space *space, int orconf) { const int op = changes_list != NULL ? TK_UPDATE : TK_DELETE; - u32 mask = 0; + uint64_t mask = 0; assert(new == 1 || new == 0); for (struct sql_trigger *p = trigger; p != NULL; p = p->next) { @@ -992,7 +992,7 @@ sql_trigger_colmask(Parse *parser, struct sql_trigger *trigger, TriggerPrg *prg = sql_row_trigger(parser, p, space, orconf); if (prg != NULL) - mask |= prg->aColmask[new]; + mask |= prg->column_mask[new]; } } diff --git a/src/box/sql/update.c b/src/box/sql/update.c index cb303c107..f4351aee6 100644 --- a/src/box/sql/update.c +++ b/src/box/sql/update.c @@ -100,7 +100,6 @@ sqlUpdate(Parse * pParse, /* The parser context */ /* List of triggers on pTab, if required. */ struct sql_trigger *trigger; int tmask; /* Mask of TRIGGER_BEFORE|TRIGGER_AFTER */ - int newmask; /* Mask of NEW.* columns accessed by BEFORE triggers */ int iEph = 0; /* Ephemeral table holding all primary key values */ int nKey = 0; /* Number of elements in regKey */ int aiCurOnePass[2]; /* The write cursors opened by WHERE_ONEPASS */ @@ -332,18 +331,15 @@ sqlUpdate(Parse * pParse, /* The parser context */ */ if (is_pk_modified || hasFK != 0 || trigger != NULL) { assert(space != NULL); - u32 oldmask = hasFK ? space->fkey_mask : 0; + uint64_t oldmask = hasFK ? space->fkey_mask : 0; oldmask |= sql_trigger_colmask(pParse, trigger, pChanges, 0, TRIGGER_BEFORE | TRIGGER_AFTER, space, on_error); for (i = 0; i < (int)def->field_count; i++) { - if (oldmask == 0xffffffff - || (i < 32 && (oldmask & MASKBIT32(i)) != 0) || - sql_space_column_is_in_pk(space, i)) { - testcase(oldmask != 0xffffffff && i == 31); - sqlExprCodeGetColumnOfTable(v, def, - pk_cursor, i, - regOld + i); + if (column_mask_fieldno_is_set(oldmask, i) || + sql_space_column_is_in_pk(space, i)) { + sqlExprCodeGetColumnOfTable(v, def, pk_cursor, + i, regOld + i); } else { sqlVdbeAddOp2(v, OP_Null, 0, regOld + i); } @@ -364,22 +360,20 @@ sqlUpdate(Parse * pParse, /* The parser context */ * may have modified them). So not loading those that are not going to * be used eliminates some redundant opcodes. */ - newmask = sql_trigger_colmask(pParse, trigger, pChanges, 1, - TRIGGER_BEFORE, space, on_error); + uint64_t newmask = sql_trigger_colmask(pParse, trigger, pChanges, 1, + TRIGGER_BEFORE, space, on_error); for (i = 0; i < (int)def->field_count; i++) { j = aXRef[i]; if (j >= 0) { sqlExprCode(pParse, pChanges->a[j].pExpr, regNew + i); - } else if (0 == (tmask & TRIGGER_BEFORE) || i > 31 - || (newmask & MASKBIT32(i))) { + } else if ((tmask & TRIGGER_BEFORE) == 0 || + column_mask_fieldno_is_set(newmask, i) != 0) { /* This branch loads the value of a column that will not be changed * into a register. This is done if there are no BEFORE triggers, or * if there are one or more BEFORE triggers that use this value via * a new.* reference in a trigger program. */ - testcase(i == 31); - testcase(i == 32); sqlExprCodeGetColumnToReg(pParse, def, i, pk_cursor, regNew + i); } else { -- 2.20.1