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 ACC9C28542 for ; Fri, 15 Feb 2019 12:05:08 -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 zyL7q_4h5kzh for ; Fri, 15 Feb 2019 12:05:08 -0500 (EST) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 F1F1728541 for ; Fri, 15 Feb 2019 12:05:07 -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: Vladislav Shpilevoy Message-ID: Date: Fri, 15 Feb 2019 20:05:05 +0300 MIME-Version: 1.0 In-Reply-To: <26ee3cd45826f7de3290d14b37524d181ef320af.1549629707.git.kshcherbatov@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed 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, Kirill Shcherbatov 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 +