Re: [Tarantool-patches] [PATCH v5 11/52] sql: introduce mem_destroy()

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thank you for the review! My answers, diff and new patch below.

On Sun, Apr 11, 2021 at 07:46:05PM +0200, Vladislav Shpilevoy wrote:
> Thanks for working on this!
> 
> See 3 comments below.
> 
> On 09.04.2021 19:36, Mergen Imeev via Tarantool-patches wrote:
> > This patch introduces mem_destroy(). This function should be used to
> > free and destroing all objects owned by MEM, if necessary.
> 
> 1. "to free and destroy".
> 
Fixed.

> > Part of #5818
> > ---
> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index 5135637d9..805dc7054 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -92,6 +92,38 @@ mem_create(struct Mem *mem)
> > +
> > +void
> > +mem_destroy(struct Mem *mem)
> > +{
> > +	mem_clear(mem);
> > +	if (mem->szMalloc > 0)
> > +		sqlDbFree(mem->db, mem->zMalloc);
> > +	mem->n = 0;
> > +	mem->z = NULL;
> > +	mem->szMalloc = 0;
> > +	mem->zMalloc = NULL;
> 
> 2. You could move szMalloc and zMalloc nullification under the 'if'
> above.
> 
Done.

> > +}
> > @@ -1384,7 +1359,7 @@ sqlValueFree(sql_value * v)
> >  {
> >  	if (!v)
> >  		return;
> > -	sqlVdbeMemRelease((Mem *) v);
> > +	mem_destroy((Mem *) v);
> 
> 3. No need for whitespace after unary operators.
Fixed.



diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 805dc7054..e9062b602 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -116,12 +116,13 @@ void
 mem_destroy(struct Mem *mem)
 {
 	mem_clear(mem);
-	if (mem->szMalloc > 0)
+	if (mem->szMalloc > 0) {
 		sqlDbFree(mem->db, mem->zMalloc);
+		mem->szMalloc = 0;
+		mem->zMalloc = NULL;
+	}
 	mem->n = 0;
 	mem->z = NULL;
-	mem->szMalloc = 0;
-	mem->zMalloc = NULL;
 }
 
 static inline bool
@@ -1359,7 +1360,7 @@ sqlValueFree(sql_value * v)
 {
 	if (!v)
 		return;
-	mem_destroy((Mem *) v);
+	mem_destroy(v);
 	sqlDbFree(((Mem *) v)->db, v);
 }
 

New patch:

commit 4efcd4a2b2a6654448a33affd6aa43410938f3cf
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Thu Mar 4 14:07:35 2021 +0300

    sql: introduce mem_destroy()
    
    This patch introduces mem_destroy(). This function should be used to
    free and destroy all objects owned by MEM, if necessary.
    
    Part of #5818

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 99ce938d5..13d8dd32c 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1796,7 +1796,7 @@ minMaxFinalize(sql_context * context)
 		if (pRes->flags) {
 			sql_result_value(context, pRes);
 		}
-		sqlVdbeMemRelease(pRes);
+		mem_destroy(pRes);
 	}
 }
 
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 5135637d9..e9062b602 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -92,6 +92,39 @@ mem_create(struct Mem *mem)
 #endif
 }
 
+static inline void
+mem_clear(struct Mem *mem)
+{
+	if ((mem->flags & (MEM_Agg | MEM_Dyn | MEM_Frame)) != 0) {
+		if ((mem->flags & MEM_Agg) != 0)
+			sql_vdbemem_finalize(mem, mem->u.func);
+		assert((mem->flags & MEM_Agg) == 0);
+		if ((mem->flags & MEM_Dyn) != 0) {
+			assert(mem->xDel != SQL_DYNAMIC && mem->xDel != NULL);
+			mem->xDel((void *)mem->z);
+		} else if ((mem->flags & MEM_Frame) != 0) {
+			struct VdbeFrame *frame = mem->u.pFrame;
+			frame->pParent = frame->v->pDelFrame;
+			frame->v->pDelFrame = frame;
+		}
+	}
+	mem->flags = MEM_Null;
+	mem->field_type = field_type_MAX;
+}
+
+void
+mem_destroy(struct Mem *mem)
+{
+	mem_clear(mem);
+	if (mem->szMalloc > 0) {
+		sqlDbFree(mem->db, mem->zMalloc);
+		mem->szMalloc = 0;
+		mem->zMalloc = NULL;
+	}
+	mem->n = 0;
+	mem->z = NULL;
+}
+
 static inline bool
 mem_has_msgpack_subtype(struct Mem *mem)
 {
@@ -203,56 +236,6 @@ vdbeMemAddTerminator(Mem * pMem)
 	return 0;
 }
 
-/*
- * If the memory cell contains a value that must be freed by
- * invoking the external callback in Mem.xDel, then this routine
- * will free that value.  It also sets Mem.flags to MEM_Null.
- *
- * This is a helper routine for sqlVdbeMemSetNull() and
- * for sqlVdbeMemRelease().  Use those other routines as the
- * entry point for releasing Mem resources.
- */
-static SQL_NOINLINE void
-vdbeMemClearExternAndSetNull(Mem * p)
-{
-	assert(VdbeMemDynamic(p));
-	if (p->flags & MEM_Agg) {
-		sql_vdbemem_finalize(p, p->u.func);
-		assert((p->flags & MEM_Agg) == 0);
-		testcase(p->flags & MEM_Dyn);
-	}
-	if (p->flags & MEM_Dyn) {
-		assert(p->xDel != SQL_DYNAMIC && p->xDel != 0);
-		p->xDel((void *)p->z);
-	} else if (p->flags & MEM_Frame) {
-		VdbeFrame *pFrame = p->u.pFrame;
-		pFrame->pParent = pFrame->v->pDelFrame;
-		pFrame->v->pDelFrame = pFrame;
-	}
-	p->flags = MEM_Null;
-}
-
-/*
- * Release memory held by the Mem p, both external memory cleared
- * by p->xDel and memory in p->zMalloc.
- *
- * This is a helper routine invoked by sqlVdbeMemRelease() in
- * the unusual case where there really is memory in p that needs
- * to be freed.
- */
-static SQL_NOINLINE void
-vdbeMemClear(Mem * p)
-{
-	if (VdbeMemDynamic(p)) {
-		vdbeMemClearExternAndSetNull(p);
-	}
-	if (p->szMalloc) {
-		sqlDbFree(p->db, p->zMalloc);
-		p->szMalloc = 0;
-	}
-	p->z = 0;
-}
-
 /*
  * Make an shallow copy of pFrom into pTo.  Prior contents of
  * pTo are freed.  The pFrom->z field is not duplicated.  If
@@ -262,7 +245,7 @@ vdbeMemClear(Mem * p)
 static SQL_NOINLINE void
 vdbeClrCopy(Mem * pTo, const Mem * pFrom, int eType)
 {
-	vdbeMemClearExternAndSetNull(pTo);
+	mem_clear(pTo);
 	assert(!VdbeMemDynamic(pTo));
 	sqlVdbeMemShallowCopy(pTo, pFrom, eType);
 }
@@ -1108,7 +1091,7 @@ sqlVdbeMemGrow(Mem * pMem, int n, int bPreserve)
 			pMem->zMalloc = sqlDbMallocRaw(pMem->db, n);
 		}
 		if (pMem->zMalloc == 0) {
-			sqlVdbeMemSetNull(pMem);
+			mem_clear(pMem);
 			pMem->z = 0;
 			pMem->szMalloc = 0;
 			return -1;
@@ -1160,7 +1143,7 @@ sqlVdbeMemClearAndResize(Mem * pMem, int szNew)
 void
 mem_set_bool(struct Mem *mem, bool value)
 {
-	sqlVdbeMemSetNull(mem);
+	mem_clear(mem);
 	mem->u.b = value;
 	mem->flags = MEM_Bool;
 	mem->field_type = FIELD_TYPE_BOOLEAN;
@@ -1169,7 +1152,7 @@ mem_set_bool(struct Mem *mem, bool value)
 void
 mem_set_ptr(struct Mem *mem, void *ptr)
 {
-	sqlVdbeMemRelease(mem);
+	mem_destroy(mem);
 	mem->flags = MEM_Ptr;
 	mem->u.p = ptr;
 }
@@ -1177,8 +1160,7 @@ mem_set_ptr(struct Mem *mem, void *ptr)
 void
 mem_set_i64(struct Mem *mem, int64_t value)
 {
-	if (VdbeMemDynamic(mem))
-		sqlVdbeMemSetNull(mem);
+	mem_clear(mem);
 	mem->u.i = value;
 	int flag = value < 0 ? MEM_Int : MEM_UInt;
 	MemSetTypeFlag(mem, flag);
@@ -1188,8 +1170,7 @@ mem_set_i64(struct Mem *mem, int64_t value)
 void
 mem_set_u64(struct Mem *mem, uint64_t value)
 {
-	if (VdbeMemDynamic(mem))
-		sqlVdbeMemSetNull(mem);
+	mem_clear(mem);
 	mem->u.u = value;
 	MemSetTypeFlag(mem, MEM_UInt);
 	mem->field_type = FIELD_TYPE_UNSIGNED;
@@ -1198,8 +1179,7 @@ mem_set_u64(struct Mem *mem, uint64_t value)
 void
 mem_set_int(struct Mem *mem, int64_t value, bool is_neg)
 {
-	if (VdbeMemDynamic(mem))
-		sqlVdbeMemSetNull(mem);
+	mem_clear(mem);
 	if (is_neg) {
 		assert(value < 0);
 		mem->u.i = value;
@@ -1214,7 +1194,7 @@ mem_set_int(struct Mem *mem, int64_t value, bool is_neg)
 void
 mem_set_double(struct Mem *mem, double value)
 {
-	sqlVdbeMemSetNull(mem);
+	mem_clear(mem);
 	if (sqlIsNaN(value))
 		return;
 	mem->u.r = value;
@@ -1251,7 +1231,7 @@ sqlVdbeMemSetStr(Mem * pMem,	/* Memory cell to set to string value */
 
 	/* If z is a NULL pointer, set pMem to contain an SQL NULL. */
 	if (!z) {
-		sqlVdbeMemSetNull(pMem);
+		mem_clear(pMem);
 		return 0;
 	}
 
@@ -1291,11 +1271,11 @@ sqlVdbeMemSetStr(Mem * pMem,	/* Memory cell to set to string value */
 		}
 		memcpy(pMem->z, z, nAlloc);
 	} else if (xDel == SQL_DYNAMIC) {
-		sqlVdbeMemRelease(pMem);
+		mem_destroy(pMem);
 		pMem->zMalloc = pMem->z = (char *)z;
 		pMem->szMalloc = sqlDbMallocSize(pMem->db, pMem->zMalloc);
 	} else {
-		sqlVdbeMemRelease(pMem);
+		mem_destroy(pMem);
 		pMem->z = (char *)z;
 		pMem->xDel = xDel;
 		flags |= ((xDel == SQL_STATIC) ? MEM_Static : MEM_Dyn);
@@ -1323,21 +1303,17 @@ sqlVdbeMemSetStr(Mem * pMem,	/* Memory cell to set to string value */
  *
  * This routine calls the Mem.xDel destructor to dispose of values that
  * require the destructor.  But it preserves the Mem.zMalloc memory allocation.
- * To free all resources, use sqlVdbeMemRelease(), which both calls this
+ * To free all resources, use mem_destroy(), which both calls this
  * routine to invoke the destructor and deallocates Mem.zMalloc.
  *
  * Use this routine to reset the Mem prior to insert a new value.
  *
- * Use sqlVdbeMemRelease() to complete erase the Mem prior to abandoning it.
+ * Use mem_destroy() to complete erase the Mem prior to abandoning it.
  */
 void
 sqlVdbeMemSetNull(Mem * pMem)
 {
-	if (VdbeMemDynamic(pMem)) {
-		vdbeMemClearExternAndSetNull(pMem);
-	} else {
-		pMem->flags = MEM_Null;
-	}
+	mem_clear(pMem);
 }
 
 /*
@@ -1347,7 +1323,7 @@ sqlVdbeMemSetNull(Mem * pMem)
 void
 sqlVdbeMemSetZeroBlob(Mem * pMem, int n)
 {
-	sqlVdbeMemRelease(pMem);
+	mem_destroy(pMem);
 	pMem->flags = MEM_Blob | MEM_Zero;
 	pMem->n = 0;
 	if (n < 0)
@@ -1384,7 +1360,7 @@ sqlValueFree(sql_value * v)
 {
 	if (!v)
 		return;
-	sqlVdbeMemRelease((Mem *) v);
+	mem_destroy(v);
 	sqlDbFree(((Mem *) v)->db, v);
 }
 
@@ -1407,34 +1383,10 @@ releaseMemArray(Mem * p, int N)
 {
 	if (p && N) {
 		Mem *pEnd = &p[N];
-		sql *db = p->db;
 		do {
 			assert((&p[1]) == pEnd || p[0].db == p[1].db);
 			assert(sqlVdbeCheckMemInvariants(p));
-
-			/* This block is really an inlined version of sqlVdbeMemRelease()
-			 * that takes advantage of the fact that the memory cell value is
-			 * being set to NULL after releasing any dynamic resources.
-			 *
-			 * The justification for duplicating code is that according to
-			 * callgrind, this causes a certain test case to hit the CPU 4.7
-			 * percent less (x86 linux, gcc version 4.1.2, -O6) than if
-			 * sqlMemRelease() were called from here. With -O2, this jumps
-			 * to 6.6 percent. The test case is inserting 1000 rows into a table
-			 * with no indexes using a single prepared INSERT statement, bind()
-			 * and reset(). Inserts are grouped into a transaction.
-			 */
-			testcase(p->flags & MEM_Agg);
-			testcase(p->flags & MEM_Dyn);
-			testcase(p->flags & MEM_Frame);
-			if (p->
-			    flags & (MEM_Agg | MEM_Dyn | MEM_Frame)) {
-				sqlVdbeMemRelease(p);
-			} else if (p->szMalloc) {
-				sqlDbFree(db, p->zMalloc);
-				p->szMalloc = 0;
-			}
-
+			mem_destroy(p);
 			p->flags = MEM_Undefined;
 		} while ((++p) < pEnd);
 	}
@@ -1876,8 +1828,7 @@ sqlVdbeMemCopy(Mem * pTo, const Mem * pFrom)
 {
 	int rc = 0;
 
-	if (VdbeMemDynamic(pTo))
-		vdbeMemClearExternAndSetNull(pTo);
+	mem_clear(pTo);
 	memcpy(pTo, pFrom, MEMCELLSIZE);
 	pTo->flags &= ~MEM_Dyn;
 	if (pTo->flags & (MEM_Str | MEM_Blob)) {
@@ -1917,7 +1868,7 @@ sqlVdbeMemMove(Mem * pTo, Mem * pFrom)
 {
 	assert(pFrom->db == 0 || pTo->db == 0 || pFrom->db == pTo->db);
 
-	sqlVdbeMemRelease(pTo);
+	mem_destroy(pTo);
 	memcpy(pTo, pFrom, sizeof(Mem));
 	pFrom->flags = MEM_Null;
 	pFrom->szMalloc = 0;
@@ -1952,25 +1903,6 @@ sqlVdbeMemMakeWriteable(Mem * pMem)
 	return 0;
 }
 
-/*
- * Release any memory resources held by the Mem.  Both the memory that is
- * free by Mem.xDel and the Mem.zMalloc allocation are freed.
- *
- * Use this routine prior to clean up prior to abandoning a Mem, or to
- * reset a Mem back to its minimum memory utilization.
- *
- * Use sqlVdbeMemSetNull() to release just the Mem.xDel space
- * prior to inserting new content into the Mem.
- */
-void
-sqlVdbeMemRelease(Mem * p)
-{
-	assert(sqlVdbeCheckMemInvariants(p));
-	if (VdbeMemDynamic(p) || p->szMalloc) {
-		vdbeMemClear(p);
-	}
-}
-
 int
 sql_vdbemem_finalize(struct Mem *mem, struct func *func)
 {
@@ -2517,7 +2449,7 @@ port_lua_get_vdbemem(struct port *base, uint32_t *size)
 	return (struct sql_value *)val;
 error:
 	for (int i = 0; i < argc; i++)
-		sqlVdbeMemRelease(&val[i]);
+		mem_destroy(&val[i]);
 	region_truncate(region, region_svp);
 	return NULL;
 }
@@ -2589,7 +2521,7 @@ port_c_get_vdbemem(struct port *base, uint32_t *size)
 	return (struct sql_value *) val;
 error:
 	for (int i = 0; i < port->size; i++)
-		sqlVdbeMemRelease(&val[i]);
+		mem_destroy(&val[i]);
 	region_truncate(region, region_svp);
 	return NULL;
 }
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 2a3d1078e..82b3084fb 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -98,6 +98,10 @@ mem_str(const struct Mem *mem);
 void
 mem_create(struct Mem *mem);
 
+/** Free all allocated memory in MEM and set MEM to NULL. */
+void
+mem_destroy(struct Mem *mem);
+
 /* One or more of the following flags are set to indicate the validOK
  * representations of the value stored in the Mem struct.
  *
@@ -407,7 +411,6 @@ int sqlVdbeMemCopy(Mem *, const Mem *);
 void sqlVdbeMemShallowCopy(Mem *, const Mem *, int);
 void sqlVdbeMemMove(Mem *, Mem *);
 int sqlVdbeMemMakeWriteable(Mem *);
-void sqlVdbeMemRelease(Mem * p);
 
 /**
  * Memory cell mem contains the context of an aggregate function.
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 74bf7f903..7cc72dc38 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2500,7 +2500,7 @@ case OP_MakeRecord: {
 		 * to be passed to Tarantool. Before that, make
 		 * sure previously allocated memory has gone.
 		 */
-		sqlVdbeMemRelease(pOut);
+		mem_destroy(pOut);
 		pOut->flags = MEM_Blob | MEM_Ephem;
 		pOut->n = tuple_size;
 		pOut->z = tuple;
@@ -4454,7 +4454,7 @@ case OP_Program: {        /* jump */
 		if (!pFrame) {
 			goto no_mem;
 		}
-		sqlVdbeMemRelease(pRt);
+		mem_destroy(pRt);
 		pRt->flags = MEM_Frame;
 		pRt->u.pFrame = pFrame;
 
@@ -4747,7 +4747,7 @@ case OP_AggStep: {
 	struct func_sql_builtin *func = (struct func_sql_builtin *)pCtx->func;
 	func->call(pCtx, pCtx->argc, pCtx->argv);
 	if (pCtx->is_aborted) {
-		sqlVdbeMemRelease(&t);
+		mem_destroy(&t);
 		goto abort_due_to_error;
 	}
 	assert(t.flags==MEM_Null);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index b48e45770..cbf32ccdf 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -130,7 +130,7 @@ struct VdbeCursor {
  * is linked into the Vdbe.pDelFrame list. The contents of the Vdbe.pDelFrame
  * list is deleted when the VM is reset in VdbeHalt(). The reason for doing
  * this instead of deleting the VdbeFrame immediately is to avoid recursive
- * calls to sqlVdbeMemRelease() when the memory cells belonging to the
+ * calls to mem_destroy() when the memory cells belonging to the
  * child frame are released.
  *
  * The currently executing frame is stored in Vdbe.pFrame. Vdbe.pFrame is
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 671338361..a195f8dfd 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -681,9 +681,7 @@ vdbeUnbind(Vdbe * p, int i)
 	}
 	i--;
 	pVar = &p->aVar[i];
-	sqlVdbeMemRelease(pVar);
-	pVar->flags = MEM_Null;
-	pVar->field_type = field_type_MAX;
+	mem_destroy(pVar);
 	return 0;
 }
 
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 263fe5b00..bb87bb902 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -579,7 +579,7 @@ sqlStat4ProbeFree(UnpackedRecord * pRec)
 		int part_count = pRec->key_def->part_count;
 		struct Mem *aMem = pRec->aMem;
 		for (int i = 0; i < part_count; i++)
-			sqlVdbeMemRelease(&aMem[i]);
+			mem_destroy(&aMem[i]);
 		sqlDbFree(aMem[0].db, pRec);
 	}
 }