

<HTML><BODY><div>Hi, Sergey!<br>Thank you for the review! I will fix the nitpicks in the next version</div><div>of the patch.</div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Вторник, 27 июля 2021, 17:00 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16273944470547128863_BODY">Hi, Mikhail!<br><br>Thanks for the patch!<br><br>On 22.07.21, Mikhail Shishatskiy wrote:<br><br>| memprof: report all JIT-related allocations as internal<br><br>Width limit for commit title is 50 symbols including subsystem name<br>considering our code style guide [1].<br><br>> There are cases when the memory profiler attempts to attribute<br>> allocations triggered by the JIT engine recording phase<br>> with a Lua function to be recorded. In this case,<br>> lj_debug_frameline() may return BC_NOPOS (i.e. a negative value).<br>><br>> Previously, these situations were ignored and the profiler<br>> reported, that the source line was equal to zero.<br>><br>> This patch adjusts profiler behavior to treat allocations<br>> described above as internal by dumping ASOURCE_INT if<br>> lj_debug_frameline() returns a negative value.<br><br>Maybe it is better to catch out the JIT-related allocation and associate<br>it with the corresponding trace after these issues [2][3] will be<br>resolved, isn't it?<br>Otherwise after fixing other nitpicks patch LGTM.<br><br>Thoughts?</div></div></div></div></blockquote><div> </div><div>I’ve tested the behavior of such allocations: all of them are actually connected to</div><div>the growth of general-purpose vectors in the JIT state. It is a controversial question,</div><div>should we report them as trace-related or as internal. IMO, internal is better, as</div><div>we do not confuse users by mixing the number of internally allocated bytes with bytes</div><div>allocated from the actual payload.</div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>><br>> Resolves tarantool/tarantool#5679<br>> ---<br>><br>> Issue: <a href="https://github.com/tarantool/tarantool/issues/5679" target="_blank">https://github.com/tarantool/tarantool/issues/5679</a><br>> Branch: <a href="https://github.com/tarantool/luajit/tree/shishqa/gh-5679-report-jit-allocations-as-internal" target="_blank">https://github.com/tarantool/luajit/tree/shishqa/gh-5679-report-jit-allocations-as-internal</a><br><br>Sorry, I can't find Tarantool's standalone branch with these changes to<br>check CI. May you please share it here?</div></div></div></div></blockquote><div> </div><div>Strange, I can not find it too, but remember that I have pushed it. I think</div><div>I can rebase to the branch [1], as the current patch uses the same changes</div><div>in the memprof tests. And when [1] will be approved, I will resend the new </div><div>version with a shorter commit message title and some changes in tests (see below).</div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>><br>> src/lj_memprof.c | 28 +++++++----<br>> .../misclib-memprof-lapi.test.lua | 50 ++++++++++++-------<br>> 2 files changed, 51 insertions(+), 27 deletions(-)<br>><br>> diff --git a/src/lj_memprof.c b/src/lj_memprof.c<br>> index 2c1ef3b8..b4985b8e 100644<br>> --- a/src/lj_memprof.c<br>> +++ b/src/lj_memprof.c</div></div></div></div></blockquote><div><snipped></div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>><br>> +-- Test for marking jit-related allocations as internal.<br>> +-- See also <a href="https://github.com/tarantool/tarantool/issues/5679" target="_blank">https://github.com/tarantool/tarantool/issues/5679</a>.<br>> jit.on()<br>> +symbols, events = generate_parsed_output(TMP_BINFILE, function()<br><br>Why do you use this instead of default_payload?<br>Please drop a comment here.</div></div></div></div></blockquote><div> </div><div>I’ve checked again and realized, that I can use default_payload to test this behavior :)</div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>> + for _ = 1, 100 do<br>> + local _ = {_, _}<br>> + end<br>> +end)<br>> +alloc = fill_ev_type(events, symbols, "alloc")<br>> +test:ok(alloc[0] == nil)<br>> +<br>> os.exit(test:check() and 0 or 1)<br>> --<br>> 2.32.0<br>><br><br>[1]: <a href="https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message" target="_blank">https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message</a><br>[2]: <a href="https://github.com/tarantool/tarantool/issues/5814" target="_blank">https://github.com/tarantool/tarantool/issues/5814</a><br>[3]: <a href="https://github.com/tarantool/tarantool/issues/5815" target="_blank">https://github.com/tarantool/tarantool/issues/5815</a><br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div><div>[1]: https://github.com/tarantool/luajit/tree/shishqa/gh-5814-group-allocations-on-trace-by-trace-number</div><div>--</div><div>Best regards,</div><div>Mikhail Shishatskiy</div></BODY></HTML>