[Tarantool-patches] [PATCH luajit v2 2/3] memprof: enrich symtab when meeting new prototype
Sergey Kaplun
skaplun at tarantool.org
Tue Jan 25 11:14:59 MSK 2022
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
More information about the Tarantool-patches
mailing list