From: Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Timur Safin <tsafin@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] FW: [PATCH v1 09/10] sql: refactor vdbeaux.c Date: Sat, 13 Feb 2021 18:33:57 +0300 [thread overview] Message-ID: <20210213153357.GC110441@tarantool.org> (raw) In-Reply-To: <048e01d6fec9$1b363fb0$51a2bf10$@tarantool.org> On Tue, Feb 09, 2021 at 12:51:14PM +0300, Timur Safin wrote: > > > -----Original Message----- > From: Timur Safin <tsafin@tarantool.org> > Sent: Tuesday, February 9, 2021 12:50 PM > To: 'imeevma@tarantool.org' <imeevma@tarantool.org> > Subject: RE: [PATCH v1 09/10] sql: refactor vdbeaux.c > > There are similar notes about neg/pos vs signed/unsigned in > function names as I've mentioned earlier. Answered in another letter. > > But also some extra comments, please see below... > > : From: imeevma@tarantool.org <imeevma@tarantool.org> > : Subject: [PATCH v1 09/10] sql: refactor vdbeaux.c > : > : --- > : src/box/sql/vdbeaux.c | 266 +++++++++++++++++++----------------------- > : 1 file changed, 122 insertions(+), 144 deletions(-) > : > : diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c > : index 7b8a1e1d8..4ffb34a4e 100644 > : --- a/src/box/sql/vdbeaux.c > : +++ b/src/box/sql/vdbeaux.c > : @@ -1382,13 +1376,25 @@ sqlVdbeList(Vdbe * p) > : if (apSub[j] == pOp->p4.pProgram) > : break; > : } > : - if (j == nSub && > : - sqlVdbeMemGrow(pSub, nByte, > : - nSub != 0) == 0) { > : - apSub = (SubProgram **) pSub->z; > : - apSub[nSub++] = pOp->p4.pProgram; > : - pSub->flags |= MEM_Blob; > : - pSub->n = nSub * sizeof(SubProgram *); > : + if (j == nSub) { > : + size_t svp = region_used(&fiber()->gc); > : + struct SubProgram **buf = (SubProgram **) > : + region_aligned_alloc(&fiber()->gc, > : + nByte, > : + alignof(struct > : SubProgram)); > : + if (buf == NULL) { > : + diag_set(OutOfMemory, nByte, > : + "region_aligned_alloc", > : + "buf"); > : + p->is_aborted = true; > : + return -1; > : + } > : + if (nSub > 0) > : + memcpy(buf, pSub->z, pSub->n); > : + buf[nSub++] = pOp->p4.pProgram; > : + mem_set_bin(pSub, (char *)buf, nByte, 0, > : + false); > : + region_truncate(&fiber()->gc, svp); > : } > : } > : } > > This was quite unexpected. I'd rather expect that refactoring would > reduce number of code used inside of functions, not make them > verbose. Could you please explain me why this extra code would > become necessary? (And why not wrap them elsewhere?) > In this case function changed MEM without using mem_set_*() functions. I believe that is not what we want. Now it each time copy value from MEM, append new value to copied value and set it to MEM. It may be not so efficient, however I believe it shouldn't be a problem here since EXPLAIN is not something that should be as fast as possible. > : @@ -1402,41 +1408,26 @@ sqlVdbeList(Vdbe * p) > : mem_set_i64(pMem, pOp->p3); > : pMem++; > : > : - if (sqlVdbeMemClearAndResize(pMem, 256)) { > : - assert(p->db->mallocFailed); > : - return -1; > : - } > : - pMem->flags = MEM_Str | MEM_Term; > : - zP4 = displayP4(pOp, pMem->z, pMem->szMalloc); > : - > : - if (zP4 != pMem->z) { > : - pMem->n = 0; > : - sqlVdbeMemSetStr(pMem, zP4, -1, 1, 0); > : - } else { > : - assert(pMem->z != 0); > : - pMem->n = sqlStrlen30(pMem->z); > : - } > : + size_t size = 256; > : + char *tmp_buf = (char *) static_alloc(size); > : + assert(tmp_buf != NULL); > : + zP4 = displayP4(pOp, tmp_buf, size); > : + mem_set_str(pMem, zP4, strlen(zP4), 0, true); > : pMem++; > > Please, please, not introduce any newer usage of static_alloc, > it's very fragile and might work only inside of controlled context of > single module (i.e. swim). If used by multiple modules - > results are unpredictable. Any other allocator is fine if used > by multiple fibers, but not static_alloc. Replaced by region_alloc(). > > Regards, > Timur >
next prev parent reply other threads:[~2021-02-13 15:34 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-02-01 8:14 [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking Mergen Imeev via Tarantool-patches 2021-02-01 8:14 ` [Tarantool-patches] [PATCH v1 01/10] sql: introduce mem_set_*() functions Mergen Imeev via Tarantool-patches 2021-02-01 8:14 ` [Tarantool-patches] [PATCH v1 02/10] sql: Initialize MEM in sqlVdbeAllocUnpackedRecord() Mergen Imeev via Tarantool-patches 2021-02-01 8:14 ` [Tarantool-patches] [PATCH v1 03/10] sql: introduce mem_is_*() functions Mergen Imeev via Tarantool-patches 2021-02-01 8:14 ` [Tarantool-patches] [PATCH v1 04/10] sql: introduce mem_convert_to_binary() Mergen Imeev via Tarantool-patches 2021-02-01 8:14 ` [Tarantool-patches] [PATCH v1 05/10] sql: refactor vdbesort.c Mergen Imeev via Tarantool-patches 2021-02-01 8:15 ` [Tarantool-patches] [PATCH v1 06/10] sql: refactor sql/func.c Mergen Imeev via Tarantool-patches 2021-02-01 8:15 ` [Tarantool-patches] [PATCH v1 07/10] sql: refactor vdbetrace.c Mergen Imeev via Tarantool-patches 2021-02-01 8:15 ` [Tarantool-patches] [PATCH v1 08/10] sql: refactor vdbeapi.c Mergen Imeev via Tarantool-patches 2021-02-01 8:15 ` [Tarantool-patches] [PATCH v1 09/10] sql: refactor vdbeaux.c Mergen Imeev via Tarantool-patches 2021-02-09 9:51 ` [Tarantool-patches] FW: " Timur Safin via Tarantool-patches 2021-02-13 15:33 ` Mergen Imeev via Tarantool-patches [this message] 2021-02-28 17:35 ` Vladislav Shpilevoy via Tarantool-patches 2021-02-01 8:15 ` [Tarantool-patches] [PATCH v1 10/10] sql: refactor vdbe.c Mergen Imeev via Tarantool-patches [not found] ` <047f01d6fec7$b5a90bb0$20fb2310$@tarantool.org> 2021-02-13 15:26 ` Mergen Imeev via Tarantool-patches 2021-02-09 9:36 ` [Tarantool-patches] [PATCH v1 00/10] Encapsulate MEM type changing and checking Timur Safin via Tarantool-patches 2021-02-13 15:13 ` 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=20210213153357.GC110441@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imeevma@tarantool.org \ --cc=tsafin@tarantool.org \ --subject='Re: [Tarantool-patches] FW: [PATCH v1 09/10] sql: refactor vdbeaux.c' \ /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