From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 97EFE6EC5E; Mon, 30 Aug 2021 17:09:43 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 97EFE6EC5E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1630332583; bh=4d/Z0uAE1gnc9A/NrHV5kp6dmRJC8TkBp38Dxb7lUew=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=bzu40zf5LUvaPeS2sHw0a3+CrQC0i1aK1GOfRJ3TRZDWWGtpNuuFT7qbpNXuekcGi KRDYcqhtZFdUbW/62RkWr//Lakwb4CX8GF+LSiI+PFZ71mcq09nAFZwE+J4ByUzdTn EpZOUzR3uWtTJHFhAgKa2FsMWJJi3Patq/Gk28EQ= Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (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 0CCF06EC5E for ; Mon, 30 Aug 2021 17:09:41 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0CCF06EC5E Received: by smtp44.i.mail.ru with esmtpa (envelope-from ) id 1mKhyn-0006Bf-2s; Mon, 30 Aug 2021 17:09:41 +0300 Date: Mon, 30 Aug 2021 17:08:20 +0300 To: Maxim Kokryashkin Message-ID: References: <5f67b8ba222f1071ee3f989353a14361c7fb1676.1629457244.git.m.kokryashkin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5f67b8ba222f1071ee3f989353a14361c7fb1676.1629457244.git.m.kokryashkin@tarantool.org> X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD91BCCB18F2C129F87F36E61E9E4584E9D182A05F538085040CDE1E203A99504CA53DDE390024CECD6970DFCF39FD29174EEBC26F70640C604 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE77633BACAB33B9508C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE723A0121E4B9CF535EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BBCA57AF85F7723F25519C829FA8AA2B2BF36764786B50D99CC7F00164DA146DAFE8445B8C89999728AA50765F7900637F6B57BC7E64490618DEB871D839B7333395957E7521B51C2DFABB839C843B9C08941B15DA834481F8AA50765F7900637F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8B0E0583254ED37F2E75ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A5B0D6700D8EE54C3C899FC9C22FB34479A8B6B5FD760E7039D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA756888F6138A55F47E410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D341ADA1A41A420E9B2B67BA8A9CEFBD54473F37C0CC421DD29B5939D10F314DC6A10978FB7855CA0851D7E09C32AA3244CB1854E9A8B1141E3403218D94244D44EB018FE5BB746DCD1927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj4DnN7V8kJ6saKqOIbEdSsA== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB471BCA6C6F25C289DDFA508ED082963E334AD54B71D4632CBF2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v2 1/3] utils: add CRC32 hash implementation X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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: > > > > > 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? > > > > >> +#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! > > > > 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 > 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 , so add it in dependencies too. includes inside . includes , which includes . > 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. > diff --git a/src/Makefile.original b/src/Makefile.original > index 031f0778..2a57c022 100644 > --- a/src/Makefile.original > +++ b/src/Makefile.original > 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 > 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 @@ > +/* > +*/ > + > +#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 Typo: Looks like the extra one. :) > +#include This is excess, see the comment near usage below. > + > +#include "lj_utils.h" > + > +static const uint32_t crc32_table[] = > +{ > +}; > + > +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 ): | 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 > -- > 2.32.0 > [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-August/025707.html -- Best regards, Sergey Kaplun