[Tarantool-patches] [PATCH] sysprof: change C configuration API
Sergey Kaplun
skaplun at tarantool.org
Thu May 19 09:56:50 MSK 2022
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
More information about the Tarantool-patches
mailing list