From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (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 896AE4765E0 for ; Sat, 26 Dec 2020 18:27:01 +0300 (MSK) Date: Sat, 26 Dec 2020 18:26:15 +0300 From: Sergey Kaplun Message-ID: <20201226152615.GX9101@root> References: <20201226142225.GH5396@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201226142225.GH5396@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH luajit v2 2/7] core: introduce write buffer 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 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 > > > > > > > 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 \ > > > > > - 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, looks buggy > and I propose to make these changes manually. Feel free to ignore. is included before in , 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 > > > > > 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 @@ > > > > > + /* > > + ** 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. > > > + */ > > > > > 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 @@ > > > > > +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 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 . 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); > > > > > -- > > 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