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 3B2946EC40; Wed, 22 Sep 2021 10:52:57 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3B2946EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1632297177; bh=WzuklTOL3qbaKjZf+yxBGYBDj/unBd7xb+RmVypulqM=; 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=a4r6UyPqaS9SQEFYNQYANre1ZupVcQExuUg1RL/7Y2+hdHSZExzhv4Gum/fmiVruC ffSdwFPj7q+Aj9AjQBtZ2zA89mWFEPUazXujawlVFr6nlpwOf2tfFdzP1qis3+z8WV ajnNDT9X7AjCOw/vBxprEb1QTdF/ZziOX06CGrzE= Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (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 EEF326EC40 for ; Wed, 22 Sep 2021 10:52:55 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org EEF326EC40 Received: by smtp29.i.mail.ru with esmtpa (envelope-from ) id 1mSx3m-0003yI-UF; Wed, 22 Sep 2021 10:52:55 +0300 Date: Wed, 22 Sep 2021 10:51:28 +0300 To: Maxim Kokryashkin Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD91AE02D33A9C88A2F7B0FCECE251A4B9EABD2FC9AA46919D800894C459B0CD1B9EE47BB25F3D24BC5CB9ED2EE0C97E2210B575D37962C932B485234136DCFB6CD X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE720512D700D056E85EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063781769915DC205ABEEA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BBCA57AF85F7723F2B2FEBE4E09B5518286F40ADD15265E6CCC7F00164DA146DAFE8445B8C89999728AA50765F790063783E00425F71A4181389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8EDCF5861DED71B2F389733CBF5DBD5E9B5C8C57E37DE458B9E9CE733340B9D5F3BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E73542F54486E6D6388DC4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A5CC3641867FE40867B0CF1AAEAAEDD90CBB92ACFE23BE8366D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA756EBE15880F1DDB596D6546786ADF492D5A0AA20F8A030721A0D681C3DB17998AE1F9F0699A565DD98E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D347324AA9FA07FF01E612E7B23BEBDEF6A0637FFD727226072FC515999BA219D061354A5773B7454CD1D7E09C32AA3244CB07B90901741E783FC57EE5923C4219435DA7DC5AF9B58C0927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojnvI84oHUDXBByzaiMagXzA== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB42C057861608B6932FB2438ADBAACC2499D6A0DB4D54771A5F2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 3/7] memprof: move symtab to a separate module 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 patch! Please consider my comments below. On 08.09.21, Maxim Kokryashkin wrote: > Considering the symbol table format, it is obvious that it is suitable > not only for memprof, but for other profiling modules too. This commit > moves the symbol table to the separate module, so modules other than > memprof will be able to access it. > > Part of tarantool/tarantool#781 > --- > src/CMakeLists.txt | 1 + > src/Makefile.dep.original | 4 +++- > src/lj_memprof.c | 37 ++----------------------------- > src/lj_memprof.h | 36 ------------------------------ > src/lj_symtab.c | 36 ++++++++++++++++++++++++++++++ > src/lj_symtab.h | 46 +++++++++++++++++++++++++++++++++++++++ Amalgamated build should be updated as well. > 6 files changed, 88 insertions(+), 72 deletions(-) > create mode 100644 src/lj_symtab.c > create mode 100644 src/lj_symtab.h > > diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt > index 50c23cae..0f3d888f 100644 > --- a/src/CMakeLists.txt > +++ b/src/CMakeLists.txt > @@ -60,6 +60,7 @@ make_source_list(SOURCES_UTILS > SOURCES > lj_alloc.c > lj_char.c > + lj_symtab.c Minor: Don't sure that it is SOURCES_UTILS, but more likely SOURCES_PROFILER. > lj_utils_leb128.c > lj_vmmath.c > lj_wbuf.c > diff --git a/src/Makefile.dep.original b/src/Makefile.dep.original > index da5abf0d..b7d82719 100644 > --- a/src/Makefile.dep.original > +++ b/src/Makefile.dep.original > diff --git a/src/lj_memprof.c b/src/lj_memprof.c > index 2c1ef3b8..fba688bc 100644 > --- a/src/lj_memprof.c > +++ b/src/lj_memprof.c > @@ -19,40 +19,7 @@ > #include "lj_frame.h" > #include "lj_debug.h" > > -/* --------------------------------- Symtab --------------------------------- */ > - > -static const unsigned char ljs_header[] = {'l', 'j', 's', LJS_CURRENT_VERSION, > - 0x0, 0x0, 0x0}; > - > @@ -249,7 +216,7 @@ int lj_memprof_start(struct lua_State *L, const struct lj_memprof_options *opt) > > /* Init output. */ > lj_wbuf_init(&mp->out, mp_opt->writer, mp_opt->ctx, mp_opt->buf, mp_opt->len); > - dump_symtab(&mp->out, mp->g); > + lj_symtab_dump(&mp->out, mp->g); > > /* Write prologue. */ > lj_wbuf_addn(&mp->out, ljm_header, ljm_header_len); > diff --git a/src/lj_memprof.h b/src/lj_memprof.h > index 3417475d..33a9b869 100644 > --- a/src/lj_memprof.h > +++ b/src/lj_memprof.h > @@ -16,42 +16,6 @@ > #include "lj_def.h" > #include "lj_wbuf.h" > > -#define LJS_CURRENT_VERSION 0x1 This a little bit confusing for me: Symtab version in tarantool branch is 0, not 1. > - > -/* > -** symtab format: > -*/ > - > -#define SYMTAB_LFUNC ((uint8_t)0) > -#define SYMTAB_FINAL ((uint8_t)0x80) > > #define LJM_CURRENT_FORMAT_VERSION 0x01 > > diff --git a/src/lj_symtab.c b/src/lj_symtab.c > new file mode 100644 > index 00000000..93e19353 > --- /dev/null > +++ b/src/lj_symtab.c Minor: please add some module description here. Also, COPYRIGHT should be duplicated here too. See for example. > @@ -0,0 +1,36 @@ > +#define lj_symtab_c > +#define LUA_CORE > + > +#include "lj_symtab.h" > + > +static const unsigned char ljs_header[] = {'l', 'j', 's', LJS_CURRENT_VERSION, > + 0x0, 0x0, 0x0}; Please use tabs for alignment here. > + > +void lj_symtab_dump(struct lj_wbuf *out, const struct global_State *g) > +{ > +} > + Nit: Excess empty line. > diff --git a/src/lj_symtab.h b/src/lj_symtab.h > new file mode 100644 > index 00000000..9ea2c949 > --- /dev/null > +++ b/src/lj_symtab.h > @@ -0,0 +1,46 @@ Minor: please add some module description here. Also, COPYRIGHT should be duplicated here too. See for example. > +#ifndef LJ_SYMTAB > +#define LJ_SYMTAB Minor: It should be _LJ_SYMTAB_H, like for other header files. > + > +#include "lj_wbuf.h" > +#include "lj_obj.h" > + > +/* > +** symtab format: > +*/ > + > +#define LJS_CURRENT_VERSION 0x1 Nit: I suppose this can be mention before format description. I.e. it will be look like the following: This symtab version has the following format: ... Feel free to ignore. > + > +#define SYMTAB_LFUNC ((uint8_t)0) > +#define SYMTAB_FINAL ((uint8_t)0x80) > + Minor: please add some comments for function's description. > +void lj_symtab_dump(struct lj_wbuf *out, const struct global_State *g); > + > +#endif > -- > 2.33.0 > -- Best regards, Sergey Kaplun