Re: [Tarantool-patches] [PATCH luajit v2 2/7] core: introduce write buffer module

[Igor Munkin <imun@tarantool.org>]

Sergey,

Thanks for the patch! LGTM, except the nits below.

On 25.12.20, Sergey Kaplun wrote:
> This patch introduces the standalone module for writing data to the
> file, socket or memory (and so on) via the special buffer.
> The module provides the API for buffer initial setup
> and its convenient usage.
> 
> Part of tarantool/tarantool#5442
> ---
> 
> Changes in v2:
>   - Removed custom memcpy.
>   - lj_wbuf_addn() fills buffer to the end first and then flushes.
>   - Changed assert in lj_wbuf_flush() to early return.
> 
>  src/Makefile     |   2 +-
>  src/Makefile.dep |  23 ++++----
>  src/lj_wbuf.c    | 141 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/lj_wbuf.h    |  87 +++++++++++++++++++++++++++++
>  src/ljamalg.c    |   1 +
>  5 files changed, 242 insertions(+), 12 deletions(-)
>  create mode 100644 src/lj_wbuf.c
>  create mode 100644 src/lj_wbuf.h
> 

<snipped>

> diff --git a/src/Makefile.dep b/src/Makefile.dep
> index 75409bf..59ed450 100644
> --- a/src/Makefile.dep
> +++ b/src/Makefile.dep
> @@ -211,28 +211,29 @@ lj_vmevent.o: lj_vmevent.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h \
>   lj_vm.h lj_vmevent.h
>  lj_vmmath.o: lj_vmmath.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h \
>   lj_ir.h lj_vm.h
> +lj_wbuf.o: lj_wbuf.c lj_wbuf.h lj_def.h lua.h luaconf.h lj_utils.h
>  ljamalg.o: ljamalg.c lua.h luaconf.h lauxlib.h lj_gc.c lj_obj.h lj_def.h \

<snipped>

> - lj_utils_leb128.c lj_utils.h lib_aux.c lib_base.c lj_libdef.h lib_math.c \
> - lib_string.c lib_table.c lib_io.c lib_os.c lib_package.c lib_debug.c \
> - lib_bit.c lib_jit.c lib_ffi.c lib_misc.c lib_init.c
> + lj_utils_leb128.c lib_aux.c lib_base.c lj_libdef.h lib_math.c lib_string.c \
> + lib_table.c lib_io.c lib_os.c lib_package.c lib_debug.c lib_bit.c lib_jit.c \
> + lib_ffi.c lib_misc.c lib_init.c

Minor: Why this part is changed? If this is done on purpose, please move
this change to the previous patch. Otherwise, <make depends> looks buggy
and I propose to make these changes manually. Feel free to ignore.

>  luajit.o: luajit.c lua.h luaconf.h lauxlib.h lualib.h luajit.h lj_arch.h

<snipped>

> diff --git a/src/lj_wbuf.c b/src/lj_wbuf.c
> new file mode 100644
> index 0000000..8f090eb
> --- /dev/null
> +++ b/src/lj_wbuf.c
> @@ -0,0 +1,141 @@

<snipped>

> +  /*
> +  ** Very unlikely: We are told to write a large buffer at once.
> +  ** Buffer not belong to us so we must to pump data

Typo: s/Buffer not belong/Buffer doesn't belong/.

> +  ** through buffer.

Typo: s/through buffer/through the buffer/.

> +  */

<snipped>

> diff --git a/src/lj_wbuf.h b/src/lj_wbuf.h
> new file mode 100644
> index 0000000..58a109e
> --- /dev/null
> +++ b/src/lj_wbuf.h
> @@ -0,0 +1,87 @@

<snipped>

> +typedef size_t (*lj_wbuf_writer)(const void **data, size_t len, void *opt);
> +
> +/* Write buffer. */
> +struct lj_wbuf {
> +  /*
> +  ** Buffer writer which will called at buffer write.

Typo: s/will called at buffer write/is called on the buffer flush/.

> +  ** 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 you consider the <len> as a size of the new buffer, then you need to
pass its pointer but not its value to the writer. Otherwise this
requirement has no sense IMHO.

> +  ** If *data == NULL stream stops.

Minor: It would be nice to describe the contract of this function near
its typedef above.

> +  */
> +  lj_wbuf_writer writer;
> +  /* Context for writer function. */
> +  void *ctx;
> +  /* Buffer size. */
> +  size_t size;
> +  /* Saved errno in case of error. */
> +  int saved_errno;

There is an empty padding to fit the alignment here. It's better to move
this field right prior to the <flags>.

> +  /* Start of buffer. */
> +  uint8_t *buf;
> +  /* Current position in buffer. */
> +  uint8_t *pos;
> +  /* Internal flags. */
> +  volatile uint8_t flags;
> +};
> +
> +/* Init buffer. */

Typo: s/Init buffer/Initialize the buffer/.

> +void lj_wbuf_init(struct lj_wbuf *buf, lj_wbuf_writer writer, void *ctx,
> +		  uint8_t *mem, size_t size);

<snipped>

> -- 
> 2.28.0
> 

-- 
Best regards,
IM