From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 15CE845C304 for ; Fri, 11 Dec 2020 11:51:52 +0300 (MSK) Date: Fri, 11 Dec 2020 11:51:48 +0300 From: Igor Munkin Message-ID: <20201211085148.GM5396@tarantool.org> References: <20201027112913.7927-1-skaplun@tarantool.org> <20201103124024.GG5396@tarantool.org> <20201116055244.GA19025@root> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20201116055244.GA19025@root> Subject: Re: [Tarantool-discussions] [RFC] rfc: describe a LuaJIT memory profiler List-Id: Tarantool development process List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Kaplun Cc: tarantool-discussions@dev.tarantool.org, Alexander Turenko 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! > > > > > > > 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. > > > > + > > > +Extended functions to control profiler are added to . > > > +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? > > > > +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. > > > > > What does this "Overrides" attribute mean? > > What allocation this reallocation overrides. Well, I guess I get it, but doubt that "overrides" fits this definition. > > > > +#### 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? > > > + > > > > [1]: https://github.com/luavela/luavela/blob/master/src/lextlib.h#L173L175 > > > -- > Best regards, > Sergey Kaplun -- Best regards, IM