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 B6A4C6EC60; Mon, 29 Mar 2021 11:10:04 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B6A4C6EC60 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617005404; bh=U+VMr4pPHRy0kajoKHuNPCTgRKzBCRz9G6I8/mposCA=; 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=KopsXTFsLWihPTmmVQ9Uti1y7tgVX5t20nXvUnADm2Vu4SFBWpiA8S6GTlyT/KhJA qdPeHJ4C0MM4kcpeIHrK9asLy+aIO5Ng/Q0uLzcAJwmXznkvTRkGEb+WKDMuoc+CSS T9V9SxJ+vr7RoSZxRWbtLznnAcFBRKu9ur44Ko2w= Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (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 0B4D66EC60 for ; Mon, 29 Mar 2021 11:10:02 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0B4D66EC60 Received: by smtp50.i.mail.ru with esmtpa (envelope-from ) id 1lQmyH-0005au-Ep; Mon, 29 Mar 2021 11:10:02 +0300 Date: Mon, 29 Mar 2021 11:09:07 +0300 To: Igor Munkin Message-ID: References: <20210309175422.25432-1-skaplun@tarantool.org> <20210325201459.GG29703@tarantool.org> <20210326101035.GH29703@tarantool.org> <20210326183856.GI29703@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210326183856.GI29703@tarantool.org> X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD9ED7173E37F4E329498ADEA61F680B110809A4DE3E6FC56EA182A05F538085040451BE64E54C0B2FC9DB2CA8D10101B8818B7F386CE804853A122A99B1234F69A X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE73AA63C5F29446501EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637D7C7E03580C3DB018638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C7428A34725AB662D3644ADD095BC057194D81C8DE72F676EA471835C12D1D9774AD6D5ED66289B5278DA827A17800CE77A825AB47F0FC8649FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3A12191B5F2BB8629117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947C0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E7352B32DCB92A8600C6C4224003CC83647689D4C264860C145E X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2AD77751E876CB595E8F7B195E1C97831A3465ED807B80BD5FCB540272E22000E X-C1DE0DAB: 0D63561A33F958A5063F6BD165C8BD5F9EDC6597F75E927729E143E6C9C82997D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D349379E7F8541B6C9A7C6F9E8594C7F158404B741C90F475E6FD6512ADB3A5D5FE8B420954E7B922A71D7E09C32AA3244C6A5F378507ED66D891A0FA4F450DAB188894E9C85370243EFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojljIiQOC84rRwe9xzEVnfXQ== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB462001822A4EB7F4C05D564664438AD0FD6E2BA72FD04D86FF2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 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: 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" Igor, On 26.03.21, Igor Munkin wrote: > 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 ;) I wanted to demonstrate the order of the differences, not counting exactly :). > > > > > 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 :) Ditto. > > > > > 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. You misunderstand my point of view, which I am trying to explain. In my world, the code should be consistent in one translation unit first time, and with the whole product code second. But when the code is consistent with the naming of the entire program, then, in particular, it will be consistent inside TU by transitive property. If we are homogeneous throughout the program, then there will be no need for a dispute in naming as such. The most common naming right now is using `old` LuaJIT-wide. `orignins` looks like an exception too -- there is only `orign` used in , there is no old or , AFAICS. So there can be used `orign` form of naming only (and only in this TU), in my opinion, since it will not be refactored. I fully understand your point, but, in my opinion, the benefits received from it are less than from my proposal, because this is an exception, but not a rule, and the code may become more variegated -- it is hard to tell from which chunk of the code this particular name usage came from; why the variable names two lines above have different naming style, and so on. > > > > > 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. Personally, I see no difference between these 2 proposals -- neither in readability of the code (it's too common to use `G(L)` macro LuaJIT-wide, but there is nothing bad in creating an additional variable), nor in its performance. We can use an additional variable, if you think that it looks better. > > > > > > 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. Answered above. > > > > > > 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. I feel just the reverse :). Tried to explain my point of view above. > > > > > > > > > "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". This is not about LGTM, but about a miscommunication, I hope we will understand each other's point of view and come to a consensus. > > > > > > > > > > with the . s/old_vmstate/oldvmstate/g > > > > > > > > > > > -- > > Best regards, > > Sergey Kaplun > > -- > Best regards, > IM -- Best regards, Sergey Kaplun