From: Mikhail Shishatskiy via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: "Sergey Kaplun" <skaplun@tarantool.org>, "Mikhail Shishatskiy via Tarantool-patches" <tarantool-patches@dev.tarantool.org>, "Igor Munkin" <imun@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH luajit v1] memprof: report all JIT-related allocations as internal Date: Thu, 29 Jul 2021 09:24:47 +0300 [thread overview] Message-ID: <1627539886.806204028@f716.i.mail.ru> (raw) In-Reply-To: <YQARRxl5MQPuRPza@root> [-- Attachment #1: Type: text/plain, Size: 3544 bytes --] 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 <snipped> >> >> +-- 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 [-- Attachment #2: Type: text/html, Size: 5617 bytes --]
prev parent reply other threads:[~2021-07-29 6:24 UTC|newest] Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-22 11:46 Mikhail Shishatskiy via Tarantool-patches 2021-07-27 13:59 ` Sergey Kaplun via Tarantool-patches 2021-07-29 6:24 ` Mikhail Shishatskiy via Tarantool-patches [this message]
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=1627539886.806204028@f716.i.mail.ru \ --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 v1] memprof: report all JIT-related 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