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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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