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 85F136EC40; Thu, 12 Aug 2021 20:58:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 85F136EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1628791139; bh=FuKmlg2Pw5Zx4m++ENLPHY78MFcUnITv/u50wzuq3FA=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=nYgoJf0H+cNZx9pZT7XSNvDAZzl5j9dJPxtT5B6Ko8f860ouMLivlxojz4MBvqzB9 UJLREjaPTvn2E1QmQrmZdyF05Ycx1HfQeKkY4Z3pxgJ+hxgCGreWTluVoosIHycYZb a1xzBYPISkLkYD++/8l+KGIYgL1RmkErTFRplreU= Received: from smtpng2.i.mail.ru (smtpng2.i.mail.ru [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 660156EC40 for ; Thu, 12 Aug 2021 20:58:57 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 660156EC40 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1mEEym-0005Pm-7a; Thu, 12 Aug 2021 20:58:56 +0300 Date: Thu, 12 Aug 2021 20:35:16 +0300 To: Sergey Kaplun Cc: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org Message-ID: <20210812173516.GN27855@tarantool.org> References: <20210618181416.25454-1-skaplun@tarantool.org> <20210801123417.GA27855@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD9D5AC6413C25DCF08CC98B8FCC5CD86F3182A05F53808504087942A89D8F2B9B12729B78502288EC1B5183A87AD4F6191D6EA2EECE508DEE8 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE78E88BD1CA827EF00C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE7195F30236A8D43B4EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BBCA57AF85F7723F256BC2C75212568E37983B02376FA0A00CC7F00164DA146DAFE8445B8C89999728AA50765F7900637F6B57BC7E64490618DEB871D839B7333395957E7521B51C2DFABB839C843B9C08941B15DA834481F8AA50765F7900637F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8B9149C560DC76099D75ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A5F341C5C5CB97AFE9F7052780FFB6FAA94E6E7F98CCFE4D94D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA752DA3D96DA0CEF5C48E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D342B94C1DAF75C4D226C3CAD0D98D01474A5310C3E6D7626561E918658094C22F06FC3FE267755BB121D7E09C32AA3244C0C8F330D4137E83DB9033862F607EB0DE3D93501275E802F927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojKW4rnL99YhKeFIWE+JCGaw== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D748F70C8D2B4C22A06BAA961998FC2E9A7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH] lua: refactor port_lua_do_dump and encode_lua_call 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: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Sergey, Thanks for the fixes! LGTM, with a several typos in the commit message mentioned below. Moreover, please rebase your branch on the current master to check nothing is broken. On 02.08.21, Sergey Kaplun wrote: > Hi, Igor! > > Thanks for the review! > > > The new commit message is the following: > > =================================================================== > lua: refactor port_lua_do_dump and encode_lua_call > > The old code flow was the following: > > 1) `struct port_lua` given to `port_lua_do_dump()` has Lua stack with > arguments to encode to MessagePack. > > 2) The main coroutine `tarantool_L` is used to call `encode_lua_call()` > or `encode_lua_call_16`() via `lua_cpcall()`. > > 3) Objects on port coroutine are encoded via `luamp_encode()` or > `luamp_encode_call16()`. > > 4) This encoding may raise an error on unprotected `port->L` coroutine. > This coroutine has no protected frame on it and this call should fail > in pure Lua. > > Calling anything on unprotected coroutine is not allowed in Lua [1]: > > | If an error happens outside any protected environment, Lua calls a > | panic function > > Lua 5.1 sets protection only for specific lua_State [2] and calls a > panic function if we raise an error on unprotected lua_State [3]. > > Nevertheless, no panic occurs now due to two facts: > * The first one is LuaJIT's support of C++ exception handling [4] that > allows to raise an error in Lua and catch it in C++ or vice versa. But > documentation still doesn't allow raising errors on unprotected > coroutines (at least we must use try-catch block). > * The second one is the patch made in LuaJIT to restore currently > executed coroutine, when C function or fast function raises an > error [5][6] (see the related issue here [7][8]). > > For these reasons, when an error occurs, the unwinder searches and finds > the C-protected stack frame from the `lua_cpcall()` for `tarantool_L` > coroutine and unwinds until that point (without aforementioned patches > LuaJIT just calls a panic function and exit). Typo: s/exit/exits/. > > If an error is raised, and `lua_cpcall()` returns not `LUA_OK`, then the > error from `port->L` coroutine is converted into a Tarantool error and a > diagnostic is set. > > The auxiliary usage of `tarantool_L` coroutine doesn't respect Lua > idiomatic of usage. Internal unwinder used on M1 is not such flexible, Typo: Too much "usage", so I propose the following wording for the previous sentence: | Such auxiliary usage of `tarantool_L` is not idiomatic for Lua. > so such misuse leads to panic call. Also the `tarantool_L` usage is > redundant. So this patch drops it and uses only minor coroutine instead Typo: Again, not minor coroutine, but port coroutine (as we agreed in the previous review). > with `lua_pcall()`. > > Functions to encode are saved as entrance in the `LUA_REGISTRY` table to Typo: s/saved as entrance in/saved to/. > reduce GC pressure, like it is done for other handlers [9]. > > [1]: https://www.lua.org/manual/5.2/manual.html#4.6 > [2]: https://www.lua.org/source/5.1/lstate.h.html#lua_State > [3]: https://www.lua.org/source/5.1/ldo.c.html#luaD_throw > [4]: https://luajit.org/extensions.html#exceptions > [5]: https://github.com/tarantool/luajit/commit/ed412cd9f55fe87fd32a69c86e1732690fc5c1b0 > [6]: https://github.com/tarantool/luajit/commit/97699d9ee2467389b6aea21a098e38aff3469b5f > [7]: https://github.com/tarantool/tarantool/issues/1516 > [8]: https://www.freelists.org/post/luajit/Issue-with-PCALL-in-21 > [9]: https://github.com/tarantool/tarantool/commit/e88c0d21ab765d4c53bed2437c49d77b3ffe4216 > > Closes #6248 > Closes #4617 > =================================================================== > > > > --- > > > > > > Branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-refactor-lua-call > > > See the benchmarks sources here [1]. > > > > > > Before patch: > > > | Encode map: 189851357 mcs, 15.8 K ps > > > | Encode seq: 187926351 mcs, 16.0 K ps > > > | Encode str: 185451675 mcs, 16.2 K ps > > > | Encode dig: 184833396 mcs, 16.2 K ps > > > > > > After patch: > > > | Encode map: 187814261 mcs, 16.0 K ps > > > | Encode seq: 183755028 mcs, 16.3 K ps > > > | Encode str: 181571626 mcs, 16.5 K ps > > > | Encode dig: 181572998 mcs, 16.5 K ps > > > > > > Looks like the perf doesn't degrade at least. > > > > At first, I would like to emphasize that we have no option for merging > > or not the fix for this issue. > > > > Re benchmarks: It's worth to mention you're measuring two performance > > critical changes: effect and lower GC pressure. So, it's > > interesting to see the following benchmarks: > > * one with disabled GC and GC stats > > Here the results with disabled GC: > Before patch: > > Encode map: 4679394 mcs, 21.4 K ps > Encode seq: 4559824 mcs, 21.9 K ps > Encode str: 4574213 mcs, 21.9 K ps > Encode dig: 4595043 mcs, 21.8 K ps > Encode mul: 5978444 mcs, 16.7 K ps > > After: > > Encode map: 4739110 mcs, 21.1 K ps > Encode seq: 4528261 mcs, 22.1 K ps > Encode str: 4576910 mcs, 21.8 K ps > Encode dig: 4506142 mcs, 22.2 K ps > Encode mul: 6016659 mcs, 16.6 K ps > > I suppose, that values are almost the same, at least within the margin > of error. > Note: I reduced amount of iterations 30 times. So inaccuracy increased. > > > * one with considerable amount of elements on Lua stack, but not > > triggering stack resize (AFAIU, 200 is too much) > > Tried with 40 items on the stack: > > Without GC: > > Master: > Encode mul: 4895280 mcs, 20.4 K ps > > Branch: > Encode mul: 4896076 mcs, 20.4 K ps > > With GC: > > Master: > Encode mul: 5123580 mcs, 19.5 K ps > > Branch: > Encode mul: 5050863 mcs, 19.8 K ps > > Seems pretty equal too. Mystery. Anyway, the current performance is not lost and this is great! > > > > > Here are my points: > > * There is no such huge increase as a result of reducing GC pressure > > * Moving 1-5 8-byte elements is neglible for performance > > * Moving 200(*) elements as a result of the guest stack resize affects > > both patched and vanilla versions > > * measurements are affected by resizing (considering your > > perf stats) > > > > Anyway, though these are kinda independent changes, we see no > > performance degradation using both of them in a single patch, so I guess > > we have no reason to worry about. > > > > (*) I'm not sure about the exact amount of the elements to be moved. > > Exactly 200. > > > -- > Best regards, > Sergey Kaplun -- Best regards, IM