From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 dev.tarantool.org (Postfix) with ESMTPS id AEAF146971A for ; Fri, 13 Dec 2019 16:56:27 +0300 (MSK) Date: Fri, 13 Dec 2019 16:56:26 +0300 From: Nikita Pettik Message-ID: <20191213135626.GA82116@tarantool.org> References: <0a6806665bbc165dffc23967be57135a46bbad62.1574277369.git.korablev@tarantool.org> <8bd6cb33-4a55-8f4c-60cf-bc54928f220e@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <8bd6cb33-4a55-8f4c-60cf-bc54928f220e@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 11/16] sql: introduce sql_stmt_sizeof() function List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org On 03 Dec 23:51, Vladislav Shpilevoy wrote: > Thanks for the patch! > > See 4 comments below. > > On 20/11/2019 22:28, Nikita Pettik wrote: > > To implement memory quota of prepared statement cache, we have to > > estimate size of prepared statement. This function attempts at that. > > > > Part of #2592 > > --- > > src/box/execute.h | 8 ++++++++ > > src/box/sql/vdbeapi.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 61 insertions(+) > > > > diff --git a/src/box/execute.h b/src/box/execute.h > > index 6702a18cc..d5b4d8421 100644 > > --- a/src/box/execute.h > > +++ b/src/box/execute.h > > @@ -116,6 +116,14 @@ extern const struct port_vtab port_sql_vtab; > > int > > sql_finalize(struct sql_stmt *stmt); > > > > +/** > > + * Calculate estimated size of memory occupied by VM. > > + * See sqlVdbeMakeReady() for details concerning allocated > > + * memory. > > + */ > > +size_t > > +sql_stmt_sizeof(const struct sql_stmt *stmt); > > + > > 1. Lets call it 'sql_stmt_sizeof_estimated()'. To emphasize that > the size is not exact. TBO it seems to be too long. I'd better use Konstantin's version: sql_stmt_est_size(). > > /** > > * Prepare (compile into VDBE byte-code) statement. > > * > > diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c > > index da528a4dc..10135bb68 100644 > > --- a/src/box/sql/vdbeapi.c > > +++ b/src/box/sql/vdbeapi.c > > @@ -805,6 +805,59 @@ sql_stmt_schema_version(sql_stmt *stmt) > > return v->schema_ver; > > } > > > > +size_t > > +sql_stmt_sizeof(const sql_stmt *stmt) > > 2. You have 'struct' in the declaration, but not > here. I think struct should be added there too. Fixed. > > +{ > > + struct Vdbe *v = (struct Vdbe *) stmt; > > + size_t size = sizeof(*v); > > + /* Resulting set */ > > + size += sizeof(struct Mem) * v->nResColumn * COLNAME_N; > > 3. Could we account column names too? aColName member. It is exactly what is accounted here. Fixed a bit comment: - /* Resulting set */ + /* Names and types of result set columns */ Memory cells containing values of result set are already accounted in v->nMem. > > + /* Opcodes */ > > + size += sizeof(struct VdbeOp) * v->nOp; > > + /* Memory cells */ > > + size += sizeof(struct Mem) * v->nMem; > > + /* Bindings */ > > + size += sizeof(struct Mem) * v->nVar; > > + /* Bindings included in the result set */ > > + size += sizeof(uint32_t) * v->res_var_count; > > + /* Cursors */ > > + size += sizeof(struct VdbeCursor *) * v->nCursor; > > + > > + for (int i = 0; i < v->nOp; ++i) { > > + /* Estimate size of p4 operand. */ > > + if (v->aOp[i].p4type != P4_NOTUSED) { > > 4. You can reduce the indentation: > > - Try to invert the check and do 'continue' when P4 is > unused; > > - 'Case' should be aligned under 'switch' according to > the code style. Thanks, refactord: @@ -825,33 +825,33 @@ sql_stmt_sizeof(const sql_stmt *stmt) for (int i = 0; i < v->nOp; ++i) { /* Estimate size of p4 operand. */ - if (v->aOp[i].p4type != P4_NOTUSED) { - switch (v->aOp[i].p4type) { - case P4_DYNAMIC: - case P4_STATIC: - if (v->aOp[i].opcode == OP_Blob || - v->aOp[i].opcode == OP_String) - size += v->aOp[i].p1; - else if (v->aOp[i].opcode == OP_String8) - size += strlen(v->aOp[i].p4.z); - break; - case P4_BOOL: - size += sizeof(v->aOp[i].p4.b); - break; - case P4_INT32: - size += sizeof(v->aOp[i].p4.i); - break; - case P4_UINT64: - case P4_INT64: - size += sizeof(*v->aOp[i].p4.pI64); - break; - case P4_REAL: - size += sizeof(*v->aOp[i].p4.pReal); - break; - default: - size += sizeof(v->aOp[i].p4.p); - break; - } + if (v->aOp[i].p4type == P4_NOTUSED) + continue; + switch (v->aOp[i].p4type) { + case P4_DYNAMIC: + case P4_STATIC: + if (v->aOp[i].opcode == OP_Blob || + v->aOp[i].opcode == OP_String) + size += v->aOp[i].p1; + else if (v->aOp[i].opcode == OP_String8) + size += strlen(v->aOp[i].p4.z); + break; + case P4_BOOL: + size += sizeof(v->aOp[i].p4.b); + break; + case P4_INT32: + size += sizeof(v->aOp[i].p4.i); + break; + case P4_UINT64: + case P4_INT64: + size += sizeof(*v->aOp[i].p4.pI64); + break; + case P4_REAL: + size += sizeof(*v->aOp[i].p4.pReal); + break; + default: + size += sizeof(v->aOp[i].p4.p); + break; } } size += strlen(v->zSql); > > + switch (v->aOp[i].p4type) { > > + case P4_DYNAMIC: > > + case P4_STATIC: > > + if (v->aOp[i].opcode == OP_Blob || > > + v->aOp[i].opcode == OP_String) > > + size += v->aOp[i].p1; > > + else if (v->aOp[i].opcode == OP_String8) > > + size += strlen(v->aOp[i].p4.z); > > + break; > > + case P4_BOOL: > > + size += sizeof(v->aOp[i].p4.b); > > + break; > > + case P4_INT32: > > + size += sizeof(v->aOp[i].p4.i); > > + break; > > + case P4_UINT64: > > + case P4_INT64: > > + size += sizeof(*v->aOp[i].p4.pI64); > > + break; > > + case P4_REAL: > > + size += sizeof(*v->aOp[i].p4.pReal); > > + break; > > + default: > > + size += sizeof(v->aOp[i].p4.p); > > + break; > > + } > > + }