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 8DB686F872; Fri, 30 Apr 2021 00:09:46 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 8DB686F872 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1619730586; bh=uOOqn0FzdGVqHwheBVdP6iXTMkUbTa80LU7n/aR34Ss=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=ppyh7DB2NPlcms1C93zCjSFSweF1HO5axiqSXxfkXBsGFwkkBxBRaaORQroaR9u/T p6DEvQKK8fPuia8l2FzyvT8g7A52zIHWXUonTTj/kxL7Te4MmC6UUJBTJkzDpBmHzL NN+33bwxHm+y4QaM9ssBw+MZr9JqFHTlDQOcPftk= Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 00DE96F872 for ; Fri, 30 Apr 2021 00:09:44 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 00DE96F872 Received: by smtp45.i.mail.ru with esmtpa (envelope-from ) id 1lcDuq-0003F1-5A; Fri, 30 Apr 2021 00:09:44 +0300 To: imeevma@tarantool.org Cc: tarantool-patches@dev.tarantool.org References: <3f495b512c6d1a0e494c111ad37083f98c9988ad.1619540891.git.imeevma@gmail.com> Message-ID: <2ee164bc-76e0-0a42-aaa1-64583c3d7105@tarantool.org> Date: Thu, 29 Apr 2021 23:09:43 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: <3f495b512c6d1a0e494c111ad37083f98c9988ad.1619540891.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD95978C26455E69BE022900C3053DBED7681036D2C478A411E182A05F53808504046D6C39538ECA583BD73DC88D390FE27583669D952EE9090AC298F26272623C0 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7FBCED7D376B82B5EEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378D70459434292EC88638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B268C33441C9152820D8AD7654C07AC41B267281413F282BCCD2E47CDBA5A96583C09775C1D3CA48CFE97D2AE7161E217F117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE7A3E989B1926288338941B15DA834481F9449624AB7ADAF37BA3038C0950A5D3613377AFFFEAFD2691661749BA6B97735B0122D76E9D428ED7B076A6E789B0E97A8DF7F3B2552694A1E7802607F20496D49FD398EE364050FB28585415E75ADA93D5BA627BF9F2FCFB3661434B16C20AC78D18283394535A9E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8B2303E78B907142AC75ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2368A440D3B0F6089093C9A16E5BC824A2A04A2ABAA09D25379311020FFC8D4AD9380091DD32562D35E83B15DBCF54158 X-C1DE0DAB: 0D63561A33F958A52380D13D733E3A31AA84EBB4BA31BDEDC2C775AB7093DA0DD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3451BBE684D17D7221E9DCA89E9EB7C2A4EC1ED901D81136601B9AF93812C78C6B7DC88B62264166751D7E09C32AA3244CA06533FB2768B9F9D862C6FF18CE24EBA90944CA99CF22E3729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojVCmWsvwT1Har7JxRLbbJBQ== X-Mailru-Sender: 504CC1E875BF3E7D9BC0E5172ADA31104E83C43081694B5BEEA8A225B0BDC96DD7AFE68DAC5C114C07784C02288277CA03E0582D3806FB6A5317862B1921BA260ED6CFD6382C13A6112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2 3/3] sql: replace MEM-type flags by enum mem_type 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: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" I appreciate the work you did here! >>> mem->field_type = FIELD_TYPE_VARBINARY; >>> return 0; >>> }> @@ -1168,9 +1229,9 @@ mem_get_bin(const struct Mem *mem, const char **s) >>> int >>> mem_len(const struct Mem *mem, uint32_t *len) >>> { >>> - if ((mem->flags & (MEM_Str | MEM_Blob)) == 0) >>> + if (mem->type != MEM_STR && mem->type != MEM_BIN) >>> return -1; >> >> 8. Is it -1 for MAP and ARRAY intentionally? Previously they >> were included into MEM_Blob. >> >> I see mem_concat() didn't check for subtypes. Does it mean we >> could concat two MP_ARRAYs into something invalid? If we could, >> your patch probably just fixed it. Could you check and add a >> test if so? >> > Fixed. You were right, it is actually possible to concatenate two maps, map and > array, map and varbinary and so on. I returned ability to do this. Should I add > a test? That behaviour is not correct, and should raise an error I think. You can add a test, but if it works now, then it should have a comment saying that this test is bad and must start failing in the future. And now you might test it just to ensure it does not crash anywhere. >>> @@ -114,175 +120,153 @@ struct Mem { >> >> <...> >> >>> static inline bool >>> mem_is_agg(const struct Mem *mem) >>> { >>> - return (mem->flags & MEM_Agg) != 0; >>> + return mem->type == MEM_AGG; >>> } >>> >>> static inline bool >>> mem_is_bytes(const struct Mem *mem) >>> { >>> - return (mem->flags & (MEM_Blob | MEM_Str)) != 0; >>> + enum mem_type type = mem->type; >>> + return type == MEM_BIN || type == MEM_MAP || type == MEM_ARRAY || >>> + type == MEM_STR; >>> } >> 15. It was good when we could check several rare cases at one >> branch. Maybe you could preserve the bitwise structure of the >> types? Then we could keep the bit operations and save a few >> branches. You didn't think of it, or do we go for normal numbers >> intentionally? >> > I did it, although I'm not sure if this is the right choice: although it makes > our code more compact in some cases, it makes it a little less readable. I just realized another issue which you will get with UUID and DECIMAL. They are not present in mp_type, so there won't be a direct mapping anyway. They are encoded as MP_EXT + MP_UUID/MP_DECIMAL. It fits into a 64bit integer, but it is a binary header, and is not in mp_type value range. The same for MEM_TYPE_PTR, MEM_TYPE_FRAME, and other SQL-specific. See 5 comments below. > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > index b6ff6397f..4f189cac4 100644 > --- a/src/box/sql/mem.c > +++ b/src/box/sql/mem.c > @@ -89,39 +89,36 @@ mem_str(const struct Mem *mem) <...> > static inline void > mem_clear(struct Mem *mem) > { > - if ((mem->flags & (MEM_Agg | MEM_Dyn | MEM_Frame)) != 0) { > - if ((mem->flags & MEM_Agg) != 0) > - sql_vdbemem_finalize(mem, mem->u.func); > - assert((mem->flags & MEM_Agg) == 0); > - if ((mem->flags & MEM_Dyn) != 0) { > - assert(mem->xDel != SQL_DYNAMIC && mem->xDel != NULL); > - mem->xDel((void *)mem->z); > - } else if ((mem->flags & MEM_Frame) != 0) { > - struct VdbeFrame *frame = mem->u.pFrame; > - frame->pParent = frame->v->pDelFrame; > - frame->v->pDelFrame = frame; > - } > - } > - mem->flags = MEM_Null; > + if (mem->type == MEM_TYPE_AGG) { > + sql_vdbemem_finalize(mem, mem->u.func); > + assert(mem->type != MEM_TYPE_AGG); > + } else if (mem->type == MEM_TYPE_FRAME) { > + struct VdbeFrame *frame = mem->u.pFrame; > + frame->pParent = frame->v->pDelFrame; > + frame->v->pDelFrame = frame; > + } else if ((mem->flags & MEM_Dyn) != 0) { > + assert(mem->xDel != SQL_DYNAMIC && mem->xDel != NULL); > + mem->xDel((void *)mem->z); > + } 1. With the new bitwise structure you could reduce the diff. For example, here there was one 'if' for a non-common case of something allocated dynamically. Now there are 3 'if's. > + mem->type = MEM_TYPE_NULL; > + mem->flags = 0; > mem->field_type = field_type_MAX; > } > @@ -268,11 +271,13 @@ mem_set_str0_allocated(struct Mem *mem, char *value) > int > mem_copy_str(struct Mem *mem, const char *value, uint32_t len) > { > - if ((mem->flags & (MEM_Str | MEM_Blob)) != 0 && mem->z == value) { > + if ((mem->type == MEM_TYPE_STR || > + mem->type == MEM_TYPE_BIN) && mem->z == value) { 2. The only reason I proposed to keep the types in their own bits was to reduce number of the branches in the 'if's. So as you could write flags & (MEM_TYPE_STR | MEM_TYPE_BIN) != 0 for checking for multiple types instead of using || which adds an implicit branch. The same in mem_copy_bin(), mem_to_int(), mem_to_int_precise(), mem_to_double(), mem_to_number(), end of mem_cast_explicit() (the FIELD_TYPE_SCALAR case), in a few places in mem_cast_implicit(), mem_cast_implicit_old(), mem_get_int(), mem_get_uint(), try_return_null(), mem_concat(), get_number(), mem_cmp_bin(), sqlVdbeCheckMemInvariants(), end of memTracePrint(). Maybe more which I missed. > /* Own value, but might be ephemeral. Make it own if so. */ > if (sqlVdbeMemGrow(mem, len, 1) != 0) > return -1; > - mem->flags = MEM_Str; > + mem->type = MEM_TYPE_STR; > + mem->flags = 0; > mem->field_type = FIELD_TYPE_STRING; > return 0; > } > @@ -2258,40 +2330,37 @@ sqlVdbeMemTooBig(Mem * p) > int > sqlMemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pColl) > { > - int f1, f2; > int res; > - int combined_flags; > > - f1 = pMem1->flags; > - f2 = pMem2->flags; > - combined_flags = f1 | f2; > + enum mem_type type1 = pMem1->type; > + enum mem_type type2 = pMem2->type; > > /* If one value is NULL, it is less than the other. If both values > * are NULL, return 0. > */ > - if (combined_flags & MEM_Null) { > - return (f2 & MEM_Null) - (f1 & MEM_Null); > - } > + if (type1 == MEM_TYPE_NULL || type2 == MEM_TYPE_NULL) 3. With the bitwise types you could have combined_types and keep only one branch in such places. > + return (int)(type2 == MEM_TYPE_NULL) - > + (int)(type1 == MEM_TYPE_NULL); > > diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h > index 6fc15617d..a4a0d2223 100644 > --- a/src/box/sql/mem.h > +++ b/src/box/sql/mem.h > @@ -37,73 +37,78 @@ struct region; > struct mpstream; > struct VdbeFrame; > > -/* > - * Internally, the vdbe manipulates nearly all SQL values as Mem > - * structures. Each Mem struct may cache multiple representations (string, > - * integer etc.) of the same value. > - */ > +enum mem_type { > + MEM_TYPE_NULL = 1u << MP_NIL, > + MEM_TYPE_UINT = 1u << MP_UINT, > + MEM_TYPE_INT = 1u << MP_INT, > + MEM_TYPE_STR = 1u << MP_STR, > + MEM_TYPE_BIN = 1u << MP_BIN, > + MEM_TYPE_ARRAY = 1u << MP_ARRAY, > + MEM_TYPE_MAP = 1u << MP_MAP, > + MEM_TYPE_BOOL = 1u << MP_BOOL, > + MEM_TYPE_FLOAT = 1u << MP_FLOAT, > + MEM_TYPE_DOUBLE = 1u << MP_DOUBLE, > + MEM_TYPE_INVALID = 1u << MP_EXT, > + MEM_TYPE_FRAME = 1u << (MP_EXT + 1), > + MEM_TYPE_PTR = 1u << (MP_EXT + 2), > + MEM_TYPE_AGG = 1u << (MP_EXT + 3), 4. Why do you stick to mp_types? Why can't have your own bits? Anyway I don't see now any easy conversions between mem_type and mp_type. It is also kind of incorrect because MEM_TYPE_PTR = MP_EXT + 2 is not a pointer in MessagePack, and the same for the other extensions. > +}; > + > +/** Internally, the vdbe manipulates nearly all SQL values as Mem structures. */ > struct Mem { > union MemValue { > - double r; /* Real value used when MEM_Real is set in flags */ > - i64 i; /* Integer value used when MEM_Int is set in flags */ > - uint64_t u; /* Unsigned integer used when MEM_UInt is set. */ > - bool b; /* Boolean value used when MEM_Bool is set in flags */ > - int nZero; /* Used when bit MEM_Zero is set in flags */ > - void *p; /* Generic pointer */ > + /** Double value when MEM type is MEM_TYPE_DOUBLE. */ > + double r; 5. When I talked about fixing a comment location, I meant that single comment you needed to change anyway. But ok, lets change all alongside. > + /** Negative integer value when MEM type is MEM_TYPE_INT. */ > + i64 i; > + /** Unsigned integer value when MEM type is MEM_TYPE_UINT. */ > + uint64_t u; > + /** Boolean value when MEM type is MEM_TYPE_BOOL. */ > + bool b;