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 D17386EC58; Mon, 2 Aug 2021 17:52:45 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D17386EC58 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627915965; bh=BoX67yCXKQYzKrM3NDqus2ghMjIy7v9dljAm1PB+WU8=; 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=ow23EGLqlkYEkL7zxgFRspWGo6vDr+Tm8CVBMnGeAaO9haHP8kE9hRzH9Uz/COjd+ N3DPqN2YMy4DKEuE3Kfk/YXu6p7i3hcgeHJOw5YLAS5y2LjCSzsImqNufvvE5IuRl8 CPwLo9DVL5k59CAMDLLwZBywmmsiAe2B0/K+HnU0= Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [94.100.177.102]) (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 0F2426EC58 for ; Mon, 2 Aug 2021 17:52:44 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0F2426EC58 Received: by smtp42.i.mail.ru with esmtpa (envelope-from ) id 1mAZJ5-0001nE-5y; Mon, 02 Aug 2021 17:52:43 +0300 Date: Mon, 2 Aug 2021 17:51:29 +0300 To: Sergey Ostanevich Message-ID: References: <1733a6045e7ae1ff2cac8c4a49bcdca3388f65aa.1625587322.git.skaplun@tarantool.org> <20210727135941.GR27855@tarantool.org> <9C3661B1-0D21-42B7-94F6-C9C14FCEBCCD@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <9C3661B1-0D21-42B7-94F6-C9C14FCEBCCD@tarantool.org> X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD941C43E597735A9C30A5AB0699C09BB51E5FD76225F0C99C3182A05F538085040A2B1DF84D8DC10750701B6DDA198BC0FDDAC546D81CE3793FDEA74731E5EFA33 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7081BBE264C6D7F42EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637B1850F7A782C2F9B8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8CB2CE5DD9A36440865F9E329B5FADCBF117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B1BE95B8C87527B4BA471835C12D1D977C4224003CC8364762BB6847A3DEAEFB0F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7C565C1E6824D8037B43847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A53DC6863B9A82E97BC712A5188AAC840A8DE67FE36E9162F1D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA751B940EDA0DFB0535410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34BDCC71B3781C9C95A90CEF141156E7143D0C09CD14062FE33C9C4D6304D8DD8CB46DA553E07E21EF1D7E09C32AA3244CAF7B177891518CA35A6CC38E2DED22F2D08D48398F32B4A6FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj9N286KAyvN6RmGEoUgJYoA== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB442302B3837817AFC28EF4729401905450ED3938688C942E7F2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 1/2] Add support for full-range 64 bit lightuserdata. 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: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergos! Thanks for the review! On 01.08.21, Sergey Ostanevich wrote: > Hi! Thanks for the patch! > > Some minor message fixes, one great gag from Mike’s code and a > test request. > > Regards, > Sergos > > > > > The new commit message is the following: > > > > =================================================================== > > Add support for full-range 64 bit lightuserdata. > > > > (cherry picked from commit e9af1abec542e6f9851ff2368e7f196b6382a44c) > > > > LuaJIT uses special NaN-tagging technique to store internal type on > > the Lua stack. In case LJ_GC64 first 13 bits are set in special NaN > ^^^^^^^ ^ > In case of the Fixed. > > type (0xfff8...). FPU generates the only one type. The next 4 bits are > ^^^^^^^^^^^ > Which one and how is it relevant? Yep, it can be dropped. > > > used for an internal LuaJIT type of object on stack. The next 47 bits > > are used for storing this object's content. For userdata, it is its > > address. In case arm64 the pointer can have more than 47 significant > ^^^^^ > For Fixed. > > bits [1]. In this case the error BADLU error is raised. > > > > For the support of full 64-bit range lightuserdata pointers two new > > fields in GCState are added: > > > > `lightudseg` - vector of segments of lightuserdata. Each element keeps > > 32-bit value. 25 MSB equal to MSB of lightuserdata address, the rest are > ^ > 64bit Fixed. > > filled with zeros. The length of the vector is power of 2. > > > > `lightudnum` - the length - 1 of aforementioned vector (up to 255). > > > > When lightuserdata is pushed on the stack, if its segment is not stored > > > in vector new value is appended on top of this vector. The maximum > ^^^^^^^^^ to Fixed. > > At first I want you to put it as ’not found’ instead of ’not stored’. > Then I start thinking over ‘on top’ for a vector and I got a strange > feeling... > > > Now tell me, every time you put a LUD pointer to stack you have to roll > over all present segments in this '>>>' plain loop below? Yep, praying for the "Fast path". Side note: Mike likes to "teach" people how they *should* write code, and how they *shouldn't*. Also, he often tells that huge slowdowns and lags, when the "wrong" code is running are teaching them to avoid bad code. I suppose, he thinks, that intensive usage of userdata is a bad pattern in Lua and more important in LuaJIT... > > --- a/src/lj_api.c > +++ b/src/lj_api.c > +#if LJ_64 > +static void *lightud_intern(lua_State *L, void *p) > +{ > + global_State *g = G(L); > + uint64_t u = (uint64_t)p; > + uint32_t up = lightudup(u); > + uint32_t *segmap = mref(g->gc.lightudseg, uint32_t); > + MSize segnum = g->gc.lightudnum; > + if (segmap) { > + MSize seg; > >>> + for (seg = 0; seg <= segnum; seg++) > >>> + if (segmap[seg] == up) /* Fast path. */ > >>> + return (void *)(((uint64_t)seg << LJ_LIGHTUD_BITS_LO) | lightudlo(u)); > + segnum++; > + } > + if (!((segnum-1) & segnum) && segnum != 1) { > + if (segnum >= (1 << LJ_LIGHTUD_BITS_SEG)) lj_err_msg(L, LJ_ERR_BADLU); > + lj_mem_reallocvec(L, segmap, segnum, segnum ? 2*segnum : 2u, uint32_t); > + setmref(g->gc.lightudseg, segmap); > + } > + g->gc.lightudnum = segnum; > + segmap[segnum] = up; > + return (void *)(((uint64_t)segnum << LJ_LIGHTUD_BITS_LO) | lightudlo(u)); > +} > +#endif > + > > Can’t help to laugh at Mike’s /* Fast path */, brilliant isn’t it? > Perhaps addition of a new segment is not so often - and is counted to 256 - > so we can easily sort the array each time to make it log(n) rather (n) for > each lua_pushlightuserdata()? Mike's style... Also, I suggest to avoid sorting optimization for now by two reasons: 1) We have no goal to beat everyone at ARM __yet__. Just make it breathing. 2) We have no performance tests to measure such changes (I hope __yet__, too). > > > > > > > See the iterative patch below. > > > > =================================================================== > > diff --git a/test/tarantool-tests/lj-49-bad-lightuserdata.test.lua b/test/tarantool-tests/lj-49-bad-lightuserdata.test.lua > > This one tests the LUD push/pop to/fro stack. How about those > > > all internal usage of lightuserdata (for hooks, > > profilers, built-in package, IR and so on) is changed to special values > > on Lua Stack. > > Can you add at least _some_ test to verify memprof is fine? Memprof avoids such extroversions. Do you mean the test for `jit.p`? The new commit message is the following: =================================================================== Add support for full-range 64 bit lightuserdata. (cherry picked from commit e9af1abec542e6f9851ff2368e7f196b6382a44c) LuaJIT uses special NaN-tagging technique to store internal type on the Lua stack. In case of LJ_GC64 the first 13 bits are set in special NaN type (0xfff8...). The next 4 bits are used for an internal LuaJIT type of object on stack. The next 47 bits are used for storing this object's content. For userdata, it is its address. For arm64 a pointer can have more than 47 significant bits [1]. In this case the error BADLU error is raised. For the support of full 64-bit range lightuserdata pointers two new fields in GCState are added: `lightudseg` - vector of segments of lightuserdata. Each element keeps 32-bit value. 25 MSB equal to MSB of lightuserdata 64-bit address, the rest are filled with zeros. The lentgh of the vector is power of 2. `lightudnum` - the length - 1 of aforementioned vector (up to 255). When lightuserdata is pushed on the stack, if its segment is not stored in vector new value is appended to of this vector. The maximum amount of segments is 256. BADLU error is raised in case when user tried to add userdata with the new 257-th segment, so the whole VA-space isn't covered by this patch. Also, in this patch all internal usage of lightuserdata (for hooks, profilers, built-in package, IR and so on) is changed to special values on Lua Stack. Also, conversion of TValue to FFI C type with store is no longer compiled for lightuserdata. [1]: https://www.kernel.org/doc/html/latest/arm64/memory.html Sergey Kaplun: * added the description and the test for the problem Resolves tarantool/tarantool#2712 Needed for tarantool/tarantool#6154 =================================================================== Branch is force-pushed. > -- Best regards, Sergey Kaplun