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

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

[Sergey Kaplun via Tarantool-patches]

Resizing of the Lua stack is not reported as internal allocation.
Moreover, it may lead to crash inside Lua or FF frames.

Profiler performs reallocation first and after reports corresponding
event. When the stack is resized for local function arguments, the link
to previous frame is invalid in the cause of reallocation. Therefore,
assertion in `debug_framepc()` failes, because of invalid function
reference at previous frame.

Resolves tarantool/tarantool#5842
Follows up tarantool/tarantool#5442
---
Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5842-memprof-core-on-resizestack
Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5842-memprof-core-on-resizestack
Issue: https://github.com/tarantool/tarantool/issues/5842


 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;
+
   lua_assert((MSize)(tvref(L->maxstack)-oldst)==L->stacksize-LJ_STACK_EXTRA-1);
+
+  setvmstate(G(L), INTERP);
   st = (TValue *)lj_mem_realloc(L, tvref(L->stack),
 				(MSize)(oldsize*sizeof(TValue)),
 				(MSize)(realsize*sizeof(TValue)));
@@ -80,6 +84,8 @@ static void resizestack(lua_State *L, MSize n)
   L->top = (TValue *)((char *)L->top + delta);
   for (up = gcref(L->openupval); up != NULL; up = gcnext(up))
     setmref(gco2uv(up)->v, (TValue *)((char *)uvval(gco2uv(up)) + delta));
+
+  G(L)->vmstate = old_vmstate;
 }
 
 /* Relimit stack after error, in case the limit was overdrawn. */
diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
index 1c36c8a..93cc348 100644
--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
@@ -125,5 +125,23 @@ test:ok(check_alloc_report(alloc, 25, 18, 100))
 -- Collect all previous allocated objects.
 test:ok(free.INTERNAL.num == 102)
 
+-- Test for https://github.com/tarantool/tarantool/issues/5842.
+-- We do not interested in report itself.
+misc.memprof.start("/dev/null")
+-- We need to cause stack resize for local variables at function
+-- call. Let's create a new coroutine (all slots are free).
+-- It has 1 slot for dummy frame + 39 free slots + 5 extra slots
+-- (so-called red zone) + 2 * LJ_FR2 slots. So 50 local variables
+-- is enough.
+local payload_str = ""
+for i = 1, 50 do
+  payload_str = payload_str..("local v%d = %d\n"):format(i, i)
+end
+local f, errmsg = loadstring(payload_str)
+assert(f, errmsg)
+local co = coroutine.create(f)
+coroutine.resume(co)
+misc.memprof.stop()
+
 jit.on()
 os.exit(test:check() and 0 or 1)
-- 
2.28.0

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

[Sergey Ostanevich via Tarantool-patches]

Hi!

Thanks for the patch!

> On 9 Mar 2021, at 20:54, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> Resizing of the Lua stack is not reported as internal allocation.
> Moreover, it may lead to crash inside Lua or FF frames.
> 
> Profiler performs reallocation first and after reports corresponding
> event. When the stack is resized for local function arguments, the link
> to previous frame is invalid in the cause of reallocation. Therefore,
				  ^^^^^^^^ case
> assertion in `debug_framepc()` failes, because of invalid function
				fails.   ^^^ is it just dubbing the link 
					     is invalid? Then just remove.

> reference at previous frame.
> 
> Resolves tarantool/tarantool#5842
> Follows up tarantool/tarantool#5442
> ---
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5842-memprof-core-on-resizestack
> Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5842-memprof-core-on-resizestack
> Issue: https://github.com/tarantool/tarantool/issues/5842
> 
> 
> 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;
> +
>   lua_assert((MSize)(tvref(L->maxstack)-oldst)==L->stacksize-LJ_STACK_EXTRA-1);
> +
> +  setvmstate(G(L), INTERP);

This looks like a hack. But even so - why not to return it back right after the
realloc, where you supposedly use lua_getinfo? 

The righteous way would be to fix the debug machinery anyhow.

>   st = (TValue *)lj_mem_realloc(L, tvref(L->stack),
> 				(MSize)(oldsize*sizeof(TValue)),
> 				(MSize)(realsize*sizeof(TValue)));
> @@ -80,6 +84,8 @@ static void resizestack(lua_State *L, MSize n)
>   L->top = (TValue *)((char *)L->top + delta);
>   for (up = gcref(L->openupval); up != NULL; up = gcnext(up))
>     setmref(gco2uv(up)->v, (TValue *)((char *)uvval(gco2uv(up)) + delta));
> +
> +  G(L)->vmstate = old_vmstate;
> }
> 
> /* Relimit stack after error, in case the limit was overdrawn. */
> diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> index 1c36c8a..93cc348 100644
> --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> @@ -125,5 +125,23 @@ test:ok(check_alloc_report(alloc, 25, 18, 100))
> -- Collect all previous allocated objects.
> test:ok(free.INTERNAL.num == 102)
> 
> +-- Test for https://github.com/tarantool/tarantool/issues/5842.
> +-- We do not interested in report itself.
     We ->			     ourselves?
> +misc.memprof.start("/dev/null")
> +-- We need to cause stack resize for local variables at function
> +-- call. Let's create a new coroutine (all slots are free).
> +-- It has 1 slot for dummy frame + 39 free slots + 5 extra slots
> +-- (so-called red zone) + 2 * LJ_FR2 slots. So 50 local variables
> +-- is enough.
> +local payload_str = ""
> +for i = 1, 50 do
> +  payload_str = payload_str..("local v%d = %d\n"):format(i, i)
> +end
> +local f, errmsg = loadstring(payload_str)
> +assert(f, errmsg)
> +local co = coroutine.create(f)
> +coroutine.resume(co)
> +misc.memprof.stop()
> +
> jit.on()
> os.exit(test:check() and 0 or 1)
> — 
> 2.28.0
> 


Regards,
Sergos

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergos!

Thanks for the review!

On 10.03.21, Sergey Ostanevich wrote:
> Hi!
> 
> Thanks for the patch!
> 
> > On 9 Mar 2021, at 20:54, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > Resizing of the Lua stack is not reported as internal allocation.
> > Moreover, it may lead to crash inside Lua or FF frames.
> > 
> > Profiler performs reallocation first and after reports corresponding
> > event. When the stack is resized for local function arguments, the link
> > to previous frame is invalid in the cause of reallocation. Therefore,
> 				  ^^^^^^^^ case
> > assertion in `debug_framepc()` failes, because of invalid function
> 				fails.   ^^^ is it just dubbing the link 
> 					     is invalid? Then just remove.

Fixed and force-pushed to the branch. The commit message now is the
following:

===================================================================
memprof: report stack resizing as internal event

Resizing of the Lua stack is not reported as internal allocation.
Moreover, it may lead to crash inside Lua or FF frames.

Profiler performs reallocation first and after reports corresponding
event. When the stack is resized for local function arguments, the link
to previous frame is invalid in case of reallocation. Therefore,
assertion in `debug_framepc()` fails.

Resolves tarantool/tarantool#5842
Follows up tarantool/tarantool#5442
===================================================================

> 
> > reference at previous frame.
> > 
> > Resolves tarantool/tarantool#5842
> > Follows up tarantool/tarantool#5442
> > ---
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5842-memprof-core-on-resizestack
> > Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5842-memprof-core-on-resizestack
> > Issue: https://github.com/tarantool/tarantool/issues/5842
> > 
> > 
> > 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;
> > +
> >   lua_assert((MSize)(tvref(L->maxstack)-oldst)==L->stacksize-LJ_STACK_EXTRA-1);
> > +
> > +  setvmstate(G(L), INTERP);
> 
> This looks like a hack. But even so - why not to return it back right after the
> realloc, where you supposedly use lua_getinfo? 

The Lua state L is inconsistent during stack resizing, so it can't be
interpreted in any way, until reallocation and corresponding converting
are finished.

> 
> The righteous way would be to fix the debug machinery anyhow.

Don't get your point here. Lua state is invalid, debug machinery works
correct, as I understand.

> 
> >   st = (TValue *)lj_mem_realloc(L, tvref(L->stack),
> > 				(MSize)(oldsize*sizeof(TValue)),
> > 				(MSize)(realsize*sizeof(TValue)));
> > @@ -80,6 +84,8 @@ static void resizestack(lua_State *L, MSize n)
> >   L->top = (TValue *)((char *)L->top + delta);
> >   for (up = gcref(L->openupval); up != NULL; up = gcnext(up))
> >     setmref(gco2uv(up)->v, (TValue *)((char *)uvval(gco2uv(up)) + delta));
> > +
> > +  G(L)->vmstate = old_vmstate;
> > }
> > 
> > /* Relimit stack after error, in case the limit was overdrawn. */
> > diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> > index 1c36c8a..93cc348 100644
> > --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
> > +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> > @@ -125,5 +125,23 @@ test:ok(check_alloc_report(alloc, 25, 18, 100))
> > -- Collect all previous allocated objects.
> > test:ok(free.INTERNAL.num == 102)
> > 
> > +-- Test for https://github.com/tarantool/tarantool/issues/5842.
> > +-- We do not interested in report itself.
>      We ->			     ourselves?

I've removed confusing "itself". See the iterative patch below,
branch is force-pushed.

===================================================================
diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
index 93cc348..9b9fb76 100644
--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
@@ -126,7 +126,7 @@ test:ok(check_alloc_report(alloc, 25, 18, 100))
 test:ok(free.INTERNAL.num == 102)
 
 -- Test for https://github.com/tarantool/tarantool/issues/5842.
--- We do not interested in report itself.
+-- We do not interested in this report.
 misc.memprof.start("/dev/null")
 -- We need to cause stack resize for local variables at function
 -- call. Let's create a new coroutine (all slots are free).
===================================================================

> > +misc.memprof.start("/dev/null")
> > +-- We need to cause stack resize for local variables at function
> > +-- call. Let's create a new coroutine (all slots are free).
> > +-- It has 1 slot for dummy frame + 39 free slots + 5 extra slots
> > +-- (so-called red zone) + 2 * LJ_FR2 slots. So 50 local variables
> > +-- is enough.
> > +local payload_str = ""
> > +for i = 1, 50 do
> > +  payload_str = payload_str..("local v%d = %d\n"):format(i, i)
> > +end
> > +local f, errmsg = loadstring(payload_str)
> > +assert(f, errmsg)
> > +local co = coroutine.create(f)
> > +coroutine.resume(co)
> > +misc.memprof.stop()
> > +
> > jit.on()
> > os.exit(test:check() and 0 or 1)
> > — 
> > 2.28.0
> > 
> 
> 
> Regards,
> Sergos
> 
> 

-- 
Best regards,
Sergey Kaplun

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

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! Please consider my comments below.

On 09.03.21, Sergey Kaplun wrote:
> Resizing of the Lua stack is not reported as internal allocation.

Was it done intentionally? You claim this as a plain fact with no
rationale. If this is a bug (by design) then mention it explicitly here.
Otherwise, you need to provide a reason why you change the semantics of
Lua stack reallocations in terms of memprof. For me it looks like a bug
we missed in the previous release: user has nothing to do with guest
stack reallocations (like with JIT related ones), hence these memory
manipulations are totally internal. Maybe you have another vision
regarding this.

> Moreover, it may lead to crash inside Lua or FF frames.

This is a symptom, not another reason. So "As a result" fits better than
"Moreover" here.

> 
> Profiler performs reallocation first and after reports corresponding

Profiler makes no allocations. I believe you would like to say that
reallocation occurs at first and after memprof reports the event.

Typo: s/reports corresponding/reports the corresponding/.

> event. When the stack is resized for local function arguments, the link
> to previous frame is invalid in the cause of reallocation. Therefore,
> assertion in `debug_framepc()` failes, because of invalid function
> reference at previous frame.

Typo: s/at previous/at the previous/.

> 
> Resolves tarantool/tarantool#5842
> Follows up tarantool/tarantool#5442
> ---
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5842-memprof-core-on-resizestack
> Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5842-memprof-core-on-resizestack
> Issue: https://github.com/tarantool/tarantool/issues/5842
> 
> 
>  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?

> +
>    lua_assert((MSize)(tvref(L->maxstack)-oldst)==L->stacksize-LJ_STACK_EXTRA-1);
> +
> +  setvmstate(G(L), INTERP);

We didn't notice this before. Now you leave not a single word regarding
this hack. How come?

>    st = (TValue *)lj_mem_realloc(L, tvref(L->stack),
>  				(MSize)(oldsize*sizeof(TValue)),
>  				(MSize)(realsize*sizeof(TValue)));
> @@ -80,6 +84,8 @@ static void resizestack(lua_State *L, MSize n)
>    L->top = (TValue *)((char *)L->top + delta);
>    for (up = gcref(L->openupval); up != NULL; up = gcnext(up))
>      setmref(gco2uv(up)->v, (TValue *)((char *)uvval(gco2uv(up)) + delta));
> +
> +  G(L)->vmstate = old_vmstate;
>  }
>  
>  /* Relimit stack after error, in case the limit was overdrawn. */
> diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> index 1c36c8a..93cc348 100644
> --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> @@ -125,5 +125,23 @@ test:ok(check_alloc_report(alloc, 25, 18, 100))
>  -- Collect all previous allocated objects.
>  test:ok(free.INTERNAL.num == 102)
>  
> +-- Test for https://github.com/tarantool/tarantool/issues/5842.
> +-- We do not interested in report itself.
> +misc.memprof.start("/dev/null")
> +-- We need to cause stack resize for local variables at function
> +-- call. Let's create a new coroutine (all slots are free).
> +-- It has 1 slot for dummy frame + 39 free slots + 5 extra slots
> +-- (so-called red zone) + 2 * LJ_FR2 slots. So 50 local variables
> +-- is enough.
> +local payload_str = ""
> +for i = 1, 50 do
> +  payload_str = payload_str..("local v%d = %d\n"):format(i, i)
> +end
> +local f, errmsg = loadstring(payload_str)
> +assert(f, errmsg)

Minor: you can use a TAP check here, but feel free to leave a plain
assert, since you expect <loadstring> works fine always.

> +local co = coroutine.create(f)
> +coroutine.resume(co)
> +misc.memprof.stop()
> +
>  jit.on()
>  os.exit(test:check() and 0 or 1)
> -- 
> 2.28.0
> 

-- 
Best regards,
IM

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

[Sergey Kaplun via Tarantool-patches]

Igor,

Thanks for the review!

On 25.03.21, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! Please consider my comments below.
> 
> On 09.03.21, Sergey Kaplun wrote:
> > Resizing of the Lua stack is not reported as internal allocation.
> 
> Was it done intentionally? You claim this as a plain fact with no
> rationale. If this is a bug (by design) then mention it explicitly here.

It is obviously a bug.

> Otherwise, you need to provide a reason why you change the semantics of
> Lua stack reallocations in terms of memprof. For me it looks like a bug
> we missed in the previous release: user has nothing to do with guest
> stack reallocations (like with JIT related ones), hence these memory
> manipulations are totally internal. Maybe you have another vision
> regarding this.
> 
> > Moreover, it may lead to crash inside Lua or FF frames.
> 
> This is a symptom, not another reason. So "As a result" fits better than
> "Moreover" here.

Reworded.

> 
> > 
> > Profiler performs reallocation first and after reports corresponding
> 
> Profiler makes no allocations. I believe you would like to say that
> reallocation occurs at first and after memprof reports the event.
> 
> Typo: s/reports corresponding/reports the corresponding/.

Fixed.

> 
> > event. When the stack is resized for local function arguments, the link
> > to previous frame is invalid in the cause of reallocation. Therefore,
> > assertion in `debug_framepc()` failes, because of invalid function
> > reference at previous frame.
> 
> Typo: s/at previous/at the previous/.

Fixed.

> 

See the new commit message below, branch is force-pushed:

===================================================================
memprof: report stack resizing as internal event

Resizing of the Lua stack is not reported as internal allocation
as it should. As a result, it may lead to crash inside Lua or FF
frames.

When the memory profiler runs, reallocation occurs first, and after
profiler reports the corresponding event. When the stack is resized for
local function arguments, the link to previous the frame is invalid in
the case of reallocation. Therefore, the assertion in `debug_framepc()`
fails.

Resolves tarantool/tarantool#5842
Follows up tarantool/tarantool#5442
===================================================================

> > 
> > Resolves tarantool/tarantool#5842
> > Follows up tarantool/tarantool#5442
> > ---
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5842-memprof-core-on-resizestack
> > Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5842-memprof-core-on-resizestack
> > Issue: https://github.com/tarantool/tarantool/issues/5842
> > 
> > 
> >  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
with the <lj_state.c>. s/old_vmstate/oldvmstate/g

See the iterative patch below.

> 
> > +
> >    lua_assert((MSize)(tvref(L->maxstack)-oldst)==L->stacksize-LJ_STACK_EXTRA-1);
> > +
> > +  setvmstate(G(L), INTERP);
> 
> We didn't notice this before. Now you leave not a single word regarding
> this hack. How come?

Added the comment. But I don't get how it is connected to our notice.
May be it should be mentioned in docs?

See the iterative patch below. Branch is force-pushed.
===================================================================
diff --git a/src/lj_state.c b/src/lj_state.c
index ea9abd4..c86e098 100644
--- a/src/lj_state.c
+++ b/src/lj_state.c
@@ -64,10 +64,15 @@ 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;
+  int32_t oldvmstate = G(L)->vmstate;
 
   lua_assert((MSize)(tvref(L->maxstack)-oldst)==L->stacksize-LJ_STACK_EXTRA-1);
 
+  /*
+  ** Lua stack is inconsistent durent reallocation, profilers
+  ** depends on vmstate during reports, so set vmstate to INTERP
+  ** to avoid inconsistent behaviour.
+  */
   setvmstate(G(L), INTERP);
   st = (TValue *)lj_mem_realloc(L, tvref(L->stack),
                                 (MSize)(oldsize*sizeof(TValue)),
@@ -85,7 +90,7 @@ static void resizestack(lua_State *L, MSize n)
   for (up = gcref(L->openupval); up != NULL; up = gcnext(up))
     setmref(gco2uv(up)->v, (TValue *)((char *)uvval(gco2uv(up)) + delta));
 
-  G(L)->vmstate = old_vmstate;
+  G(L)->vmstate = oldvmstate;
 }
 
 /* Relimit stack after error, in case the limit was overdrawn. */
===================================================================

> 
> >    st = (TValue *)lj_mem_realloc(L, tvref(L->stack),
> >  				(MSize)(oldsize*sizeof(TValue)),
> >  				(MSize)(realsize*sizeof(TValue)));
> > @@ -80,6 +84,8 @@ static void resizestack(lua_State *L, MSize n)
> >    L->top = (TValue *)((char *)L->top + delta);
> >    for (up = gcref(L->openupval); up != NULL; up = gcnext(up))
> >      setmref(gco2uv(up)->v, (TValue *)((char *)uvval(gco2uv(up)) + delta));
> > +
> > +  G(L)->vmstate = old_vmstate;
> >  }
> >  
> >  /* Relimit stack after error, in case the limit was overdrawn. */
> > diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> > index 1c36c8a..93cc348 100644
> > --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
> > +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> > @@ -125,5 +125,23 @@ test:ok(check_alloc_report(alloc, 25, 18, 100))
> >  -- Collect all previous allocated objects.
> >  test:ok(free.INTERNAL.num == 102)
> >  
> > +-- Test for https://github.com/tarantool/tarantool/issues/5842.
> > +-- We do not interested in report itself.
> > +misc.memprof.start("/dev/null")
> > +-- We need to cause stack resize for local variables at function
> > +-- call. Let's create a new coroutine (all slots are free).
> > +-- It has 1 slot for dummy frame + 39 free slots + 5 extra slots
> > +-- (so-called red zone) + 2 * LJ_FR2 slots. So 50 local variables
> > +-- is enough.
> > +local payload_str = ""
> > +for i = 1, 50 do
> > +  payload_str = payload_str..("local v%d = %d\n"):format(i, i)
> > +end
> > +local f, errmsg = loadstring(payload_str)
> > +assert(f, errmsg)
> 
> Minor: you can use a TAP check here, but feel free to leave a plain
> assert, since you expect <loadstring> works fine always.

We test not `loadstring()` here, assert is for sure that there is no
syntax error in the generated chunk.
Ignoring.

> 
> > +local co = coroutine.create(f)
> > +coroutine.resume(co)
> > +misc.memprof.stop()
> > +
> >  jit.on()
> >  os.exit(test:check() and 0 or 1)
> > -- 
> > 2.28.0
> > 
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun

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

[Igor Munkin via Tarantool-patches]

Sergey,

On 26.03.21, Sergey Kaplun wrote:
> Igor,
> 
> Thanks for the review!
> 
> On 25.03.21, Igor Munkin wrote:
> > Sergey,
> > 
> > Thanks for the patch! Please consider my comments below.
> > 

<snipped>

> 
> See the new commit message below, branch is force-pushed:
> 
> ===================================================================
> memprof: report stack resizing as internal event
> 
> Resizing of the Lua stack is not reported as internal allocation
> as it should. As a result, it may lead to crash inside Lua or FF
> frames.
> 
> When the memory profiler runs, reallocation occurs first, and after
> profiler reports the corresponding event. When the stack is resized for
> local function arguments, the link to previous the frame is invalid in

Typo: s/to previous the frame/to the previous frame/.

> the case of reallocation. Therefore, the assertion in `debug_framepc()`
> fails.
> 
> Resolves tarantool/tarantool#5842
> Follows up tarantool/tarantool#5442
> ===================================================================
> 
> > > 
> > > Resolves tarantool/tarantool#5842
> > > Follows up tarantool/tarantool#5442
> > > ---
> > > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5842-memprof-core-on-resizestack
> > > Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5842-memprof-core-on-resizestack
> > > Issue: https://github.com/tarantool/tarantool/issues/5842
> > > 
> > > 
> > >  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
that approach "intact", it shows that the semantics of your code are the
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
combinations. And nobody can stop contributor from this, since it is
"more consistent for the current TU".

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

> with the <lj_state.c>. s/old_vmstate/oldvmstate/g
> 
> See the iterative patch below.
> 
> > 
> > > +
> > >    lua_assert((MSize)(tvref(L->maxstack)-oldst)==L->stacksize-LJ_STACK_EXTRA-1);
> > > +
> > > +  setvmstate(G(L), INTERP);
> > 
> > We didn't notice this before. Now you leave not a single word regarding
> > this hack. How come?
> 
> Added the comment. But I don't get how it is connected to our notice.
> May be it should be mentioned in docs?

I meant that we pushed this bug into the trunk and didn't notice it.
Comment is totally enough, thanks!

> 
> See the iterative patch below. Branch is force-pushed.
> ===================================================================
> diff --git a/src/lj_state.c b/src/lj_state.c
> index ea9abd4..c86e098 100644
> --- a/src/lj_state.c
> +++ b/src/lj_state.c
> @@ -64,10 +64,15 @@ 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;
> +  int32_t oldvmstate = G(L)->vmstate;
>  
>    lua_assert((MSize)(tvref(L->maxstack)-oldst)==L->stacksize-LJ_STACK_EXTRA-1);
>  
> +  /*
> +  ** Lua stack is inconsistent durent reallocation, profilers

Typo: s/profilers/profiler/ or s/depends/depend/.

> +  ** depends on vmstate during reports, so set vmstate to INTERP
> +  ** to avoid inconsistent behaviour.
> +  */
>    setvmstate(G(L), INTERP);
>    st = (TValue *)lj_mem_realloc(L, tvref(L->stack),
>                                  (MSize)(oldsize*sizeof(TValue)),
> @@ -85,7 +90,7 @@ static void resizestack(lua_State *L, MSize n)
>    for (up = gcref(L->openupval); up != NULL; up = gcnext(up))
>      setmref(gco2uv(up)->v, (TValue *)((char *)uvval(gco2uv(up)) + delta));
>  
> -  G(L)->vmstate = old_vmstate;
> +  G(L)->vmstate = oldvmstate;
>  }
>  
>  /* Relimit stack after error, in case the limit was overdrawn. */
> ===================================================================
> 
> > 
> > >    st = (TValue *)lj_mem_realloc(L, tvref(L->stack),
> > >  				(MSize)(oldsize*sizeof(TValue)),
> > >  				(MSize)(realsize*sizeof(TValue)));
> > > @@ -80,6 +84,8 @@ static void resizestack(lua_State *L, MSize n)
> > >    L->top = (TValue *)((char *)L->top + delta);
> > >    for (up = gcref(L->openupval); up != NULL; up = gcnext(up))
> > >      setmref(gco2uv(up)->v, (TValue *)((char *)uvval(gco2uv(up)) + delta));
> > > +
> > > +  G(L)->vmstate = old_vmstate;
> > >  }
> > >  
> > >  /* Relimit stack after error, in case the limit was overdrawn. */

<snipped>

> > > -- 
> > > 2.28.0
> > > 
> > 
> > -- 
> > Best regards,
> > IM
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

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

[Sergey Kaplun via Tarantool-patches]

Igor,

I've fixed typos in the commit message and the comment.

On 26.03.21, Igor Munkin wrote:
> Sergey,
> 
> On 26.03.21, Sergey Kaplun wrote:
> > Igor,
> > 
> > Thanks for the review!
> > 
> > On 25.03.21, Igor Munkin wrote:
> > > Sergey,
> > > 
> > > Thanks for the patch! Please consider my comments below.
> > > 
> 
> <snipped>
> 
> > 
> > See the new commit message below, branch is force-pushed:
> > 
> > ===================================================================
> > memprof: report stack resizing as internal event
> > 
> > Resizing of the Lua stack is not reported as internal allocation
> > as it should. As a result, it may lead to crash inside Lua or FF
> > frames.
> > 
> > When the memory profiler runs, reallocation occurs first, and after
> > profiler reports the corresponding event. When the stack is resized for
> > local function arguments, the link to previous the frame is invalid in
> 
> Typo: s/to previous the frame/to the previous frame/.

Fixed.

> 
> > the case of reallocation. Therefore, the assertion in `debug_framepc()`
> > fails.
> > 
> > Resolves tarantool/tarantool#5842
> > Follows up tarantool/tarantool#5442
> > ===================================================================

The new commit message is the following:
===================================================================
memprof: report stack resizing as internal event

Resizing of the Lua stack is not reported as internal allocation
as it should. As a result, it may lead to crash inside Lua or FF
frames.

When the memory profiler runs, reallocation occurs first, and after
profiler reports the corresponding event. When the stack is resized for
local function arguments, the link to the previous frame is invalid in
the case of reallocation. Therefore, the assertion in `debug_framepc()`
fails.

Resolves tarantool/tarantool#5842
Follows up tarantool/tarantool#5442
===================================================================

> > 
> > > > 
> > > > Resolves tarantool/tarantool#5842
> > > > Follows up tarantool/tarantool#5442
> > > > ---
> > > > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5842-memprof-core-on-resizestack
> > > > Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5842-memprof-core-on-resizestack
> > > > Issue: https://github.com/tarantool/tarantool/issues/5842
> > > > 
> > > > 
> > > >  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

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

The huge part of them was introduced via memory profiler by us.

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

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

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

> 
> > with the <lj_state.c>. s/old_vmstate/oldvmstate/g
> > 
> > See the iterative patch below.
> > 
> > > 
> > > > +
> > > >    lua_assert((MSize)(tvref(L->maxstack)-oldst)==L->stacksize-LJ_STACK_EXTRA-1);
> > > > +
> > > > +  setvmstate(G(L), INTERP);
> > > 
> > > We didn't notice this before. Now you leave not a single word regarding
> > > this hack. How come?
> > 
> > Added the comment. But I don't get how it is connected to our notice.
> > May be it should be mentioned in docs?
> 
> I meant that we pushed this bug into the trunk and didn't notice it.
> Comment is totally enough, thanks!
> 
> > 
> > See the iterative patch below. Branch is force-pushed.
> > ===================================================================
> > diff --git a/src/lj_state.c b/src/lj_state.c
> > index ea9abd4..c86e098 100644
> > --- a/src/lj_state.c
> > +++ b/src/lj_state.c
> > @@ -64,10 +64,15 @@ 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;
> > +  int32_t oldvmstate = G(L)->vmstate;
> >  
> >    lua_assert((MSize)(tvref(L->maxstack)-oldst)==L->stacksize-LJ_STACK_EXTRA-1);
> >  
> > +  /*
> > +  ** Lua stack is inconsistent durent reallocation, profilers
> 
> Typo: s/profilers/profiler/ or s/depends/depend/.

Fixed, force-pushed.

===================================================================
diff --git a/src/lj_state.c b/src/lj_state.c
index c86e098..5701572 100644
--- a/src/lj_state.c
+++ b/src/lj_state.c
@@ -70,7 +70,7 @@ static void resizestack(lua_State *L, MSize n)
 
   /*
   ** Lua stack is inconsistent durent reallocation, profilers
-  ** depends on vmstate during reports, so set vmstate to INTERP
+  ** depend on vmstate during reports, so set vmstate to INTERP
   ** to avoid inconsistent behaviour.
   */
   setvmstate(G(L), INTERP);
===================================================================

> 
> > +  ** depends on vmstate during reports, so set vmstate to INTERP
> > +  ** to avoid inconsistent behaviour.
> > +  */
> >    setvmstate(G(L), INTERP);
> >    st = (TValue *)lj_mem_realloc(L, tvref(L->stack),
> >                                  (MSize)(oldsize*sizeof(TValue)),
> > @@ -85,7 +90,7 @@ static void resizestack(lua_State *L, MSize n)
> >    for (up = gcref(L->openupval); up != NULL; up = gcnext(up))
> >      setmref(gco2uv(up)->v, (TValue *)((char *)uvval(gco2uv(up)) + delta));
> >  
> > -  G(L)->vmstate = old_vmstate;
> > +  G(L)->vmstate = oldvmstate;
> >  }
> >  
> >  /* Relimit stack after error, in case the limit was overdrawn. */
> > ===================================================================
> > 
> > > 
> > > >    st = (TValue *)lj_mem_realloc(L, tvref(L->stack),
> > > >  				(MSize)(oldsize*sizeof(TValue)),
> > > >  				(MSize)(realsize*sizeof(TValue)));
> > > > @@ -80,6 +84,8 @@ static void resizestack(lua_State *L, MSize n)
> > > >    L->top = (TValue *)((char *)L->top + delta);
> > > >    for (up = gcref(L->openupval); up != NULL; up = gcnext(up))
> > > >      setmref(gco2uv(up)->v, (TValue *)((char *)uvval(gco2uv(up)) + delta));
> > > > +
> > > > +  G(L)->vmstate = old_vmstate;
> > > >  }
> > > >  
> > > >  /* Relimit stack after error, in case the limit was overdrawn. */
> 
> <snipped>
> 
> > > > -- 
> > > > 2.28.0
> > > > 
> > > 
> > > -- 
> > > Best regards,
> > > IM
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun

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

[Igor Munkin via Tarantool-patches]

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

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

[Sergey Kaplun via Tarantool-patches]

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

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

[Igor Munkin via Tarantool-patches]

Sergey,

On 29.03.21, Sergey Kaplun wrote:
> Igor,
> 

<snipped>

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

I see no changes on the branch, hence we won't. Anyway, you have the
necessary amount of LGTMs, so I'll push your patch to the trunk later.

> 

<snipped>

> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

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

[Igor Munkin via Tarantool-patches]

On 29.03.21, Igor Munkin wrote:
> Sergey,
> 
> On 29.03.21, Sergey Kaplun wrote:
> > Igor,
> > 
> 
> <snipped>
> 
> > > > > 
> > > > > "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.
> 
> I see no changes on the branch, hence we won't. Anyway, you have the
> necessary amount of LGTMs, so I'll push your patch to the trunk later.

Oops, seems you have only mine (at least I see no explicit LGTM from
Sergos in the thread). Will not rush then.

> 
> > 
> 
> <snipped>
> 
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
IM

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

[Sergey Ostanevich via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 1399 bytes --]

Hi!

Had a good time reading all your comments, thanks!

As for the reason I still not blessed this - I got good 
explanation why stack resizing should be guarded for the whole
part of the code it was in original patch, so

LGTM.

Sergos

> On 29 Mar 2021, at 11:35, Igor Munkin <imun@tarantool.org> wrote:
> 
> On 29.03.21, Igor Munkin wrote:
>> Sergey,
>> 
>> On 29.03.21, Sergey Kaplun wrote:
>>> Igor,
>>> 
>> 
>> <snipped>
>> 
>>>>>> 
>>>>>> "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.
>> 
>> I see no changes on the branch, hence we won't. Anyway, you have the
>> necessary amount of LGTMs, so I'll push your patch to the trunk later.
> 
> Oops, seems you have only mine (at least I see no explicit LGTM from
> Sergos in the thread). Will not rush then.
> 
>> 
>>> 
>> 
>> <snipped>
>> 
>>> 
>>> -- 
>>> Best regards,
>>> Sergey Kaplun
>> 
>> -- 
>> Best regards,
>> IM
> 
> -- 
> Best regards,
> IM


[-- Attachment #2: Type: text/html, Size: 8703 bytes --]

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

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the series into tarantool-2.7 and tarantool branches in
tarantool/luajit and bumped a new version in 2.7 and master.

On 09.03.21, Sergey Kaplun wrote:
> Resizing of the Lua stack is not reported as internal allocation.
> Moreover, it may lead to crash inside Lua or FF frames.
> 
> Profiler performs reallocation first and after reports corresponding
> event. When the stack is resized for local function arguments, the link
> to previous frame is invalid in the cause of reallocation. Therefore,
> assertion in `debug_framepc()` failes, because of invalid function
> reference at previous frame.
> 
> Resolves tarantool/tarantool#5842
> Follows up tarantool/tarantool#5442
> ---
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5842-memprof-core-on-resizestack
> Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5842-memprof-core-on-resizestack
> Issue: https://github.com/tarantool/tarantool/issues/5842
> 
> 
>  src/lj_state.c                                 |  6 ++++++
>  .../misclib-memprof-lapi.test.lua              | 18 ++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 

<snipped>

> -- 
> 2.28.0
> 

-- 
Best regards,
IM