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

[Sergey Kaplun <skaplun@tarantool.org>]

Igor,

Thanks for the review!

On 26.12.20, Igor Munkin wrote:
> 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.

<lj_utils.h> is included before in <lj_wbuf.c>, so it should be added
right after it, without duplicates later.

> 
> >  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/.

Fixed. Thanks!

> 
> > +  ** through buffer.
> 
> Typo: s/through buffer/through the buffer/.

Fixed.

> 
> > +  */
> 
> <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/.

Fixed.

> 
> > +  ** 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.

Yes, this is misleading comment. I rewrote it the following way:
| *data should contain a buffer of at least the initial size.

> 
> > +  ** If *data == NULL stream stops.
> 
> Minor: It would be nice to describe the contract of this function near
> its typedef above.

OK, moved it 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>.

My bad! Thanks!

> 
> > +  /* 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/.

Fixed!

> 
> > +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

See the iterative patch below. Branch is force-pushed.

===================================================================
diff --git a/src/lj_wbuf.c b/src/lj_wbuf.c
index 8f090eb..ef46545 100644
--- a/src/lj_wbuf.c
+++ b/src/lj_wbuf.c
@@ -82,8 +82,8 @@ void lj_wbuf_addn(struct lj_wbuf *buf, const void *src, size_t n)
     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.
+  ** Buffer doesn't belong to us so we must to pump data
+  ** through the buffer.
   */
   while (LJ_UNLIKELY(n > buf->size)) {
     const size_t left = wbuf_left(buf);
diff --git a/src/lj_wbuf.h b/src/lj_wbuf.h
index 58a109e..77a7cf4 100644
--- a/src/lj_wbuf.h
+++ b/src/lj_wbuf.h
@@ -31,32 +31,32 @@
 #define STREAM_ERR_IO 0x1
 #define STREAM_STOP   0x2
 
+/*
+** Buffer writer which is called on the buffer flush.
+** Should return amount of written bytes on success or zero in case of error.
+** *data should contain a buffer of at least the initial size.
+** If *data == NULL stream stops.
+*/
 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.
-  ** 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.
-  */
   lj_wbuf_writer writer;
   /* Context for writer function. */
   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;
+  /* Saved errno in case of error. */
+  int saved_errno;
   /* Internal flags. */
   volatile uint8_t flags;
 };
 
-/* 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);
 
===================================================================

-- 
Best regards,
Sergey Kaplun