Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 luajit] tools: introduce --leak-only memprof parser option
Date: Wed, 14 Apr 2021 16:12:52 +0300	[thread overview]
Message-ID: <20210414131252.GZ29703@tarantool.org> (raw)
In-Reply-To: <YHbSnclh8HHdAfBt@root>

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 <process.lua> 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 <process.lua> 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
> > > 

<snipped>

> > > diff --git a/tools/memprof.lua b/tools/memprof.lua
> > > index 9f962085..c6c5f587 100644
> > > --- a/tools/memprof.lua
> > > +++ b/tools/memprof.lua

<snipped>

> > > @@ -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.

> 

<snipped>

> > > 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
> > 
> > <snipped>
> > 
> > > @@ -42,4 +42,43 @@ function M.render(events, symbols)
> > >    end
> > >  end
> > >  
> > > +function M.profile_info(events, symbols)
> > > +  print("ALLOCATIONS")
> > 
> > Why did you silently change <stdout:write> to <print> here?
> 
> This module uses `print()` everywhere. <memprof.lua> 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.

> 

<snipped>

> > > 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 @@

<snipped>

> > > +  -- Realloc and free events are pretty the same.
> > 
> > And what is the difference in alloc events except they have no <primary>
> > 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 <heap> and
> > <symbols> 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 <dheap> and <symbols> 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.

> 

<snipped>

> > > -- 
> > > 2.31.0
> > > 
> > 
> > -- 
> > Best regards,
> > IM
> 
> [1]: https://github.com/tarantool/tarantool/issues/5994
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

  reply	other threads:[~2021-04-14 13:13 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-31 17:29 Sergey Kaplun via Tarantool-patches
2021-04-08 12:49 ` Sergey Ostanevich via Tarantool-patches
2021-04-14 11:32   ` Sergey Kaplun via Tarantool-patches
2021-04-13  7:43 ` Igor Munkin via Tarantool-patches
2021-04-14 11:31   ` Sergey Kaplun via Tarantool-patches
2021-04-14 13:12     ` Igor Munkin via Tarantool-patches [this message]
2021-04-14 14:34 ` Igor Munkin via Tarantool-patches

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=20210414131252.GZ29703@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 luajit] tools: introduce --leak-only memprof parser option' \
    /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