[Tarantool-patches] [PATCH luajit 3/7] memprof: move symtab to a separate module
Sergey Kaplun
skaplun at tarantool.org
Wed Sep 22 10:51:28 MSK 2021
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
<skipped>
> 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};
> -
<snipped>
> @@ -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:
<snipped>
> -*/
> -
> -#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 <lj_memprof.c> 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)
> +{
<snipped>
> +}
> +
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 <lj_memprof.h> 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:
<snipped>
> +*/
> +
> +#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
More information about the Tarantool-patches
mailing list