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
next prev parent 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