Re: [Tarantool-patches] [PATCH luajit v2] memprof: report JIT-side allocations as internal

[Mikhail Shishatskiy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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