Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin <imun@tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit v1 04/11] profile: introduce symtab write module
Date: Mon, 21 Dec 2020 13:30:52 +0300	[thread overview]
Message-ID: <20201221103052.GU5396@tarantool.org> (raw)
In-Reply-To: <b86b1e8db026e782b2430510eb321b669782a277.1608142899.git.skaplun@tarantool.org>

Sergey,

Thanks for the patch! Please consider minor comments below.

On 16.12.20, Sergey Kaplun wrote:
> This patch adds profile writer that writes all necessary Lua
> functions prototypes info like GCproto address, name of the chunk this
> function was defined in and number of the first line of it.
> See <ljp_symtab.h> for details.
> 
> Usage of this module will be added at the next patches.

I propose the following wording to make it a bit clearer:
| core: introduce symtab dumping module
|
| This patch introduces the routine dumping the definitions of all
| loaded Lua functions via the write buffer introduced in the previous
| patch. The following information is recorded for each function:
| * GCproto address
| * The name of the Lua chunk where this function is defined
| * The line number where this function is defined (i.e. the line where
|   its signature locates)

> 
> Part of tarantool/tarantool#5442
> ---
>  src/Makefile             |  2 +-
>  src/Makefile.dep         |  2 ++
>  src/profile/ljp_symtab.c | 55 ++++++++++++++++++++++++++++++++++++++
>  src/profile/ljp_symtab.h | 57 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 115 insertions(+), 1 deletion(-)
>  create mode 100644 src/profile/ljp_symtab.c
>  create mode 100644 src/profile/ljp_symtab.h
> 
> diff --git a/src/Makefile b/src/Makefile
> index 4b1d937..e00265c 100644
> --- a/src/Makefile
> +++ b/src/Makefile

Please, adjust these changes considering the comments to the first
patch. I propose to name this <lj_wbuf_symtab.*> considering the naming
in the third patch.

> @@ -469,7 +469,7 @@ DASM_FLAGS= $(DASM_XFLAGS) $(DASM_AFLAGS)

<snipped>

> diff --git a/src/profile/ljp_symtab.c b/src/profile/ljp_symtab.c
> new file mode 100644
> index 0000000..5a17c97
> --- /dev/null
> +++ b/src/profile/ljp_symtab.c
> @@ -0,0 +1,55 @@

<snipped>

> +#define LJS_CURRENT_VERSION 2

Why is it already the second version?

> +
> +static const unsigned char ljs_header[] = {'l', 'j', 's', LJS_CURRENT_VERSION,
> +					   0x0, 0x0, 0x0};
> +
> +static void symtab_write_prologue(struct ljp_buffer *out)

Why do you need a separate routine instead of making <write_buffer>
function in <ljp_write.c> public?

> +{

<snipped>

> +void ljp_symtab_write(struct ljp_buffer *out, const struct global_State *g)
> +{
> +  const GCobj *o;
> +  const GCRef *iter = &g->gc.root;
> +
> +  symtab_write_prologue(out);
> +
> +  while (NULL != (o = gcref(*iter))) {
> +    switch (o->gch.gct) {

<snipped>

> +    case (~LJ_TTRACE): {
> +      /* TODO: Implement dumping a trace info */
> +      break;
> +    }

Minor: So this case be dropped for now?

> +    default: {

<snipped>

> diff --git a/src/profile/ljp_symtab.h b/src/profile/ljp_symtab.h
> new file mode 100644
> index 0000000..3a40d98
> --- /dev/null
> +++ b/src/profile/ljp_symtab.h
> @@ -0,0 +1,57 @@

<snipped>

> +struct global_State;
> +struct ljp_buffer;

Why do not simply include <lj_obj.h> and <profile/ljp_write.h>?
Otherwise, just move it close to the function signature and mention the
reason why you omit these headers inclusion.

> +

<snipped>

> +
> +/* Writes the symbol table of the VM g to out. */

Strictly saying this routine dumps the symbol table to *ljp_buffer* and
I guess it should be mentioned in its name e.g. <lj_wbuf_addsymtab>.

> +void ljp_symtab_write(struct ljp_buffer *out, const struct global_State *g);
> +
> +#endif
> -- 
> 2.28.0
> 

-- 
Best regards,
IM

  reply	other threads:[~2020-12-21 10:30 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-16 19:13 [Tarantool-patches] [PATCH luajit v1 00/11] LuaJIT memory profiler Sergey Kaplun
2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 01/11] build: add src dir in building Sergey Kaplun
2020-12-20 21:27   ` Igor Munkin
2020-12-23 18:20     ` Sergey Kaplun
2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 02/11] utils: introduce leb128 reader and writer Sergey Kaplun
2020-12-20 22:44   ` Igor Munkin
2020-12-23 22:34     ` Sergey Kaplun
2020-12-24  9:11       ` Igor Munkin
2020-12-25  8:46         ` Sergey Kaplun
2020-12-23 16:50   ` Sergey Ostanevich
2020-12-23 22:36     ` Sergey Kaplun
2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 03/11] profile: introduce profiler writing module Sergey Kaplun
2020-12-21  9:24   ` Igor Munkin
2020-12-24  6:46     ` Sergey Kaplun
2020-12-24 15:45       ` Sergey Ostanevich
2020-12-24 21:20         ` Sergey Kaplun
2020-12-25  9:37           ` Igor Munkin
2020-12-25 10:13             ` Sergey Kaplun
2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 04/11] profile: introduce symtab write module Sergey Kaplun
2020-12-21 10:30   ` Igor Munkin [this message]
2020-12-24  7:00     ` Sergey Kaplun
2020-12-24  9:36       ` Igor Munkin
2020-12-25  8:45         ` Sergey Kaplun
2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 05/11] vm: introduce LFUNC and FFUNC vmstates Sergey Kaplun
2020-12-25 11:07   ` Sergey Ostanevich
2020-12-25 11:23     ` Sergey Kaplun
2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 06/11] core: introduce new mem_L field Sergey Kaplun
2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 07/11] debug: move debug_frameline to public module API Sergey Kaplun
2020-12-20 22:46   ` Igor Munkin
2020-12-24  6:50     ` Sergey Kaplun
2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 08/11] profile: introduce memory profiler Sergey Kaplun
2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 09/11] misc: add Lua API for " Sergey Kaplun
2020-12-24 16:32   ` Sergey Ostanevich
2020-12-24 21:25     ` Sergey Kaplun
2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 10/11] tools: introduce tools directory Sergey Kaplun
2020-12-20 22:46   ` Igor Munkin
2020-12-24  6:47     ` Sergey Kaplun
2020-12-16 19:13 ` [Tarantool-patches] [PATCH luajit v1 11/11] profile: introduce profile parser Sergey Kaplun
2020-12-24 23:09   ` Igor Munkin
2020-12-25  8:41     ` Sergey Kaplun
2020-12-21 10:43 ` [Tarantool-patches] [PATCH luajit v1 00/11] LuaJIT memory profiler Igor Munkin
2020-12-24  7:02   ` Sergey Kaplun

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=20201221103052.GU5396@tarantool.org \
    --to=imun@tarantool.org \
    --cc=skaplun@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit v1 04/11] profile: introduce symtab write module' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox