Re: [Tarantool-patches] [PATCH v2 11/16] sql: introduce sql_stmt_sizeof() function

[Nikita Pettik <korablev@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;
> > +			}
> > +		}