[Tarantool-patches] [PATCH luajit v2] memprof: report JIT-side allocations as internal

[Tarantool-patches] [PATCH luajit v2] memprof: report JIT-side allocations as internal

[Mikhail Shishatskiy via Tarantool-patches]

There are cases when the memory profiler attempts to attribute
allocations triggered by the JIT engine recording phase
with a Lua function to be recorded. In this case,
lj_debug_frameline() may return BC_NOPOS (i.e. a negative value).

Previously, these situations were ignored and the profiler
reported, that the source line was equal to zero.

This patch adjusts profiler behavior to treat allocations
described above as internal by dumping ASOURCE_INT if
lj_debug_frameline() returns a negative value.

Resolves tarantool/tarantool#5679
---

Issue: https://github.com/tarantool/tarantool/issues/5679
Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5679-report-jit-allocations-as-internal
CI: https://github.com/tarantool/tarantool/tree/shishqa/gh-5679-report-jit-allocations-as-internal

Changes in v2:
  - Fixed commit title to fit in width of 50 symbols;
  - Rebased to the branch [1];
  - Reused default_payload() as a payload to test patch behavior.

[1]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number

 src/lj_memprof.c                              | 28 ++++++++++++-------
 .../misclib-memprof-lapi.test.lua             | 16 ++++++++---
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/src/lj_memprof.c b/src/lj_memprof.c
index 87512c3a..e4d819a0 100644
--- a/src/lj_memprof.c
+++ b/src/lj_memprof.c
@@ -108,19 +108,27 @@ static void memprof_write_lfunc(struct lj_wbuf *out, uint8_t aevent,
 				GCfunc *fn, struct lua_State *L,
 				cTValue *nextframe)
 {
-  const BCLine line = lj_debug_frameline(L, fn, nextframe);
-  lj_wbuf_addbyte(out, aevent | ASOURCE_LFUNC);
-  lj_wbuf_addu64(out, (uintptr_t)funcproto(fn));
   /*
-  ** Line is >= 0 if we are inside a Lua function.
-  ** There are cases when the memory profiler attempts
-  ** to attribute allocations triggered by JIT engine recording
-  ** phase with a Lua function to be recorded. At this case
-  ** lj_debug_frameline() may return BC_NOPOS (i.e. a negative value).
-  ** Equals to zero when LuaJIT is built with the
+  ** Line equals to zero when LuaJIT is built with the
   ** -DLUAJIT_DISABLE_DEBUGINFO flag.
   */
-  lj_wbuf_addu64(out, line >= 0 ? (uint64_t)line : 0);
+  const BCLine line = lj_debug_frameline(L, fn, nextframe);
+
+  if (line < 0) {
+    /*
+    ** Line is >= 0 if we are inside a Lua function.
+    ** There are cases when the memory profiler attempts
+    ** to attribute allocations triggered by JIT engine recording
+    ** phase with a Lua function to be recorded. It this case,
+    ** lj_debug_frameline() may return BC_NOPOS (i.e. a negative value).
+    ** We report such allocations as internal in order not to confuse users.
+    */
+    lj_wbuf_addbyte(out, aevent | ASOURCE_INT);
+  } else {
+    lj_wbuf_addbyte(out, aevent | ASOURCE_LFUNC);
+    lj_wbuf_addu64(out, (uintptr_t)funcproto(fn));
+    lj_wbuf_addu64(out, (uint64_t)line);
+  }
 }
 
 static void memprof_write_cfunc(struct lj_wbuf *out, uint8_t aevent,
diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
index b7e456e1..96864c6a 100644
--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
@@ -12,7 +12,7 @@ utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
 local tap = require("tap")
 
 local test = tap.test("misc-memprof-lapi")
-test:plan(16)
+test:plan(17)
 
 jit.off()
 jit.flush()
@@ -81,7 +81,7 @@ local function fill_ev_type(events, symbols, event_type)
   for _, event in pairs(events[event_type]) do
     local addr = event.loc.addr
     local traceno = event.loc.traceno
-    if traceno ~= 0 then
+    if traceno ~= 0 and symbols.trace[traceno] then
       local trace_loc = symbols.trace[traceno]
       addr = trace_loc.addr
       ev_type[trace_loc.line] = {
@@ -214,9 +214,17 @@ misc.memprof.stop()
 -- Test profiler with enabled JIT.
 jit.on()
 
--- Pregenerate traces to get expected output:
-default_payload()
+-- On this run traces are generated, JIT-related allocations
+-- will be recorded as well.
+symbols, events = generate_parsed_output(default_payload)
+
+alloc = fill_ev_type(events, symbols, "alloc")
+
+-- Test for marking JIT-related allocations as internal.
+-- See also https://github.com/tarantool/tarantool/issues/5679.
+test:ok(alloc[0] == nil)
 
+-- Run already generated traces.
 symbols, events = generate_parsed_output(default_payload)
 
 alloc = fill_ev_type(events, symbols, "alloc")
-- 
2.32.0

Re: [Tarantool-patches] [PATCH luajit v2] memprof: report JIT-side allocations as internal

[Sergey Kaplun via Tarantool-patches]

Hi, thanks for the fixes!

Please, move test adjustment mentioned below to the patch for the symtab
with group allocations by trace's number.
Otherwise, LGTM.

On 29.07.21, Mikhail Shishatskiy wrote:
> There are cases when the memory profiler attempts to attribute
> allocations triggered by the JIT engine recording phase
> with a Lua function to be recorded. In this case,
> lj_debug_frameline() may return BC_NOPOS (i.e. a negative value).
> 
> Previously, these situations were ignored and the profiler
> reported, that the source line was equal to zero.
> 
> This patch adjusts profiler behavior to treat allocations
> described above as internal by dumping ASOURCE_INT if
> lj_debug_frameline() returns a negative value.
> 
> Resolves tarantool/tarantool#5679
> ---
> 
> Issue: https://github.com/tarantool/tarantool/issues/5679
> Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5679-report-jit-allocations-as-internal
> CI: https://github.com/tarantool/tarantool/tree/shishqa/gh-5679-report-jit-allocations-as-internal
> 
> Changes in v2:
>   - Fixed commit title to fit in width of 50 symbols;
>   - Rebased to the branch [1];
>   - Reused default_payload() as a payload to test patch behavior.
> 
> [1]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number
> 
>  src/lj_memprof.c                              | 28 ++++++++++++-------
>  .../misclib-memprof-lapi.test.lua             | 16 ++++++++---
>  2 files changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/src/lj_memprof.c b/src/lj_memprof.c
> index 87512c3a..e4d819a0 100644
> --- a/src/lj_memprof.c
> +++ b/src/lj_memprof.c

<snipped>

> diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> index b7e456e1..96864c6a 100644
> --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> @@ -12,7 +12,7 @@ utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
>  local tap = require("tap")
>  
>  local test = tap.test("misc-memprof-lapi")
> -test:plan(16)
> +test:plan(17)
>  
>  jit.off()
>  jit.flush()
> @@ -81,7 +81,7 @@ local function fill_ev_type(events, symbols, event_type)
>    for _, event in pairs(events[event_type]) do
>      local addr = event.loc.addr
>      local traceno = event.loc.traceno
> -    if traceno ~= 0 then
> +    if traceno ~= 0 and symbols.trace[traceno] then

This should be moved to the commit, that changes symtab, this change
looks unrelated to this patch, but relates to previous one [1].

>        local trace_loc = symbols.trace[traceno]
>        addr = trace_loc.addr
>        ev_type[trace_loc.line] = {
> @@ -214,9 +214,17 @@ misc.memprof.stop()

<snipped>

> -- 
> 2.32.0
> 

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-July/024944.html

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v2] memprof: report JIT-side allocations as internal

[Mikhail Shishatskiy via Tarantool-patches]

Hi, Sergey!
Thanks for the review!

On 14.08.2021 14:24, Sergey Kaplun wrote:
>Hi, thanks for the fixes!
>
>Please, move test adjustment mentioned below to the patch for the symtab
>with group allocations by trace's number.
>Otherwise, LGTM.
>
>On 29.07.21, Mikhail Shishatskiy wrote:
>> There are cases when the memory profiler attempts to attribute
>> allocations triggered by the JIT engine recording phase
>> with a Lua function to be recorded. In this case,
>> lj_debug_frameline() may return BC_NOPOS (i.e. a negative value).
>>
>> Previously, these situations were ignored and the profiler
>> reported, that the source line was equal to zero.
>>
>> This patch adjusts profiler behavior to treat allocations
>> described above as internal by dumping ASOURCE_INT if
>> lj_debug_frameline() returns a negative value.
>>
>> Resolves tarantool/tarantool#5679
>> ---
>>
>> Issue: https://github.com/tarantool/tarantool/issues/5679
>> Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5679-report-jit-allocations-as-internal
>> CI: https://github.com/tarantool/tarantool/tree/shishqa/gh-5679-report-jit-allocations-as-internal
>>
>> Changes in v2:
>> - Fixed commit title to fit in width of 50 symbols;
>> - Rebased to the branch [1];
>> - Reused default_payload() as a payload to test patch behavior.
>>
>> [1]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number
>>
>> src/lj_memprof.c | 28 ++++++++++++-------
>> .../misclib-memprof-lapi.test.lua | 16 ++++++++---
>> 2 files changed, 30 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/lj_memprof.c b/src/lj_memprof.c
>> index 87512c3a..e4d819a0 100644
>> --- a/src/lj_memprof.c
>> +++ b/src/lj_memprof.c
>
><snipped>
>
>> diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>> index b7e456e1..96864c6a 100644
>> --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
>> +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>> @@ -12,7 +12,7 @@ utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
>> local tap = require("tap")
>>
>> local test = tap.test("misc-memprof-lapi")
>> -test:plan(16)
>> +test:plan(17)
>>
>> jit.off()
>> jit.flush()
>> @@ -81,7 +81,7 @@ local function fill_ev_type(events, symbols, event_type)
>> for _, event in pairs(events[event_type]) do
>> local addr = event.loc.addr
>> local traceno = event.loc.traceno
>> - if traceno ~= 0 then
>> + if traceno ~= 0 and symbols.trace[traceno] then
>
>This should be moved to the commit, that changes symtab, this change
>looks unrelated to this patch, but relates to previous one [1].

Fixed in the patchset [1] and rebased to the branch [2]

>
>> local trace_loc = symbols.trace[traceno]
>> addr = trace_loc.addr
>> ev_type[trace_loc.line] = {
>> @@ -214,9 +214,17 @@ misc.memprof.stop()
>
><snipped>
>
>> --
>> 2.32.0
>>
>
>[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-July/024944.html
>
>--
>Best regards,
>Sergey Kaplun

Also, I slightly changed the test by deleting the check for deallocations
number because now trace generation happens for memprof payload, too, and
we can meet untrivial JIT-related deallocations in the second run.

---
diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
index f84b6df0..17bcb2f1 100644
--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
@@ -231,19 +231,25 @@ test:test("jit-output", function(subtest)
     jit.opt.start(3, "hotloop=10")
     jit.flush()
   
- -- Pregenerate traces to fill symtab entries in the next run.
- default_payload()
-
+ -- On this run traces are generated, JIT-related allocations
+ -- will be recorded as well.
     local symbols, events = generate_parsed_output(default_payload)
   
     local alloc = fill_ev_type(events, symbols, "alloc")
- local free = fill_ev_type(events, symbols, "free")
+
+ -- Test for marking JIT-related allocations as internal.
+ -- See also https://github.com/tarantool/tarantool/issues/5679.
+ subtest:ok(alloc[0] == nil)
+
+ -- Run already generated traces.
+ symbols, events = generate_parsed_output(default_payload)
+
+ alloc = fill_ev_type(events, symbols, "alloc")
   
     -- We expect, that loop will be compiled into a trace.
     subtest:ok(check_alloc_report(alloc, 1, 37, 32, 20))
     -- See same checks with jit.off().
     subtest:ok(check_alloc_report(alloc, 0, 34, 32, 2))
- subtest:ok(free.INTERNAL.num == 22)
   
     -- Restore default JIT settings.
     jit.opt.start(unpack(jit_opt_default))
--
2.32.0

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-August/025680.html
[2]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number

Issue: https://github.com/tarantool/tarantool/issues/5679
Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5679-report-jit-allocations-as-internal
Tarantool branch: https://github.com/tarantool/tarantool/tree/shishqa/gh-5679-report-jit-allocations-as-internal

Best regards,
Mikhail Shishatskiy

Re: [Tarantool-patches] [PATCH luajit v2] memprof: report JIT-side allocations as internal

[Mikhail Shishatskiy via Tarantool-patches]

Rebased to the updated gh-5814 [1] branch.

@ChangeLog
======================================================================
##feature/luajit

* Now memory profiler reports JIT-related allocations, which are not
   connected to a particular trace, as INTERNAL.
======================================================================

[1]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number

On 21.08.2021 11:43, Mikhail Shishatskiy via Tarantool-patches wrote:
>Hi, Sergey!
>Thanks for the review!
>
>On 14.08.2021 14:24, Sergey Kaplun wrote:
>>Hi, thanks for the fixes!
>>
>>Please, move test adjustment mentioned below to the patch for the symtab
>>with group allocations by trace's number.
>>Otherwise, LGTM.
>>
>>On 29.07.21, Mikhail Shishatskiy wrote:
>>>There are cases when the memory profiler attempts to attribute
>>>allocations triggered by the JIT engine recording phase
>>>with a Lua function to be recorded. In this case,
>>>lj_debug_frameline() may return BC_NOPOS (i.e. a negative value).
>>>
>>>Previously, these situations were ignored and the profiler
>>>reported, that the source line was equal to zero.
>>>
>>>This patch adjusts profiler behavior to treat allocations
>>>described above as internal by dumping ASOURCE_INT if
>>>lj_debug_frameline() returns a negative value.
>>>
>>>Resolves tarantool/tarantool#5679
>>>---
>>>
>>>Issue: https://github.com/tarantool/tarantool/issues/5679
>>>Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5679-report-jit-allocations-as-internal
>>>CI: https://github.com/tarantool/tarantool/tree/shishqa/gh-5679-report-jit-allocations-as-internal
>>>
>>>Changes in v2:
>>>- Fixed commit title to fit in width of 50 symbols;
>>>- Rebased to the branch [1];
>>>- Reused default_payload() as a payload to test patch behavior.
>>>
>>>[1]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number
>>>
>>>src/lj_memprof.c | 28 ++++++++++++-------
>>>.../misclib-memprof-lapi.test.lua | 16 ++++++++---
>>>2 files changed, 30 insertions(+), 14 deletions(-)
>>>
>>>diff --git a/src/lj_memprof.c b/src/lj_memprof.c
>>>index 87512c3a..e4d819a0 100644
>>>--- a/src/lj_memprof.c
>>>+++ b/src/lj_memprof.c
>>
>><snipped>
>>
>>>diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>>>index b7e456e1..96864c6a 100644
>>>--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
>>>+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>>>@@ -12,7 +12,7 @@ utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
>>>local tap = require("tap")
>>>
>>>local test = tap.test("misc-memprof-lapi")
>>>-test:plan(16)
>>>+test:plan(17)
>>>
>>>jit.off()
>>>jit.flush()
>>>@@ -81,7 +81,7 @@ local function fill_ev_type(events, symbols, event_type)
>>>for _, event in pairs(events[event_type]) do
>>>local addr = event.loc.addr
>>>local traceno = event.loc.traceno
>>>- if traceno ~= 0 then
>>>+ if traceno ~= 0 and symbols.trace[traceno] then
>>
>>This should be moved to the commit, that changes symtab, this change
>>looks unrelated to this patch, but relates to previous one [1].
>
>Fixed in the patchset [1] and rebased to the branch [2]
>
>>
>>>local trace_loc = symbols.trace[traceno]
>>>addr = trace_loc.addr
>>>ev_type[trace_loc.line] = {
>>>@@ -214,9 +214,17 @@ misc.memprof.stop()
>>
>><snipped>
>>
>>>--
>>>2.32.0
>>>
>>
>>[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-July/024944.html
>>
>>--
>>Best regards,
>>Sergey Kaplun
>
>Also, I slightly changed the test by deleting the check for deallocations
>number because now trace generation happens for memprof payload, too, and
>we can meet untrivial JIT-related deallocations in the second run.
>
>---
>diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>index f84b6df0..17bcb2f1 100644
>--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
>+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>@@ -231,19 +231,25 @@ test:test("jit-output", function(subtest)
>    jit.opt.start(3, "hotloop=10")
>    jit.flush()
>- -- Pregenerate traces to fill symtab entries in the next run.
>- default_payload()
>-
>+ -- On this run traces are generated, JIT-related allocations
>+ -- will be recorded as well.
>    local symbols, events = generate_parsed_output(default_payload)
>    local alloc = fill_ev_type(events, symbols, "alloc")
>- local free = fill_ev_type(events, symbols, "free")
>+
>+ -- Test for marking JIT-related allocations as internal.
>+ -- See also https://github.com/tarantool/tarantool/issues/5679.
>+ subtest:ok(alloc[0] == nil)
>+
>+ -- Run already generated traces.
>+ symbols, events = generate_parsed_output(default_payload)
>+
>+ alloc = fill_ev_type(events, symbols, "alloc")
>    -- We expect, that loop will be compiled into a trace.
>    subtest:ok(check_alloc_report(alloc, 1, 37, 32, 20))
>    -- See same checks with jit.off().
>    subtest:ok(check_alloc_report(alloc, 0, 34, 32, 2))
>- subtest:ok(free.INTERNAL.num == 22)
>    -- Restore default JIT settings.
>    jit.opt.start(unpack(jit_opt_default))
>--
>2.32.0
>
>[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-August/025680.html
>[2]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number
>
>Issue: https://github.com/tarantool/tarantool/issues/5679
>Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5679-report-jit-allocations-as-internal
>Tarantool branch: https://github.com/tarantool/tarantool/tree/shishqa/gh-5679-report-jit-allocations-as-internal
>
>Best regards,
>Mikhail Shishatskiy

Re: [Tarantool-patches] [PATCH luajit v2] memprof: report JIT-side allocations as internal

[Igor Munkin via Tarantool-patches]

Misha,

Thanks for the patch! LGTM, except the single nit.

On 21.08.21, Mikhail Shishatskiy wrote:
> Hi, Sergey!
> Thanks for the review!
> 

<snipped>

> +
> + -- Test for marking JIT-related allocations as internal.
> + -- See also https://github.com/tarantool/tarantool/issues/5679.
> + subtest:ok(alloc[0] == nil)

As a result of the changes made in scope of #5814, this line need to be
adjusted the following way:
| subtest:ok(alloc.line[0] == nil)

> +
> + -- Run already generated traces.

<snipped>

> 
> Best regards,
> Mikhail Shishatskiy

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit v2] memprof: report JIT-side allocations as internal

[Igor Munkin via Tarantool-patches]

Misha,

I've checked the patch into tarantool branch in tarantool/luajit and
bumped a new version in master.

On 29.07.21, Mikhail Shishatskiy wrote:
> There are cases when the memory profiler attempts to attribute
> allocations triggered by the JIT engine recording phase
> with a Lua function to be recorded. In this case,
> lj_debug_frameline() may return BC_NOPOS (i.e. a negative value).
> 
> Previously, these situations were ignored and the profiler
> reported, that the source line was equal to zero.
> 
> This patch adjusts profiler behavior to treat allocations
> described above as internal by dumping ASOURCE_INT if
> lj_debug_frameline() returns a negative value.
> 
> Resolves tarantool/tarantool#5679
> ---
> 
> Issue: https://github.com/tarantool/tarantool/issues/5679
> Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5679-report-jit-allocations-as-internal
> CI: https://github.com/tarantool/tarantool/tree/shishqa/gh-5679-report-jit-allocations-as-internal
> 
> Changes in v2:
>   - Fixed commit title to fit in width of 50 symbols;
>   - Rebased to the branch [1];
>   - Reused default_payload() as a payload to test patch behavior.
> 
> [1]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number
> 
>  src/lj_memprof.c                              | 28 ++++++++++++-------
>  .../misclib-memprof-lapi.test.lua             | 16 ++++++++---
>  2 files changed, 30 insertions(+), 14 deletions(-)
> 

<snipped>

> -- 
> 2.32.0
> 

-- 
Best regards,
IM