Re: [Tarantool-discussions] [RFC] rfc: describe a LuaJIT memory profiler

[Igor Munkin <imun@tarantool.org>]

Sergey,

Thanks for the clarification! I read the doc once more and answered the
remaining questions below. I guess we have resolved the major points so
I wait for the second version of the RFC.

On 16.11.20, Sergey Kaplun wrote:
> Igor, thanks for your comments and for the review!
> 

<snipped>

> > > 
> > > I have several questions to ask:
> > > Should we extend profiler options with these two field:
> > >  * `buffer_size` -- to determine size of buffer for profiling data
> > >    instead of spikes it.
> > 
> > As Sergos already proposed offline, it's better to provide buffer by
> > host application (i.e. Tarantool) and use a default internal one if no
> > buffer is passed. I suggest to try this at first.
> 
> I suggest two possible solutions:
> 
> 1) Change writer interface -- it should report new buffer start and its
> length after the call. Return value still represents how much was
> written.
> 
> | typedef size_t (*ljp_writer)(const char **data, size_t *len, void *opt);
> instead of
> | typedef size_t (*ljp_writer)(const void *data, size_t len, void *opt);
> 
> 2) Add additional interface to allocate a new buffer each time.
> | typedef void *(*ljp_alloc)(size_t len, void *opt);
> It will invokes after each corresponding write.
> 
> I suppose we should provide benchmarks for current solution first.
> Thoughts?

I personally prefer the first one, but it's hard to say with no code
aside, so I'll glance your PoC on the branch a bit later.

> 
> > 
> > >  * `sample_size` -- to control frequency of dump of profile events
> > >    like Go memory profiler does.
> > 
> > Does this option control how often the collected data is flushed from
> > the buffer? Otherwise I have no idea what this option does.
> 
> This option control frequency of collectting data. In other words, it
> means that profiler reports not all events, but events that happen after
> each `sample_size` bytes allocation/deallocation. Also, each new
> function object allocation event should be reported. It is necessary to
> be able to restore allocation event inside that function by parsing
> utility. This means not each allocation is counted, but only
> allocations every N kB. This makes sampling very cheap and suitable for
> production.  On the other hand, the collected data may be incomplete.
> 
> This will affect OVERRIDES section obviously -- it becomes useless and
> can be dropped.

I believe this is totally out of PoC scope.

> 

<snipped>

> > > +
> > > +Extended functions to control profiler are added to <lmisclib.h>.
> > > +Profiler is configured by this options structure:
> > > +
> > > +```c
> > > +/* Profiler options. */
> > > +struct luam_Prof_options {
> > > +  /* Options for the profile writer and final callback. */
> > > +  void *arg;
> > > +  /*
> > > +  ** Writer function for profile events.
> > > +  ** Should return amount of written bytes on success or zero in case of error.
> > > +  */
> > > +  size_t (*writer)(const void *data, size_t len, void *arg);
> > > +  /*
> > > +  ** Callback on profiler stopping. Required for correctly cleaning
> > > +  ** at vm shoutdown when profiler still running.
> > > +  ** Returns zero on success.
> > > +  */
> > > +  int (*on_stop)(void *arg);
> > > +};
> > > +```
> > 
> > Well, maybe it's better to introduce a special interface to fill this
> > struct? Something similar to luaE_coveragestart_cb[1]. As a result the
> > structure is encapsulated in LuaJIT, that looks more convenient for the
> > further maintenance.
> 
> Yes, but on the other side for each profiler we should create N
> additional interfaces how to start it.

As well as N additional structs for profiler options. So what?

> 

<snipped>

> > > +Where `fname` is name of the file where profile events are written. This
> > > +function is just a wrapper to `luaM_memprof_start()`. Writer for this function
> > > +perform `fwrite()` for each call. Final callback calls `fclose()` at the end of
> > > +profiling. If it is impossible to open a file for writing or
> > > +`luaM_memprof_start()` returns error status the function returns `false` value.
> > 
> > What about the error itself? It would be nice to see the exact reason
> > why memprof is failed to start.
> 
> We can additionally push `perror()` value here. Thoughts?

OK.

> 

<snipped>

> > 
> > What does this "Overrides" attribute mean?
> 
> What allocation this reallocation overrides.

Well, I guess I get it, but doubt that "overrides" fits this definition.

> 

<snipped>

> > > +#### Dump of Lua universe

Let's sort the needy from the greedy right here. While reading the RFC
once more, I bethought to move this part to a separate document to not
spoil this one. Thoughts?

> > > +

<snipped>

> > 
> > [1]: https://github.com/luavela/luavela/blob/master/src/lextlib.h#L173L175
> > 

<snipped>

> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM