[Tarantool-patches] [PATCH v5 11/52] sql: introduce mem_destroy()
Mergen Imeev
imeevma at tarantool.org
Tue Apr 13 15:42:05 MSK 2021
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 at 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);
}
}
More information about the Tarantool-patches
mailing list