[Tarantool-patches] [PATCH v4 06/16] sql: introduce mem_append()

Mergen Imeev imeevma at tarantool.org
Tue Oct 5 12:32:50 MSK 2021


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

On Mon, Oct 04, 2021 at 11:52:43PM +0200, Vladislav Shpilevoy wrote:
> Thanks for working on this!
> 
> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index 079083fa1..b8ead8f41 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -1925,6 +1925,27 @@ mem_move(struct Mem *to, struct Mem *from)
> >  	from->zMalloc = NULL;
> >  }
> >  
> > +int
> > +mem_append(struct Mem *mem, const char *value, uint32_t len)
> > +{
> > +	assert((mem->type & (MEM_TYPE_BIN | MEM_TYPE_STR)) != 0);
> > +	if (len == 0)
> > +		return 0;
> > +	int new_size = mem->n + len;
> > +	if (((mem->flags & (MEM_Static | MEM_Dyn | MEM_Ephem)) != 0) ||
> > +	    mem->szMalloc < new_size) {
> > +		/*
> > +		 * Force exponential buffer size growth to avoid having to call
> > +		 * this routine too often.
> > +		 */
> > +		if (sqlVdbeMemGrow(mem, new_size + mem->n, 1) != 0)
> 
> Looks like you could call sqlVdbeMemGrow() without all these checks
> above. The grow function does them already.
> 
True, fixed. There is a case where the behavior will be different - I
mean the case where new_size < szMalloc and new_size + n > szMalloc,
but that doesn't seem to be a very common case.

Sorry, I wrote this comment in the wrong place first (in the first patch).
> > +			return -1;
> > +	}
> > +	memcpy(&mem->z[mem->n], value, len);
> > +	mem->n = new_size;
> > +	return 0;
> > +}

Diff:

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index de4c41ccf..343a730b3 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -1934,15 +1934,12 @@ mem_append(struct Mem *mem, const char *value, uint32_t len)
 	if (len == 0)
 		return 0;
 	int new_size = mem->n + len;
-	if (((mem->flags & (MEM_Static | MEM_Dyn | MEM_Ephem)) != 0) ||
-	    mem->szMalloc < new_size) {
-		/*
-		 * Force exponential buffer size growth to avoid having to call
-		 * this routine too often.
-		 */
-		if (sqlVdbeMemGrow(mem, new_size + mem->n, 1) != 0)
-			return -1;
-	}
+	/*
+	 * Force exponential buffer size growth to avoid having to call this
+	 * routine too often.
+	 */
+	if (sqlVdbeMemGrow(mem, new_size + mem->n, 1) != 0)
+		return -1;
 	memcpy(&mem->z[mem->n], value, len);
 	mem->n = new_size;
 	return 0;


New patch:


commit ea59a75e3ebbe7138fab0bb73d25a46367698e74
Author: Mergen Imeev <imeevma at gmail.com>
Date:   Mon Sep 6 13:49:47 2021 +0300

    sql: introduce mem_append()
    
    This patch introduces the mem_append() function. This function appends
    the specified string to the end of the STRING or VARBINARY contained in
    MEM. In case MEM needs to increase the size of allocated memory,
    extra memory is allocated in an attempt to reduce the total number of
    allocations.
    
    Needed for #4145

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index f1b61dec6..343a730b3 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -1927,6 +1927,24 @@ mem_move(struct Mem *to, struct Mem *from)
 	from->zMalloc = NULL;
 }
 
+int
+mem_append(struct Mem *mem, const char *value, uint32_t len)
+{
+	assert((mem->type & (MEM_TYPE_BIN | MEM_TYPE_STR)) != 0);
+	if (len == 0)
+		return 0;
+	int new_size = mem->n + len;
+	/*
+	 * Force exponential buffer size growth to avoid having to call this
+	 * routine too often.
+	 */
+	if (sqlVdbeMemGrow(mem, new_size + mem->n, 1) != 0)
+		return -1;
+	memcpy(&mem->z[mem->n], value, len);
+	mem->n = new_size;
+	return 0;
+}
+
 int
 mem_concat(const struct Mem *a, const struct Mem *b, struct Mem *result)
 {
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 1574da22d..b567aea50 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -582,6 +582,14 @@ mem_copy_as_ephemeral(struct Mem *to, const struct Mem *from);
 void
 mem_move(struct Mem *to, struct Mem *from);
 
+/**
+ * Append the given string to the end of the STRING or VARBINARY contained in
+ * MEM. In case MEM needs to increase the size of allocated memory, additional
+ * memory is allocated in an attempt to reduce the total number of allocations.
+ */
+int
+mem_append(struct Mem *mem, const char *value, uint32_t len);
+
 /**
  * Concatenate strings or binaries from the first and the second MEMs and write
  * to the result MEM. In case the first MEM or the second MEM is NULL, the
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 4c2bd11ba..3015760e1 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1294,14 +1294,9 @@ sqlVdbeList(Vdbe * p)
 					if (mem_copy_bin(pSub, bin, size) != 0)
 						return -1;
 				} else if (j == nSub) {
-					struct Mem tmp;
-					mem_create(&tmp);
-					uint32_t size = sizeof(SubProgram *);
 					char *bin = (char *)&pOp->p4.pProgram;
-					mem_set_bin_ephemeral(&tmp, bin, size);
-					int rc = mem_concat(pSub, &tmp, pSub);
-					mem_destroy(&tmp);
-					if (rc != 0)
+					uint32_t size = sizeof(SubProgram *);
+					if (mem_append(pSub, bin, size) != 0)
 						return -1;
 				}
 			}


More information about the Tarantool-patches mailing list