Re: [Tarantool-patches] [PATCH luajit v1 03/11] profile: introduce profiler writing module

[Igor Munkin <imun@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