[Tarantool-patches] [PATCH luajit v1] memprof: report all JIT-related allocations as internal
Sergey Kaplun
skaplun at tarantool.org
Tue Jul 27 16:59:35 MSK 2021
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
More information about the Tarantool-patches
mailing list