Re: [Tarantool-patches] [PATCH luajit v2 1/7] utils: introduce leb128 reader and writer

[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 several nits below.
> 
> On 25.12.20, Sergey Kaplun wrote:
> > Most of the numeric data written by the memory profiler is encoded
> > via LEB128 compression. This patch introduces the module for encoding
> > and decoding 64bit number to LEB128 form.
> > 
> > Part of tarantool/tarantool#5442
> > ---
> > 
> > Changes in v2:
> >   - Removed reader funciton's parameter named guard.
> >   - Code style fixes.
> > 
> >  src/Makefile          |   3 +-
> >  src/Makefile.dep      |   7 ++-
> >  src/lj_utils.h        |  58 +++++++++++++++++++
> >  src/lj_utils_leb128.c | 132 ++++++++++++++++++++++++++++++++++++++++++
> >  src/ljamalg.c         |   1 +
> >  5 files changed, 197 insertions(+), 4 deletions(-)
> >  create mode 100644 src/lj_utils.h
> >  create mode 100644 src/lj_utils_leb128.c
> > 
> > diff --git a/src/Makefile b/src/Makefile
> > index 2786348..dc2ddb6 100644
> > --- a/src/Makefile
> > +++ b/src/Makefile
> > @@ -466,6 +466,7 @@ endif
> >  DASM_FLAGS= $(DASM_XFLAGS) $(DASM_AFLAGS)
> >  DASM_DASC= vm_$(DASM_ARCH).dasc
> >  
> > +UTILS_O= lj_utils_leb128.o
> 
> Minor: I personally believe this is excess and you can simply move this
> object file to LJCORE_O list. BUILDVM_O is built with another toolchain;
> LJLIB_O is used for generating auxiliary headers with buildvm.
> Everything else is mentioned in LJCORE_O.

Reasonable. Dropped.

> 
> >  BUILDVM_O= host/buildvm.o host/buildvm_asm.o host/buildvm_peobj.o \
> >  	   host/buildvm_lib.o host/buildvm_fold.o
> >  BUILDVM_T= host/buildvm
> 
> <snipped>
> 
> > diff --git a/src/lj_utils.h b/src/lj_utils.h
> > new file mode 100644
> > index 0000000..1671e8e
> > --- /dev/null
> > +++ b/src/lj_utils.h
> > @@ -0,0 +1,58 @@
> 
> <snipped>
> 
> > +/*
> > +** Reads a value from a buffer of bytes to a int64_t output.
> 
> Typo: s/a int64_t/an int64_t/g.

Fixed. Thanks.

> 
> <g> flag means the note relates to all comments below.

Copy that.

> 
> > +** No bounds checks for the buffer. Returns number of bytes read.
> > +*/
> 
> <snipped>
> 
> > +/*
> > +** Writes a value from an signed 64-bit input to a buffer of bytes.
> 
> Typo: s/an signed/a signed/.

Fixed. Thanks.

> 
> > +** No bounds checks for the buffer. Returns number of bytes written.
> > +*/
> 
> <snipped>
> 
> > diff --git a/src/lj_utils_leb128.c b/src/lj_utils_leb128.c
> > new file mode 100644
> > index 0000000..ce8081b
> > --- /dev/null
> > +++ b/src/lj_utils_leb128.c
> > @@ -0,0 +1,132 @@
> 
> <snipped>
> 
> > +#define LINK_BIT               (0x80)
> > +#define MIN_TWOBYTE_VALUE      (0x80)
> > +#define PAYLOAD_MASK           (0x7f)
> > +#define SHIFT_STEP             (7)
> > +#define LEB_SIGN_BIT           (0x40)
> 
> Typo: Why did you change the whitespace here? Everything was OK with it
> in the previous version.

My bad. Fixed.

> 
> > +
> 
> <snipped>
> 
> > -- 
> > 2.28.0
> > 
> 
> -- 
> Best regards,
> IM

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

===================================================================
diff --git a/src/Makefile b/src/Makefile
index 3218dfd..ae4489d 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -472,7 +472,6 @@ endif
 DASM_FLAGS= $(DASM_XFLAGS) $(DASM_AFLAGS)
 DASM_DASC= vm_$(DASM_ARCH).dasc
 
-UTILS_O= lj_utils_leb128.o
 BUILDVM_O= host/buildvm.o host/buildvm_asm.o host/buildvm_peobj.o \
 	   host/buildvm_lib.o host/buildvm_fold.o
 BUILDVM_T= host/buildvm
@@ -502,7 +501,7 @@ LJCORE_O= lj_gc.o lj_err.o lj_char.o lj_bc.o lj_obj.o lj_buf.o lj_wbuf.o \
 	  lj_asm.o lj_trace.o lj_gdbjit.o \
 	  lj_ctype.o lj_cdata.o lj_cconv.o lj_ccall.o lj_ccallback.o \
 	  lj_carith.o lj_clib.o lj_cparse.o \
-	  lj_lib.o lj_alloc.o $(UTILS_O) lib_aux.o \
+	  lj_lib.o lj_alloc.o lj_utils_leb128.o lib_aux.o \
 	  $(LJLIB_O) lib_init.o
 
 LJVMCORE_O= $(LJVM_O) $(LJCORE_O)
diff --git a/src/lj_utils.h b/src/lj_utils.h
index 1671e8e..63d6c84 100644
--- a/src/lj_utils.h
+++ b/src/lj_utils.h
@@ -14,13 +14,13 @@
 #define LEB128_U64_MAXSIZE 10
 
 /*
-** Reads a value from a buffer of bytes to a int64_t output.
+** Reads a value from a buffer of bytes to an int64_t output.
 ** No bounds checks for the buffer. Returns number of bytes read.
 */
 size_t LJ_FASTCALL lj_utils_read_leb128(int64_t *out, const uint8_t *buffer);
 
 /*
-** Reads a value from a buffer of bytes to a int64_t output. Consumes no more
+** Reads a value from a buffer of bytes to an int64_t output. Consumes no more
 ** than n bytes. No bounds checks for the buffer. Returns number of bytes
 ** read. If more than n bytes is about to be consumed, returns 0 without
 ** touching out.
@@ -29,13 +29,13 @@ size_t LJ_FASTCALL lj_utils_read_leb128_n(int64_t *out, const uint8_t *buffer,
 					  size_t n);
 
 /*
-** Reads a value from a buffer of bytes to a uint64_t output.
+** Reads a value from a buffer of bytes to an uint64_t output.
 ** No bounds checks for the buffer. Returns number of bytes read.
 */
 size_t LJ_FASTCALL lj_utils_read_uleb128(uint64_t *out, const uint8_t *buffer);
 
 /*
-** Reads a value from a buffer of bytes to a uint64_t output. Consumes no more
+** Reads a value from a buffer of bytes to an uint64_t output. Consumes no more
 ** than n bytes. No bounds checks for the buffer. Returns number of bytes
 ** read. If more than n bytes is about to be consumed, returns 0 without
 ** touching out.
@@ -44,7 +44,7 @@ size_t LJ_FASTCALL lj_utils_read_uleb128_n(uint64_t *out, const uint8_t *buffer,
 					   size_t n);
 
 /*
-** Writes a value from an signed 64-bit input to a buffer of bytes.
+** Writes a value from a signed 64-bit input to a buffer of bytes.
 ** No bounds checks for the buffer. Returns number of bytes written.
 */
 size_t LJ_FASTCALL lj_utils_write_leb128(uint8_t *buffer, int64_t value);
diff --git a/src/lj_utils_leb128.c b/src/lj_utils_leb128.c
index ce8081b..0d50b83 100644
--- a/src/lj_utils_leb128.c
+++ b/src/lj_utils_leb128.c
@@ -10,11 +10,11 @@
 
 #include "lj_utils.h"
 
-#define LINK_BIT               (0x80)
-#define MIN_TWOBYTE_VALUE      (0x80)
-#define PAYLOAD_MASK           (0x7f)
-#define SHIFT_STEP             (7)
-#define LEB_SIGN_BIT           (0x40)
+#define LINK_BIT          (0x80)
+#define MIN_TWOBYTE_VALUE (0x80)
+#define PAYLOAD_MASK      (0x7f)
+#define SHIFT_STEP        (7)
+#define LEB_SIGN_BIT      (0x40)
 
 /* ------------------------- Reading LEB128/ULEB128 ------------------------- */
 
===================================================================

-- 
Best regards,
Sergey Kaplun