Re: [Tarantool-patches] [PATCH luajit] memprof: report stack resizing as internal event

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Igor,

On 26.03.21, Igor Munkin wrote:
> Sergey,
> 
> On 26.03.21, Sergey Kaplun wrote:
> > Igor,
> > 
> 
> <snipped>
> 
> > > > > >  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 <old_vmstate> is
> > > > > named <ostate>. 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 <lj_gc.c> 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. <osz>. 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: <orig_alloc> field doesn't
> fit the current naming practice. It should be <origalloc> (consider
> <orignins> in ASMState structure).
> 
> Omitting the sophistry above, you said nothing regarding the examples
> related to <vmstate> (that you've asked me to grep before). Consider the
> following: <oldh> stands for "old hook" *source-wide*, so it's quite
> clear what is stored in <oldh> 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<smth>" vs "o<smth>" 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<smth>`
LuaJIT-wide.

`orignins` looks like an exception too -- there is only `orign` used in
<lj_asm.c>, there is no old<smth> or <osmth>, 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 <orignins> 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 <lj_state.c>. s/old_vmstate/oldvmstate/g
> > > > 
> 
> <snipped>
> 
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun