Re: [Tarantool-patches] [PATCH luajit v2 2/3] memprof: enrich symtab when meeting new prototype

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Mikhail!

Thanks for the patch!

LGTM, except a few comments below.

Also, please share Tarantool's branch for tests, I can't found it :(.

On 19.01.22, Mikhail Shishatskiy wrote:
> Updated patch in line with what we discussed with Sergey offline:
> 
> New commit message:
> ===========================================================
> memprof: enrich symtab when meeting new prototype
> 
> Previously, the profiler dumped all the function prototypes only
> at the start of profiling. It was enough for most cases, but
> sometimes we may want to investigate the memory profile of module
> loading. In such a case, we expect memprof to dump new prototypes
> from parsed source code.
> 
> This patch extends memprof's streaming format with entries providing
> extra information about allocation source's symbols if they were not
> streamed at the start of profiling. This event is called <event-symtab>
> and precedes the allocation-related event. The format is the following:
> 
> | event-symtab := event-header sym?
> | sym          := sym-lua
> | sym-lua      := sym-addr sym-chunk sym-line
> 
> The `sym-addr`, `sym-chunk` and `sym-line` are the prototype's address,
> its chunk name and line where it was defined.
> 
> The profiler dumps new prototypes to the symbol table right after the
> creation of a GCproto object.
> 
> Also, the profiler parser is adjusted to recognize entries described
> above and enrich the symtab on the fly while parsing events. For this
> reason, the <utils/symtab.lua> API has been changed: the function
> `parse_sym_lfunc` became public to be available for the parser module.
> 
> Resolves tarantool/tarantool#5815
> ===========================================================
> 
> New diff:

Did I understand correctly, that after this patch there is no need in
epoch labels for prototypes? So the previous commit can be removed?

> ===========================================================
> diff --git a/src/Makefile.dep.original b/src/Makefile.dep.original
> index faa44a0b..82f34315 100644
> --- a/src/Makefile.dep.original
> +++ b/src/Makefile.dep.original
> @@ -59,7 +59,7 @@ lj_bc.o: lj_bc.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h lj_bc.h \
>   lj_bcread.o: lj_bcread.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h \
>    lj_gc.h lj_err.h lj_errmsg.h lj_buf.h lj_str.h lj_tab.h lj_bc.h \
>    lj_ctype.h lj_cdata.h lualib.h lj_lex.h lj_bcdump.h lj_state.h \
> - lj_strfmt.h
> + lj_strfmt.h lj_memprof.h

You must mention lj_wbuf.h here too, as far as it is included by
lj_memprof.h.

>   lj_bcwrite.o: lj_bcwrite.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h \
>    lj_gc.h lj_buf.h lj_str.h lj_bc.h lj_ctype.h lj_dispatch.h lj_jit.h \
>    lj_ir.h lj_strfmt.h lj_bcdump.h lj_lex.h lj_err.h lj_errmsg.h lj_vm.h
> @@ -175,7 +175,7 @@ lj_opt_split.o: lj_opt_split.c lj_obj.h lua.h luaconf.h lj_def.h \
>   lj_parse.o: lj_parse.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h \
>    lj_gc.h lj_err.h lj_errmsg.h lj_debug.h lj_buf.h lj_str.h lj_tab.h \
>    lj_func.h lj_state.h lj_bc.h lj_ctype.h lj_strfmt.h lj_lex.h lj_parse.h \
> - lj_vm.h lj_vmevent.h
> + lj_vm.h lj_vmevent.h lj_memprof.h

Ditto.

>   lj_profile.o: lj_profile.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h \
>    lj_buf.h lj_gc.h lj_str.h lj_frame.h lj_bc.h lj_debug.h lj_dispatch.h \
>    lj_jit.h lj_ir.h lj_trace.h lj_traceerr.h lj_profile.h luajit.h
> diff --git a/src/lj_bcread.c b/src/lj_bcread.c
> index 48c5e7c7..cb08599d 100644
> --- a/src/lj_bcread.c
> +++ b/src/lj_bcread.c
> @@ -22,6 +22,9 @@

<snipped>

> diff --git a/src/lj_memprof.c b/src/lj_memprof.c
> index 2d779983..9bb2483e 100644
> --- a/src/lj_memprof.c
> +++ b/src/lj_memprof.c
> @@ -66,6 +66,14 @@ static void dump_symtab_trace(struct lj_wbuf *out, const GCtrace *trace)
> 
>   #endif
> 
> +static void dump_symtab_proto(struct lj_wbuf *out, const GCproto *pt,
> +			      const global_State *g)

Looks like global_State is unused in this function.

> +{
> +  lj_wbuf_addu64(out, (uintptr_t)pt);
> +  lj_wbuf_addstring(out, proto_chunknamestr(pt));
> +  lj_wbuf_addu64(out, (uint64_t)pt->firstline);
> +}
> +
>   static void dump_symtab(struct lj_wbuf *out, const struct global_State *g)
>   {
>     const GCRef *iter = &g->gc.root;
> @@ -80,9 +88,7 @@ static void dump_symtab(struct lj_wbuf *out, const struct global_State *g)
>       case (~LJ_TPROTO): {
>         const GCproto *pt = gco2pt(o);
>         lj_wbuf_addbyte(out, SYMTAB_LFUNC);
> -      lj_wbuf_addu64(out, (uintptr_t)pt);
> -      lj_wbuf_addstring(out, proto_chunknamestr(pt));
> -      lj_wbuf_addu64(out, (uint64_t)pt->firstline);
> +      dump_symtab_proto(out, pt, g);
>         break;
>       }
>       case (~LJ_TTRACE): {
> @@ -140,6 +146,7 @@ static void memprof_write_lfunc(struct lj_wbuf *out, uint8_t aevent,

The next chunk (*) seems like unnecessary changes, why do we need them?
We can just add the comment to the old chunk if you want.

>     ** -DLUAJIT_DISABLE_DEBUGINFO flag.
>     */
>     const BCLine line = lj_debug_frameline(L, fn, nextframe);
> +  const GCproto *pt = funcproto(fn);
> 
>     if (line < 0) {
>       /*
> @@ -151,11 +158,17 @@ static void memprof_write_lfunc(struct lj_wbuf *out, uint8_t aevent,
>       ** 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);
> +    return;
>     }
> +
> +  /*
> +  ** As a prototype is a source of an allocation, it has
> +  ** already been inserted into the symtab: on the start
> +  ** of the profiling or right after the creation.
> +  */
> +  lj_wbuf_addbyte(out, aevent | ASOURCE_LFUNC);
> +  lj_wbuf_addu64(out, (uintptr_t)pt);
> +  lj_wbuf_addu64(out, (uint64_t)line);
>   }
> 
>   static void memprof_write_cfunc(struct lj_wbuf *out, uint8_t aevent,

(*) up to here.

> @@ -414,6 +427,17 @@ errio:

<snipped>

> diff --git a/src/lj_memprof.h b/src/lj_memprof.h
> index 395fb429..eda8ca0c 100644
> --- a/src/lj_memprof.h
> +++ b/src/lj_memprof.h

<snipped>

> @@ -164,4 +174,10 @@ int lj_memprof_start(struct lua_State *L, const struct lj_memprof_options *opt);
>   */
>   int lj_memprof_stop(struct lua_State *L);
> 
> +/*
> +** Enriches profiler symbol table with a new proto, if profiler

Typo: s/profiler/the profiler/

> +** is running.
> +*/
> +void lj_memprof_add_proto(const struct GCproto *pt);
> +
>   #endif
> diff --git a/src/lj_parse.c b/src/lj_parse.c
> index a6325a76..30b0caa0 100644
> --- a/src/lj_parse.c
> +++ b/src/lj_parse.c
> @@ -27,6 +27,9 @@

<snipped>

> diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> index dd973f2a..1c74b4d7 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(

<snipped>

> @@ -223,6 +224,38 @@ test:test("stack-resize", function(subtest)
>     misc.memprof.stop()
>   end)
> 
> +-- Test for extending symtab with function prototypes
> +-- while profiler is running.
> +test:test("symtab-enriching", function(subtest)
> +  subtest:plan(2)
> +
> +  local payload_str = [[
> +  local M = {
> +    tmp = string.rep("1", 100)  -- line 2.
> +  }
> +
> +  function M.payload()
> +    local str = string.rep("42", 100)  -- line 6.
> +  end
> +
> +  return M
> +  ]]
> +
> +  local symbols, events = generate_parsed_output(function()
> +    local str_chunk = assert(loadstring(payload_str, 'str_chunk'))()
> +    str_chunk.payload()
> +  end)
> +
> +  local alloc = fill_ev_type(events, symbols, "alloc")
> +
> +  subtest:ok(check_alloc_report(
> +    alloc, { source = 'str_chunk', line = 6, linedefined = 5 }, 1)
> +  )
> +  subtest:ok(check_alloc_report(
> +    alloc, { source = 'str_chunk', line = 2, linedefined = 0 }, 1)
> +  )

It is also good to test bcread case. Please, add the corresponding
test.

> +end)
> +
>   -- Test profiler with enabled JIT.
>   jit.on()
> 
> diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua
> index be5844a4..38f76f00 100644
> --- a/tools/memprof/parse.lua
> +++ b/tools/memprof/parse.lua
> @@ -13,10 +13,11 @@ local string_format = string.format

<snipped>

> diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua
> index c758b67e..00bab03a 100644
> --- a/tools/utils/symtab.lua
> +++ b/tools/utils/symtab.lua

<snipped>

> ===========================================================
> 
> On 02.12.2021 14:03, Mikhail Shishatskiy wrote:

<snipped>

> >

-- 
Best regards,
Sergey Kaplun