Re: [Tarantool-patches] [PATCH luajit v1 04/11] profile: introduce symtab write module

[Sergey Kaplun <skaplun@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 <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)

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 <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?

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 <write_buffer>
> function in <ljp_write.c> public?

Yes, make it public at the next series.

> 
> > +{
> 
> <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?

Yes, I've leave it as a reminder. I'll drop it in the next series.

> 
> > +    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.

I don't provide neither <lj_obj.h> definition nor <lj_wbuf.h> 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.

> 
> > +
> 
> <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>.

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