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 4B43721196 for ; Mon, 10 Dec 2018 09:17:33 -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 6l1RF2dkVhko for ; Mon, 10 Dec 2018 09:17:33 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 0B5CD20237 for ; Mon, 10 Dec 2018 09:17:32 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 1/6] sql: avoid calling sql_encode_table_opts() during trigger creation References: <3c4a6fd745c0a651a164cdc155404e3555078a56.1544387419.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <40776123-c105-aaae-8ed7-e2ad70c232e3@tarantool.org> Date: Mon, 10 Dec 2018 17:17:30 +0300 MIME-Version: 1.0 In-Reply-To: <3c4a6fd745c0a651a164cdc155404e3555078a56.1544387419.git.korablev@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, Nikita Pettik Thanks for the patch! See 2 comments and review fixes below. On 10/12/2018 00:30, Nikita Pettik wrote: > At the last stage of trigger creation, trigger's create statement > ("CREATE TRIGGER ...") is encoded to msgpack. Since only this string is > only member of a map to be encoded, is makes no sense to call whole > sql_encode_table_opts() function, which in turn processes table's > checks, opts for VIEW etc. > > Needed for #2647 > --- > src/box/sql/trigger.c | 20 +++++++++----------- > 1 file changed, 9 insertions(+), 11 deletions(-) > > diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c > index c38f9cd9d..c7d22b9d0 100644 > --- a/src/box/sql/trigger.c > +++ b/src/box/sql/trigger.c > @@ -212,19 +212,17 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list, > parse->nMem += 3; > int record = ++parse->nMem; > > - uint32_t opts_buff_sz = 0; > - char *data = sql_encode_table_opts(&fiber()->gc, NULL, sql_str, > - &opts_buff_sz); > - sqlite3DbFree(db, sql_str); > - if (data == NULL) { > - parse->nErr++; > - parse->rc = SQL_TARANTOOL_ERROR; > - goto cleanup; > - } > - char *opts_buff = sqlite3DbMallocRaw(db, opts_buff_sz); > + uint32_t opts_buff_sz = mp_sizeof_map(1) + > + mp_sizeof_str(strlen("sql")) + > + mp_sizeof_str(strlen(sql_str)); > + char *opts_buff = (char *) sqlite3DbMallocRaw(db, opts_buff_sz); > if (opts_buff == NULL) > goto cleanup; > - memcpy(opts_buff, data, opts_buff_sz); > + > + char *data = mp_encode_map(opts_buff, 1); > + data = mp_encode_str(data, "sql", strlen("sql")); > + data = mp_encode_str(data, sql_str, strlen(sql_str)); 1. strlen(sql_str) is called twice. Foreboding your claim that it will be optimized to 1 call - no, it won't. I disassembled this function and saw double strlen(sql_str). Only strlen("sql") is optimized to constant 3. 2. After you've deleted this invocation of sql_encode_table_opts(), struct Table *table in its second parameter became not NULL always. So I simplified this function a bit. > + sqlite3DbFree(db, sql_str); > > trigger_name = sqlite3DbStrDup(db, trigger_name); > if (trigger_name == NULL) { > My review fixes here and on the branch: ================================================================== commit c287163409a6a542273d07cb0602834dfa66df88 Author: Vladislav Shpilevoy Date: Mon Dec 10 16:12:23 2018 +0300 Review fixes diff --git a/src/box/sql.c b/src/box/sql.c index 7b41c9926..f43387f18 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -1199,16 +1199,13 @@ sql_encode_table_opts(struct region *region, struct Table *table, bool is_error = false; mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb, set_encode_error, &is_error); - bool is_view = false; int checks_cnt = 0; struct ExprList_item *a; - if (table != NULL) { - is_view = table->def->opts.is_view; - struct ExprList *checks = table->def->opts.checks; - if (checks != NULL) { - checks_cnt = checks->nExpr; - a = checks->a; - } + bool is_view = table->def->opts.is_view; + struct ExprList *checks = table->def->opts.checks; + if (checks != NULL) { + checks_cnt = checks->nExpr; + a = checks->a; } mpstream_encode_map(&stream, 1 + is_view + (checks_cnt > 0)); diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c index c7d22b9d0..418dc9147 100644 --- a/src/box/sql/trigger.c +++ b/src/box/sql/trigger.c @@ -211,17 +211,19 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list, int first_col = parse->nMem + 1; parse->nMem += 3; int record = ++parse->nMem; + int sql_str_len = strlen(sql_str); + int sql_len = strlen("sql"); uint32_t opts_buff_sz = mp_sizeof_map(1) + - mp_sizeof_str(strlen("sql")) + - mp_sizeof_str(strlen(sql_str)); + mp_sizeof_str(sql_len) + + mp_sizeof_str(sql_str_len); char *opts_buff = (char *) sqlite3DbMallocRaw(db, opts_buff_sz); if (opts_buff == NULL) goto cleanup; char *data = mp_encode_map(opts_buff, 1); - data = mp_encode_str(data, "sql", strlen("sql")); - data = mp_encode_str(data, sql_str, strlen(sql_str)); + data = mp_encode_str(data, "sql", sql_len); + data = mp_encode_str(data, sql_str, sql_str_len); sqlite3DbFree(db, sql_str); trigger_name = sqlite3DbStrDup(db, trigger_name);