Tarantool development patches archive
 help / color / mirror / Atom feed
From: Mikhail Shishatskiy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>,
	tarantool-patches@dev.tarantool.org, imun@tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit v2] memprof: report JIT-side allocations as internal
Date: Sat, 21 Aug 2021 11:43:53 +0700	[thread overview]
Message-ID: <20210821044353.kbbgcgiapat2q4ve@surf.localdomain> (raw)
In-Reply-To: <YRen3c5bEXISmsmU@root>

Hi, Sergey!
Thanks for the review!

On 14.08.2021 14:24, Sergey Kaplun wrote:
>Hi, thanks for the fixes!
>
>Please, move test adjustment mentioned below to the patch for the symtab
>with group allocations by trace's number.
>Otherwise, LGTM.
>
>On 29.07.21, Mikhail Shishatskiy wrote:
>> 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.
>>
>> 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
>> CI: https://github.com/tarantool/tarantool/tree/shishqa/gh-5679-report-jit-allocations-as-internal
>>
>> Changes in v2:
>> - Fixed commit title to fit in width of 50 symbols;
>> - Rebased to the branch [1];
>> - Reused default_payload() as a payload to test patch behavior.
>>
>> [1]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number
>>
>> src/lj_memprof.c | 28 ++++++++++++-------
>> .../misclib-memprof-lapi.test.lua | 16 ++++++++---
>> 2 files changed, 30 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/lj_memprof.c b/src/lj_memprof.c
>> index 87512c3a..e4d819a0 100644
>> --- a/src/lj_memprof.c
>> +++ b/src/lj_memprof.c
>
><snipped>
>
>> diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>> index b7e456e1..96864c6a 100644
>> --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
>> +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>> @@ -12,7 +12,7 @@ utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
>> local tap = require("tap")
>>
>> local test = tap.test("misc-memprof-lapi")
>> -test:plan(16)
>> +test:plan(17)
>>
>> jit.off()
>> jit.flush()
>> @@ -81,7 +81,7 @@ local function fill_ev_type(events, symbols, event_type)
>> for _, event in pairs(events[event_type]) do
>> local addr = event.loc.addr
>> local traceno = event.loc.traceno
>> - if traceno ~= 0 then
>> + if traceno ~= 0 and symbols.trace[traceno] then
>
>This should be moved to the commit, that changes symtab, this change
>looks unrelated to this patch, but relates to previous one [1].

Fixed in the patchset [1] and rebased to the branch [2]

>
>> local trace_loc = symbols.trace[traceno]
>> addr = trace_loc.addr
>> ev_type[trace_loc.line] = {
>> @@ -214,9 +214,17 @@ misc.memprof.stop()
>
><snipped>
>
>> --
>> 2.32.0
>>
>
>[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-July/024944.html
>
>--
>Best regards,
>Sergey Kaplun

Also, I slightly changed the test by deleting the check for deallocations
number because now trace generation happens for memprof payload, too, and
we can meet untrivial JIT-related deallocations in the second run.

---
diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
index f84b6df0..17bcb2f1 100644
--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
@@ -231,19 +231,25 @@ test:test("jit-output", function(subtest)
     jit.opt.start(3, "hotloop=10")
     jit.flush()
   
- -- Pregenerate traces to fill symtab entries in the next run.
- default_payload()
-
+ -- On this run traces are generated, JIT-related allocations
+ -- will be recorded as well.
     local symbols, events = generate_parsed_output(default_payload)
   
     local alloc = fill_ev_type(events, symbols, "alloc")
- local free = fill_ev_type(events, symbols, "free")
+
+ -- Test for marking JIT-related allocations as internal.
+ -- See also https://github.com/tarantool/tarantool/issues/5679.
+ subtest:ok(alloc[0] == nil)
+
+ -- Run already generated traces.
+ symbols, events = generate_parsed_output(default_payload)
+
+ alloc = fill_ev_type(events, symbols, "alloc")
   
     -- We expect, that loop will be compiled into a trace.
     subtest:ok(check_alloc_report(alloc, 1, 37, 32, 20))
     -- See same checks with jit.off().
     subtest:ok(check_alloc_report(alloc, 0, 34, 32, 2))
- subtest:ok(free.INTERNAL.num == 22)
   
     -- Restore default JIT settings.
     jit.opt.start(unpack(jit_opt_default))
--
2.32.0

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-August/025680.html
[2]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number

Issue: https://github.com/tarantool/tarantool/issues/5679
Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5679-report-jit-allocations-as-internal
Tarantool branch: https://github.com/tarantool/tarantool/tree/shishqa/gh-5679-report-jit-allocations-as-internal

Best regards,
Mikhail Shishatskiy

  reply	other threads:[~2021-08-21  4:43 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-29  9:34 Mikhail Shishatskiy via Tarantool-patches
2021-08-14 11:24 ` Sergey Kaplun via Tarantool-patches
2021-08-21  4:43   ` Mikhail Shishatskiy via Tarantool-patches [this message]
2021-11-24 13:01     ` Mikhail Shishatskiy 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=20210821044353.kbbgcgiapat2q4ve@surf.localdomain \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=m.shishatskiy@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit v2] memprof: report JIT-side allocations as internal' \
    /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