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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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 ;)

> 
> 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 :)

> 
> 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.

> 
> 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 <orignins> 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 <lj_state.c>. s/old_vmstate/oldvmstate/g
> > > 

<snipped>

> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM