From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 849816B962; Wed, 14 Apr 2021 16:13:06 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 849816B962 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1618405986; bh=/56fg1apGHiU7Z4m/cWHWRrYVhyN4HhT9StVdJd6OU0=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=mcuVZAl9T4cD+cJ1llo3snqPo1AB382o3S+dKS/yt3FDGuD3whrtbGSttorBZF7/W n5djVQ4XKCRErK5zYcX8r6Y4hBn6w7z5u8U6vYJyO8l09U7qnBVbpDsQgLycTyBZ2S eLJi1qqsnskprvOxx7j7hWT6KwsnzgcO40gcxRNM= Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 786F96BD23 for ; Wed, 14 Apr 2021 16:13:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 786F96BD23 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lWfKK-0000XZ-7a; Wed, 14 Apr 2021 16:13:04 +0300 Date: Wed, 14 Apr 2021 16:12:52 +0300 To: Sergey Kaplun Message-ID: <20210414131252.GZ29703@tarantool.org> References: <20210331172948.10660-1-skaplun@tarantool.org> <20210413074342.GW29703@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E74806859AC5FE18436AEED970E897805ADA4182A05F538085040BBE31578F04FFF769A111D79445986B506A1BC36D53F6B2F052B8E16650727C9 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7D63A32B630C59AACEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637C904E3CF4B5CD3198638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B27798A5BA35D35275958CE699C58E037842232221D1CC1427D2E47CDBA5A96583C09775C1D3CA48CF17B107DEF921CE79117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE77A825AB47F0FC8649FA2833FD35BB23DF004C906525384302BEBFE083D3B9BA73A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E73525A4AB119743A3B3C4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A555AC9A8FFB4A88EE6096B0DF7FFAB16D4E6E7F98CCFE4D94D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3429538671E6527D32A899B5F79A5704FB5EFD86F3AA2D0611B2402ED289171FB6762DAFA126FB6BB01D7E09C32AA3244C0B64418AFCFB5F19FAAE3C3599CEC421E3D93501275E802F927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojrcJA+pXcDuk0UJ4Kp92reA== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822B64C534B4EA61B9A1178C8833C1866C4A7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2 luajit] tools: introduce --leak-only memprof parser option X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Sergey, Thanks for the fixes! Now the changes LGTM, I'll push them to the trunk later. On 14.04.21, Sergey Kaplun wrote: > Igor, > > Thanks for the review! > > On 13.04.21, Igor Munkin wrote: > > Sergey, > > > > Thanks for the patch! Now we even have a tests, neat! > > > > Besides all comments below, I have a general one regarding the > > maintaining the tools directory. Essentially it occurs to be totally > > your domain, and I can't fluently read this code anymore. It definitely > > relates to the fact I look here once in a three months, but it only > > confirm my concerns: I'm afraid the code will be hardly maintainable in > > a year or two. Little comments source-wide in tools directory. I > > literally read this code with a notebook to write down all structures > > layout, implicit arguments, etc. We were in a hurry in the previous > > release. In the current release we were focused on the tests. We have > > lots of work to be made for memprof enhancement in the next release > > prior to announcing it as a stable. Then let's polish it in scope of > > these enhancements. Could you please file a separate issue for > > refactoring the tools code a bit? > > Yes, see [1]. Great, thanks! > > Also I suggest to create a Code style guidelines for LuaJIT (as far as > it is totally different from Tarantool's codestyle) for C and Lua. > Looks like this guidelines is good for self-checking before sending > patch for the review. > Thoughts? You've asked for this a couple of times already. I guess we need to discuss this on our LuaJIT-related regular meeting. > > > > > On 31.03.21, Sergey Kaplun wrote: > > > This patch indtroduces new memprof parser module to > > > post-process memory events. > > > > > > Memprof parser now adds postamble with the source lines of Lua chunks > > > > Never heard such a word: "postamble". I believe you mean epilogue by > > that. You also can simply use "report" here. > > It is modelled on preamble, with post- replacing pre-. > Changed to epilogue. > > > > > > (or "INTERNAL") that allocate and do not free some amount of bytes, when > > > profiler finishes. The parser also reports the number of allocation and > > > deallocation events related to each line. > > > > > > Also, this patch adds a new --leak-only memory profiler parser option. > > > When the parser runs with that option, it reports only leak > > > information. > > > > > > Resolves tarantool/tarantool#5812 > > > --- > > > Changes in v2: > > > * introduce new memprof's module to post-process parsed > > > events > > > * add tests > > > > > > ChangeLog entry (and postamble too Tarantool bump commit message): > > > > Feel free to add this into the patch for Tarantool repo. > > I'm a little bit confused here. How should I proceed the patch for the > Tarantool repo? Usually Kirill or somebody just bumping version of a > submodule with list of commits. Should I create the separate patch for > this? Also, IINM, you asked me not to reference the issues in the commit > message for Tarantool's luajit bump to avoid extra mentioning, because > you anyway need to mention the issue during the bumping. Sorry for confusing you. Frankly speaking, there is no strict rule and I can do it by myself of course. However, if you need a rule of thumb I suggest the following: whether you have changes in Tarantool repo, feel free to add ChangeLog entry alongside with them, otherwise just put it in the cover letter / patch (as you've done now). > > > > > > > > > =================================================================== > > > ##feature/luajit > > > > > > * Now memory profiler parser reports heap difference occurring during > > > the measurement interval. New memory profiler's option `--leak-only` > > > to show only heap difference is introduced. New built-in module > > > `memprof.process` is introduced to perform memory events > > > post-processing and aggregation. Now to launch memory profiler > > > via Tarantool user should use the following command: > > > `tarantool -e 'require("memprof")(arg)' - --leak-only /tmp/memprof.bin` > > > =================================================================== > > > > > > Branch with tests and added the corresponding built-in: > > > * https://github.com/tarantool/tarantool/tree/skaplun/gh-5812-memprof-memleaks-option > > > LuaJIT branch: > > > * https://github.com/tarantool/luajit/tree/skaplun/gh-5812-memprof-memleaks-option > > > Issue: https://github.com/tarantool/tarantool/issues/5812 > > > > > > .../misclib-memprof-lapi.test.lua | 21 +++++-- > > > tools/memprof.lua | 33 ++++++----- > > > tools/memprof/humanize.lua | 43 +++++++++++++- > > > tools/memprof/parse.lua | 20 +++++-- > > > tools/memprof/process.lua | 59 +++++++++++++++++++ > > > 5 files changed, 151 insertions(+), 25 deletions(-) > > > create mode 100644 tools/memprof/process.lua > > > > > > diff --git a/tools/memprof.lua b/tools/memprof.lua > > > index 9f962085..c6c5f587 100644 > > > --- a/tools/memprof.lua > > > +++ b/tools/memprof.lua > > > @@ -94,26 +101,22 @@ local function dump(inputfile) > > > local reader = bufread.new(inputfile) > > > local symbols = symtab.parse(reader) > > > local events = memprof.parse(reader, symbols) > > > - > > > - stdout:write("ALLOCATIONS", "\n") > > > - view.render(events.alloc, symbols) > > > - stdout:write("\n") > > > - > > > - stdout:write("REALLOCATIONS", "\n") > > > - view.render(events.realloc, symbols) > > > - stdout:write("\n") > > > - > > > - stdout:write("DEALLOCATIONS", "\n") > > > - view.render(events.free, symbols) > > > - stdout:write("\n") > > > - > > > + if not leak_only then > > > + view.profile_info(events, symbols) > > > + end > > > + local heap_diff = process.form_heap_diff(events, symbols) > > > + view.leak_only(heap_diff) > > > > I see not a word in issue regarding this change. So you show leaks also > > when nobody asked you. I personally don't like such approach, but I have > > no idea what Mons asked you to do. > > It was discussed with Sergos in the previous version. Also, AFAIR, Mons > really asked to show this information always. Then OK. > > > > diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua > > > index 2d5814c6..6afd3ff1 100644 > > > --- a/tools/memprof/humanize.lua > > > +++ b/tools/memprof/humanize.lua > > > > > > > > > @@ -42,4 +42,43 @@ function M.render(events, symbols) > > > end > > > end > > > > > > +function M.profile_info(events, symbols) > > > + print("ALLOCATIONS") > > > > Why did you silently change to here? > > This module uses `print()` everywhere. uses stderr to > write error messages at start and uses stdout for these messages. > But as you can see `render()` function already uses just `print()`. > So I prefer to use the same way of the output. Oh, I didn't notice. Then fine, thanks for clarifying! > > Side note: what do you mean by "silently"? How should I comment these > changes? There is not a single word in commit message regarding this. I guess it was done unintentionally or with no reason. You've provided a one here, so I'm totally OK with this change now. > > > > diff --git a/tools/memprof/process.lua b/tools/memprof/process.lua > > > new file mode 100644 > > > index 00000000..94be187e > > > --- /dev/null > > > +++ b/tools/memprof/process.lua > > > @@ -0,0 +1,59 @@ > > > + -- Realloc and free events are pretty the same. > > > > And what is the difference in alloc events except they have no > > list linking the memory manipulations? > > We are not interested in location of realloc event, do we? > I suppose that if some reallocation (table for example) is performed, > that user doesn't interesting in leaks report where this object was > reallocated, but where it was declared (and so allocated). Well, this sounds valid. Anyway, we can fix this on demand, if somebody needs more precise profiling. It would be nice to ask about it at first. > > > > > > + -- We aren't interested in aggregated alloc/free sizes for > > > + -- the event, but only for new and old size values inside > > > + -- alloc-realloc-free chain. Assuming that we have > > > + -- no collisions between different object addresses. > > > + local function process_non_alloc_events(events_by_type) > > > > Why do you need to define the function right here? To omit and > > parameters? For what? > > No. It is local function to process reallocation and deallocation > events specific to heap delta aggregation. So, it is local to this > function exactly, not for the whole module. Yes, it's local here and has and as upvalues. But I see no reason for this and you can implement it another way. Anyway, this is not a problem and my comment is nothing more than doebka, so let's move forward then. > > > > -- > > > 2.31.0 > > > > > > > -- > > Best regards, > > IM > > [1]: https://github.com/tarantool/tarantool/issues/5994 > > -- > Best regards, > Sergey Kaplun -- Best regards, IM