From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (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 D38704765E0 for ; Thu, 24 Dec 2020 10:00:56 +0300 (MSK) Date: Thu, 24 Dec 2020 10:00:11 +0300 From: Sergey Kaplun Message-ID: <20201224070011.GK9101@root> References: <20201221103052.GU5396@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201221103052.GU5396@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH luajit v1 04/11] profile: introduce symtab write module List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Igor, Thanks for the review! On 21.12.20, Igor Munkin wrote: > 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 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) Thanks, I'll merge this commit into memprof (see my comments below) and add corresponding paragraph to commit message. > > > > > 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 considering the naming > in the third patch. > > > @@ -469,7 +469,7 @@ DASM_FLAGS= $(DASM_XFLAGS) $(DASM_AFLAGS) > > > > > 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 @@ > > > > > +#define LJS_CURRENT_VERSION 2 > > Why is it already the second version? It's inherited from LuaVela. I didn't know is it right to drop version to zero. But AFAICS from your comment it is :) > > > + > > +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 > function in public? Yes, make it public at the next series. > > > +{ > > > > > +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) { > > > > > + case (~LJ_TTRACE): { > > + /* TODO: Implement dumping a trace info */ > > + break; > > + } > > Minor: So this case be dropped for now? Yes, I've leave it as a reminder. I'll drop it in the next series. > > > + default: { > > > > > 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 @@ > > > > > +struct global_State; > > +struct ljp_buffer; > > Why do not simply include and ? > Otherwise, just move it close to the function signature and mention the > reason why you omit these headers inclusion. I don't provide neither definition nor here. They are not related to this header, only used in the translation unit. This will be dropped as far as I'll merge it commit with a memprof-related one. > > > + > > > > > + > > +/* 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. . At least you can see it from function definition. I'll make this function internal for memprof only for now. > > > +void ljp_symtab_write(struct ljp_buffer *out, const struct global_State *g); > > + > > +#endif > > -- > > 2.28.0 > > > > -- > Best regards, > IM -- Best regards, Sergey Kaplun