From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id A991C6EC6F; Sat, 13 Feb 2021 18:34:03 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A991C6EC6F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1613230443; bh=w0cvlZO/p6pO4+ZAo+o4g3Qh7IFjWyjq90DyWkq50Io=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=Mfthih14pm9m6Q0THO69P7p/tvZVTVLfoZkp/fKsagXmrEzIbxtOb8iSiupXB9mcd P2soHciuPVaxXzZDV0QgTsNeko3vwix7JUDFIQSPNAMXe4qv6q9/8jwX+2/i8AzAdk w6nccLlL+1bJzheSR6p04XKjJXtE4pG8qFet2/eI= Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id B189A6EC6F for ; Sat, 13 Feb 2021 18:34:02 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B189A6EC6F Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lAwvm-00078B-Po; Sat, 13 Feb 2021 18:33:59 +0300 Date: Sat, 13 Feb 2021 18:33:57 +0300 To: Timur Safin Message-ID: <20210213153357.GC110441@tarantool.org> References: <048e01d6fec9$1b363fb0$51a2bf10$@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <048e01d6fec9$1b363fb0$51a2bf10$@tarantool.org> X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD981647AC6901E234B663C574FBA2C95D46270E7B1163DBA8E182A05F538085040F123C726A39F5A30CFA0EACE3FF9F687942FF102DD123B98AC4B62704C7C0860 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7D73594321916E098C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE7978947DCA0D4215FEA1F7E6F0F101C674E70A05D1297E1BBC6CDE5D1141D2B1C06BDF07A52BA6F1C8E19D6005D28B5CBF4CCD22B914093069FA2833FD35BB23D9E625A9149C048EE9ECD01F8117BC8BEA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18C26CFBAC0749D213D2E47CDBA5A96583BA9C0B312567BB23089D37D7C0E48F6CA18204E546F3947C616AD31D0D18CD5C57739F23D657EF2BC8A9BA7A39EFB7666BA297DBC24807EA089D37D7C0E48F6C8AA50765F7900637A261D47410D9EB31EFF80C71ABB335746BA297DBC24807EA27F269C8F02392CDC58410348177836EDE7CCDD3542A186E00306258E7E6ABB4E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A567CD5BBAFBE711C0D7AD63D90AECD18B03B56FCA023603D6D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D346B1FF97F6D0959A24F8438A246A04EB24E4F5F3F9C58544733F688730DC22515E268A7D73B08B9C01D7E09C32AA3244C8855A53C29A49137D1E2674737FB9FD2C3B3ADDA61883BB5FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj+JvDbeHF34yGZxwnkPGPuQ== X-Mailru-Sender: 689FA8AB762F73936BC43F508A06382288D7ABF77C15C84D0FB0A0075E9AB61383D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] FW: [PATCH v1 09/10] sql: refactor vdbeaux.c X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Mergen Imeev via Tarantool-patches Reply-To: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" On Tue, Feb 09, 2021 at 12:51:14PM +0300, Timur Safin wrote: > > > -----Original Message----- > From: Timur Safin > Sent: Tuesday, February 9, 2021 12:50 PM > To: '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 > : 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 >