From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: tarantool-patches@freelists.org, Kirill Shcherbatov <kshcherbatov@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v1 2/4] sql: use 64b bitmasks instead of 32b where possible Date: Fri, 15 Feb 2019 20:05:05 +0300 [thread overview] Message-ID: <cc945579-d4f0-1fc9-6888-9a411098c710@tarantool.org> (raw) In-Reply-To: <26ee3cd45826f7de3290d14b37524d181ef320af.1549629707.git.kshcherbatov@tarantool.org> Thanks for the patch! See 1 comment below. On 08/02/2019 13:52, Kirill Shcherbatov wrote: > 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 | 8 +++---- > src/box/sql/resolve.c | 19 ++++++----------- > src/box/sql/sqliteInt.h | 46 +++++++++++++++++++++++------------------ > src/box/sql/trigger.c | 14 ++++++------- > src/box/sql/update.c | 21 ++++++++----------- > 6 files changed, 53 insertions(+), 65 deletions(-) > > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > index f9c42fdec..5e134ab0c 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > @@ -484,10 +484,8 @@ sql_generate_row_delete(struct Parse *parse, struct Table *table, > */ > sqlite3VdbeAddOp2(v, OP_Copy, reg_pk, first_old_reg); > for (int i = 0; i < (int)table->def->field_count; i++) { > - testcase(mask != 0xffffffff && iCol == 31); > - testcase(mask != 0xffffffff && iCol == 32); > - if (mask == 0xffffffff > - || (i <= 31 && (mask & MASKBIT32(i)) != 0)) { > + if (i >= COLUMN_MASK_SIZE || > + column_mask_fieldno_is_set(mask, i)) { 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. > sqlite3ExprCodeGetColumnOfTable(v, table->def, > cursor, i, > first_old_reg +
next prev parent reply other threads:[~2019-02-15 17:05 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-02-08 12:52 [tarantool-patches] [PATCH v1 0/4] sql: replace 32 bit column mask Kirill Shcherbatov 2019-02-08 12:52 ` [tarantool-patches] [PATCH v1 1/4] box: introduce new helpers in column_mask.h Kirill Shcherbatov 2019-02-15 17:05 ` [tarantool-patches] " Vladislav Shpilevoy 2019-02-20 13:42 ` Kirill Shcherbatov 2019-02-22 17:51 ` Vladislav Shpilevoy 2019-02-22 18:01 ` Konstantin Osipov 2019-02-22 18:22 ` Konstantin Osipov 2019-02-08 12:52 ` [tarantool-patches] [PATCH v1 2/4] sql: use 64b bitmasks instead of 32b where possible Kirill Shcherbatov 2019-02-15 17:05 ` Vladislav Shpilevoy [this message] 2019-02-20 13:42 ` [tarantool-patches] " Kirill Shcherbatov 2019-02-08 12:52 ` [tarantool-patches] [PATCH v1 3/4] sql: got rid of redundant MASKBIT32 definition Kirill Shcherbatov 2019-02-15 17:05 ` [tarantool-patches] " Vladislav Shpilevoy 2019-02-20 13:42 ` Kirill Shcherbatov 2019-02-08 12:52 ` [tarantool-patches] [PATCH v1 4/4] sql: got rid of redundant bitmask helpers Kirill Shcherbatov 2019-02-15 17:05 ` [tarantool-patches] " Vladislav Shpilevoy 2019-02-20 13:42 ` Kirill Shcherbatov 2019-02-22 17:52 ` Vladislav Shpilevoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=cc945579-d4f0-1fc9-6888-9a411098c710@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=kshcherbatov@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v1 2/4] sql: use 64b bitmasks instead of 32b where possible' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox