Re: [Tarantool-patches] [PATCH] sysprof: enrich symtab on a new trace or a proto

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

Hi, Maxim!

Thanks for the patch!
LGTM, except a few nits.

On 11.05.22, Maxim Kokryashkin wrote:
> This commit adds functionality introduced
> in 0847e71abf7db2559e2bfa35f147ccf0112035e3 ('memprof: enrich symtab when
> meeting new prototype') and 0243fb72b05c7c63481c50151c65bef6b04e3372 ('memprof:
> enrich symtab when new trace is compiled') to sysprof.
> 
> Both the proto and the trace symtab extensions cannot be run from the
> sysprof's signal handler, so it is required to prevent sysprof from
> dumping anything in its signal handler during the process to keep the
> event stream intact. That is achieved with setting the sysprof's internal state

Typo? s/achieved with/achieved by/

> to `SPS_IDLE`.
> 
> Part of tarantool/tarantool#781
> ---
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/sysprof-rc-full-ci
> 
>  src/lj_bcread.c         |  7 +++++++
>  src/lj_parse.c          |  6 ++++++
>  src/lj_sysprof.c        | 44 ++++++++++++++++++++++++++++++++++++++++-
>  src/lj_sysprof.h        |  9 ++++++++-
>  src/lj_trace.c          |  7 +++++++
>  tools/sysprof/parse.lua | 18 ++++++++++++-----

Nit: please update Makefile.dep.original as well.

>  6 files changed, 84 insertions(+), 7 deletions(-)
> 
> diff --git a/src/lj_bcread.c b/src/lj_bcread.c
> index cb08599d..f6c7ad25 100644
> --- a/src/lj_bcread.c
> +++ b/src/lj_bcread.c

<snipped>

> diff --git a/src/lj_parse.c b/src/lj_parse.c
> index 30b0caa0..af0dc53f 100644
> --- a/src/lj_parse.c
> +++ b/src/lj_parse.c

<snipped>

> diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
> index 28d7d229..23947315 100644
> --- a/src/lj_sysprof.c
> +++ b/src/lj_sysprof.c
> @@ -238,7 +238,7 @@ static void stream_guest(struct sysprof *sp, uint32_t vmstate)

<snipped>

> diff --git a/src/lj_sysprof.h b/src/lj_sysprof.h
> index 2978bbd8..c31d61d8 100644
> --- a/src/lj_sysprof.h
> +++ b/src/lj_sysprof.h
> @@ -15,6 +15,7 @@
>  #ifndef _LJ_SYSPROF_H
>  #define _LJ_SYSPROF_H
>  

Nit: We need to check that LuaJIT is build with JIT here.
| #if LJ_HASJIT

> +#include "lj_jit.h"
>  #include "lj_obj.h"
>  #include "lmisclib.h"
>  
> @@ -80,7 +81,9 @@
>  #define LJP_FRAME_LUA_LAST  0x80
>  #define LJP_FRAME_HOST_LAST NULL
>  
> -#define LJP_SYMTAB_EVENT ((uint8_t)10)
> +#define LJP_SYMTAB_LFUNC_EVENT ((uint8_t)10)
> +#define LJP_SYMTAB_CFUNC_EVENT ((uint8_t)11)
> +#define LJP_SYMTAB_TRACE_EVENT ((uint8_t)12)

This means we should increase LJP_FORMAT_VERSION as well.

>  #define LJP_EPILOGUE_BYTE 0x80
>  
>  int lj_sysprof_configure(const struct luam_Sysprof_Config *config);
> @@ -91,4 +94,8 @@ int lj_sysprof_stop(lua_State *L);
>  
>  int lj_sysprof_report(struct luam_Sysprof_Counters *counters);
>  
> +void lj_sysprof_add_proto(const struct GCproto *pt);
> +
> +void lj_sysprof_add_trace(const struct GCtrace *tr);

Nit: We need to check that LuaJIT is build with JIT here.
| #if LJ_HASJIT

> +
>  #endif
> diff --git a/src/lj_trace.c b/src/lj_trace.c
> index 84b957c6..d7a78d4d 100644
> --- a/src/lj_trace.c
> +++ b/src/lj_trace.c

<snipped>

> diff --git a/tools/sysprof/parse.lua b/tools/sysprof/parse.lua
> index cb271784..555b6b3b 100755
> --- a/tools/sysprof/parse.lua
> +++ b/tools/sysprof/parse.lua
> @@ -33,7 +33,9 @@ M.FRAME = {

<snipped>

> @@ -128,8 +130,14 @@ local function parse_trace(reader, event, symbols)
>    -- parse_lua_callchain(reader, event)
>  end
>  
> -local function parse_symtab(reader, symbols)
> -  symtab.parse_sym_cfunc(reader, symbols)
> +local function parse_symtab(reader, symbols, vmstate)
> +  if vmstate == SYMTAB_LFUNC_EVENT then
> +    symtab.parse_sym_lfunc(reader, symbols)
> +  elseif vmstate == SYMTAB_CFUNC_EVENT then
> +    symtab.parse_sym_cfunc(reader, symbols)
> +  elseif vmstate == SYMTAB_TRACE_EVENT then
> +    symtab.parse_sym_trace(reader, symbols)
> +  end

Nit: may be we should add
| else
|   error('Unknown symtab event')
| end
check here.
Feel free to ignore.

>  end
>  
>  local event_parsers = {
> @@ -152,8 +160,8 @@ local function parse_event(reader, events, symbols)
>    if vmstate == STREAM_END then
>      -- TODO: samples & overruns
>      return false
> -  elseif vmstate == SYMTAB_EVENT then
> -    parse_symtab(reader, symbols)
> +  elseif SYMTAB_LFUNC_EVENT <= vmstate and vmstate <= SYMTAB_TRACE_EVENT then
> +    parse_symtab(reader, symbols, vmstate)
>      return true
>    end
>  
> -- 
> 2.35.1
> 

-- 
Best regards,
Sergey Kaplun