Re: [Tarantool-patches] [PATCH] sysprof: change C configuration API

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maxim!

Thanks for the patch!

LGTM, after fixes mentioned by Igor below.

On 18.05.22, Igor Munkin wrote:
> Max,
> 
> Thanks for the patch! See no critical flaws, but please consider my
> comments below.
> 
> On 11.05.22, Maxim Kokryashkin wrote:
> > This patch replaces the `lj_sysprof_configure` with individual
> > configuration handles for each config entry. Also, it chnages the
> 
> Typo: s/chnages/changes/.
> 
> > internal sysprof checks to let users run sysprof in default mode without
> > configuratons.
> > 
> > Part of tarantool/tarantool#781
> > ---
> > Branch: https://github.com/tarantool/luajit/tree/fckxorg/sysprof-rc-full-ci
> > PR with Tarantool integration:
> > https://github.com/tarantool/tarantool/pull/7123
> > 
> >  src/lib_misc.c                                | 20 +---
> >  src/lj_mapi.c                                 | 13 ++-
> >  src/lj_sysprof.c                              | 91 +++++++++++++------
> >  src/lj_sysprof.h                              |  6 +-
> >  src/lmisclib.h                                | 54 +++++------
> >  .../misclib-sysprof-capi/testsysprof.c        | 17 +---
> >  .../misclib-sysprof-lapi.test.lua             |  2 +-
> >  7 files changed, 116 insertions(+), 87 deletions(-)
> > 
> > diff --git a/src/lib_misc.c b/src/lib_misc.c
> > index e07f1b32..370307a7 100644
> > --- a/src/lib_misc.c
> > +++ b/src/lib_misc.c
> 
> <snipped>
> 
> > @@ -454,9 +441,12 @@ LJLIB_CF(misc_memprof_stop)
> >  
> >  LUALIB_API int luaopen_misc(struct lua_State *L)
> >  {
> > +  luaM_sysprof_set_writer(buffer_writer_default);
> > +  luaM_sysprof_set_on_stop(on_stop_cb_default);
> > +  luaM_sysprof_set_backtracer(NULL);
> 
> Why default backtracer is set to NULL here?
> 
> Besides, why do you need to use public API if you can use the same
> internal interfaces here? Otherwise, what is the reason to have these
> lj_sysprof_* functions?
> 
> > +
> >    LJ_LIB_REG(L, LUAM_MISCLIBNAME, misc);
> >    LJ_LIB_REG(L, LUAM_MISCLIBNAME ".memprof", misc_memprof);
> >    LJ_LIB_REG(L, LUAM_MISCLIBNAME ".sysprof", misc_sysprof);
> > -
> >    return 1;
> >  }
> 
> <snipped>
> 
> > diff --git a/src/lmisclib.h b/src/lmisclib.h
> > index b41f7f59..3ebf18ff 100644
> > --- a/src/lmisclib.h
> > +++ b/src/lmisclib.h
> > @@ -63,31 +63,29 @@ LUAMISC_API void luaM_metrics(lua_State *L, struct luam_Metrics *metrics);
> >  /* --- Sysprof - platform and lua profiler -------------------------------- */
> >  
> >  /* Profiler configurations. */
> 
> <snipped>
> 
> > +/*
> > +** Writer function for profile events. Must be async-safe, see also
> > +** `man 7 signal-safety`.
> > +** Should return amount of written bytes on success or zero in case of error.
> > +** Setting *data to NULL means end of profiling.
> > +** For details see <lj_wbuf.h>.
> > +*/
> > +typedef size_t (*sp_writer)(const void **data, size_t len, void *ctx);
> 
> Since this is a public type, please use a proper prefix for it.
> 
> > +/*
> > +** Callback on profiler stopping. Required for correctly cleaning
> > +** at VM finalization when profiler is still running.
> > +** Returns zero on success.
> > +*/
> > +typedef int (*sp_on_stop)(void *ctx, uint8_t *buf);
> 
> Ditto.
> 
> > +/*
> > +** Backtracing function for the host stack. Should call `frame_writer` on
> > +** each frame in the stack in the order from the stack top to the stack
> > +** bottom. The `frame_writer` function is implemented inside the sysprof
> > +** and will be passed to the `backtracer` function. If `frame_writer` returns
> > +** NULL, backtracing should be stopped. If `frame_writer` returns not NULL,
> > +** the backtracing should be continued if there are frames left.
> > +*/
> > +typedef void (*sp_backtracer)(void *(*frame_writer)(int frame_no, void *addr));
> 
> Ditto.
> 
> >  
> >  /*
> >  ** DEFAULT mode collects only data for luam_sysprof_counters, which is stored
> 
> <snipped>
> 
> > -- 
> > 2.35.1
> > 
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun