From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 ED27B4765E0 for ; Sat, 26 Dec 2020 12:33:38 +0300 (MSK) Date: Sat, 26 Dec 2020 12:32:52 +0300 From: Sergey Kaplun Message-ID: <20201226093252.GV9101@root> References: <20201225214245.GG5396@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201225214245.GG5396@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH luajit v2 1/7] utils: introduce leb128 reader and writer 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 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 > > > > > 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 @@ > > > > > +/* > > +** Reads a value from a buffer of bytes to a int64_t output. > > Typo: s/a int64_t/an int64_t/g. Fixed. Thanks. > > flag means the note relates to all comments below. Copy that. > > > +** No bounds checks for the buffer. Returns number of bytes read. > > +*/ > > > > > +/* > > +** 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. > > +*/ > > > > > 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 @@ > > > > > +#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. > > > + > > > > > -- > > 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