From: Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v4 06/16] sql: introduce mem_append() Date: Tue, 19 Oct 2021 13:49:10 +0300 [thread overview] Message-ID: <20211019104910.GA160099@tarantool.org> (raw) In-Reply-To: <797b7cc5-800a-69a2-0d4d-51727d0a5607@tarantool.org> Hi! Thank you for the review! My answer, diff and new patch below. On Mon, Oct 11, 2021 at 11:50:08PM +0200, Vladislav Shpilevoy wrote: > Hi! Thanks for the fixes! > > >>> 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. > > Wait, this does not look correct either. I thought sqlVdbeMemGrow() > does the exponential grow. But if it does not, we need to either patch > it, or keep some checks above. The 'standard' way is to grow only when > necessary, but at least x2. How this new way works I can't tell. It > might be worse. You will always have unused free space. Lets either > stick to the old way or patch the grow function. I think it's better to use the old way, because in all other cases we only need to reallocate the memory once, and it would be in vain to double it every time. Diff: diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index 343a730b3..de4c41ccf 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -1934,12 +1934,15 @@ mem_append(struct Mem *mem, const char *value, uint32_t len) 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; + 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; New patch: commit 7cb6f95fcf4a85b36e8c4a3f41cbcc5e8741e417 Author: Mergen Imeev <imeevma@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..de4c41ccf 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -1927,6 +1927,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(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; } }
next prev parent reply other threads:[~2021-10-19 10:49 UTC|newest] Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-10-01 12:48 [Tarantool-patches] [PATCH v4 00/16] sql: refactor aggregate functions Mergen Imeev via Tarantool-patches 2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 01/16] sql: remove MEM_Zero flag from struct MEM Mergen Imeev via Tarantool-patches 2021-10-04 21:51 ` Vladislav Shpilevoy via Tarantool-patches 2021-10-05 8:46 ` Mergen Imeev via Tarantool-patches 2021-10-05 9:42 ` Mergen Imeev via Tarantool-patches 2021-10-05 12:28 ` Mergen Imeev via Tarantool-patches 2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 02/16] sql: fix possible undefined behavior during cast Mergen Imeev via Tarantool-patches 2021-10-04 21:52 ` Vladislav Shpilevoy via Tarantool-patches 2021-10-05 9:00 ` Mergen Imeev via Tarantool-patches 2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 03/16] sql: use register P1 for number of arguments Mergen Imeev via Tarantool-patches 2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 04/16] sql: remove AggStep0 and OP_BuiltinFunction0 Mergen Imeev via Tarantool-patches 2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 05/16] sql: move collation to struct sql_context Mergen Imeev via Tarantool-patches 2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 06/16] sql: introduce mem_append() Mergen Imeev via Tarantool-patches 2021-10-04 21:52 ` Vladislav Shpilevoy via Tarantool-patches 2021-10-05 9:32 ` Mergen Imeev via Tarantool-patches 2021-10-11 21:50 ` Vladislav Shpilevoy via Tarantool-patches 2021-10-19 10:49 ` Mergen Imeev via Tarantool-patches [this message] 2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 07/16] sql: remove sql_vdbemem_finalize() Mergen Imeev via Tarantool-patches 2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 08/16] sql: refactor SUM() function Mergen Imeev via Tarantool-patches 2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 09/16] sql: refactor TOTAL() function Mergen Imeev via Tarantool-patches 2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 10/16] sql: refactor AVG() function Mergen Imeev via Tarantool-patches 2021-10-04 21:53 ` Vladislav Shpilevoy via Tarantool-patches 2021-10-05 9:48 ` Mergen Imeev via Tarantool-patches 2021-10-11 21:50 ` Vladislav Shpilevoy via Tarantool-patches 2021-10-19 11:14 ` Mergen Imeev via Tarantool-patches 2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 11/16] sql: refactor COUNT() function Mergen Imeev via Tarantool-patches 2021-10-04 21:53 ` Vladislav Shpilevoy via Tarantool-patches 2021-10-05 9:55 ` Mergen Imeev via Tarantool-patches 2021-10-11 21:51 ` Vladislav Shpilevoy via Tarantool-patches 2021-10-19 11:17 ` Mergen Imeev via Tarantool-patches 2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 12/16] sql: refactor MIN() and MAX() functions Mergen Imeev via Tarantool-patches 2021-10-04 21:54 ` Vladislav Shpilevoy via Tarantool-patches 2021-10-05 10:07 ` Mergen Imeev via Tarantool-patches 2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 13/16] sql: refactor GROUP_CONCAT() function Mergen Imeev via Tarantool-patches 2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 14/16] sql: remove copying of result in finalizers Mergen Imeev via Tarantool-patches 2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 15/16] sql: remove MEM_TYPE_AGG Mergen Imeev via Tarantool-patches 2021-10-01 12:49 ` [Tarantool-patches] [PATCH v4 16/16] sql: remove field argv from struct sql_context Mergen Imeev via Tarantool-patches 2021-10-25 20:58 ` [Tarantool-patches] [PATCH v4 00/16] sql: refactor aggregate functions Vladislav Shpilevoy via Tarantool-patches 2021-10-26 10:34 Mergen Imeev via Tarantool-patches 2021-10-26 10:34 ` [Tarantool-patches] [PATCH v4 06/16] sql: introduce mem_append() 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=20211019104910.GA160099@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imeevma@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v4 06/16] sql: introduce mem_append()' \ /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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox