[Tarantool-patches] [PATCH luajit v2 1/7] utils: introduce leb128 reader and writer
Sergey Kaplun
skaplun at tarantool.org
Sat Dec 26 12:32:52 MSK 2020
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
More information about the Tarantool-patches
mailing list