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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.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?

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