[Tarantool-patches] [PATCH luajit v1] memprof: report all JIT-related allocations as internal

[Tarantool-patches] [PATCH luajit v1] memprof: report all JIT-related 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

 src/lj_memprof.c                              | 28 +++++++----
 .../misclib-memprof-lapi.test.lua             | 50 ++++++++++++-------
 2 files changed, 51 insertions(+), 27 deletions(-)

diff --git a/src/lj_memprof.c b/src/lj_memprof.c
index 2c1ef3b8..b4985b8e 100644
--- a/src/lj_memprof.c
+++ b/src/lj_memprof.c
@@ -89,19 +89,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 06d96b3b..e35d8c29 100644
--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
@@ -7,7 +7,7 @@ require("utils").skipcond(
 local tap = require("tap")
 
 local test = tap.test("misc-memprof-lapi")
-test:plan(13)
+test:plan(14)
 
 jit.off()
 jit.flush()
@@ -22,7 +22,7 @@ local symtab = require "utils.symtab"
 local TMP_BINFILE = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%.%1.memprofdata.tmp.bin")
 local BAD_PATH = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%1/memprofdata.tmp.bin")
 
-local function payload()
+local function default_payload()
   -- Preallocate table to avoid table array part reallocations.
   local _ = table_new(100, 0)
 
@@ -37,7 +37,7 @@ local function payload()
   collectgarbage()
 end
 
-local function generate_output(filename)
+local function generate_output(filename, payload)
   -- Clean up all garbage to avoid pollution of free.
   collectgarbage()
 
@@ -52,6 +52,25 @@ local function generate_output(filename)
   assert(res, err)
 end
 
+local function generate_parsed_output(filename, payload)
+  local res, err = pcall(generate_output, filename, payload)
+
+  -- Want to cleanup carefully if something went wrong.
+  if not res then
+    os.remove(filename)
+    error(err)
+  end
+
+  local reader = bufread.new(filename)
+  local symbols = symtab.parse(reader)
+  local events = memprof.parse(reader)
+
+  -- We don't need it any more.
+  os.remove(filename)
+
+  return symbols, events
+end
+
 local function fill_ev_type(events, symbols, event_type)
   local ev_type = {}
   for _, event in pairs(events[event_type]) do
@@ -107,20 +126,7 @@ test:ok(res == nil and err:match("profiler is not running"))
 test:ok(type(errno) == "number")
 
 -- Test profiler output and parse.
-res, err = pcall(generate_output, TMP_BINFILE)
-
--- Want to cleanup carefully if something went wrong.
-if not res then
-  os.remove(TMP_BINFILE)
-  error(err)
-end
-
-local reader = bufread.new(TMP_BINFILE)
-local symbols = symtab.parse(reader)
-local events = memprof.parse(reader, symbols)
-
--- We don't need it any more.
-os.remove(TMP_BINFILE)
+local symbols, events = generate_parsed_output(TMP_BINFILE, default_payload)
 
 local alloc = fill_ev_type(events, symbols, "alloc")
 local free = fill_ev_type(events, symbols, "free")
@@ -166,5 +172,15 @@ local co = coroutine.create(f)
 coroutine.resume(co)
 misc.memprof.stop()
 
+-- Test for marking jit-related allocations as internal.
+-- See also https://github.com/tarantool/tarantool/issues/5679.
 jit.on()
+symbols, events = generate_parsed_output(TMP_BINFILE, function()
+  for _ = 1, 100 do
+    local _ = {_, _}
+  end
+end)
+alloc = fill_ev_type(events, symbols, "alloc")
+test:ok(alloc[0] == nil)
+
 os.exit(test:check() and 0 or 1)
-- 
2.32.0

Re: [Tarantool-patches] [PATCH luajit v1] memprof: report all JIT-related allocations as internal

[Sergey Kaplun via Tarantool-patches]

Hi, Mikhail!

Thanks for the patch!

On 22.07.21, Mikhail Shishatskiy wrote:

| memprof: report all JIT-related allocations as internal

Width limit for commit title is 50 symbols including subsystem name
considering our code style guide [1].

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

Maybe it is better to catch out the JIT-related allocation and associate
it with the corresponding trace after these issues [2][3] will be
resolved, isn't it?
Otherwise after fixing other nitpicks patch LGTM.

Thoughts?

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

Sorry, I can't find Tarantool's standalone branch with these changes to
check CI. May you please share it here?

> 
>  src/lj_memprof.c                              | 28 +++++++----
>  .../misclib-memprof-lapi.test.lua             | 50 ++++++++++++-------
>  2 files changed, 51 insertions(+), 27 deletions(-)
> 
> diff --git a/src/lj_memprof.c b/src/lj_memprof.c
> index 2c1ef3b8..b4985b8e 100644
> --- a/src/lj_memprof.c
> +++ b/src/lj_memprof.c
> @@ -89,19 +89,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 06d96b3b..e35d8c29 100644
> --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> @@ -7,7 +7,7 @@ require("utils").skipcond(
>  local tap = require("tap")
>  
>  local test = tap.test("misc-memprof-lapi")
> -test:plan(13)
> +test:plan(14)
>  
>  jit.off()
>  jit.flush()
> @@ -22,7 +22,7 @@ local symtab = require "utils.symtab"
>  local TMP_BINFILE = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%.%1.memprofdata.tmp.bin")
>  local BAD_PATH = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%1/memprofdata.tmp.bin")
>  
> -local function payload()
> +local function default_payload()
>    -- Preallocate table to avoid table array part reallocations.
>    local _ = table_new(100, 0)
>  
> @@ -37,7 +37,7 @@ local function payload()
>    collectgarbage()
>  end
>  
> -local function generate_output(filename)
> +local function generate_output(filename, payload)
>    -- Clean up all garbage to avoid pollution of free.
>    collectgarbage()
>  
> @@ -52,6 +52,25 @@ local function generate_output(filename)
>    assert(res, err)
>  end
>  
> +local function generate_parsed_output(filename, payload)
> +  local res, err = pcall(generate_output, filename, payload)
> +
> +  -- Want to cleanup carefully if something went wrong.
> +  if not res then
> +    os.remove(filename)
> +    error(err)
> +  end
> +
> +  local reader = bufread.new(filename)
> +  local symbols = symtab.parse(reader)
> +  local events = memprof.parse(reader)
> +
> +  -- We don't need it any more.
> +  os.remove(filename)
> +
> +  return symbols, events
> +end
> +
>  local function fill_ev_type(events, symbols, event_type)
>    local ev_type = {}
>    for _, event in pairs(events[event_type]) do
> @@ -107,20 +126,7 @@ test:ok(res == nil and err:match("profiler is not running"))
>  test:ok(type(errno) == "number")
>  
>  -- Test profiler output and parse.
> -res, err = pcall(generate_output, TMP_BINFILE)
> -
> --- Want to cleanup carefully if something went wrong.
> -if not res then
> -  os.remove(TMP_BINFILE)
> -  error(err)
> -end
> -
> -local reader = bufread.new(TMP_BINFILE)
> -local symbols = symtab.parse(reader)
> -local events = memprof.parse(reader, symbols)
> -
> --- We don't need it any more.
> -os.remove(TMP_BINFILE)
> +local symbols, events = generate_parsed_output(TMP_BINFILE, default_payload)
>  
>  local alloc = fill_ev_type(events, symbols, "alloc")
>  local free = fill_ev_type(events, symbols, "free")
> @@ -166,5 +172,15 @@ local co = coroutine.create(f)
>  coroutine.resume(co)
>  misc.memprof.stop()
>  
> +-- Test for marking jit-related allocations as internal.
> +-- See also https://github.com/tarantool/tarantool/issues/5679.
>  jit.on()
> +symbols, events = generate_parsed_output(TMP_BINFILE, function()

Why do you use this instead of default_payload?
Please drop a comment here.

> +  for _ = 1, 100 do
> +    local _ = {_, _}
> +  end
> +end)
> +alloc = fill_ev_type(events, symbols, "alloc")
> +test:ok(alloc[0] == nil)
> +
>  os.exit(test:check() and 0 or 1)
> -- 
> 2.32.0
> 

[1]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message
[2]: https://github.com/tarantool/tarantool/issues/5814
[3]: https://github.com/tarantool/tarantool/issues/5815

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v1] memprof: report all JIT-related allocations as internal

[Mikhail Shishatskiy via Tarantool-patches]

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


Hi, Sergey!
Thank you for the review! I will fix the nitpicks in the next version
of the patch.
  
>Вторник, 27 июля 2021, 17:00 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:
> 
>Hi, Mikhail!
>
>Thanks for the patch!
>
>On 22.07.21, Mikhail Shishatskiy wrote:
>
>| memprof: report all JIT-related allocations as internal
>
>Width limit for commit title is 50 symbols including subsystem name
>considering our code style guide [1].
>
>> 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.
>
>Maybe it is better to catch out the JIT-related allocation and associate
>it with the corresponding trace after these issues [2][3] will be
>resolved, isn't it?
>Otherwise after fixing other nitpicks patch LGTM.
>
>Thoughts?
 
I’ve tested the behavior of such allocations: all of them are actually connected to
the growth of general-purpose vectors in the JIT state. It is a controversial question,
should we report them as trace-related or as internal. IMO, internal is better, as
we do not confuse users by mixing the number of internally allocated bytes with bytes
allocated from the actual payload.
 
>>
>> 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
>
>Sorry, I can't find Tarantool's standalone branch with these changes to
>check CI. May you please share it here?
 
Strange, I can not find it too, but remember that I have pushed it. I think
I can rebase to the branch [1], as the current patch uses the same changes
in the memprof tests. And when [1] will be approved, I will resend the new 
version with a shorter commit message title and some changes in tests (see below).
 
>>
>> src/lj_memprof.c | 28 +++++++----
>> .../misclib-memprof-lapi.test.lua | 50 ++++++++++++-------
>> 2 files changed, 51 insertions(+), 27 deletions(-)
>>
>> diff --git a/src/lj_memprof.c b/src/lj_memprof.c
>> index 2c1ef3b8..b4985b8e 100644
>> --- a/src/lj_memprof.c
>> +++ b/src/lj_memprof.c
<snipped>
>>
>> +-- Test for marking jit-related allocations as internal.
>> +-- See also  https://github.com/tarantool/tarantool/issues/5679 .
>> jit.on()
>> +symbols, events = generate_parsed_output(TMP_BINFILE, function()
>
>Why do you use this instead of default_payload?
>Please drop a comment here.
 
I’ve checked again and realized, that I can use default_payload to test this behavior :)
 
>> + for _ = 1, 100 do
>> + local _ = {_, _}
>> + end
>> +end)
>> +alloc = fill_ev_type(events, symbols, "alloc")
>> +test:ok(alloc[0] == nil)
>> +
>> os.exit(test:check() and 0 or 1)
>> --
>> 2.32.0
>>
>
>[1]:  https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message
>[2]:  https://github.com/tarantool/tarantool/issues/5814
>[3]:  https://github.com/tarantool/tarantool/issues/5815
>
>--
>Best regards,
>Sergey Kaplun
 
[1]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number
--
Best regards,
Mikhail Shishatskiy

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