Hi, Sergey! Thank you for the review! I will fix the nitpicks in the next version of the patch.   >Вторник, 27 июля 2021, 17:00 +03:00 от Sergey Kaplun : >  >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?   I’ve tested the behavior of such allocations: all of them are actually connected to the growth of general-purpose vectors in the JIT state. It is a controversial question, should we report them as trace-related or as internal. IMO, internal is better, as we do not confuse users by mixing the number of internally allocated bytes with bytes allocated from the actual payload.   >> >> 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?   Strange, I can not find it too, but remember that I have pushed it. I think I can rebase to the branch [1], as the current patch uses the same changes in the memprof tests. And when [1] will be approved, I will resend the new  version with a shorter commit message title and some changes in tests (see below).   >> >> 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 >> >> +-- 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.   I’ve checked again and realized, that I can use default_payload to test this behavior :)   >> + 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   [1]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number -- Best regards, Mikhail Shishatskiy