From: Igor Munkin <imun@tarantool.org> To: Sergey Kaplun <skaplun@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit v1 03/11] profile: introduce profiler writing module Date: Mon, 21 Dec 2020 12:24:15 +0300 [thread overview] Message-ID: <20201221092415.GT5396@tarantool.org> (raw) In-Reply-To: <0adaa12ef5be0efc1d9c79ab746b8149c252d661.1608142899.git.skaplun@tarantool.org> 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 <write_buffer_sys> part: it is overcomplicated for the current uses. 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 <lj_wbuf*> is a good prefix for this subsystem. 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. > > 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. > > 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. > @@ -469,6 +469,7 @@ DASM_FLAGS= $(DASM_XFLAGS) $(DASM_AFLAGS) <snipped> > 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 <unistd.h> > +#include <errno.h> > + > +#include "profile/ljp_write.h" > +#include "utils/leb128.h" > +#include "lj_def.h" <snipped> > +/* 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 <write> or <fwrite> right here for now and redesign the wbuf API later if necessary (it's internal, so I see no problem). > +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 <ljp_write_flush_buffer> (hell these names). > +{ > + 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! > + > + 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). <snipped> > +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). <snipped> > +void ljp_write_init(struct ljp_buffer *buf, ljp_writer writer, void *ctx, > + uint8_t *mem, size_t size) > +{ > + buf->ctx = ctx; > + buf->writer = writer; > + buf->buf = mem; > + buf->pos = mem; > + buf->size = size; > + buf->flags = 0; > + buf->saved_errno = 0; > +} > + > +void ljp_write_terminate(struct ljp_buffer *buf) > +{ > + ljp_write_init(buf, NULL, NULL, NULL, 0); > +} <snipped> > +/* 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. > + 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. > + write_buffer(buf, s, l); > +} > + <snipped> > 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 <stdint.h> > + > +/* > +** Data format for strings: > +** > +** string := string-len string-payload > +** string-len := <ULEB128> > +** string-payload := <BYTE> {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/. > + 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 <ljp_write_*>, but the first parameter (i.e. "self") is ljp_buffer. As a result such names as <ljp_write_errno> looks confusing, since errno is actually written nowhere. I suggest to name everything with <lj_wbuf_*> prefix, so the names <lj_wbuf_errno> and <lj_wbuf_test_flag> fits the resulting value. Furthermore, the routines appending the data to the buffer can be renamed the following way: ljp_write_<type> -> lj_wbuf_add<type> (consider Lua standart buffer API). Thoughts? > + > +/* 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
next prev parent reply other threads:[~2020-12-21 9:24 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-16 19:13 [Tarantool-patches] [PATCH luajit v1 00/11] LuaJIT memory profiler Sergey Kaplun 2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 01/11] build: add src dir in building Sergey Kaplun 2020-12-20 21:27 ` Igor Munkin 2020-12-23 18:20 ` Sergey Kaplun 2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 02/11] utils: introduce leb128 reader and writer Sergey Kaplun 2020-12-20 22:44 ` Igor Munkin 2020-12-23 22:34 ` Sergey Kaplun 2020-12-24 9:11 ` Igor Munkin 2020-12-25 8:46 ` Sergey Kaplun 2020-12-23 16:50 ` Sergey Ostanevich 2020-12-23 22:36 ` Sergey Kaplun 2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 03/11] profile: introduce profiler writing module Sergey Kaplun 2020-12-21 9:24 ` Igor Munkin [this message] 2020-12-24 6:46 ` Sergey Kaplun 2020-12-24 15:45 ` Sergey Ostanevich 2020-12-24 21:20 ` Sergey Kaplun 2020-12-25 9:37 ` Igor Munkin 2020-12-25 10:13 ` Sergey Kaplun 2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 04/11] profile: introduce symtab write module Sergey Kaplun 2020-12-21 10:30 ` Igor Munkin 2020-12-24 7:00 ` Sergey Kaplun 2020-12-24 9:36 ` Igor Munkin 2020-12-25 8:45 ` Sergey Kaplun 2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 05/11] vm: introduce LFUNC and FFUNC vmstates Sergey Kaplun 2020-12-25 11:07 ` Sergey Ostanevich 2020-12-25 11:23 ` Sergey Kaplun 2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 06/11] core: introduce new mem_L field Sergey Kaplun 2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 07/11] debug: move debug_frameline to public module API Sergey Kaplun 2020-12-20 22:46 ` Igor Munkin 2020-12-24 6:50 ` Sergey Kaplun 2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 08/11] profile: introduce memory profiler Sergey Kaplun 2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 09/11] misc: add Lua API for " Sergey Kaplun 2020-12-24 16:32 ` Sergey Ostanevich 2020-12-24 21:25 ` Sergey Kaplun 2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 10/11] tools: introduce tools directory Sergey Kaplun 2020-12-20 22:46 ` Igor Munkin 2020-12-24 6:47 ` Sergey Kaplun 2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 11/11] profile: introduce profile parser Sergey Kaplun 2020-12-24 23:09 ` Igor Munkin 2020-12-25 8:41 ` Sergey Kaplun 2020-12-21 10:43 ` [Tarantool-patches] [PATCH luajit v1 00/11] LuaJIT memory profiler Igor Munkin 2020-12-24 7:02 ` Sergey Kaplun
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=20201221092415.GT5396@tarantool.org \ --to=imun@tarantool.org \ --cc=skaplun@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit v1 03/11] profile: introduce profiler writing module' \ /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