From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 A49E44765E0 for ; Thu, 24 Dec 2020 09:47:41 +0300 (MSK) Date: Thu, 24 Dec 2020 09:46:56 +0300 From: Sergey Kaplun Message-ID: <20201224064656.GH9101@root> References: <0adaa12ef5be0efc1d9c79ab746b8149c252d661.1608142899.git.skaplun@tarantool.org> <20201221092415.GT5396@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201221092415.GT5396@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH luajit v1 03/11] profile: introduce profiler writing module List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Hi, Igor! Thanks for the review! On 21.12.20, Igor Munkin wrote: > Sergey, > > Thanks for the patch! Please consider the comments below. I expected > that we agreed we need an MVP for now, so the module API is fine except > the part: it is overcomplicated for the current uses. We can try this API for now. This may lead to thoughts about improving C API. > > On 16.12.20, Sergey Kaplun wrote: > > This patch introduces module for writing profile data. > > Its usage will be added at the next patches. > > > > It can be used for memory profiler or for signal-based > > cpu profiler. > > I see nothing strongly related to the profiler, so it's simply an > internal write buffer. Then let's settle for the naming at first. I > guess is a good prefix for this subsystem. OK, it looks better. > > So, I propose the following wording for the commit message: > | core: introduce write buffer module > | > | This patch introduces the standalone module for writing data to the > | file via the special buffer. The module provides the API for buffer > | initial setup and its convenient usage. > > Feel free to adjust the description on you own and even describe the > introduced API in a more verbose way. Thanks, sounds well! > > > > > Part of tarantool/tarantool#5442 > > --- > > > > Custom memcpy function (see below) makes sense if this module will be > > used for cpu/sample profiler based on a signal-based timer. Else it can > > be easily redefined. > > I suggest to not overcomplicate all this machinery added for an a single > profiler. Let's drop all memcpy-related hacks. By the way we can simply > incorporate it when it is necessary. Sure. > > > > > src/Makefile | 5 +- > > src/Makefile.dep | 2 + > > src/profile/ljp_write.c | 195 ++++++++++++++++++++++++++++++++++++++++ > > src/profile/ljp_write.h | 84 +++++++++++++++++ > > 4 files changed, 284 insertions(+), 2 deletions(-) > > create mode 100644 src/profile/ljp_write.c > > create mode 100644 src/profile/ljp_write.h > > > > diff --git a/src/Makefile b/src/Makefile > > index be7ed95..4b1d937 100644 > > --- a/src/Makefile > > +++ b/src/Makefile > > Please, adjust these changes considering the comments to the first > patch. You can find the proposed naming above. Done. > > > @@ -469,6 +469,7 @@ DASM_FLAGS= $(DASM_XFLAGS) $(DASM_AFLAGS) > > > > > diff --git a/src/profile/ljp_write.c b/src/profile/ljp_write.c > > new file mode 100644 > > index 0000000..de7202d > > --- /dev/null > > +++ b/src/profile/ljp_write.c > > @@ -0,0 +1,195 @@ > > +/* > > +** Low-level writer for LuaJIT Profiler. > > +** > > +** Major portions taken verbatim or adapted from the LuaVela. > > +** Copyright (C) 2015-2019 IPONWEB Ltd. > > +*/ > > + > > +#include > > +#include > > + > > +#include "profile/ljp_write.h" > > +#include "utils/leb128.h" > > +#include "lj_def.h" > > > > > +/* Wraps a write syscall ensuring all data have been written. */ > > I see no syscall wrapped here. Anyway, IIRC we discussed that we don't > need such complex interfaces in scope of MVP. So you can just use > or right here for now and redesign the wbuf API later > if necessary (it's internal, so I see no problem). It *may* be wrapped here (encapsulated in writer function). As I say above, it can lead us to some thoughts about C API. It is internal interface and we can change it whenever we want. Thoughts? > > > +static void write_buffer_sys(struct ljp_buffer *buffer, const void **data, > > + size_t len) > > Why do you need a separate function for this? I guess this should be > moved right to the (hell these names). I suppose it not necessary. I'll merge it with flush. > > > +{ > > + void *ctx = buffer->ctx; > > + size_t written; > > + > > + lua_assert(!ljp_write_test_flag(buffer, STREAM_STOP)); > > + > > + written = buffer->writer(data, len, ctx); > > Well, I believe you can use buffer->ctx instead of the additional > variable here. Trust me, you can! Yep :) > > > + > > + if (LJ_UNLIKELY(written < len)) { > > + write_set_flag(buffer, STREAM_ERR_IO); > > + write_save_errno(buffer); > > + } > > + if (LJ_UNLIKELY(*data == NULL)) { > > + write_set_flag(buffer, STREAM_STOP); > > + write_save_errno(buffer); > > + } > > +} > > + > > +static LJ_AINLINE size_t write_bytes_buffered(const struct ljp_buffer *buf) > > I propose s/write_bytes_buffered/lj_wbuf_len/ (consider sbuflen macro). Fixed. > > > > > +static LJ_AINLINE int write_buffer_has(const struct ljp_buffer *buf, size_t n) > > I propose s/write_buffer_has/lj_wbuf_left/ (consider sbufleft macro). Fixed. > > > +/* Writes n bytes from an arbitrary buffer src to the output. */ > > +static void write_buffer(struct ljp_buffer *buf, const void *src, size_t n) > > +{ > > + if (LJ_UNLIKELY(ljp_write_test_flag(buf, STREAM_STOP))) > > + return; > > + /* > > + ** Very unlikely: We are told to write a large buffer at once. > > + ** Buffer not belong to us so we must to pump data > > + ** through buffer. > > + */ > > + while (LJ_UNLIKELY(n > buf->size)) { > > + ljp_write_flush_buffer(buf); > > Why do you need to flush the buffer on start? I guess you can fill the > buffer till it becomes full and only then flush. Thanks! Good point! > > > + write_memcpy(buf->pos, src, buf->size); > > + buf->pos += (ptrdiff_t)buf->size; > > + n -= buf->size; > > + } > > + > > + write_reserve(buf, n); > > + write_memcpy(buf->pos, src, n); > > + buf->pos += (ptrdiff_t)n; > > +} > > + > > +/* Writes a \0-terminated C string to the output buffer. */ > > +void ljp_write_string(struct ljp_buffer *buf, const char *s) > > +{ > > + const size_t l = strlen(s); > > + > > + ljp_write_u64(buf, (uint64_t)l); > > This is unclear that the check that profiling is still active is made in > scope of the callee. I'll add the comment here. > > > + write_buffer(buf, s, l); > > +} > > + > > > > > diff --git a/src/profile/ljp_write.h b/src/profile/ljp_write.h > > new file mode 100644 > > index 0000000..29c1669 > > --- /dev/null > > +++ b/src/profile/ljp_write.h > > @@ -0,0 +1,84 @@ > > +/* > > +** Low-level event streaming for LuaJIT Profiler. > > +** NB! Please note that all events may be streamed inside a signal handler. > > +** This means effectively that only async-signal-safe library functions and > > +** syscalls MUST be used for streaming. Check with `man 7 signal` when in > > +** doubt. > > +** Major portions taken verbatim or adapted from the LuaVela. > > +** Copyright (C) 2015-2019 IPONWEB Ltd. > > +*/ > > + > > +#ifndef _LJP_WRITE_H > > +#define _LJP_WRITE_H > > + > > +#include > > + > > +/* > > +** Data format for strings: > > +** > > +** string := string-len string-payload > > +** string-len := > > +** string-payload := {string-len} > > +** > > +** Note. > > +** For strings shorter than 128 bytes (most likely scenario in our case) > > +** we write the same amount of data (1-byte ULEB128 + actual payload) as we > > +** would have written with straightforward serialization (actual payload + \0), > > +** but make parsing easier. > > +*/ > > + > > +/* Stream errors. */ > > +#define STREAM_ERR_IO 0x1 > > +#define STREAM_STOP 0x2 > > + > > +typedef size_t (*ljp_writer)(const void **data, size_t len, void *opt); > > + > > +/* Write buffer for profilers. */ > > +struct ljp_buffer { > > + /* > > + ** Buffer writer which will called at buffer write. > > + ** Should return amount of written bytes on success or zero in case of error. > > + ** *data should contain new buffer of size greater or equal to len. > > + ** If *data == NULL stream stops. > > + */ > > + ljp_writer writer; > > + /* Context to writer function. */ > > Typo: s/Context to/Context for/. Thanks. > > > + void *ctx; > > + /* Buffer size. */ > > + size_t size; > > + /* Saved errno in case of error. */ > > + int saved_errno; > > + /* Start of buffer. */ > > + uint8_t *buf; > > + /* Current position in buffer. */ > > + uint8_t *pos; > > + /* Internal flags. */ > > + volatile uint8_t flags; > > +}; > > Well, I don't get why the functions are called , but the > first parameter (i.e. "self") is ljp_buffer. As a result such names as > looks confusing, since errno is actually written > nowhere. I suggest to name everything with prefix, so the > names and fits the resulting value. > Furthermore, the routines appending the data to the buffer can be > renamed the following way: ljp_write_ -> lj_wbuf_add > (consider Lua standart buffer API). Thoughts? Yes, it's more consistent with LuaJIT internals too. > > > + > > +/* Write string. */ > > +void ljp_write_string(struct ljp_buffer *buf, const char *s); > > + > > +/* Write single byte. */ > > +void ljp_write_byte(struct ljp_buffer *buf, uint8_t b); > > + > > +/* Write uint64_t in uleb128 format. */ > > +void ljp_write_u64(struct ljp_buffer *buf, uint64_t n); > > + > > +/* Immediatly flush buffer. */ > > +void ljp_write_flush_buffer(struct ljp_buffer *buf); > > + > > +/* Init buffer. */ > > +void ljp_write_init(struct ljp_buffer *buf, ljp_writer writer, void *ctx, > > + uint8_t *mem, size_t size); > > + > > +/* Check flags. */ > > +int ljp_write_test_flag(const struct ljp_buffer *buf, uint8_t flag); > > + > > +/* Return saved errno. */ > > +int ljp_write_errno(const struct ljp_buffer *buf); > > + > > +/* Set pointers to NULL and reset flags. */ > > +void ljp_write_terminate(struct ljp_buffer *buf); > > + > > +#endif > > -- > > 2.28.0 > > > > -- > Best regards, > IM -- Best regards, Sergey Kaplun