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 C7E676EC5E; Fri, 9 Apr 2021 20:38:24 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C7E676EC5E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617989904; bh=3fiij5rpYFFFAyOi7gDwbpdQcUCm+OTaYB7y+x/apYA=; h=To:Cc:Date:In-Reply-To:References:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=lK90AU3zze5WjO5pwbBr9mUvlGw6Z5Kf8s64Kt0J4N8opc0f4JxaR4mMZDK1MD92O 07pGfRafrDR52s6CDWIAXMUOD91y6Mel5WDZcDR6QJWYeMcmuVuxock3Zqfn27dD2/ YSDL7Nu6j/B+2vX+urJre8Dvdg3aRDP7/yyPQgaY= 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 63D436EC5E for ; Fri, 9 Apr 2021 20:36:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 63D436EC5E Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lUv3y-0007Zo-Hg; Fri, 09 Apr 2021 20:36:59 +0300 To: v.shpilevoy@tarantool.org, tsafin@tarantool.org Cc: tarantool-patches@dev.tarantool.org Date: Fri, 9 Apr 2021 20:36:58 +0300 Message-Id: <91cec4dbcc1d931b70cdba3c70d77e7c58a00675.1617984948.git.imeevma@gmail.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E7480BE79914FF86F9151AC38CC435EA4A654182A05F538085040701B45EEB9FA19B38E9CD29451F8B16D524B2DF64E60E3197E6B90B0CD8AC2B6 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE700B5EAEB6F2DE1BDEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006375F0BD5CF353A411D8638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B282EEB9AFF3804802F68B596A23E8D7704A8204C501E24153D2E47CDBA5A96583C09775C1D3CA48CFED8438A78DFE0A9E117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE709B92020B71E24959FA2833FD35BB23DF004C906525384302BEBFE083D3B9BA73A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E735E4A630A5B664A4FFC4224003CC83647689D4C264860C145E X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2AD77751E876CB595E8F7B195E1C978312BB74A4DD3278FF20ECBE94767D537CD X-C1DE0DAB: C20DE7B7AB408E4181F030C43753B8186998911F362727C414F749A5E30D975CD0035DD76F8A8A4FFA16E2B56A31967743A097166FF64CB19C2B6934AE262D3EE7EAB7254005DCED7532B743992DF240BDC6A1CF3F042BAD6DF99611D93F60EF0417BEADF48D1460699F904B3F4130E343918A1A30D5E7FCCB5012B2E24CD356 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34B9F55CA4D2956E305828ADEE9A425383AFE9F4ACB94B3E557206FE32FF0DFC0E563526055257F4FF1D7E09C32AA3244CB4C7B312C21D04826D3D4698751DF9337C0C08F7987826B9FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojyO2lHpuZu4TgQdoZrVfEOg== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822ED7A81BC0B6D708E750D3A252858D81383D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: [Tarantool-patches] [PATCH v5 14/52] sql: introduce mem_copy_as_ephemeral() 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: imeevma@tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thank you for the review! My answers and new patch below. On 30.03.2021 02:01, Vladislav Shpilevoy wrote: > Thanks for the patch! > > See 3 comments below. > > On 23.03.2021 10:35, Mergen Imeev via Tarantool-patches wrote: >> This patch intoduces mem_copy_as_ephemeral(). This function copies value >> from source MEM to destination MEM. In case value is of string or binary >> type and its allocation type is not static, it copied as value with >> ephemeral allocation type. >> >> Part of #5818 >> --- >> src/box/sql/mem.c | 58 ++++++++++++++++++++-------------------------- >> src/box/sql/mem.h | 15 ++++++------ >> src/box/sql/vdbe.c | 10 ++++---- >> 3 files changed, 38 insertions(+), 45 deletions(-) >> >> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c >> index f12441d7c..30c568970 100644 >> --- a/src/box/sql/mem.c >> +++ b/src/box/sql/mem.c >> @@ -289,6 +289,25 @@ mem_copy(struct Mem *to, const struct Mem *from) >> return 0; >> } >> >> +int >> +mem_copy_as_ephemeral(struct Mem *to, const struct Mem *from) > > 1. Why didn't you keep its return type void? > Fixed, now this function has void return type. >> +{ >> + mem_clear(to); >> + to->u = from->u; >> + to->flags = from->flags; >> + to->subtype = from->subtype; >> + to->field_type = from->field_type; >> + to->n = from->n; >> + to->z = from->z; >> + if ((to->flags & (MEM_Str | MEM_Blob)) == 0) >> + return 0; >> + if ((to->flags & (MEM_Static | MEM_Ephem)) != 0) >> + return 0; >> + to->flags &= (MEM_Str | MEM_Blob | MEM_Term | MEM_Zero | MEM_Subtype); >> + to->flags |= MEM_Ephem; >> + return 0; >> +} >> @@ -593,9 +598,12 @@ sqlVdbeMemAboutToChange(Vdbe * pVdbe, Mem * pMem) >> int i; >> Mem *pX; >> for (i = 0, pX = pVdbe->aMem; i < pVdbe->nMem; i++, pX++) { >> - if (pX->pScopyFrom == pMem) { >> - pX->flags |= MEM_Undefined; >> - pX->pScopyFrom = 0; >> + if ((pX->flags & (MEM_Blob | MEM_Str)) != 0 && >> + (pX->flags & (MEM_Ephem | MEM_Static)) == 0) { >> + if (pX->pScopyFrom == pMem) { >> + pX->flags |= MEM_Undefined; >> + pX->pScopyFrom = 0; >> + } > > 2. Why did you change that? > This check is only useful for strings and binaries, since they may be lost due to change of another MEM. Also, due to this function it was possible that value of type other than MEM_Blob or MEM_Str will have MEM_Ephem set. This is wrong, I believe. >> } >> } >> pMem->pScopyFrom = 0; >> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c >> index 05e0f78c1..55083fb23 100644 >> --- a/src/box/sql/vdbe.c >> +++ b/src/box/sql/vdbe.c >> @@ -979,7 +979,7 @@ case OP_Copy: { >> pOut = &aMem[pOp->p2]; >> assert(pOut!=pIn1); >> while( 1) { >> - sqlVdbeMemShallowCopy(pOut, pIn1, MEM_Ephem); >> + mem_copy_as_ephemeral(pOut, pIn1); >> Deephemeralize(pOut); > > 3. You could turn mem_copy_as_ephemeral + Deephemeralize into mem_copy > the previous commit where mem_copy was introduced. Thanks, fixed in previous patch. New patch: commit 91cec4dbcc1d931b70cdba3c70d77e7c58a00675 Author: Mergen Imeev Date: Thu Mar 4 18:19:06 2021 +0300 sql: introduce mem_copy_as_ephemeral() This patch intoduces mem_copy_as_ephemeral(). This function copies value from source MEM to destination MEM. In case value is of string or binary type and its allocation type is not static, it copied as value with ephemeral allocation type. Part of #5818 diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index ea3917fe3..f75661e04 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -296,6 +296,25 @@ mem_copy(struct Mem *to, const struct Mem *from) return 0; } +void +mem_copy_as_ephemeral(struct Mem *to, const struct Mem *from) +{ + mem_clear(to); + to->u = from->u; + to->flags = from->flags; + to->subtype = from->subtype; + to->field_type = from->field_type; + to->n = from->n; + to->z = from->z; + if ((to->flags & (MEM_Str | MEM_Blob)) == 0) + return; + if ((to->flags & (MEM_Static | MEM_Ephem)) != 0) + return; + to->flags &= (MEM_Str | MEM_Blob | MEM_Term | MEM_Zero | MEM_Subtype); + to->flags |= MEM_Ephem; + return; +} + static inline bool mem_has_msgpack_subtype(struct Mem *mem) { @@ -407,20 +426,6 @@ vdbeMemAddTerminator(Mem * pMem) return 0; } -/* - * Make an shallow copy of pFrom into pTo. Prior contents of - * pTo are freed. The pFrom->z field is not duplicated. If - * pFrom->z is used, then pTo->z points to the same thing as pFrom->z - * and flags gets srcType (either MEM_Ephem or MEM_Static). - */ -static SQL_NOINLINE void -vdbeClrCopy(Mem * pTo, const Mem * pFrom, int eType) -{ - mem_clear(pTo); - assert(!VdbeMemDynamic(pTo)); - sqlVdbeMemShallowCopy(pTo, pFrom, eType); -} - /* * Both *pMem1 and *pMem2 contain string values. Compare the two values * using the collation sequence pColl. As usual, return a negative , zero @@ -1989,22 +1994,6 @@ vdbe_mem_alloc_blob_region(struct Mem *vdbe_mem, uint32_t size) return 0; } -void -sqlVdbeMemShallowCopy(Mem * pTo, const Mem * pFrom, int srcType) -{ - assert(pTo->db == pFrom->db); - if (VdbeMemDynamic(pTo)) { - vdbeClrCopy(pTo, pFrom, srcType); - return; - } - memcpy(pTo, pFrom, MEMCELLSIZE); - if ((pFrom->flags & MEM_Static) == 0) { - pTo->flags &= ~(MEM_Dyn | MEM_Static | MEM_Ephem); - assert(srcType == MEM_Ephem || srcType == MEM_Static); - pTo->flags |= srcType; - } -} - /* * Transfer the contents of pFrom to pTo. Any existing value in pTo is * freed. If pFrom contains ephemeral data, a copy is made. diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h index af36f31a2..3898888ff 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -81,12 +81,6 @@ struct Mem { #endif }; -/* - * Size of struct Mem not including the Mem.zMalloc member or anything that - * follows. - */ -#define MEMCELLSIZE offsetof(Mem,zMalloc) - bool mem_is_null(const struct Mem *mem); @@ -176,6 +170,14 @@ mem_destroy(struct Mem *mem); int mem_copy(struct Mem *to, const struct Mem *from); +/** + * Copy content of MEM from one MEM to another. In case source MEM contains + * string or binary and allocation type is not STATIC, this value is copied as + * value with ephemeral allocation type. + */ +void +mem_copy_as_ephemeral(struct Mem *to, const struct Mem *from); + /* One or more of the following flags are set to indicate the validOK * representations of the value stored in the Mem struct. * @@ -475,7 +477,6 @@ mem_is_type_compatible(struct Mem *mem, enum field_type type); int vdbe_mem_alloc_blob_region(struct Mem *vdbe_mem, uint32_t size); -void sqlVdbeMemShallowCopy(Mem *, const Mem *, int); void sqlVdbeMemMove(Mem *, Mem *); int sqlVdbeMemMakeWriteable(Mem *); diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index abd49b9bb..bbeabf238 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -74,9 +74,12 @@ sqlVdbeMemAboutToChange(Vdbe * pVdbe, Mem * pMem) int i; Mem *pX; for (i = 0, pX = pVdbe->aMem; i < pVdbe->nMem; i++, pX++) { - if (pX->pScopyFrom == pMem) { - pX->flags |= MEM_Undefined; - pX->pScopyFrom = 0; + if (mem_is_bytes(pX) && !mem_is_ephemeral(pX) && + !mem_is_static(pX)) { + if (pX->pScopyFrom == pMem) { + pX->flags |= MEM_Undefined; + pX->pScopyFrom = 0; + } } } pMem->pScopyFrom = 0; @@ -946,7 +949,7 @@ case OP_Variable: { /* out2 */ goto too_big; } pOut = vdbe_prepare_null_out(p, pOp->p2); - sqlVdbeMemShallowCopy(pOut, pVar, MEM_Static); + mem_copy_as_ephemeral(pOut, pVar); UPDATE_MAX_BLOBSIZE(pOut); break; } @@ -1034,7 +1037,7 @@ case OP_SCopy: { /* out2 */ pIn1 = &aMem[pOp->p1]; pOut = &aMem[pOp->p2]; assert(pOut!=pIn1); - sqlVdbeMemShallowCopy(pOut, pIn1, MEM_Ephem); + mem_copy_as_ephemeral(pOut, pIn1); #ifdef SQL_DEBUG if (pOut->pScopyFrom==0) pOut->pScopyFrom = pIn1; #endif @@ -2313,7 +2316,7 @@ case OP_Column: { if (mem_is_null(pDest) && (uint32_t) p2 >= pC->field_ref.field_count && default_val_mem != NULL) { - sqlVdbeMemShallowCopy(pDest, default_val_mem, MEM_Static); + mem_copy_as_ephemeral(pDest, default_val_mem); } pDest->field_type = field_type; op_column_out: @@ -4491,7 +4494,7 @@ case OP_Param: { /* out2 */ pOut = vdbe_prepare_null_out(p, pOp->p2); pFrame = p->pFrame; pIn = &pFrame->aMem[pOp->p1 + pFrame->aOp[pFrame->pc].p1]; - sqlVdbeMemShallowCopy(pOut, pIn, MEM_Ephem); + mem_copy_as_ephemeral(pOut, pIn); break; }