Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Maxim Kokryashkin <max.kokryashkin@gmail.com>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit 3/7] memprof: move symtab to a separate module
Date: Wed, 22 Sep 2021 10:51:28 +0300
Message-ID: <YUrggOFAjFfwaLrT@root> (raw)
In-Reply-To: <a41d1a4c2ac69e5039f9f4403604e6d4983a310d.1631122521.git.m.kokryashkin@tarantool.org>

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

  reply	other threads:[~2021-09-22  7:52 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-08 17:50 [Tarantool-patches] [PATCH luajit 0/7] misc: introduce sysprof Maxim Kokryashkin via Tarantool-patches
2021-09-08 17:50 ` [Tarantool-patches] [PATCH luajit 1/7] core: save current frame top to lj_obj Maxim Kokryashkin via Tarantool-patches
2021-09-20 17:21   ` Sergey Kaplun via Tarantool-patches
2021-09-08 17:50 ` [Tarantool-patches] [PATCH luajit 2/7] core: separate the profiling timer from lj_profile Maxim Kokryashkin via Tarantool-patches
2021-09-21 11:13   ` Sergey Kaplun via Tarantool-patches
2021-09-23 11:37     ` Mikhail Shishatskiy via Tarantool-patches
2021-09-08 17:50 ` [Tarantool-patches] [PATCH luajit 3/7] memprof: move symtab to a separate module Maxim Kokryashkin via Tarantool-patches
2021-09-22  7:51   ` Sergey Kaplun via Tarantool-patches [this message]
2021-09-22  8:14     ` Sergey Kaplun via Tarantool-patches
2021-09-23 14:51   ` [Tarantool-patches] [PATCH luajit v2] " Maxim Kokryashkin via Tarantool-patches
2021-09-08 17:50 ` [Tarantool-patches] [PATCH luajit 4/7] core: introduce lua and platform profiler Maxim Kokryashkin via Tarantool-patches
2021-09-29  6:53   ` Sergey Kaplun via Tarantool-patches
2021-09-08 17:50 ` [Tarantool-patches] [PATCH luajit 5/7] memprof: add profile common section Maxim Kokryashkin via Tarantool-patches
2021-10-05 10:48   ` Sergey Kaplun via Tarantool-patches
2021-10-06 19:15     ` [Tarantool-patches] [PATCH luajit v2] " Maxim Kokryashkin via Tarantool-patches
2021-09-08 17:50 ` [Tarantool-patches] [PATCH luajit 6/7] sysprof: introduce Lua API Maxim Kokryashkin via Tarantool-patches
2021-10-05 15:36   ` Sergey Kaplun via Tarantool-patches
2021-09-08 17:50 ` [Tarantool-patches] [PATCH luajit 7/7] tools: introduce parsers for sysprof Maxim Kokryashkin via Tarantool-patches
2021-10-07 11:28   ` Sergey Kaplun via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YUrggOFAjFfwaLrT@root \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=max.kokryashkin@gmail.com \
    --cc=skaplun@tarantool.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git