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 369EE6B944; Tue, 13 Apr 2021 19:18:38 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 369EE6B944 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1618330718; bh=DDDIrng6kRdRF0fkA60GFF96ktkq4Ktyzlc/0cnbTBA=; 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=sG3ZKfcw9pFzTJ8u2qcoVP+MGdq+xr5h4QxSl85qcm15slq5RIdjr4muVHtMOeIAE Fl4bK1/DrTvVr3Om9aB/ASOfW1d1B2UHGhxkt5gY+tWWCFFVi3YCcvj4lBT3fmVz3B EZykICWZAmnG5VcfPqyse3fCg1pSmpVEjzLKOgnM= Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 7B5C06BD2D for ; Tue, 13 Apr 2021 19:18:36 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7B5C06BD2D Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1lWLkJ-0000gW-D2; Tue, 13 Apr 2021 19:18:35 +0300 Date: Tue, 13 Apr 2021 19:18:34 +0300 To: Vladislav Shpilevoy Message-ID: <20210413161834.GA161146@tarantool.org> References: <14bb43fcd1f34bef5c92279ae6c274d1e437cc86.1617984948.git.imeevma@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E7480257C85EA0BB7A95D5E28B957962BB550182A05F53808504046495459E7A9F706263C642C308F6621C798CE65A81E88773940D2EE2118850D X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE711269A7C2F827F16EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637B997C8222C70C3D98638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B277D37404031B2011EC3BEDCC7B1D56B4CB629EEF1311BF91D2E47CDBA5A96583C09775C1D3CA48CFED8438A78DFE0A9E117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE7820CF4CC0E318EFB9FA2833FD35BB23DF004C906525384302BEBFE083D3B9BA73A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E7354E672349037D5FA5C4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A56A61D04C97186E3FD04AAF18DCFED24900136D72DD647991D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D341ADA1A41A420E9B2800453410099C3F362CEC88E4D41ACEFD018388647122EC28B4E1EB13C0F3C9B1D7E09C32AA3244CFA4077FAA9D024A1AFDF012473FD665097FE24653F78E668FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojnA7/qPBUIXET+iF+y/0O1A== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822481D01F9EDAE9CC3545ED9CE0BC105BC83D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v5 13/52] sql: introduce mem_copy() 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" Thank you for the review! My answer, diff and new patch below. On Sun, Apr 11, 2021 at 08:06:24PM +0200, Vladislav Shpilevoy wrote: > Thanks for the patch! > > >> Also what was wrong with sqlVdbeMemCopy's way of using sqlVdbeMemMakeWriteable? > >> > > I see that this as a hack. It changes dynamic or allocated type (only type!) to > > ephemeral and then calls sqlVdbeMemMakeWriteable(), which converts ephemeral > > value to allocated value. Isn't it better to just directly copy? > > Yes, your way sounds better. > > > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > > index 25b2e75ee..ea3917fe3 100644 > > --- a/src/box/sql/mem.c > > +++ b/src/box/sql/mem.c > > @@ -267,6 +267,35 @@ mem_destroy(struct Mem *mem) > > mem->zMalloc = NULL; > > } > > > > +int > > +mem_copy(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 0; > > + if ((to->flags & MEM_Static) != 0) > > + return 0; > > + if ((to->flags & (MEM_Zero | MEM_Blob)) == (MEM_Zero | MEM_Blob)) > > + return sqlVdbeMemExpandBlob(to); > > + if (to->szMalloc == 0) > > + to->zMalloc = sqlDbMallocRaw(to->db, to->n); > > + else > > + to->zMalloc = sqlDbReallocOrFree(to->db, to->zMalloc, to->n); > > You can call realloc always. It turns into malloc when > the pointer is NULL, which is the case for szMalloc == 0 > I think. > Thanks, fixed. > > + if (to->zMalloc == NULL) > > + return -1; > > + to->szMalloc = sqlDbMallocSize(to->db, to->zMalloc); > > + memcpy(to->zMalloc, to->z, to->n); > > + to->z = to->zMalloc; > > + to->flags &= (MEM_Str | MEM_Blob | MEM_Term | MEM_Subtype); > > + return 0; > > +} Diff: diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index 78a4fe1a5..79b330141 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -141,10 +141,7 @@ mem_copy(struct Mem *to, const struct Mem *from) return 0; if ((to->flags & (MEM_Zero | MEM_Blob)) == (MEM_Zero | MEM_Blob)) return sqlVdbeMemExpandBlob(to); - if (to->szMalloc == 0) - to->zMalloc = sqlDbMallocRaw(to->db, to->n); - else - to->zMalloc = sqlDbReallocOrFree(to->db, to->zMalloc, to->n); + to->zMalloc = sqlDbReallocOrFree(to->db, to->zMalloc, to->n); if (to->zMalloc == NULL) return -1; to->szMalloc = sqlDbMallocSize(to->db, to->zMalloc); New patch: commit 38aceb6474bf967fc464561f5ea6e5e934257924 Author: Mergen Imeev Date: Tue Mar 23 00:53:03 2021 +0300 sql: introduce mem_copy() This patch introduces mem_copy(). This function copies value from source MEM to destination MEM. In case value is string or binary and have not static allocation type, it is copied to newly allocated memory. Part of #5818 diff --git a/src/box/sql/func.c b/src/box/sql/func.c index a0108220f..0b85bf365 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -1769,13 +1769,13 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv) bool is_max = (func->flags & SQL_FUNC_MAX) != 0; cmp = sqlMemCompare(pBest, pArg, pColl); if ((is_max && cmp < 0) || (!is_max && cmp > 0)) { - sqlVdbeMemCopy(pBest, pArg); + mem_copy(pBest, pArg); } else { sqlSkipAccumulatorLoad(context); } } else { pBest->db = sql_context_db_handle(context); - sqlVdbeMemCopy(pBest, pArg); + mem_copy(pBest, pArg); } } diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index 5eef15c62..79b330141 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -125,6 +125,32 @@ mem_destroy(struct Mem *mem) mem->z = NULL; } +int +mem_copy(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 0; + if ((to->flags & MEM_Static) != 0) + return 0; + if ((to->flags & (MEM_Zero | MEM_Blob)) == (MEM_Zero | MEM_Blob)) + return sqlVdbeMemExpandBlob(to); + to->zMalloc = sqlDbReallocOrFree(to->db, to->zMalloc, to->n); + if (to->zMalloc == NULL) + return -1; + to->szMalloc = sqlDbMallocSize(to->db, to->zMalloc); + memcpy(to->zMalloc, to->z, to->n); + to->z = to->zMalloc; + to->flags &= (MEM_Str | MEM_Blob | MEM_Term | MEM_Subtype); + return 0; +} + static inline bool mem_has_msgpack_subtype(struct Mem *mem) { @@ -1818,28 +1844,6 @@ vdbe_mem_alloc_blob_region(struct Mem *vdbe_mem, uint32_t size) return 0; } -/* - * Make a full copy of pFrom into pTo. Prior contents of pTo are - * freed before the copy is made. - */ -int -sqlVdbeMemCopy(Mem * pTo, const Mem * pFrom) -{ - int rc = 0; - - mem_clear(pTo); - memcpy(pTo, pFrom, MEMCELLSIZE); - pTo->flags &= ~MEM_Dyn; - if (pTo->flags & (MEM_Str | MEM_Blob)) { - if (0 == (pFrom->flags & MEM_Static)) { - pTo->flags |= MEM_Ephem; - rc = sqlVdbeMemMakeWriteable(pTo); - } - } - - return rc; -} - void sqlVdbeMemShallowCopy(Mem * pTo, const Mem * pFrom, int srcType) { diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h index 041d8a414..86dcdaec0 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -306,6 +306,14 @@ mem_create(struct Mem *mem); void mem_destroy(struct Mem *mem); +/** + * 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 to + * newly allocated by destination MEM memory. + */ +int +mem_copy(struct Mem *to, const struct Mem *from); + /** * Simple type to str convertor. It is used to simplify * error reporting. @@ -553,7 +561,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); -int sqlVdbeMemCopy(Mem *, const Mem *); 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 eb7d77f7e..a4718cef7 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1008,11 +1008,7 @@ case OP_Copy: { pOut = &aMem[pOp->p2]; assert(pOut!=pIn1); while( 1) { - sqlVdbeMemShallowCopy(pOut, pIn1, MEM_Ephem); - Deephemeralize(pOut); -#ifdef SQL_DEBUG - pOut->pScopyFrom = 0; -#endif + mem_copy(pOut, pIn1); REGISTER_TRACE(p, pOp->p2+pOp->p3-n, pOut); if ((n--)==0) break; pOut++; diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c index 2a3561d42..7951996ea 100644 --- a/src/box/sql/vdbeapi.c +++ b/src/box/sql/vdbeapi.c @@ -229,7 +229,7 @@ sql_result_text64(sql_context * pCtx, void sql_result_value(sql_context * pCtx, sql_value * pValue) { - sqlVdbeMemCopy(pCtx->pOut, pValue); + mem_copy(pCtx->pOut, pValue); } void diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index 7ccc6a957..cd89fc899 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -2333,7 +2333,7 @@ sqlVdbeGetBoundValue(Vdbe * v, int iVar, u8 aff) if (!mem_is_null(pMem)) { sql_value *pRet = sqlValueNew(v->db); if (pRet) { - sqlVdbeMemCopy((Mem *) pRet, pMem); + mem_copy(pRet, pMem); sql_value_apply_type(pRet, aff); } return pRet; diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c index bb87bb902..91cba9962 100644 --- a/src/box/sql/vdbemem.c +++ b/src/box/sql/vdbemem.c @@ -415,8 +415,7 @@ stat4ValueFromExpr(Parse * pParse, /* Parse context */ if ((v = pParse->pReprepare) != 0) { pVal = valueNew(db, pAlloc); if (pVal) { - rc = sqlVdbeMemCopy((Mem *) pVal, - &v->aVar[iBindVar - 1]); + rc = mem_copy(pVal, &v->aVar[iBindVar - 1]); if (rc == 0) sql_value_apply_type(pVal, type); pVal->db = pParse->db;