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 792186F3C7; Fri, 26 Mar 2021 21:39:08 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 792186F3C7 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1616783948; bh=zMUbZv78NTrQy6DsDwzIUib7S+3X1ARZHlV687th6f4=; 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=YaufuxTQ2omZukubIXv67A7nzvJUUjuzZUyeW4XkKnvzitGlUEmIbhJPR+I+spyY4 C3fcyJAszv5SGTlWkq5QUtbhcwiM0o/2YqwEdUH9/Y6jsYXq9WikqPdGgzZQp3fTZY 1IVim6TqNP75LcFU+edEu2lIACjCtCKDQGwwzSz8= 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 896D86F3C7 for ; Fri, 26 Mar 2021 21:39:06 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 896D86F3C7 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1lPrMP-0001xl-FE; Fri, 26 Mar 2021 21:39:05 +0300 Date: Fri, 26 Mar 2021 21:38:56 +0300 To: Sergey Kaplun Message-ID: <20210326183856.GI29703@tarantool.org> References: <20210309175422.25432-1-skaplun@tarantool.org> <20210325201459.GG29703@tarantool.org> <20210326101035.GH29703@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-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9ED7173E37F4E32947A0146560F8BA709498CFB6209D8582A182A05F538085040B88869F079BFF0CCB468C5722620E1951EB904FDEA072D79A7D52051D98144F7 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE74B7EDB5828CF9C27EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006379C3A27B859DE23D18638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C5DD32608FC869F5D8B192BEB0321B7B47E0BAE2336E32EE8A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE77A825AB47F0FC8649FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C353FA85A707D24CADCC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB86D1867E19FE14079C09775C1D3CA48CFC5EA940A35A165FF2DBA43225CD8A89F890A246B268E114EC6EABA9B74D0DA47B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5FC6F5C3E7885173CE098D1F7E68BAA4FB3E1B97BE584CBDDD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34EE6683D9E546CC622F7B89D4F74260E1779F2D4E9C01125BBA4ED5CD8985705171DCA742D792C7881D7E09C32AA3244CEDADB6C792D9859BE7ECEF1AB8F6EF1DF522A1CF68F4BE05927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojapPp7P/VpAhrleuqq0SJNQ== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822717F7852AB3864F364FB83E56B72C2F5A7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] memprof: report stack resizing as internal event 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 Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Sergey, On 26.03.21, Sergey Kaplun wrote: > Igor, > > > > > > src/lj_state.c | 6 ++++++ > > > > > .../misclib-memprof-lapi.test.lua | 18 ++++++++++++++++++ > > > > > 2 files changed, 24 insertions(+) > > > > > > > > > > diff --git a/src/lj_state.c b/src/lj_state.c > > > > > index 1ed79a5..ea9abd4 100644 > > > > > --- a/src/lj_state.c > > > > > +++ b/src/lj_state.c > > > > > @@ -64,7 +64,11 @@ static void resizestack(lua_State *L, MSize n) > > > > > MSize oldsize = L->stacksize; > > > > > MSize realsize = n + 1 + LJ_STACK_EXTRA; > > > > > GCobj *up; > > > > > + int32_t old_vmstate = G(L)->vmstate; > > > > > > > > Please consider the naming and the workflow in lj_gc.c for such > > > > situations: G(L) is stored into a separate variable and is > > > > named . It makes grep for such spots much easier, doesn't it? > > > > > > You can see more by grepping vmstate. Made naming more consistent > > > > I did it before sending the review. I did it again now. My opinion is > > not changed. This is "idiomatic" approach to "push" and "pop" vmstate > > used only in lj_gc.c since this is not required elsewhere. If you move > > But not only in some old value is saved to be reused later: > > | $ grep -rn -P '\Wold[A-Za-z]' src/l*.c | grep -v -e define -e "\*\*" -e "\*/" -e "\*/" | wc -l > | 123 It's worth to notice that almost half of the entries are related to lj_alloc.c containing dlmalloc sources. Hence, you're cheating a bit ;) > > Instead one-letter abbreviation (grep out comments and most frequently > used constructions): > > | $ grep -rn -P '\Wo[^l]' src/l*.c | grep -v -e "\Wo\W" -e "\Wo[12]" | grep -v \ > | -e obj -e open -e op -e opt -e octet -e out -e on \ > | -e ofs -e offs -e ok -e oddspill -e os -e orign \ > | -e define -e overhead -e or \ > | -e "\*/" -e "\*\*" -e "/\*" | wc -l > | 25 It's also worth to mention that you've excluded several valid entries too, e.g. . Cheating again, but I don't mind :) > > The huge part of them was introduced via memory profiler by us. That's why I so concerned about naming now: field doesn't fit the current naming practice. It should be (consider in ASMState structure). Omitting the sophistry above, you said nothing regarding the examples related to (that you've asked me to grep before). Consider the following: stands for "old hook" *source-wide*, so it's quite clear what is stored in if you see this variable in the sources. You introduce another identifier for "old vmstate", thereby introduce ambiguity for this entity name. And then you argue about unrelevant statistics about "old" vs "o" naming. > > So, looks like your codestyle suggestion contradicts to codestyle > of LuaJIT as it is. It is not described anywhere else, so looks like > that source of true is the sources by themseles. That's what I'm talking about: just use the same names as Mike does. Re G(L) explicit usage: there is other code around using G(L), so I'm OK. Introducing a new variable will enlarge the diff, since you'd need to adjust the old code too. Disregard this proposal. > > > that approach "intact", it shows that the semantics of your code are the > > It's already the same, it is obvious by reading, plus comment describes > the behaviour. > > > same. Otherwise, every other occurrence of such vmstate "pushing" and > > "popping" allows to introduce own naming: pstate, prevstate, > > prev_vmstate, sstate, save_state, savevmstate -- there are lots of > > None of them is usual in use for LuaJIT's C codebase, so they can't be > used. OK, then origstate, origvmstate. They are fine and used in LuaJIT sources, since you've grepped out entries. > > > combinations. And nobody can stop contributor from this, since it is > > "more consistent for the current TU". > > But with your approach naming will be choosed not like the most usual, > but "same in the other file". I think that it will lead to the code > inconsistency even in one TU. It's not about "most usual", it's about "fitting this particular case", but you continue talking about your vision. I've heard you, but you haven't heard me, unfortunately. > > > > > "Feel free to prove the opposite"(c) :) > > > > The current naming is much better than the previous one, but I still > > propose to save the original on and save G(L) into a new variable. > > Ignoring for now. Well, if you just need my LGTM, you got it. I'll even push this patch by myself. But try to hear my arguments even if they look like "bred deda". > > > > > > with the . s/old_vmstate/oldvmstate/g > > > > > -- > Best regards, > Sergey Kaplun -- Best regards, IM