Tarantool development patches archive
 help / color / mirror / Atom feed
From: Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: v.shpilevoy@tarantool.org, tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 05/15] sql: introduce mem_append()
Date: Sat, 25 Sep 2021 14:06:05 +0300
Message-ID: <20210925110605.GB290467@tarantool.org> (raw)
In-Reply-To: <660d25eecfe9f32dc610f89da493622bba84f13e.1632220375.git.imeevma@gmail.com>

Thank you for the review! I added check for length of appended string. If it
is 0, than we do not have to do anything. The result can be static or ephemeral
now, but not sure that this should be an issue. On the other hand, we do not
have to allocate memory for static or ephemeral MEMs, when zero-length string is
appended to them. Diff and new patch below.

On Tue, Sep 21, 2021 at 01:59:10PM +0300, Mergen Imeev via Tarantool-patches wrote:
> This patch introduces function mem_append(). This function appends 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. The main use of this function is to add multiple strings to
> a single MEM.
> 
> Needed for #4145
> ---
>  src/box/sql/mem.c     | 19 +++++++++++++++++++
>  src/box/sql/mem.h     |  8 ++++++++
>  src/box/sql/vdbeaux.c |  9 ++-------
>  3 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index cc0fd836b..a9011fc63 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -1953,6 +1953,25 @@ 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);
> +	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;
> +	}
> +	memcpy(&mem->z[mem->n], value, len);
> +	mem->n = new_size;
> +	return 0;
> +}
> +
>  int
>  mem_concat(struct Mem *a, struct Mem *b, struct Mem *result)
>  {
> diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
> index 0da45b8af..54a1e931b 100644
> --- a/src/box/sql/mem.h
> +++ b/src/box/sql/mem.h
> @@ -597,6 +597,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 b7e44a386..6f13d2b03 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -1300,14 +1300,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;
>  				}
>  			}
> -- 
> 2.25.1
> 

Diff:


diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index a9011fc63..f64cbe10a 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -1957,6 +1957,8 @@ 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) {


New patch:

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

    sql: introduce mem_append()
    
    This patch introduces function mem_append(). This function appends 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. The main use of this function is to add multiple strings to
    a single MEM.
    
    Needed for #4145

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index cc0fd836b..f64cbe10a 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -1953,6 +1953,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)
+			return -1;
+	}
+	memcpy(&mem->z[mem->n], value, len);
+	mem->n = new_size;
+	return 0;
+}
+
 int
 mem_concat(struct Mem *a, struct Mem *b, struct Mem *result)
 {
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 0da45b8af..54a1e931b 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -597,6 +597,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;
 				}
 			}

  reply	other threads:[~2021-09-25 11:06 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <cover.1632220375.git.imeevma@gmail.com>
2021-09-21 10:59 ` [Tarantool-patches] [PATCH v2 01/15] sql: fix possible undefined behavior during cast Mergen Imeev via Tarantool-patches
2021-09-21 10:59 ` [Tarantool-patches] [PATCH v2 02/15] sql: use register P1 for number of arguments Mergen Imeev via Tarantool-patches
2021-09-21 10:59 ` [Tarantool-patches] [PATCH v2 04/15] sql: move collation to struct sql_context Mergen Imeev via Tarantool-patches
2021-09-21 10:59 ` [Tarantool-patches] [PATCH v2 05/15] sql: introduce mem_append() Mergen Imeev via Tarantool-patches
2021-09-25 11:06   ` Mergen Imeev via Tarantool-patches [this message]
2021-09-21 10:59 ` [Tarantool-patches] [PATCH v2 06/15] sql: remove sql_vdbemem_finalize() Mergen Imeev via Tarantool-patches
2021-09-22 22:47   ` Vladislav Shpilevoy via Tarantool-patches
2021-09-25 11:13     ` Mergen Imeev via Tarantool-patches
2021-09-21 10:59 ` [Tarantool-patches] [PATCH v2 07/15] sql: rework SUM() Mergen Imeev via Tarantool-patches
2021-09-22 22:48   ` Vladislav Shpilevoy via Tarantool-patches
2021-09-25 11:17     ` Mergen Imeev via Tarantool-patches
2021-09-21 10:59 ` [Tarantool-patches] [PATCH v2 08/15] sql: rework TOTAL() Mergen Imeev via Tarantool-patches
2021-09-25 11:20   ` Mergen Imeev via Tarantool-patches
2021-09-21 10:59 ` [Tarantool-patches] [PATCH v2 09/15] sql: rework AVG() Mergen Imeev via Tarantool-patches
2021-09-22 22:48   ` Vladislav Shpilevoy via Tarantool-patches
2021-09-25 11:32     ` Mergen Imeev via Tarantool-patches
2021-09-21 10:59 ` [Tarantool-patches] [PATCH v2 10/15] sql: rework COUNT() Mergen Imeev via Tarantool-patches
2021-09-25 11:34   ` Mergen Imeev via Tarantool-patches
2021-09-21 10:59 ` [Tarantool-patches] [PATCH v2 11/15] sql: rework MIN() and MAX() Mergen Imeev via Tarantool-patches
2021-09-25 11:36   ` Mergen Imeev via Tarantool-patches
2021-09-21 10:59 ` [Tarantool-patches] [PATCH v2 12/15] sql: rework GROUP_CONCAT() Mergen Imeev via Tarantool-patches
2021-09-22 22:49   ` Vladislav Shpilevoy via Tarantool-patches
2021-09-25 11:42     ` Mergen Imeev via Tarantool-patches
2021-09-29  7:03       ` Mergen Imeev via Tarantool-patches
2021-09-21 10:59 ` [Tarantool-patches] [PATCH v2 13/15] sql: remove copying of result in finalizers Mergen Imeev via Tarantool-patches
2021-09-22 22:50   ` Vladislav Shpilevoy via Tarantool-patches
2021-09-25 11:47     ` Mergen Imeev via Tarantool-patches
2021-09-21 10:59 ` [Tarantool-patches] [PATCH v2 14/15] sql: remove MEM_TYPE_AGG Mergen Imeev via Tarantool-patches
2021-09-21 10:59 ` [Tarantool-patches] [PATCH v2 15/15] sql: remove field argv from struct sql_context Mergen Imeev via Tarantool-patches
2021-09-22 22:51   ` Vladislav Shpilevoy via Tarantool-patches
2021-09-25 12:03     ` Mergen Imeev via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210925110605.GB290467@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git