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 <skaplun@tarantool.org>:

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