Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Mikhail Shishatskiy <m.shishatskiy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit v4 3/4] memprof: group allocations on traces by traceno
Date: Thu, 11 Nov 2021 18:34:37 +0300	[thread overview]
Message-ID: <YY04DZEQX5Xafr4m@tarantool.org> (raw)
In-Reply-To: <20211104130156.f2botlihlfhwd3yh@surf.localdomain>

Misha,

Thanks for the fixes! LGTM now.

On 04.11.21, Mikhail Shishatskiy wrote:
> Hi, Igor!
> Thank you for the review!
> 
> New commit message:
> ============================================================
> memprof: group allocations on traces by traceno
> 
> When LuaJIT executes a trace, the trace number is stored as
> the virtual machine state. So, we can treat this number as
> an allocation event source in memprof and report allocation events
> from traces as well.
> 
> Previously, all the allocations from traces were marked as INTERNAL.
> 
> This patch introduces the functionality described above by adding
> a new allocation source type named ASOURCE_TRACE. If allocation event
> occurs when trace is executed, trace number and trace's mcode starting
> address are streamed to a binary file:
> 
> | loc-trace  := trace-no trace-addr
> | trace-no   := <ULEB128>
> | trace-addr := <ULEB128>
> 
> Also, the memory profiler parser is adjusted to recognize entries
> mentioned above. The <loc> structure is extended with field <traceno>,
> representing trace number. Trace locations are demangled as
> 
> | TRACE [<trace-no>] <trace-addr>
> 
> Resolves tarantool/tarantool#5814
> ============================================================
> 
> Diff:
> ============================================================
> diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> index 0328c06d..4b7140e9 100644
> --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> @@ -113,14 +113,15 @@ local function form_source_line(line)
>     return string.format("@%s:%d", arg[0], line)
>   end
> 
> -local function check_alloc_report(alloc, traceno, line, function_line, nevents)
> +local function check_alloc_report(alloc, location, nevents)
>     local expected_name, event
> -  if traceno ~= 0 then
> +  local traceno = location.traceno
> +  if traceno then
>       expected_name = string.format("TRACE [%d]", traceno)
>       event = alloc.trace[traceno]
>     else
> -    expected_name = form_source_line(function_line)
> -    event = alloc.line[line]
> +    expected_name = form_source_line(location.linedefined)
> +    event = alloc.line[location.line]
>     end
>     assert(expected_name == event.name, ("got='%s', expected='%s'"):format(
>       event.name,
> @@ -173,9 +174,9 @@ test:test("output", function(subtest)
>     -- one is the number of allocations. 1 event - alocation of
>     -- table by itself + 1 allocation of array part as far it is
>     -- bigger than LJ_MAX_COLOSIZE (16).
> -  subtest:ok(check_alloc_report(alloc, 0, 34, 32, 2))
> +  subtest:ok(check_alloc_report(alloc, { line = 34, linedefined = 32 }, 2))
>     -- 20 strings allocations.
> -  subtest:ok(check_alloc_report(alloc, 0, 39, 32, 20))
> +  subtest:ok(check_alloc_report(alloc, { line = 39, linedefined = 32 }, 20))
> 
>     -- Collect all previous allocated objects.
>     subtest:ok(free.INTERNAL.num == 22)
> @@ -238,9 +239,9 @@ test:test("jit-output", function(subtest)
>     local free = fill_ev_type(events, symbols, "free")
> 
>     -- We expect, that loop will be compiled into a trace.
> -  subtest:ok(check_alloc_report(alloc, 1, 37, 32, 20))
> +  subtest:ok(check_alloc_report(alloc, { traceno = 1 }, 20))
>     -- See same checks with jit.off().
> -  subtest:ok(check_alloc_report(alloc, 0, 34, 32, 2))
> +  subtest:ok(check_alloc_report(alloc, { line = 34, linedefined = 32 }, 2))
>     subtest:ok(free.INTERNAL.num == 22)
> 
>     -- Restore default JIT settings.
> diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua
> index 85945fb2..c4acc4cb 100644
> --- a/tools/utils/symtab.lua
> +++ b/tools/utils/symtab.lua
> @@ -77,26 +77,22 @@ function M.id(loc)
>     return string_format("f%#xl%dt%d", loc.addr, loc.line, loc.traceno)
>   end
> 
> -local function demangle_lfunc(symtab, loc)
> +function M.demangle(symtab, loc)
> +  if loc.traceno ~= 0 then
> +    return string_format("TRACE [%d] %#x", loc.traceno, loc.addr)
> +  end
> +
>     local addr = loc.addr
> 
>     if addr == 0 then
>       return "INTERNAL"
> -  elseif symtab[addr] then
> -    return string_format("%s:%d", symtab[addr].source, loc.line)
>     end
> -  return string_format("CFUNC %#x", addr)
> -end
> 
> -local function demangle_trace(loc)
> -  return string_format("TRACE [%d] %#x", loc.traceno, loc.addr)
> -end
> -
> -function M.demangle(symtab, loc)
> -  if loc.traceno ~= 0 then
> -    return demangle_trace(loc)
> +  if symtab[addr] then
> +    return string_format("%s:%d", symtab[addr].source, loc.line)
>     end
> -  return demangle_lfunc(symtab, loc)
> +
> +  return string_format("CFUNC %#x", addr)
>   end
> 
>   return M
> ============================================================
> 
> Issue: https://github.com/tarantool/tarantool/issues/5814
> Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number
> CI: https://github.com/tarantool/tarantool/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number
> 
> On 27.10.2021 16:56, Igor Munkin wrote:
> >Misha,
> >
> >Thanks for the patch! Please, consider the comments below.
> >
> >On 29.09.21, Mikhail Shishatskiy wrote:

<snipped>

> 
> >
> >Typo: s/streamed/is streamed/.
> 
> I assume, there should be s/streamed/are streamed/.

Precisely.

> 

<snipped>

> >>
> >> Resolves tarantool/tarantool#5814
> >
> >Does this patch "resolve" the issue or only being a "part of" it?
> 
> IMO, this patch _resolves_ the issue, as we are now able to get
> information about allocations on particular traces:
> 
> | TRACE [<trace-no>] <trace-addr>
> 
> And this is exactly what was requested in the issue.

Got it, thanks.

> 

<snipped>

> >
> >-- 
> >Best regards,
> >IM
> 
> --
> Best regards,
> Mikhail Shishatskiy

-- 
Best regards,
IM

  parent reply	other threads:[~2021-11-11 15:34 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-20  7:05 [Tarantool-patches] [PATCH luajit v3 0/4] memprof: group allocations on traces by trace number Mikhail Shishatskiy via Tarantool-patches
2021-08-20  7:05 ` [Tarantool-patches] [PATCH luajit v3 1/5] core: add const to lj_debug_line proto parameter Mikhail Shishatskiy via Tarantool-patches
2021-09-16 15:29   ` Igor Munkin via Tarantool-patches
2021-08-20  7:05 ` [Tarantool-patches] [PATCH luajit v3 2/5] test: separate memprof Lua API tests into subtests Mikhail Shishatskiy via Tarantool-patches
2021-09-16 15:29   ` Igor Munkin via Tarantool-patches
2021-08-20  7:05 ` [Tarantool-patches] [PATCH luajit v3 3/5] memprof: dump traceno if allocate from trace Mikhail Shishatskiy via Tarantool-patches
2021-09-16 15:32   ` Igor Munkin via Tarantool-patches
2021-09-29 19:21     ` Mikhail Shishatskiy via Tarantool-patches
2021-08-20  7:05 ` [Tarantool-patches] [PATCH luajit v3 4/5] memprof: extend symtab with info about traces Mikhail Shishatskiy via Tarantool-patches
2021-09-16 15:32   ` Igor Munkin via Tarantool-patches
2021-09-29 19:21     ` Mikhail Shishatskiy via Tarantool-patches
2021-08-20  7:05 ` [Tarantool-patches] [PATCH luajit v3 5/5] luajit: change order of modules Mikhail Shishatskiy via Tarantool-patches
2021-09-16 15:32   ` Igor Munkin via Tarantool-patches
2021-09-29 20:07 ` [Tarantool-patches] [PATCH luajit v4 0/4] memprof: group allocations on traces by traceno Mikhail Shishatskiy via Tarantool-patches
2021-09-29 20:07   ` [Tarantool-patches] [PATCH luajit v4 1/4] test: separate memprof Lua API tests into subtests Mikhail Shishatskiy via Tarantool-patches
2021-10-27 13:56     ` Igor Munkin via Tarantool-patches
2021-10-27 15:07     ` Sergey Kaplun via Tarantool-patches
2021-09-29 20:07   ` [Tarantool-patches] [PATCH luajit v4 2/4] memprof: refactor location parsing Mikhail Shishatskiy via Tarantool-patches
2021-10-27 13:56     ` Igor Munkin via Tarantool-patches
     [not found]       ` <20211104130010.mcvnra6e4yl5moo2@surf.localdomain>
2021-11-10 15:38         ` Igor Munkin via Tarantool-patches
2021-09-29 20:07   ` [Tarantool-patches] [PATCH luajit v4 3/4] memprof: group allocations on traces by traceno Mikhail Shishatskiy via Tarantool-patches
2021-10-27 13:56     ` Igor Munkin via Tarantool-patches
     [not found]       ` <20211104130156.f2botlihlfhwd3yh@surf.localdomain>
2021-11-11 15:34         ` Igor Munkin via Tarantool-patches [this message]
2021-09-29 20:07   ` [Tarantool-patches] [PATCH luajit v4 4/4] memprof: add info about trace start to symtab Mikhail Shishatskiy via Tarantool-patches
2021-11-01 16:31     ` Igor Munkin via Tarantool-patches
     [not found]       ` <20211104130228.x6qcne5xeh544hm7@surf.localdomain>
2021-11-12 13:34         ` Igor Munkin via Tarantool-patches
2021-11-17  8:17           ` Sergey Kaplun via Tarantool-patches
2021-11-22 15:11           ` Mikhail Shishatskiy via Tarantool-patches
2021-11-24 12:42             ` Mikhail Shishatskiy via Tarantool-patches
2021-11-24 16:44             ` Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YY04DZEQX5Xafr4m@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=m.shishatskiy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit v4 3/4] memprof: group allocations on traces by traceno' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox