Tarantool development patches archive
 help / color / mirror / Atom feed
From: Timur Safin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: "imeevma" <imeevma@tarantool.org>,
	"Sergey Ostanevich" <sergos@tarantool.org>
Cc: <tarantool-patches@dev.tarantool.org>
Subject: [Tarantool-patches] FW: [PATCH v1 09/10] sql: refactor vdbeaux.c
Date: Tue, 9 Feb 2021 12:51:14 +0300	[thread overview]
Message-ID: <048e01d6fec9$1b363fb0$51a2bf10$@tarantool.org> (raw)
In-Reply-To: 



-----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.

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?)

: @@ -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.

Regards,
Timur


  reply	other threads:[~2021-02-09  9:51 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   ` Timur Safin via Tarantool-patches [this message]
2021-02-13 15:33     ` [Tarantool-patches] FW: " Mergen Imeev via Tarantool-patches
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='048e01d6fec9$1b363fb0$51a2bf10$@tarantool.org' \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=sergos@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