Re: [Tarantool-patches] [PATCH luajit v2 1/3] utils: add CRC32 hash implementation

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maxim!

Thanks for the fixes!
Please, consider my comments below.

On 20.08.21, Maxim Kokryashkin wrote:
> This commit adds an implementation of the CRC32 hash. The
> implementation is taken from GNU libberty so it differs from the
> classic CRC32 implementation. The values are not reflected, and there
> is no final XOR value. These differences make it easy to compose the
> values of multiple blocks.
> 
> That hash implementation will be used in the memprof's symtab later on.
> 
> Part of tarantool/tarantool#5813
> ---
> On 28.07.21 Sergey Kaplun wrote:
> 
> <snipped>
> 
> > IINM we should compare this approach with a full dump of all .so
> > functions. May you provide some benches of your approach against the
> > aforementioned?
> As for benchmarks, I'll provide them later in the separate letter.

Yep, seen it here[1].
Do you check the reason of such huge difference (more than x200!!!)?
I suspect the hash calculation from the whole library, but don't check it.
Also, I didn't get the following:
Are these measurements done for LuaJIT or for Tarantool. Tarantool has
more libraries, so it would be nice to see benchmark for it too.

Also, as far as this way is such heavy, do we need this patch?

> 
> <snipped>
> 
> >> +#define _GNU_SOURCE
> > Locally, I need to define not _GNU_SOURCE, but __USE_GNU. May you please
> > check what exactly define is required and why?
> Defining __USE_GNU by yourself is a terrible practice as it is an 
> internal definition of the glibc. Consodering this, you should stick
> to _GNU_SOURCE

OK, thanks for clarification!

> 
> <snipped>
> 
>  src/CMakeLists.txt        |   1 +
>  src/Makefile.dep.original |   3 +-
>  src/Makefile.original     |   2 +-
>  src/lj_utils.h            |  29 ++++++++++
>  src/lj_utils_hash.c       | 110 ++++++++++++++++++++++++++++++++++++++

Nit: may be lj_utils_crc32.c is better here (at least we know the hash
type).
Feel free to ignore.

>  src/ljamalg.c             |   1 +
>  6 files changed, 144 insertions(+), 2 deletions(-)
>  create mode 100644 src/lj_utils_hash.c
> 
> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index 809aac68..74c7a205 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt

<snipped>

> diff --git a/src/Makefile.dep.original b/src/Makefile.dep.original
> index f3672413..3dc6e9b4 100644
> --- a/src/Makefile.dep.original
> +++ b/src/Makefile.dep.original
> @@ -208,6 +208,7 @@ lj_trace.o: lj_trace.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h \
>   lj_vm.h lj_vmevent.h lj_target.h lj_target_*.h
>  lj_udata.o: lj_udata.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h \
>   lj_gc.h lj_udata.h
> +lj_utils_hash.o: lj_utils_hash.c lj_utils.h

Dependencies are defined as the following:
1) The file itself.
2) Each header and all included LuaJIT headers in it.

For example: lj_utils_leb128.o depends on the .c file itself. It
includes <lj_utils.h>, so add it in dependencies too.
<lj_utils.h> includes inside <lj_def.h>. <lj_def.h> includes <lua.h>,
which includes <luaconf.h>.

>  lj_utils_leb128.o: lj_utils_leb128.c lj_utils.h lj_def.h lua.h luaconf.h
>  lj_vmevent.o: lj_vmevent.c lj_obj.h lua.h luaconf.h lj_def.h lj_arch.h \
>   lj_str.h lj_tab.h lj_state.h lj_dispatch.h lj_bc.h lj_jit.h lj_ir.h \
> @@ -234,7 +235,7 @@ ljamalg.o: ljamalg.c lua.h luaconf.h lauxlib.h lj_gc.c lj_obj.h lj_def.h \

Also, I see tons of errors like the following:

| /usr/lib/gcc/x86_64-pc-linux-gnu/9.3.0/../../../../x86_64-pc-linux-gnu/bin/ld: libluajit.a(ljamalg.o): in function `lj_err_throw':
| ljamalg.c:(.text+0xaf50): multiple definition of `lj_err_throw'; libluajit.a(lj_err.o):lj_err.c:(.text+0x270): first defined here

This problem is known, IINM.

How do you check this patch?
If you have unrelated patch, which fixes the mentioned build type,
please send it to the mailing list.

<snipped>

> diff --git a/src/Makefile.original b/src/Makefile.original
> index 031f0778..2a57c022 100644
> --- a/src/Makefile.original
> +++ b/src/Makefile.original

<snipped>

> diff --git a/src/lj_utils.h b/src/lj_utils.h
> index f5c15797..fe179753 100644
> --- a/src/lj_utils.h
> +++ b/src/lj_utils.h

<snipped>

> diff --git a/src/lj_utils_hash.c b/src/lj_utils_hash.c
> new file mode 100644
> index 00000000..8a7f0869
> --- /dev/null
> +++ b/src/lj_utils_hash.c
> @@ -0,0 +1,110 @@
> +/*

<snipped>

> +*/
> +
> +#define lj_utils_hash

Typo: s/lj_utils_hash/lj_utils_hash_c/

Minor: Please, add LUA_CORE define for consistency with other c modules.

> +
> +#include <stdio.h>

Typo: Looks like the extra one. :)

> +#include <assert.h>

This is excess, see the comment near usage below.

> +
> +#include "lj_utils.h"
> +
> +static const uint32_t crc32_table[] =
> +{

<snipped>

> +};
> +
> +uint32_t lj_utils_crc32 (const char *buf, size_t len, uint32_t init)
> +{
> +  assert(buf != NULL);

Please, use `lua_assert()` instead -- it is common practise for all
LuaJIT code.

> +
> +  uint32_t crc = init;

When build like the following (the `CCWARN` comment line in the
<src/Makefile.original>):

| cmake -DCMAKE_C_FLAGS="-Wextra -Wdeclaration-after-statement \
| -Wredundant-decls -Wshadow -Wpointer-arith" -DCMAKE_BUILD_TYPE=Debug \
| -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON . && make -j

I get the following compile warnings:

| /home/burii/reviews/luajit/csymbols/src/lj_utils_hash.c: In function 'lj_utils_crc32':
| /home/burii/reviews/luajit/csymbols/src/lj_utils_hash.c:104:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
|   104 |   uint32_t crc = init;

> +  while (len--) {
> +    crc = (crc << 8) ^ crc32_table[((crc >> 24) ^ *buf) & 255];
> +    buf++;
> +  }
> +  return crc;
> +}
> diff --git a/src/ljamalg.c b/src/ljamalg.c
> index 3f7e6860..3a667a68 100644
> --- a/src/ljamalg.c
> +++ b/src/ljamalg.c

<snipped>

> -- 
> 2.32.0
> 

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-August/025707.html

-- 
Best regards,
Sergey Kaplun